fix(advisor): accept same-turn submit repair

[rsliter]

Summary

The Advisor controller rejected a valid submit_review repair when the SDK completed one failed call and one successful call in the same challenge-and-record response.
This change accepts that bounded repair while preserving fail-closed validation and canonical state finalization.

Related Issue

Fixes #9630.

Changes

• Recognize one settled failed terminal submit followed by one settled successful submit when repair is configured and the provider reports no turn error.
• Continue rejecting malformed, unsettled, all-failed, extra-attempt, and post-success activity before pending review state becomes canonical.
• Add protocol and complete session regressions for the same-response repair, and clarify the owning Advisor protocol README.
• Record the escaped-defect root cause: the controller recognized repair only after a separate continuation prompt, while the existing test treated the valid same-response sequence as an expected rejection.

Type of Change

• Code change (feature, bug fix, or refactor)
• Code change with doc updates
• Doc only (prose changes, no code sample modifications)
• Doc only (includes code sample changes)

Quality Gates

• Tests added or updated for changed behavior
• Existing tests cover changed behavior, justification:
• Tests not applicable, justification:
• Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
• Sensitive-path review completed or maintainer-approved waiver recorded, reviewer/approval link/justification: Complete state-transition review confirmed that pending review state finalizes only after full flow validation. Provider errors, malformed or unsettled attempts, extra attempts, all-failed attempts, and activity after success remain fail closed.
• Non-success, skipped, or missing CI check accepted by maintainer, check name, approval link, and follow-up issue:

Documentation Writer Review

• Documentation writer subagent reviewed the completed changes
• Result: no-docs-needed
• Evidence: tools/pr-review-advisor/README.md owns the internal Advisor protocol. No public command, configuration, API, policy schema, or supported product workflow changed.
• Agent: Codex Desktop

DGX Station Hardware Evidence

• Tested on DGX Station
• Tested commit:
• Station profile/scenario:
• Result:
• Supporting evidence:

Verification

• PR description includes a Signed-off-by: line and every commit appears as Verified in GitHub
• Normal pre-commit, commit-msg, and pre-push hooks passed, or npm run validate:pr passed after refreshing origin/main when hooks were skipped or unavailable
• Targeted behavior tests pass for the current change set, or tests are marked not applicable above, command/result or justification: Focused protocol and session tests passed 55/55. The complete Advisor integration suite passed 423/423. npm run build:cli, npm run typecheck, npm run checks:repository, and npm run test:titles:check passed.
• Applicable broad gate passed, npm test for broad runtime/test-harness changes; npm run check for repo-wide validation/coverage changes, command/result:
• Quality Gates section completed with required justifications or waivers
• No secrets, API keys, or credentials committed
• npm run docs builds without warnings (doc changes only)
• Doc pages follow the style guide (doc changes only)
• New doc pages include SPDX header and frontmatter (new pages only)

---

Signed-off-by: Rebecca Sliter 571084+rsliter@users.noreply.github.com

Summary by CodeRabbit

• Bug Fixes

  • Improved terminal submission validation for advisor sessions.
  • Supports recovery from one failed submission when followed by a successful retry in the same turn.
  • Rejects incomplete, repeated, overlapping, or post-success submission activity.
  • Prevents pending review state from being committed when validation fails.
  • Same-turn successful submissions now complete without unnecessary repair prompts or errors.

• Documentation

  • Clarified terminal submission and state-commit behavior in the review workflow documentation.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 5dbb76a6-27e4-4c0f-9077-352a3d86b048

📥 Commits

Reviewing files that changed from the base of the PR and between dbf48ba and f597c77.

📒 Files selected for processing (5)

• test/advisor-session-context-tools.test.ts
• test/advisor-session-runner.test.ts
• tools/advisors/session.mts
• tools/advisors/turn-protocol.mts
• tools/pr-review-advisor/README.md

Included review availability: Your plan provides up to 12 included reviews per hour; 9 remain after this review.

---

📝 Walkthrough

Walkthrough

The advisor now accepts one failed terminal-submit attempt followed by one successful same-turn repair. The protocol rejects malformed, unsettled, repeated, overlapping, extra, and post-success activity. Session state, tests, and workflow documentation reflect the updated contract.

Changes

Terminal-submit repair

Layer / File(s)	Summary
Terminal-submit sequence validation tools/advisors/turn-protocol.mts	Validation tracks terminal-submit attempts and accepts either one success or one failed attempt followed by one successful repair. Invalid or post-success activity fails validation.
Session repair-state integration tools/advisors/session.mts	terminalSubmitRepaired is derived from hasCompletedTerminalSubmitRepair.
Repair contract and regression coverage test/advisor-session-context-tools.test.ts, test/advisor-session-runner.test.ts, tools/pr-review-advisor/README.md	Tests cover valid and invalid repair sequences. Documentation describes bounded retry handling and atomic pending-state commit.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to f597c

This localized repair enables valid same-turn submit recovery while retaining fail-closed validation; no actionable merge-blocking risk remains after normal checks and review.

Sequence Diagram(s)

sequenceDiagram
  participant AdvisorSession
  participant TurnProtocol
  participant TerminalSubmitEvents
  AdvisorSession->>TurnProtocol: evaluate current turn
  TurnProtocol->>TerminalSubmitEvents: inspect submit starts and completions
  TerminalSubmitEvents-->>TurnProtocol: return settled outcomes
  TurnProtocol-->>AdvisorSession: report repair completion

Loading

Possibly related PRs

• NVIDIA/NemoClaw#9590: Introduced the advisor terminal-submit repair protocol refined by this change.

Suggested labels: v0.0.112

Suggested reviewers: apurvvkumaria, ericksoa

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the primary change: accepting a same-turn terminal submit repair.
Linked Issues check	✅ Passed	The changes implement the bounded failed-then-successful repair and retain rejection for invalid sequences required by issue #9630.
Out of Scope Changes check	✅ Passed	The protocol, session logic, tests, and documentation changes directly support the objectives in issue #9630.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/fix-9630-advisor-submit-repair

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

PR Review Advisor — Informational

Advisor assessment: Informational / low confidence
Next action: No advisor follow-up needed.
Findings: 0 blockers · 0 warnings · 0 suggestions
Status: PR review advisor failed: PR review advisor SDK execution failed: session: challenge-and-record must make exactly 1 submit_review submit attempt(s), with 0 failed and 1 successful completion (observed 2 starts, 1 successful, and 1 failed completions); turn: challenge-and-record: challenge-and-record must make exactly 1 submit_review submit attempt(s), with 0 failed and 1 successful completion (observed 2 starts, 1 successful, and 1 failed completions)

Model lanes

• GPT-5.6 Terra (primary): Failed
• Nemotron 3 Ultra (second opinion): Failed

Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests.

E2E guidance

Advisory only. A maintainer can dispatch the default E2E suite for the commit under review.

Recommended E2E: None

Workflow run details

This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge.

[github-code-quality]

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall line coverage in commit f597c77 in the codex/fix-9630-advis... branch remains at 96%, unchanged from commit dbf48ba in the main branch.

TypeScript / code-coverage/cli

The overall line coverage in commit f597c77 in the codex/fix-9630-advis... branch is 83%. The line coverage in commit 772ce00 in the main branch is 82%.

Show a line coverage summary of the most impacted files.

File	main 772ce00	codex/fix-9630-advis... f597c77	+/-
src/lib/onboard...file-builder.ts	95%	91%	-4%
src/lib/onboard...al-inference.ts	80%	84%	+4%
src/lib/onboard...nt-authority.ts	75%	79%	+4%
src/lib/shields...ate-mutation.ts	70%	78%	+8%
src/lib/inferen...time-adapter.ts	53%	62%	+9%
src/lib/inferen...apter-common.ts	61%	91%	+30%
src/lib/inferen...er-lifecycle.ts	6%	41%	+35%
src/lib/onboard...ma-authority.ts	0%	74%	+74%
src/lib/onboard...-transaction.ts	0%	79%	+79%
src/lib/onboard...ma-inference.ts	0%	79%	+79%

---

_{Updated August 19, 2026 18:32 UTC}

[udsy19]

Outside contributor here, not a maintainer — offering evidence rather than a decision.

What holds up

I imported tools/advisors/turn-protocol.mts at this head (f597c7743) directly under node — the module has no imports, so it loads unmodified with type stripping — and drove hasCompletedTerminalSubmitRepair, repairableTerminalSubmitToolName, advisorTurnFlowErrors and terminalSubmitRepairErrors with the real challenge-and-record tool configuration (activeToolNames = read/grep/find/ls + the four recording tools, terminalSubmitRepairToolNames = the four recording tools).

Every fail-closed case in the description reproduces:

flow	hasCompletedTerminalSubmitRepair	accepted by advisorTurnFlowErrors
start, end(err), start, end(ok)	true	yes
start, end(err), start, end(err)	false	no
start, end(ok), start, end(err)	false	no
start, start, end(err), end(ok) (overlap)	false	no
start, end(err), start, end(ok), text	false	no

The double-repair concern I went in expecting does not exist. When the same-response repair is recognised, repairableTerminalSubmitToolName cannot also fire: it requires counts.starts === 1 && counts.completions === 1 and !successfulToolNames.has(toolName), and both are false once two attempts have settled with one success. So the continuation prompt is not additionally issued, which matches the new runner assertion that the transcript contains no terminal_submit_repair_start.

Two more things I checked and liked:

• The change removes ["failed then successful initial attempts", …] from the rejection table in advisor-session-context-tools.test.ts rather than leaving a case that now contradicts the implementation. That inversion is the honest edit, and it is what makes the new positive case meaningful.
• CONTRIBUTING.md:524 asks for the escaped-defect narrative "in the issue or pull-request narrative", and the fourth Changes bullet supplies exactly that — the controller recognised repair only after a separate continuation prompt, and the existing test encoded the valid sequence as an expected rejection. That is the part reviewers most often skip.

The one substantive thing I found: the two repair routes accept different contracts

terminalSubmitRepaired can now become true by two routes. The continuation route validates its repair flow through terminalSubmitRepairErrors, which rejects prose and rejects any tool outside terminalSubmitRepairToolNames. The new same-response route reaches the same true through terminalSubmitAttemptSequence, which only inspects events while a submit call is open — so anything that happens between the settled failed attempt and the successful one is invisible to it.

Executed, same module, same tool configuration, varying only what sits between the two submit attempts:

interleaved between the failed and the successful submit	same-response route	continuation route
nothing	accepted	accepted
assistant prose	accepted	rejected — emitted prose during repair
record_findings	accepted	accepted
read	accepted	rejected — called unexpected tool read

The doc comment updated in this diff describes them as one rule: "The turn permits one success, or one failed attempt followed by one success when repair is enabled." On the evidence above the same-response half of that sentence admits prose and repository reads that the continuation half does not.

I want to bound this honestly, because I think it is smaller than it first looks:

• The same-response route cannot grant a tool the turn had not already activated. read/grep/find/ls were live for challenge-and-record anyway, so nothing new becomes reachable — what differs is only what the validator accepts after the fact.
• The continuation route narrows the tool set deliberately (session.setActiveToolsByName([...terminalSubmitRepairToolNames, submitRepairToolName]) plus contextTools.deactivate()). That narrowing is a property of the continuation, not of the repair contract, so it is arguable that the same-response route was never meant to reproduce it.
• Nothing here weakens the commit boundary. Pending state still finalises only after the full flow validates, and every malformed / unsettled / extra-attempt / post-success shape stays rejected, as the first table shows.

So I would not call it a live failure. It is a contract difference between two paths that set the same flag, and the new rejection table (overlap, unsettled, all-failed, third attempt, activity-after-success) is the natural place it would be pinned — a row such as [ledgerStart, ledgerFailure, analysisEvent, ledgerStart, ledgerSuccess] would either lock the current permissiveness in deliberately or surface it. Either outcome is better than leaving it implicit.

Cross-PR overlap worth flagging: open PR #9645

tools/pr-review-advisor/README.md is edited by both this PR and open PR #9645, which links no issue, so issue-keyed duplicate detection cannot see the pair. I ran the overlap with a staleness control rather than comparing the two heads directly:

git merge-tree --write-tree <9652 head> <main>   -> clean
git merge-tree --write-tree <9645 head> <main>   -> CONFLICT tools/pr-review-advisor/trusted-guidance.mts   (only)
git merge-tree --write-tree <9652 head> <9645 head>
                                                 -> CONFLICT tools/pr-review-advisor/README.md

The README conflict appears only in the pair, so it is a genuine overlap and not either branch being behind main.

The textual resolution looks mechanical — this PR rewrites numbered item 7 of the protocol list, #9645 rewrites items 5 and 8 and drops a trailing line. They are disjoint list items. The part that is not mechanical is that both rewrite prose describing submit_review: this PR describes its attempt protocol, #9645 rewrites the recommendation-derivation paragraph further down the same file. Whoever lands second is worth asking to re-read the merged section as a whole rather than accepting a hunk-level resolution. The runtimes do not overlap at all — #9645 touches nothing under tools/advisors/.

Limits

I did not run the repository test suite, npm run typecheck, npm run build:cli, or CI, and I did not run an advisor session against a live provider. There is no node_modules in my checkout and I installed nothing. Both tables above come from importing this PR's own turn-protocol.mts and calling its exported functions with hand-built AdvisorTurnFlowEvent arrays — the real module, but not the repository's test harness, and not the SDK. I have not verified the 55/55 and 423/423 pass counts in the description.

[prekshivyas]

Reviewed commit f597c774367b69cfaaf66bd0de1df82ee77788b3 against base dbf48bae9d35beda8d781205a46e881d6f8f900a.

The controller accepts only a settled failed, successful submit sequence when repair is configured and no provider error exists. It rejects overlaps, unsettled calls, reversed or extra outcomes, and any activity after success. Pending review state still becomes canonical only after the complete turn passes validation; failed turns discard it. I found no blocking issue.

Test evidence: focused protocol/session tests passed 55/55; the complete Advisor test set passed 424/424; git diff --check passed. The two Advisor workflow lanes fail on the exact pre-fix base-commit controller behavior addressed here.

Security review:

• Secrets and credentials: PASS — no credential handling changes.
• Input validation: PASS — terminal events are validated for settlement, ordering, cardinality, and post-success activity.
• Authentication and authorization: PASS — no permission or identity changes.
• Dependencies: PASS — no dependency changes.
• Error handling and logging: PASS — provider errors and malformed flows remain fail-closed.
• Cryptography: PASS — no cryptographic changes.
• Configuration and deployment: PASS — no configuration or deployment changes.
• Testing and coverage: PASS — valid same-turn repair plus malformed, unsettled, repeated, overlapping, all-failed, and post-success cases are covered.
• System-level safety: PASS — canonical review state is committed only after full turn validation and is discarded on failure.