Skip to content

feat(plain): report the step count in --json output - #1333

Open
addyCooks wants to merge 3 commits into
Nano-Collective:mainfrom
addyCooks:feat/agent-eval-steps-1197
Open

addyCooks wants to merge 3 commits into
Nano-Collective:mainfrom
addyCooks:feat/agent-eval-steps-1197

Conversation

@addyCooks

Copy link
Copy Markdown
Contributor

Part of #1197

Description

The agent evaluation harness in #1197 needs a step count per run, and nothing in nanocoder reports one today. toolCalls.length doesn't work as a stand-in: one step can make zero tool calls (an empty-turn nudge or a malformed-call retry) or several at once.

This is the one change to nanocoder's own code that the harness needs, so it lands on its own before the harness.

  • runPlainConversation counts every model round-trip, retried turns included, and returns it as steps on all three outcome variants (success, tool-approval-required, error).
  • The --plain --json report includes steps.
  • The three reports emitted before the model is ever called (untrusted directory, init failure, user-prompt-submit hook veto) report steps: 0, so the field is always present.
  • docs/features/commands.md documents the new field.

Agent behaviour is unchanged; this only counts and reports.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Changeset

  • Added a changeset describing this change for the changelog

Testing

Automated Tests

  • New features include passing tests in .spec.ts/tsx files
  • All existing tests pass (pnpm test:all completes successfully)
  • Tests cover both success and error scenarios

Manual Testing

  • Tested with Ollama
  • Tested with OpenRouter
  • Tested with OpenAI-compatible API
  • Tested MCP integration (if applicable)

Checklist

  • If this was for an open issue, I was assigned to it
  • Code follows project style guidelines
  • Self-review completed
  • Documentation updated (if needed)
  • No breaking changes (or clearly documented)
  • Appropriate logging added using structured logging

The agent evaluation harness (Nano-Collective#1197) needs a per-run step count, and
nothing in nanocoder reports one. toolCalls.length is not a substitute:
a step can issue zero tool calls (an empty-turn nudge, a malformed
retry) or several at once.

runPlainConversation now counts every model round-trip, retried turns
included, and returns it as steps on all three outcome variants. The
--plain --json report carries it through, and the three reports emitted
before the model is ever called (untrusted directory, init failure, a
user-prompt-submit hook veto) report steps: 0, so the field is always
present.

Part of Nano-Collective#1197.
@github-actions github-actions Bot added the area:docs Documentation label Sep 15, 2026
@github-actions

Copy link
Copy Markdown
Contributor

nc-review: needs work — 1 blocking, 1 important

@addyCooks — there is a blocking item below.

PR #1333 adds a steps counter to PlainConversationOutcome and surfaces it in --plain --json reports, intended as part of #1197. The increment point, return-path coverage, docs, and changeset are coherent, but adding steps: number as a required field on the outcome discriminated union will break tsc: at least 15 existing PlainConversationOutcome literals in source/plain/shell.spec.ts are not updated to include steps. The PR will not compile as shown.

🔴 blocking · correctness · source/plain/conversation.ts:76

PlainConversationOutcome is changed from a union whose variants have no required field besides kind + the discriminator-specific fields, to one where every variant requires steps: number. This is a breaking change to the exported type, but the diff does not update the call sites that build PlainConversationOutcome literals directly.

In source/plain/shell.spec.ts at least 15 sites construct the literal directly — 13 with kind: "success" (lines ~139, ~172, ~256, ~293, ~406, ~527, ~565, ~644, ~684, ~726, ~931, ~998, ~1042) and 2 with kind: "error" (lines ~336, ~763) — and pass them to makeFakeRunPlainConversation, whose parameter is typed PlainConversationOutcome. With steps: number required, every one of those call sites becomes a tsc error (Property 'steps' is missing in type ... but required in type ...), so the PR will not compile. (Only the one site the diff does update, the "well-formed report" test at line ~256, currently passes steps: 3.)

Two correct fixes: either make steps optional (steps?: number) on the three outcome variants and default it to undefined when reporting in shell.ts, or update all ~15 existing literal sites in source/plain/shell.spec.ts to include steps: <n>. Optional is the smaller and less invasive change and matches the spirit of usage? already being optional on the same variants.

🟠 important · completeness · source/plain/conversation.spec.ts

The new tests cover the success path, the empty-turn nudge cap, the empty-turn recovery, the auto-approved tool path, the tool-approval-required path, and the pre-loop abort. They do not cover:

  • the malformed-retry path (maxMalformedRetries) — a model that emits malformed XML tool calls, then self-corrects on a later attempt: every retry is a model round-trip and should bump steps, but no test asserts this.
  • the truncated-turn continuation path (maxTruncatedTurns) — a finishReason: 'length' continuation is also a step of its own and should bump steps, again with no assertion.
  • the final-turn defensive fallback (Conversation exceeded ${maxTurns} turns) — the post-loop error return reports steps and should equal maxTurns; no test exists.

Given the rubric's note that retry paths have shipped with untested assertions before, and that the harness in #1197 will rely on steps for measurement, those branches are the ones whose step-counting correctness most needs a regression test.


🔴 blocking · 🟠 a reviewer would ask for a change · ⚪ optional

Automated code review — correctness, security, design, tests, plus duplicates and scope. A human still decides; this is not a substitute for review and is not exhaustive. The required status checks separately cover lint, formatting, types, unused dependencies, the test suite and the build. This bot never merges. Maintainers can rerun with /re-review.

@github-actions github-actions Bot added the agent:needs-work nc-review found blocking findings label Sep 15, 2026
…aths

Adds steps assertions to the malformed-retry and truncated-turn tests and a new test for the post-loop 'exceeded maxTurns' fallback, so every retry path the eval harness relies on is covered. Gives every PlainConversationOutcome literal in shell.spec.ts a steps value to match the now-required field.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent:needs-work nc-review found blocking findings area:docs Documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant