fix(#6932): move seenResult flag after successful unmarshal

[fullsend-ai-coder]

Summary

Move seenResult = true from before json.Unmarshal to after the successful unmarshal in parseClaudeStream. This fixes a flag-before-validation ordering bug where a failed result event unmarshal would suppress the deferred fallback TokensEvent, producing zero tokens instead of best-effort cumulative tokens.

Related Issue

Closes #6932

Changes

• Move seenResult = true to after the successful json.Unmarshal call in the case "result" branch of parseClaudeStream (internal/runtime/claude_progress.go)
• Add TestParseClaudeStreamMalformedResultFallsBackToTokensEvent to verify the deferred TokensEvent fires when the result event has valid outer JSON but malformed inner fields

Testing

• New test TestParseClaudeStreamMalformedResultFallsBackToTokensEvent passes, confirming deferred emission fires on unmarshal failure
• All existing parseClaudeStream and progressParser tests pass (31 tests)
• TestProgressParserResultOverwritesIncrementalTokens still confirms valid result events suppress deferred emission
• go vet, gofmt, go build pass
• Package coverage at 87.9% (above 80% threshold)

---

Closes #6932

Post-script verification

• Branch is not main/master (agent/6932-move-seenresult-after-unmarshal)
• Secret scan passed (gitleaks — 8a593ad7b64875c0c4016f8f3bd151cc6c83df8a..HEAD)
• PR body secret scan passed (gitleaks — no-git)

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:04 PM UTC · Completed 10:14 PM UTC

Commit: 2263842 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $2.50

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

Risk Assessment: low (1/5)

Details

Re-review: Tier 1 signals (2 files, 66 lines, no protected/dependency paths, bot author, 0.50 test ratio) are unchanged from the prior assessment, and the PR's own files are byte-identical to the prior head SHA — the only intervening change is an unrelated merge commit from main. Tier 2 churn on claude_progress.go remains elevated but was already priced into the prior score, so the composite anchors to the prior score of 1 (low).

Previous run

Risk Assessment: low (1/5)

Details

Re-review: production file unchanged since prior assessment; only the test file was rewritten to fix the test-adequacy finding, so Tier 1 signals (file count, line count, no protected/dependency paths, bot author) are unchanged from the prior run. Tier 2 churn history on claude_progress.go is unchanged and was already priced into the prior score. Composite anchors to the prior score of 1 (low).

Previous run (2)

Risk Assessment: low (1/5)

Details

Minimal 2-file change (1 production line + 55 test lines) with excellent test ratio, no protected paths, no dependencies, bot author. Tier 2 slightly elevated by high regression history on claude_progress.go but offset by very recent code age and no reverts. Composite rounds to 1 (low).

[fullsend-ai-review]

Looks good to me

Previous run

Looks good to me

Previous run (2)

Review

The production fix itself is correct: moving seenResult = true to after the successful json.Unmarshal in the case "result" branch matches the linked issue's (#6932) proposed change exactly, and no scope creep is present.

Findings

Medium

• [test-adequacy] internal/runtime/claude_progress_test.go:1338 — TestParseClaudeStreamMalformedResultFallsBackToTokensEvent does not actually exercise the deferred TokensEvent path it claims to lock in. The fixture totals 3000+1600+400+100=5100, which is >= tokenThreshold (5000, claude_progress.go:19), so the message_delta handler already emits an incremental TokensEvent and sets lastEmittedTotal to 5100. The EOF/deferred fallback only fires when total > lastEmittedTotal; 5100 > 5100 is false, so the deferred event never fires — even with this PR's fix applied. The test's assertions (len(tokens) == 0 fails, then checks the last event's fields) are satisfied by the incremental event alone. Reverting seenResult = true back to before the unmarshal (i.e., reintroducing the Move seenResult flag after successful result event unmarshal #6932 bug) would still pass this test unchanged, because the incremental event fires regardless of seenResult's value or timing. This means the new regression test does not guard against the regression it was written for.
  Remediation: Drop the fixture's cumulative total below tokenThreshold (mirror TestParseClaudeStreamFinalTokensEventOnCancel, which deliberately uses 2650 to stay under threshold — e.g. use input/output/cache values summing to well under 5000), so no incremental TokensEvent fires during message_delta. Then assert that exactly one TokensEvent is emitted (the deferred one) with the expected cumulative values. That is the only fixture shape that fails when seenResult is set before a successful unmarshal.

---

Next steps:

• /fs-fix — agent addresses review findings automatically
• /fs-fix <your instruction> — agent fixes with your specific guidance
• Push commits directly — review re-runs automatically on push
• /fs-fix-stop — disable automatic fix runs for this PR

Previous run (3)

Looks good to me

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:48 AM UTC · Completed 12:05 PM UTC

Commit: 6a4c3f1 · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $2.00

Superseded by updated review

[fullsend-ai-coder]

🤖 Finished Fix · ✅ Success · Started 12:07 PM UTC · Completed 12:16 PM UTC

Commit: 6a4c3f1 · View workflow run →

Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $0.81

[fullsend-ai-coder]

🔧 Fix agent — iteration 1 (bot-triggered)

Fixed the single medium-severity test-adequacy finding: the new regression test's fixture total crossed tokenThreshold, causing an incremental TokensEvent to mask the deferred-event assertion, so the test passed even with the #6932 bug reintroduced. Lowered the fixture below threshold and tightened the assertion to require exactly one (deferred) TokensEvent. Verified locally by reverting the production fix and confirming the test now fails as expected, then restored the fix and confirmed the test (and full package suite) pass.

Fixed (1):

1. TestParseClaudeStreamMalformedResultFallsBackToTokensEvent does not actually exercise the deferred TokensEvent path (test-adequacy, claude_progress_test.go:1338) (internal/runtime/claude_progress_test.go): Verified the finding: the fixture's cumulative total (3000+1600+400+100=5100) was >= tokenThreshold (5000), so the message_delta handler already emitted an incremental TokensEvent and set lastEmittedTotal=5100, making the deferred EOF fallback (total > lastEmittedTotal) never fire (5100 > 5100 is false). Confirmed by temporarily reverting the production fix (moving seenResult=true back before the unmarshal): the old test still passed, proving it didn't guard the regression. Lowered the fixture to 1000/300/200/50 (total 1550, well under tokenThreshold), mirroring TestParseClaudeStreamFinalTokensEventOnCancel, and changed the assertion from len(tokens) == 0 to len(tokens) != 1 with values checked against the single deferred event. Re-verified against the reverted (buggy) code: the updated test now correctly fails (0 TokensEvents observed instead of 1), and passes against the actual fix.

Tests: passed

Decision points

• Chose new fixture token values (1000/300/200/50, cumulative 1550) instead of just lowering the existing values slightly (alternatives: Scale down proportionally from the original 3000/1600/400/100, Reuse the exact values from TestParseClaudeStreamFinalTokensEventOnCancel (2000/500/100/50); rationale: Used distinct round numbers clearly under tokenThreshold (5000) and distinguishable from the existing cancel-on-EOF test's fixture, while keeping the same well-under-threshold pattern the reviewer's remediation asked for.)

---

Next steps:

• /fs-review — request a re-review of the changes
• /fs-fix <your instruction> — run another fix pass with specific guidance
• Push commits directly — review re-runs automatically on push
  _{Updated by fullsend fix agent}

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:18 PM UTC · Completed 12:30 PM UTC

Commit: 8464597 · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $1.04

Superseded by updated review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:19 AM UTC · Completed 7:29 AM UTC

Commit: e1bc61c · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $1.28

Superseded by updated review

[maruiz93]

/fs-fix fix test-adequance issue

[fullsend-ai-coder]

🤖 Finished Fix · ✅ Success · Started 7:47 AM UTC · Completed 7:52 AM UTC

Commit: e1bc61c · View workflow run →

Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $0.31

[fullsend-ai-coder]

🔧 Fix agent — iteration 2 (human-triggered)

Investigated the human instruction to fix the 'test-adequacy' finding on PR #6935. Found it was already resolved in a prior fix iteration: TestParseClaudeStreamMalformedResultFallsBackToTokensEvent now uses a fixture total (1550) below tokenThreshold and asserts exactly one deferred TokensEvent. Verified correctness by reverting the production fix locally and confirming the test fails as expected, then restoring it and confirming all internal/runtime tests pass. No further code changes were required.

Disagreed (1):

1. fix test-adequance issue: Verified against the current code: the test-adequacy finding (TestParseClaudeStreamMalformedResultFallsBackToTokensEvent not exercising the deferred TokensEvent path) was already fixed in fix iteration 1 (commit 6a4c3f1). The fixture now totals 1550 tokens, well under tokenThreshold (5000), so no incremental TokensEvent masks the assertion, and the test asserts exactly one TokensEvent. I confirmed by temporarily reverting the production fix (moving seenResult=true back before json.Unmarshal): the test correctly fails (0 TokensEvents observed) against the buggy code and passes against the current fix. The latest review comment on this PR (head SHA e1bc61c) already reads 'Looks good to me' with no outstanding findings. No code change was needed; working tree is clean.

Tests: passed

Decision points

• No code change made in this run (alternatives: Re-modify the test fixture again, Verify existing fix and leave as-is; rationale: The test-adequacy issue referenced by the human instruction was already resolved in a prior fix iteration and independently verified here by reproducing the original bug and confirming the test catches it; re-touching already-correct code would be out of scope and risk regressing a verified-correct test.)

---

Next steps:

• /fs-review — request a re-review of the changes
• /fs-fix <your instruction> — run another fix pass with specific guidance
• Push commits directly — review re-runs automatically on push
  _{Updated by fullsend fix agent}

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 9:19 AM UTC · Completed 9:28 AM UTC

Commit: e1bc61c · View workflow run →

Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $1.73

[fullsend-ai-retro]

PR #6935 (closes #6932) was a correct, minimal one-line fix to parseClaudeStream plus a regression test, and the final merged state is sound. The notable finding is a review-quality delta: the initial review run (2026-09-02, claude runtime/opus, high effort, $2.50) approved the PR ('Looks good to me') even though the new regression test's fixture totaled 5100 tokens — at/above the tokenThreshold constant (5000) — which meant an earlier code path already satisfied the test's assertions regardless of the fix, so the test could not actually catch a reintroduction of the #6932 bug. A second review run on 2026-09-14, on a byte-identical diff (confirmed by the PR's own risk-assessment comment), caught this exact issue at Medium severity. Between the two runs, an org-level config change (fullsend-ai/fullsend PR #7116, commit 80dcbef, merged 2026-09-08) had switched this repo's review (and code/prioritize) agents from the default claude runtime to pi, with correctness/security sub-agents routed to xai/grok-4.6. So the same diff was approved by one runtime/config and flagged by another — a clean natural experiment that PR #7116 merged without any committed follow-up evaluation. The fix agent resolved the finding correctly and cheaply ($0.81), verified by reverting-and-restoring the production fix locally. A human (maruiz93) approved before the finding surfaced and, ~19h after it had already been fixed and re-reviewed clean twice, issued a redundant /fs-fix — the fix agent handled this gracefully (declined, no code change, $0.31), so this looks like a one-off, not a pattern worth its own proposal. On the test-adequacy failure mode itself: this is evidence for, not against, several already-open issues — fullsend-ai/agents#1273 ('fix/code agent writes regression-test mocks that don't reproduce the actual bug precondition'), #5330 ('review agent test-adequacy check should verify tests exercise changed code'), fullsend-ai/agents#1113 (fixture test-path specificity), and fullsend-ai/agents#1140 (review agent inconsistent verdicts across runs) — none of which specifically examine cross-runtime/cross-config comparisons, which is the angle this retro proposes capturing. One proposal below: log this before/after runtime-migration data point in the repo's existing review-autonomy-evidence corpus and add an explicit re-evaluation checkpoint, since #7116 changed a load-bearing config with no measurement plan.

Proposals filed

• Capture PR #6935 as runtime-migration evidence in review-autonomy-evidence.md and add a re-evaluation checkpoint for PR #7116 (in fullsend-ai/fullsend)