fix(goal): stop runaway loops and let the judge credit artifact evidence#344
Merged
Conversation
Goal mode could loop forever re-confirming a finished task. Three causes, all fixed here on top of the existing impossible-goal abandonment (#322): - The evaluator judged only execution-shaped evidence (test output, exit codes), so a docs/research goal whose deliverable is files-that-exist could never be marked met. The judge now accepts the requested artifact itself as evidence and is told to judge the condition exactly as written, without inventing stricter acceptance criteria. - The evidence window was the last 20 raw tool calls, so read-only churn on re-confirmation turns pushed the original writes out of view and the judge lost sight of the work that satisfied the goal. Replaced with summarizeToolCallsForGoal: dedupes repeats into counts, classifies state-changing actions vs read-only inspections, and keeps every distinct action while capping read noise — the deliverable stays visible no matter how long the agent loops. - Added a stall guard: when the evaluator returns the same not-met reason GOAL_STALL_LIMIT (3) turns running without converging, the loop clears the goal and spends one final turn reporting to the user instead of re-firing. Distinct from an impossible verdict, which still ends the loop immediately. Tests: 56 goal-module tests (incl. stall detection + summary) and 64 client tests pass; typecheck, eslint, prettier clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
👀 Quinn is reviewing — verdict (PASS / WARN / FAIL) + findings to follow. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughImplements goal stall detection: adds a tool-call summarizer, tracks repeated identical not-met evaluations in GoalManager, refines evaluator prompts, and makes the client clear and exit stalled goals with a final explanatory continuation message. ChangesGoal Stall Detection with Tool-Call Summarization
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/core/src/goal/GoalManager.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. Comment |
There was a problem hiding this comment.
QA Audit — PR #344 | fix(goal): stop runaway loops and let the judge credit artifact evidence
VERDICT: WARN (CI not yet terminal — re-review pending)
CI Status
- CodeQL: in_progress
- Lint: queued
Diff Review
Three interlocking fixes targeting goal-mode infinite loops:
GoalManager.recordEvaluation— now returns a consecutive-streak count for same-reason not-met verdicts; resets on reason-change, goal-clear, achieve, or fail. Correct early.- Stall guard in
client.ts— whenrepeatedReason >= GOAL_STALL_LIMIT, clears goal then fires one final user-facing turn viasendMessageStream. The clear-first ordering prevents nested re-entry — correct. summarizeToolCallsForGoalreplaces the raw last-20 slice with dedup + action/inspection classification. The fix is sound: unknown/MCP/shell treated as actions and preserved, read-only capped without silent loss.
Observations
- GAPS (2):
- Diff truncated at 200 / 667 lines — new files
toolCallSummary.ts,toolCallSummary.test.ts,goalEvaluator.tschanges, andindex.tsexports are unreadable. The PR claims 56 goal-module tests + 64 client tests pass; I cannot verify the test file additions or the evaluator rule changes. GOAL_STALL_LIMITvalue is unverified — defined in the off-screenindex.tsportion.
- Diff truncated at 200 / 667 lines — new files
- MEDIUM —
client.ts:1053:endTurnSpan('ok')unconditionally aftersendMessageStreamregardless of whether the stall message turn succeeded or errored. Minor (endTurnSpan is telemetry), but worth noting. - LOW: The early
returnwhenevalResultis undefined skips stall counting on evaluator throw — likely intentional but worth a comment for the next reader.
Checks: 3 | Passed: 2 (visible diff) | Failed: 0 | Gaps: 2
Will re-review with APPROVED once CI is terminal and the truncated portion is accessible.
— Quinn, QA Engineer
|
Submitted COMMENT review on #344. CI is still in progress (CodeQL in_progress, Lint queued), so I've issued a non-blocking WARN with Gaps flagged for the truncated diff portion. I'll re-review once checks are terminal. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@packages/core/src/core/client.ts`:
- Line 1051: The stall message stored in the variable stallReason incorrectly
refers to a "completion check" even though this branch is triggered by the goal
evaluator; update the string to use evaluator terminology (e.g., "goal
evaluator" or "evaluator") so it reads like: the goal evaluator returned the
same unmet verdict ... and keep evalResult.reason and active.condition intact to
preserve context for the user (update the message construction around
stallReason to replace "completion check" with "goal evaluator" and ensure
surrounding punctuation/quotation remains correct).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 57df645b-b9a5-43eb-be91-09a7603e3d7d
📒 Files selected for processing (8)
packages/core/src/core/client.tspackages/core/src/goal/GoalManager.test.tspackages/core/src/goal/GoalManager.tspackages/core/src/goal/goalEvaluator.tspackages/core/src/goal/index.tspackages/core/src/goal/toolCallSummary.test.tspackages/core/src/goal/toolCallSummary.tspackages/core/src/goal/types.ts
| // stand. Clearing first means the nested turn's own post-turn goal | ||
| // block sees no active goal and can't re-enter this loop. | ||
| goalManager.clearGoal(); | ||
| const stallReason = `Stopping the /goal loop: the completion check returned the same unmet verdict ${repeatedReason} turns in a row without converging ("${evalResult.reason}"). The goal "${active.condition}" is now cleared. Briefly tell the user where things stand and what they should decide or check next, then stop.`; |
There was a problem hiding this comment.
Use evaluator terminology in the stall message.
Line 1051 says “completion check returned the same unmet verdict,” but this branch is triggered by the goal evaluator. That mismatch can produce a confusing final user explanation.
Suggested wording fix
- const stallReason = `Stopping the /goal loop: the completion check returned the same unmet verdict ${repeatedReason} turns in a row without converging ("${evalResult.reason}"). The goal "${active.condition}" is now cleared. Briefly tell the user where things stand and what they should decide or check next, then stop.`;
+ const stallReason = `Stopping the /goal loop: the goal evaluator returned the same unmet verdict ${repeatedReason} turns in a row without converging ("${evalResult.reason}"). The goal "${active.condition}" is now cleared. Briefly tell the user where things stand and what they should decide or check next, then stop.`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const stallReason = `Stopping the /goal loop: the completion check returned the same unmet verdict ${repeatedReason} turns in a row without converging ("${evalResult.reason}"). The goal "${active.condition}" is now cleared. Briefly tell the user where things stand and what they should decide or check next, then stop.`; | |
| const stallReason = `Stopping the /goal loop: the goal evaluator returned the same unmet verdict ${repeatedReason} turns in a row without converging ("${evalResult.reason}"). The goal "${active.condition}" is now cleared. Briefly tell the user where things stand and what they should decide or check next, then stop.`; |
🤖 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 `@packages/core/src/core/client.ts` at line 1051, The stall message stored in
the variable stallReason incorrectly refers to a "completion check" even though
this branch is triggered by the goal evaluator; update the string to use
evaluator terminology (e.g., "goal evaluator" or "evaluator") so it reads like:
the goal evaluator returned the same unmet verdict ... and keep
evalResult.reason and active.condition intact to preserve context for the user
(update the message construction around stallReason to replace "completion
check" with "goal evaluator" and ensure surrounding punctuation/quotation
remains correct).
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
CodeQL flagged `/[.!?]+$/` (js/polynomial-redos, high): it backtracks polynomially on strings with many '!', and the reason text it runs on comes from the evaluator model (uncontrolled input). Replace it with a trailing- character scan that strips the same punctuation in linear time. Behavior is unchanged; existing stall-detection tests still pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
QA Audit — PR #344 | fix(goal): stop runaway loops and let the judge credit artifact evidence
VERDICT: INCONCLUSIVE — awaiting terminal CI
CI Status (interim)
Lint: in_progressCodeQL: in_progress- No CI access on this PR (fork or external contributor) — Gap, not a failure.
Diff Review
Three synchronized fixes to stop goal-mode infinite loops:
goalEvaluator.ts— judge now credits artifact existence (files created/edited/written) as evidence.toolCallSummary.ts(new) — dedupes repeated tool calls, classifies actions vs inspections, preserves all distinct actions in window regardless of turn count.GoalManager+client.ts— stall guard: 3 consecutive same-reason not-met evaluations clear the goal and report to user.
56 new goal tests + 64 client tests. TypeScript/ESLint/Prettier clean per description.
Observations
- CRITICAL (open, unresolved): CodeQL alert — Polynomial ReDoS in the reason-normalization regex in
GoalManager.ts. Triggered by evaluator reasons containing repeated punctuation characters. Must be fixed or formally suppressed with justification before merge. Source: GitHub Advanced Security thread (open). - MINOR: Coderabbit unresolved — line 1051 stall message says "completion check" but the signal is the goal evaluator. Terminology mismatch in the final user-facing message. Label:
⚡ Quick win. - CLAWPATCH: Structural deep review unavailable (HTTP 500 / checkout cache errors). Findings above are from diff + thread evidence only.
Will re-run check_ci and submit a formal verdict (APPROVE or CHANGES_REQUESTED) once checks are terminal.
— Quinn, QA Engineer
|
Submitted COMMENT review on #344 (interim — CI still in progress). Summary: PR addresses three root causes of goal-mode infinite loops: judge now credits artifact evidence, tool-call summarization keeps deliverables in context, and a stall guard clears the goal after 3 identical not-met verdicts. Two open threads need resolution before a terminal verdict can be issued:
Formal verdict (APPROVE or CHANGES_REQUESTED) will be submitted once |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Goal mode (
/goal) could loop forever re-confirming a task that was clearly done — the agent re-reading files, re-listing, and re-stating "complete" turn after turn while the small-model judge kept returning not met. Originally reported against a documentation goal that the judge would never accept.Root causes (all addressed here, layered on top of the already-released impossible-goal abandonment from #322):
write_file/editcalls out of the window. The judge then saw only reads + a "done" message → permanent not met → infinite loop.Changes
goalEvaluator.ts): artifact existence (files created/edited, content written, output shown) now counts as evidence; judge the condition exactly as written; don't invent stricter criteria. Preserves feat(goal): abandon impossible /goal conditions instead of looping #322'simpossibleschema and rules.summarizeToolCallsForGoal(newtoolCallSummary.ts): replaces the raw 20-call slice. Dedupes repeats into(x N)counts, classifies state-changing actions vs read-only inspections (via the canonicalToolNamesregistry + legacy-alias normalization; unknown/MCP/shell treated as actions and preserved), keeps every distinct action while capping read noise. The deliverable stays visible no matter how long the agent loops. No silent truncation — drops are noted as "N earlier omitted".GoalManager+client.ts):recordEvaluationnow returns how many consecutive not-met turns repeated the same (normalized) reason. AtGOAL_STALL_LIMIT(3), the loop clears the goal and spends one final turn reporting to the user instead of re-firing. Distinct from animpossibleverdict, which still ends the loop immediately on a single judgement.Testing
tsc,eslint, andprettierall clean.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests