Add deterministic completion policy - #719
Conversation
Extract completion decisions from the agent loop into a feature-gated typed policy with complete, incomplete, and uncertain outcomes. Preserve bounded plan-stall nudges and allow at most one semantic acceptance check for self-correcting runs. Tested: make build Tested: make test Tested: go fmt ./... Tested: go vet ./... Tested: make lint Tested: govulncheck ./... Note: repository-wide pinned golangci-lint reports 36 pre-existing unrelated findings; internal/agent/... reports 0 issues.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughAdds a deterministic completion policy with bounded continuation nudges and optional one-time semantic checking, integrates it into the headless completion gate, adds focused tests, and documents its interaction with ChangesCompletion policy
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Run
participant guards
participant completionPolicy
Run->>guards: pendingPlanItems()
Run->>completionPolicy: evaluate(collected.Text, planPending)
completionPolicy-->>Run: completionEvaluation
Run->>Run: continue, semantic-check, or finalize
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
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. |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Clean PR9. This is a faithful extraction — I traced every branch of the new completionPolicy.evaluate against the old inline logic and they line up exactly: a self-reported admission → Incomplete (checked first, so an admitted-impossible task isn't wasted on continue-nudges), a continuation cue / pending plan → bounded continue nudge, then cue→Incomplete while plan-only falls through, SelfCorrect → exactly one task-grounded semantic check, else → Complete. The bounded state (continueNudges, semanticCheckRequested) moves out of loop-locals into a policy created once per run, so it persists across turns identically, and everything stays behind RequireCompletionSignal so default/interactive runs are byte-identical.
The acceptance criteria hold up: the existing completion/plan-stall regression corpus passes unchanged (TestCompletionGate*, TestAcceptance*, TestPendingPlanAloneDoesNotForceIncomplete, TestMaxTurnsCutoffIsIncompleteUnderGate), and the new policy tests cover the decision surface — local-evidence-decides-without-a-semantic-check, bounded plan-stall protection, pending-plan-as-weak-evidence, and exactly-one-required-semantic-check. go build/vet/gofmt clean and the full agent suite is green for me (only the known Windows temp-path sandbox tests fail locally, unrelated). It's a genuine improvement too — the loop sheds ~40 net lines and the completion logic is now typed and unit-testable in isolation.
One tiny take-it-or-leave-it note: evaluate never returns Uncertain with completionActionNone, so the loop's inner action switch always matches today — but if that ever changed, the continue would re-loop with no new message appended. A default arm (or an explicit assert) would make that impossible to regress. Not blocking.
Approving. The only thing still outstanding is the Windows smoke check finishing — the macOS/Ubuntu smokes are green and my local Windows build+tests pass, so I expect it to go green; over to kevin once it does.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Reviewed the completion-policy extraction against the previous inline state machine. Bounded continuation handling, stale-plan behavior, self-reported incompletion, the one-time self-correct acceptance check, and default interactive behavior are preserved. Focused tests, build, vet, lint, and all CI checks pass. No blocking findings.
3 participants
Summary
Verification
The repository-wide pinned golangci-lint command still reports 36 pre-existing unrelated findings, including findings from the old /tmp/zero-pr717-head checkout.
Summary by CodeRabbit
New Features
Documentation
Tests