Add transcript-grounded structured task state - #761
Conversation
Project existing plan, tool-result, changed-file, verification, and completion events into a deterministic per-run snapshot. Emit content-free aggregate snapshots to traces. Carry bounded objective context through compaction and ground completion checks only when the projection matches the transcript; mismatches retain transcript-derived behavior and drop stale carried state. Tested: make fmt-check Tested: go vet ./... Tested: go test ./... Tested: go test -race ./internal/agent ./internal/trace Tested: go run ./cmd/zero-release build Tested: go run ./cmd/zero-release smoke Tested: govulncheck ./... (no vulnerabilities found) Advisory: pinned golangci-lint reports only unrelated pre-existing findings Tested: git diff HEAD --check
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
REQUEST CHANGES — one real defect, small fix. Everything else is solid.
The projection is well-built: deterministic, side-effect-free on the loop, nil-safe throughout, and genuinely additive. My one blocking issue is that the headline compaction behavior stops working in exactly the scenario it exists for. Details below, then the things I checked that are fine.
[Major] The objective is dropped at the second compaction
compareTranscript treats a plan mismatch as a whole-state mismatch:
if !valid || found != state.planObserved || (found && !reflect.DeepEqual(transcriptPlan, tracked)) {
parity = taskStateParityMismatch
}latestTaskPlan scans messages[i].ToolCalls for planToolName. But compaction elides the middle of the transcript, which is where that tool call lives — extractLatestPlan(middle) exists precisely because the plan is being removed and has to be re-preserved as text. So after the first compaction, found is false while planObserved stays true, and parity flips to mismatch.
That alone would be harmless, but appendPreservedState also drops the carried copy:
task := priorState.Task
if taskStateChecked { task = nil } // taskStateChecked is always true in Run
if taskSnapshot != nil { task = ... } // nil when parity mismatchedSo on compaction #2, taskSnapshot is nil and the previously preserved task block is deleted rather than carried. Reproduced:
compaction #1: snapshot=true objective preserved=true
compaction #2: snapshot=false objective preserved=false
>>> OBJECTIVE LOST at compaction #2
Parity does recover once the model issues a fresh update_plan (I checked — it returns to match), so this is a window rather than a permanent state. But the window is unbounded: nothing forces the model to re-plan, and a run that compacts twice without an intervening plan update loses the objective entirely. That's the long-running case the feature is built for, and it fails silently — the trace still says parity=mismatch but nothing tells you the objective went missing.
The fix is small. The objective is captured once from the run's own prompt (newTaskState(prompt, …)) and is immutable for the lifetime of the run — it cannot be stale relative to the transcript, unlike the plan and verification counts. Gating it on plan parity is over-broad. Preserve Objective unconditionally and gate only the mutable fields (Status, plan counts, verification counts) on parity. That keeps the stated safety property — no stale structured state — while making the objective actually survive.
I'd also suggest taskStateParityMismatch not be the trigger for deleting a carried block that a previous, matching compaction wrote. "I can't corroborate the plan right now" is weaker evidence than "the carried objective is wrong."
Minor
- Parity is plan-only, but named and documented as whole-state.
compareTranscriptcomparesPlan.Itemsand nothing else, yettaskStateParityMatchand the PR body's "use structured state only when it matches the transcript" read as though the whole projection is corroborated. Today the exposure is small (preservedTaskStateonly carries objective, status, and counts), but the naming invites a future field to be added to the snapshot and silently assumed verified. ConsiderplanParityMatch, or a comment stating exactly what is and isn't compared. parseTaskPlanis more lenient than the tool it now mirrors. It accepts astepalias and silently skips empty-content items;update_plan'sparsePlanItemsrequirescontentviastringArg(..., true)and hard-rejects the entire call on a bad item. So guardrails and TaskState can track a plan the tool refused to store. The underlying shape (observing arguments rather than tool results) predates this PR, but consolidating onto one parser is the moment to make the two agree — or to note inparseTaskPlan's doc that it is deliberately laxer and why.TaskStatesgrows per tool call, not per turn.observe→emitfires on every tool result, so this slice is O(tool calls) whilePrefixHashesand friends are O(turns), and there's no cap. A few hundred tool calls is a few hundred KB of extra NDJSON. Worth a cap, or emitting only on plan/verification/completion transitions and letting tool counters ride along with the next event.- Variadic optional dependencies.
newTaskState(objective, recorders ...*trace.Recorder)andnewCompactionState(options, tasks ...*taskState)silently ignore everything past the first argument. A plain nilable parameter says the same thing, can't be mis-called with two, and doesn't need thelen(...) > 0dance. compareTranscriptreads like a query but mutates. It bumpsRevisionand emits a trace event, and it's called from bothcompletionContextandsnapshotForCompaction— so a single completion check can bump the revision. Worth a name that signals the write, or a note on the method.- Nit on the privacy claim.
ObjectiveHashis a bare SHA-256 of the objective. That's not reversible, but it is confirmable: objectives are low-entropy and often short ("fix the login bug"), so anyone with the trace and a candidate list can test guesses. If "content-free" is meant strictly, an HMAC with a per-run random key gives you the same within-run drift detection without the confirmation oracle.
Checked and fine
planStatusRemainingconsolidation is behavior-preserving. I compared the old inline switch againstnormalizeTaskPlanStatusterm by term — the "not remaining" set is exactlycompleted|failedunder both, and every other vocabulary word lands onpending/in_progressas before.- Routing
observePlanUpdateandformatPlanArgumentsthroughparseTaskPlanis a genuine fix, not just a refactor. Both previously parsed raw arguments without theenforceSingleInProgresscoercion theupdate_plantool applies, so the guardrail's pending count and the post-compaction plan text could disagree with the plan the tool actually stored. They now agree, andparseTaskPlan's coercion matchesenforceSingleInProgressexactly (all but the lastin_progress→completed). Good catch to fold these together. capTaskObjectiveis UTF-8 safe — backs up withutf8.RuneStartbefore slicing, so the 512-byte cap can't split a rune.- The new
deferis sound.resultis a named return, so the deferredcompareTranscript(result.Messages)observes the final value.EmitTaskStateis nil-safe, mutex-guarded, and no-ops oncer.finishedis set, so it cannot write into a sealed recorder no matter how the defers interleave. - The parity fallback on the completion path is safe. When parity is mismatch,
completionContextfalls back toguards.pendingPlanItems(), andguardStateisn't reset by compaction, so the transcript-derived signal survives. Verified the flag flips exactly as documented. - Trace payload matches the stated scope — counts, statuses, and an objective hash; no plan text, tool output, or file paths, and only
len(ChangedFiles).
Verification
gofmt clean, go vet clean. internal/trace fully green including -race. internal/agent passes under -race -count=2 for the task-state, compaction-preserve, and completion-policy suites.
One internal/agent failure locally — TestRequestPermissionsTurnGrantAllowsLaterToolAndCleansUp — is a sandbox artifact of my environment, not this PR: it fails identically on this PR's exact base (2e267bd), which I checked out separately to confirm rather than assuming.
Nice piece of work overall — the plan-normalization consolidation is a real correctness win independent of the new feature. Fix the objective-preservation gating and I'm happy.
WalkthroughThe agent now maintains deterministic task state from plans, tools, verification, and completion events; preserves task context during compaction; and records content-free task-state snapshots in traces with NDJSON and text output support. ChangesTask state lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Run
participant taskState
participant Compaction
participant Recorder
Run->>taskState: observe plan, tool, verification, and completion events
Run->>taskState: observePlanParity(messages)
taskState->>Compaction: snapshotForCompaction(messages)
Compaction->>Run: append preserved task state
taskState->>Recorder: EmitTaskState(snapshot)
Recorder->>Run: return task-state trace entries
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/agent/loop.go (1)
578-628: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTask state can report "complete" for a turn that isn't actually final.
task.observe(..., CompletionComplete)fires at Line 581 (RequireCompletionSignal path) and Line 614 (gate-off path) before the sharedpostEditDiagnostics.drainFinalcheck at Lines 622-628, which can stillcontinuethe loop instead of returning. Between those two points, the emittedTaskStateEventtrace showsstatus: complete, only to flip back toactiveon the next turn's plan/tool/verification event.Resultitself is unaffected (it's set only after the gate), so this is purely an observability-accuracy gap in the new trace contract, but it's easy to make exact by deferring the "complete" observation past the diagnostics gate.🔧 Sketch of the fix
if options.RequireCompletionSignal { completionContext := task.completionContext(messages, guards.pendingPlanItems()) evaluation := completionPolicy.evaluate(collected.Text, completionContext) - task.observe(taskStateEvent{kind: taskStateEventCompletion, completion: evaluation}) switch evaluation.Decision { case CompletionIncomplete: + task.observe(taskStateEvent{kind: taskStateEventCompletion, completion: evaluation}) result.Incomplete = true result.IncompleteReason = evaluation.Reason result.FinalAnswer = collected.Text result.Messages = copyMessages(messages) return result, nil case CompletionUncertain: + task.observe(taskStateEvent{kind: taskStateEventCompletion, completion: evaluation}) posture.observeUncertain() ... continue case CompletionComplete: - // Local evidence is sufficient; proceed to final diagnostics. + // Deferred: recorded as complete only once past the diagnostics gate below. } - } else { - task.observe(taskStateEvent{kind: taskStateEventCompletion, completion: completionEvaluation{Decision: CompletionComplete}}) } ... if nudge := postEditDiagnostics.drainFinal(ctx); nudge != "" { messages = append(...) continue } + task.observe(taskStateEvent{kind: taskStateEventCompletion, completion: completionEvaluation{Decision: CompletionComplete}}) result.FinalAnswer = collected.Text🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agent/loop.go` around lines 578 - 628, Defer the taskStateEvent completion observation in the RequireCompletionSignal and gate-off branches until after postEditDiagnostics.drainFinal returns no nudge. Preserve the existing incomplete and uncertain handling, and emit CompletionComplete only immediately before the actual final result return so task state does not report completion when diagnostics cause another loop iteration.
🧹 Nitpick comments (2)
internal/agent/task_state.go (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueVariadic pointer param used to fake an optional argument. Both
newTaskStateandnewCompactionStatetake...*Tsolely to make one pointer argument optional, which silently swallows any extra args and is non-idiomatic Go for "optional" parameters.
internal/agent/task_state.go#L97-110: changenewTaskState(objective string, recorders ...*trace.Recorder)tonewTaskState(objective string, recorder *trace.Recorder)and have callers passnilexplicitly (loop.go'snewTaskState(prompt, options.Trace)already works either way; tests would neednewTaskState("...", nil)).internal/agent/compaction.go#L377-388: changenewCompactionState(options Options, tasks ...*taskState)tonewCompactionState(options Options, task *taskState)for the same reason.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agent/task_state.go` at line 1, Replace the variadic optional parameters in newTaskState and newCompactionState with single pointer parameters, *trace.Recorder and *taskState respectively. Update all call sites and tests to pass nil explicitly when no recorder or task is available, while preserving existing non-nil behavior.internal/agent/compaction_preserve_test.go (1)
63-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for the "no task wired, carry prior task forward" branch.
Both new tests cover
taskStateChecked=true(match and mismatch). Neither coverstaskStateChecked=falsewith a pre-existing carriedpreservedTaskState— the case where a laterCompactcall has no runningtaskStateat all and must pass through whatever was preserved earlier untouched (perappendPreservedState's own doc comment ontaskStateChecked). Worth a small regression test mirroringTestCompactDropsCarriedTaskContextAfterParityMismatchbut withtaskStateChecked: false, asserting the priorTaskblock survives.As per coding guidelines, "
**/*_test.go: ... add regression tests for behavior changes."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agent/compaction_preserve_test.go` around lines 63 - 113, Add a regression test alongside TestCompactDropsCarriedTaskContextAfterParityMismatch that supplies a prior preservedTaskState in the input, leaves CompactionOptions.taskStateChecked false with no taskState, and verifies the resulting preserved state retains the prior Task unchanged. Mirror the existing compaction setup and assertions while covering the no-task-wired carry-forward branch.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/agent/loop.go`:
- Around line 578-628: Defer the taskStateEvent completion observation in the
RequireCompletionSignal and gate-off branches until after
postEditDiagnostics.drainFinal returns no nudge. Preserve the existing
incomplete and uncertain handling, and emit CompletionComplete only immediately
before the actual final result return so task state does not report completion
when diagnostics cause another loop iteration.
---
Nitpick comments:
In `@internal/agent/compaction_preserve_test.go`:
- Around line 63-113: Add a regression test alongside
TestCompactDropsCarriedTaskContextAfterParityMismatch that supplies a prior
preservedTaskState in the input, leaves CompactionOptions.taskStateChecked false
with no taskState, and verifies the resulting preserved state retains the prior
Task unchanged. Mirror the existing compaction setup and assertions while
covering the no-task-wired carry-forward branch.
In `@internal/agent/task_state.go`:
- Line 1: Replace the variadic optional parameters in newTaskState and
newCompactionState with single pointer parameters, *trace.Recorder and
*taskState respectively. Update all call sites and tests to pass nil explicitly
when no recorder or task is available, while preserving existing non-nil
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6661b4f5-1f35-449e-84f8-132542c3c7de
📒 Files selected for processing (14)
internal/agent/compaction.gointernal/agent/compaction_preserve.gointernal/agent/compaction_preserve_test.gointernal/agent/completion_policy.gointernal/agent/completion_policy_test.gointernal/agent/guardrails.gointernal/agent/loop.gointernal/agent/task_state.gointernal/agent/task_state_test.gointernal/trace/emit.gointernal/trace/parse.gointernal/trace/recorder.gointernal/trace/task_state_test.gointernal/trace/trace.go
Keep the immutable run objective when plan calls have been elided, while admitting mutable task fields only when plan parity still matches the transcript. Make plan parsing match update_plan rejection semantics, coalesce and cap task-state trace events, rename parity to its plan-only scope, use explicit nilable dependencies, and remove confirmable objective fingerprints from traces. Tested: make fmt-check Tested: go vet ./... Tested: go test ./... Tested: go test -race ./internal/agent ./internal/trace Tested: go run ./cmd/zero-release build Tested: go run ./cmd/zero-release smoke Tested: govulncheck ./... (no vulnerabilities found) Advisory: pinned golangci-lint reports only unrelated pre-existing findings Tested: git diff HEAD --check
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
APPROVE. Re-reviewed at d9ed9c0. The major is fixed correctly — I re-ran my repro against the new head rather than reading the diff — and all six minors plus the nit are closed.
The major — verified fixed
The fix is the right one: Objective is now set unconditionally whenever a snapshot exists, and only the mutable fields are gated on parity.
task = &preservedTaskState{Objective: capTaskObjective(taskSnapshot.Objective)}
if taskSnapshot.PlanParity == taskPlanParityMatch {
task.Status = ...; task.Pending = ...; task.VerificationPassed = ...
}snapshotForCompaction no longer returns nil on mismatch (it records parity and returns the snapshot), and taskStateChecked is gone entirely — which removes the "delete the carried copy" path that was doing the damage. Re-ran the exact repro from last round:
compaction #1: parity=match objective preserved=true counts carried
compaction #2: parity=mismatch objective preserved=true
OK: objective preserved, mutable fields correctly withheld on mismatch
I also added the inverse assertion — that a matching parity still carries the plan counts — so the fix isn't just "preserve everything always." It passes. The distinction now lands exactly where it should: the immutable objective always survives, the corroboration-dependent counts don't.
Minors — all closed
- Parity scope naming. Renamed throughout:
taskStateParity→taskPlanParity,compareTranscript→observePlanParity,StateMatchesTranscript→PlanMatchesTranscript,TranscriptParity→PlanParity. The new doc comment states plainly that "objective, tool, and verification fields are not part of this comparison." That was the point — a future field can no longer be silently assumed corroborated. parseTaskPlanleniency. Now rejects empty content outright and drops thestepalias, so it matchesupdate_plan's own strictness instead of tracking plans the tool would have refused.TestParseTaskPlanRejectsArgumentsTheToolWouldRejectandTestFormatPlanArgumentsRejectsUnsupportedStepAliaspin both directions. Worth noting thelen(plan) == 0 → (nil, true)return is retained, so both sides of thereflect.DeepEqualparity check stay nil-typed and an empty plan can't produce a spurious mismatch — I checked that specifically, since the rewrite of this function was the place it could have crept in.- Trace growth. Tool results no longer emit (
event.kind != taskStateEventToolResult), with a finaltask.emit()in the defer so aggregate counts still land on an early return. Plus a hardmaxTaskStateEvents = 128cap that replaces the tail rather than the head, so the final aggregate is never the one dropped — the right end to sacrifice. - Variadic optional deps.
newTaskState(objective, recorder)andnewCompactionState(options, task)take plain parameters now. - Mutating query.
observePlanParitynames the write and documents that it mutates and emits. - Objective hash. Removed from the trace event entirely, and the type doc now says "confirmable content fingerprints are deliberately excluded." That's a cleaner resolution than the HMAC I suggested — if nothing consumes it, not emitting it is strictly better.
Verification
gofmt clean, go vet clean. internal/trace green including -race -count=2. internal/agent green under -race -count=2 for the task-state, compaction-preserve, completion-policy, and plan suites. Eight new tests landed, mapping 1:1 onto the findings.
Four internal/agent failures locally (TestRunPromptsAndAllowsOutsideWorkspaceWrite, TestRunSessionAllowsLaterOutsideWorkspaceWrite, TestRunAppliesSandboxEvenInUnsafeMode, TestRequestPermissionsTurnGrantAllowsLaterToolAndCleansUp) are sandbox artifacts of my environment — I ran all four against this PR's exact base (2e267bd) and they fail identically there.
Good turnaround. The objective/mutable split is a cleaner invariant than what was there before the review, not just a patch over the symptom. Merge is kevin's call per the program gate.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Reviewed against the program gates, with the two things I most wanted to be sure of on a task-state projection: that the trace stays content-free, and that grounding completion in the objective does not quietly change exit-code behavior. Both hold up.
Trace privacy is the strong part. TaskStateEvent carries only counts and enum strings, no objective text, no plan content, no tool output, no file paths, and deliberately not even an objective_hash, so a traced run cannot confirm a guessed objective. It is content-free by construction rather than by scrubbing, which is the right way to build this, and the round-trip test plus the bounded-growth cap (128 with tail replacement) back it up. Changed files are kept in memory for the count only and never emitted or persisted, and the objective that does land in the compaction block is the user's own prompt that already lives in the transcript, capped at 512 bytes with UTF-8-safe truncation, so nothing new leaks.
Completion behavior is preserved, not just claimed. The structured PlanPending is only trusted when observePlanParity says the projection still matches the transcript, and on a match Plan.Pending+Plan.InProgress is the same quantity the transcript path already computes via pendingPlanItems, so the decision is unchanged; on a mismatch it falls back to the transcript count. The only new prompt content is the objective appended to the acceptance-verification nudge, which is on the opt-in semantic-check path. So headless exit semantics are the same and interactive runs stay byte-identical.
The plan-parsing consolidation is the one change that could read as a regression but is actually a correctness fix. I checked parseTaskPlan against the real update_plan tool: the tool requires content (no step field) and its enforceSingleInProgress keeps only the last in_progress item, downgrading earlier ones to completed. The new shared parser matches both, where the old formatPlanArguments was looser than the tool and accepted the step alias. So dropping step and coercing multiple in_progress lines up compaction and guardrails with the tool's real semantics, and planStatusRemaining keeps its prior result.
Gates: content-free trace (4), no default-path behavior change with a transcript fallback (2), nothing touching permissions/sandbox (3) or one-result-per-call (5), and the event is opt-in and listed in OptionalEventKeys (10). One non-blocking micro-note: observePlanParity re-walks the message history for the latest plan on each completion check and each compaction; it early-exits from the tail so it is cheap in practice, just worth remembering if the completion path ever gets hot.
Deterministic projection, snapshots are copied so callers cannot mutate the source, tool results are coalesced so the trace does not balloon. Nice work, and it closes out the task-state stream. Approving.
|
Addressed in d9ed9c0.
Validation is green: formatting, vet, unrestricted full tests, focused race tests, build, smoke, and govulncheck. The advisory lint output remains limited to unrelated pre-existing findings. |
3 participants
Summary
Scope and safety
Validation
The pinned advisory golangci-lint command reports only unrelated pre-existing repository findings; no finding points to a file changed by this PR.