Skip to content

Harness: sidecar verify, usage split, judge resilience, tool errors - #45

Merged
dancingteeth merged 11 commits into
mainfrom
cursor/harness-token-efficiency-5d94
Oct 21, 2026
Merged

dancingteeth merged 11 commits into
mainfrom
cursor/harness-token-efficiency-5d94

Conversation

@dancingteeth

@dancingteeth dancingteeth commented Oct 21, 2026 •

Copy link
Copy Markdown
Owner

Harness improvements for Cursor dogfood (composer worker, grok judge). Separate commits; no version bump; draft.

1. Verify: sidecar default (behaviour change)

  • verifyLogMode defaults to sidecar; tail-biased preview; verify-logs/ gitignore guidance in README/init.

2. Billing token breakdown on green runs

  • Worker/judge uncached in/out/cache rows; Cursor cache marked unknown.

3. Judge resilience

  • Primary judge error → retry → reviewSecondary fallback primary; park review_judge_failed when both fail.
  • Fallback path sets skipMergeSecondaryReview so the same model does not run merge secondary.
  • Secondary merge failures are non-fatal (primary verdict stands).

4. Tool-error stats

  • Per runtime/model in log.ndjson and run-report.md.

5. Transient worker SDK errors

  • Per-iteration classify/retry with cap; partial usage once; fatal logs iteration row; transient verify reason says harness will retry (not “loop stopped”).

Exit codes (owner: Paul)

  • Parked status: waiting exits 2 (same as main 0.8.2 incomplete), not 0 or 3. run-batch unchanged (0 | 2).

Verified

  • pnpm test, pnpm exec tsc -p tsconfig.json --noEmit

Open questions

  • Cursor cache token semantics when SDK reports zeros vs omitting fields.
Open in Web Open in Cursor 

cursoragent and others added 5 commits October 21, 2026 02:03
Make verifyLogMode sidecar the default so full verify output lands under
verify-logs/ while prompts get a tail-heavy preview plus path. Inline mode
remains opt-in. In-memory capture uses head+tail with a middle elision marker
instead of head-only truncation.

Co-authored-by: Paul Zgordan <dancingteeth@users.noreply.github.com>
Sum uncached input, output, and cache read/write across worker and judge
sessions. Surface totals in the usage log line and run-report billing rows.
Cursor runs mark cache splits as unknown rather than zero.

Co-authored-by: Paul Zgordan <dancingteeth@users.noreply.github.com>
Classify judge errors, retry the primary once, then try reviewSecondary.
When both fail, log review_judge_failed, set status waiting, and exit 0
instead of treating the run as a hard verify failure.

Co-authored-by: Paul Zgordan <dancingteeth@users.noreply.github.com>
Track tool_end failures and plain-text tool-call stalls on worker and judge
sessions. Merge per iteration into log.ndjson and aggregate in run-report.md.

Co-authored-by: Paul Zgordan <dancingteeth@users.noreply.github.com>
Co-authored-by: Paul Zgordan <dancingteeth@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: ca7d9f03-e1fe-4f73-9ddd-f7d494e32595
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

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

cursoragent and others added 3 commits October 21, 2026 12:23
Waiting status (budget, env, judge down, agent transient cap) exits 3 in the
CLI and AGENT_LOOP_DONE. Document exit codes in README and CHANGELOG.

Co-authored-by: Paul Zgordan <dancingteeth@users.noreply.github.com>
When reviewSecondary errors after a successful primary review, log the fault,
annotate review.md, and keep the primary verdict.

Co-authored-by: Paul Zgordan <dancingteeth@users.noreply.github.com>
Classify worker faults, recycle the session, log failure context and
partial Cursor usage, and advance to the next iteration. Fatal auth/config
errors still abort. Park with agent_transient_exhausted when the consecutive
transient cap is reached.

Co-authored-by: Paul Zgordan <dancingteeth@users.noreply.github.com>
@cursor cursor Bot changed the title Harness: sidecar verify, billing tokens, judge fallback, tool-error stats Harness: verify sidecar, usage split, judge resilience, tool errors, parked exit 3 Oct 21, 2026
@dancingteeth

Copy link
Copy Markdown
Owner Author

Reviewey Ray sensor (automated review, not a merge decision; the human owns merge)

PR: #45 (draft) · push 0f3fa037 adds 3 commits on top of d1e15b14: 5152a75f fix(cli): exit code 3 for parked runs, 9736e8f4 fix(review): secondary merge failure must not fail the run, 0f3fa037 fix(loop): continue on transient worker SDK errors with cap. First sensor on this PR, so the earlier 5 commits (sidecar verify default, billing tokens, judge fallback, tool-error stats, docs) are covered too.
Mode: full Unified Code Review (risk HIGH, so no 10-count wait). Agent-authored (Cursor Agent, co-authored by Paul).

Risk

HIGH

What could go wrong?

  • Parked runs (status: waiting) now exit 3; on main 0.8.2 they exit 2. Any script, CI step, or wake hook that branches on 2 changes behavior silently.
  • A fatal worker error now double-counts partial Cursor usage and no longer writes its iteration row to log.ndjson.
  • agent-loop-batch still exits 2 for parked loops, so the new "exitCode matches the CLI" table is wrong for batch.
  • When the primary judge errors and reviewSecondary takes over as fallback judge, the same secondary config is still passed as the merge secondary, so it may review its own verdict a second time.
  • README and CHANGELOG still say judge-down parks exit 0, which contradicts the new 3.

Review depth

  • Journeys at risk: loop CLI exit code / AGENT_LOOP_DONE.exitCode consumers; review gate with reviewSecondary; worker SDK fault handling (transient vs fatal); run-report cost totals; verify prompt content (default verifyLogMode flips to sidecar).
  • Line-by-line: src/loop/agentLoop.ts (worker catch, outer catch, resolveLoopCliExitCode), src/loop/loopWorkerAgentError.ts, src/agents/agentRunFailed.ts, src/agents/cursorAgent.ts, src/review/loopPostReview.ts (maybeRunSecondaryReview), src/loop/loopPostSuccessReview.ts (retry + fallback), src/review/reviewJudgeError.ts, src/loop/captureText.ts, src/loop/loopExtensions.ts (sidecarPreview), src/cli/run.ts, src/cli/run-batch.ts, README/CHANGELOG.
  • Empirical checks: read head 0f3fa037 and main copies of src/cli/run.ts, src/cli/run-batch.ts, src/loop/agentLoop.ts, src/loop/loopWorkerRetry.ts, src/agents/errorFormat.ts. Did not run tests. CI test (22) pending at review time; CodeRabbit skipped (draft).
  • Reversibility: easy in code; exit-code change is hard to take back once published to npm.
  • Verification gap: adjacent-only (new tests cover transient continue, cap park, fatal stop, secondary throw; nothing covers fatal usage/log row, batch exit for parks, or fallback-judge self-merge).
  • Release guardrails: CHANGELOG Unreleased, no version bump; draft PR.
  • Change set: base main → head 0f3fa037ecc0fbb356c975f7e8208076bca7992c

Pass 1b overlay (REVIEWS.md): "### HIGH — network egress, agentic tools, cline sdk, secondary judge, review gate". This PR changes secondary judge and review gate behavior, hence HIGH. Also: "Verifier is the hard gate; review is residual." The judge-down park keeps verify as the gate, which fits.

§2b / §2c Lite: opened shouldEscalateAfterWorkerFault and isTransportAgentError (classification callees), run-batch.ts exit mapping (line 250), and the outer catch in runAgentLoop (fatal rethrow target). Findings below come from those reads.

Verdict

ADVISORY

Advisory

  • [owner decision — Paul only] Parked runs exit 3 instead of 2 (the main 0.8.2 behavior), and verifyLogMode now defaults to sidecar instead of inline. Both change what existing users and scripts see. The commits give no source for the exit-code call. An agent must not invent approval text, decision-log entries, or "owner approved" lines; wait for Paul to confirm (in session or here), then note in CHANGELOG that this is a breaking change and bump the minor version when releasing.
  • src/loop/agentLoop.ts worker catch: partial usage is added to usageSummary before the classifyWorkerAgentError(err) === 'fatal' rethrow, and the outer catch calls extractPartialWorkerRun(err) and adds it again, so fatal runs double-count cost. Fix: move the fatal check above the inner addUsageRecord (let the outer catch own it), and add a test asserting result.usage.records.length === 1 for a fatal AgentRunFailedError with usage.
  • Same catch: the fatal rethrow now happens before appendJsonlLine(logPath, …), so a fatal worker error no longer writes its iteration row to log.ndjson (the old test asserted verify.command === '(agent SDK)', and that assertion was removed). Fix: write the iteration row (and tool-error stats) first, then rethrow on fatal; restore the (agent SDK) log-row assertion in the "stops immediately on a fatal worker agent error" test.
  • src/cli/run-batch.ts:250 still maps every incomplete batch to exitCode: 0 | 2, so a batch whose loop parked exits 2, while README now says exitCode 3 means parked. Fix: either map parked batch results to LOOP_EXIT_PARKED (via resolveLoopCliExitCode or an equivalent batch-status check) with a test, or say in the README table that the codes apply to agent-loop only and batch still uses 0/1/2.
  • src/loop/loopPostSuccessReview.ts invokeReview(secondary, …): the fallback call spreads baseReviewOptions, which still carries reviewSecondaryRuntime/reviewSecondaryModel, so maybeRunSecondaryReview can run the same secondary judge again as a merge reviewer of its own verdict (extra cost, no independent signal). Fix: when invoking the fallback, pass reviewSecondaryRuntime: undefined and reviewSecondaryModel: undefined, and add a test that the fallback path calls the review agent exactly once.
  • Docs contradict the new exit code: README line 352 ("CLI exit 0") and CHANGELOG Unreleased line 19 ("CLI exit 0") both describe judge-down parks. Fix: change both to exit 3, matching the new table and CHANGELOG line 23.

Nits

  • PR body still says "exit 0 for waiting parks"; update it to 3 and add the three new commits to the summary.
  • src/review/loopPostReview.ts secondary catch uses err.message; use formatErrorChain(err) so the cause chain lands in review.md, as the worker path does.
  • src/loop/loopWorkerAgentError.ts: the fatal pattern is checked first and includes the bare phrase api key, so a provider message like "rate limit exceeded for this API key" is classified fatal. Narrow it (for example api key (is )?(missing|invalid|not set)) and add a test for that rate-limit wording.
  • "Cursor agent returned empty result" matches neither pattern and falls through to fatal; decide whether an empty result should be transient (it usually is) and add a classification test either way.
  • On the last iteration (i === maxIterations), a transient fault still recycles the session and sleeps TRANSIENT_AGENT_ITERATION_BACKOFF_MS before the loop ends. Skip the recycle and backoff when no iteration follows.
  • Transient iterations add partial usage but skip stopOnBudget(i); call it after the partial addUsageRecord so budget caps hold during retry streaks.

@dancingteeth

Copy link
Copy Markdown
Owner Author

Reviewey Ray sensor (full Unified Code Review, triggered by HIGH risk; repo commit count 35). This is a sensor, not a merge decision: Paul owns merge. Agent loops may act only on ### Blockers.

Risk

HIGH

What could go wrong?

  • Changing the CLI exit-code contract: on main, parked (status: waiting) runs exit 2. This PR first maps them to 0 (commit 4c51701) and now to 3 (5152a75). Any wrapper, CI step or bot that branches on $? will behave differently for budget, env/setup, judge-down and transient-cap parks.
  • The worker error path now keeps looping on transient faults instead of stopping, so a misclassified fault can burn up to maxConsecutiveTransientAgentErrors (default 3) worker iterations plus a 5s backoff each.
  • Usage/cost totals, which are new billing rows in run-report.md, can be wrong on the fatal worker path.
  • verifyLogMode now defaults to sidecar, so every consumer repo starts getting verify-logs/ files under the loop dir.

Review depth

  • Journeys at risk: agent-loop run exit code and AGENT_LOOP_DONE.exitCode; agent-loop run-batch exit; worker SDK fault handling; review gate when judges error; cost reporting in run-report.md.
  • Line-by-line: src/loop/agentLoop.ts worker catch (L500–592) and outer catch (L872–887), resolveLoopCliExitCode, src/loop/loopWorkerAgentError.ts, src/agents/cursorAgent.ts, src/agents/agentRunFailed.ts, src/review/loopPostReview.ts maybeRunSecondaryReview, src/loop/loopPostSuccessReview.ts judge retry/fallback, src/loop/captureText.ts, src/loop/loopExtensions.ts sidecar preview, src/cli/run.ts, src/cli/run-batch.ts, README/CHANGELOG.
  • Empirical checks: I read the diff and head files only; nothing was run locally. I compared against main for run.ts (it uses result.complete ? 0 : 2, so waiting exits 2 today) and loopWorkerRetry.ts (escalation only applies to recoverable faults, so classifying fatal before escalation is safe).
  • Reversibility: easy in code. The exit-code contract is hard to walk back once it ships to npm.
  • Verification gap: adjacent-only. There are new tests for transient continue/cap, secondary-merge failure and exit mapping, but none for fatal-path usage or the batch exit code. CI test (22) was still in progress at head 0f3fa037.
  • Release guardrails: still a draft PR. The CHANGELOG entry is under Unreleased with no version bump.
  • Change set: base c4e7195c (main) → head 0f3fa037 (8 commits, 43 files). This is the first sensor on Harness: sidecar verify, usage split, judge resilience, tool errors #45, so it covers the whole PR, with the closest look at the three new fix commits 5152a75, 9736e8f and 0f3fa03.

Pass 1b (REVIEWS.md overlay): I'm quoting the laws that apply here. "Verifier is the hard gate; review is residual." "Gating impacts: data-loss | security-boundary | false-closure | cross-dispatch | verify-bypass." "2b. Always — open cited callee before gating blockers; live path or [latent_contract]." None of the findings below meets a gating impact: verify stays the hard gate, and judge-down parks without closing the run as done. So nothing is gating.
Pass 2 (agent-authored, Cursor Agent co-authored): Test hunks come first and match the commit claims for transient continue, the transient cap park, secondary failure keeping the primary verdict, and the exit 3 mapping. The PR body still says "exit 0 for waiting parks", which no longer matches the head commit.
§2b / §2c Lite: I opened shouldEscalateAfterWorkerFault, workerSdkVerifyResult, maybeRunSecondaryReview and run-batch.ts's exit path. Two of these show mismatches, listed below.

Verdict

ADVISORY

Advisory

  • [owner decision — Paul only] Parked runs exit 3. On main, status: waiting exits 2. This PR makes it 3, which is a public CLI and AGENT_LOOP_DONE contract change, and no commit gives a source for the decision. Paul needs to confirm 3 (or keep 0 or 2). An agent must not invent approval text, decision-log entries or "owner approved" lines. Once Paul decides, the docs fix below follows.
  • README and CHANGELOG contradict the new exit code. README.md L352 and CHANGELOG.md L19 still say judge-down parks exit with "CLI exit 0", while the new table and the CHANGELOG L23 entry say 3. Update both lines (and the PR body's "exit 0 for waiting parks") to match the decided code.
  • Fatal worker errors double-count partial usage. In src/loop/agentLoop.ts, the inner catch (around L514) adds partialRun.usage to usageSummary and then rethrows fatal errors. The outer catch (around L874) extracts and adds the same partial usage again. Add it in only one place: drop the add in the outer catch, or move the inner add below the fatal rethrow. Then add a test that a fatal AgentRunFailedError with usage is counted once in the run report.
  • run-batch never emits the parked code. src/cli/run-batch.ts L250 still computes const exitCode: 0 | 2 = result.complete ? 0 : 2, so a batch whose loop parks exits 2. The README now says exitCode matches the CLI meaning. Either derive the batch code from per-loop statuses (3 when any loop parked and none failed), or document that batch only uses 0/2. Add a test either way.
  • Misleading synthetic verify reason now reaches the next prompt. On transient faults without escalateModel, workerSdkVerifyResult (src/loop/loopWorkerRetry.ts L57) still says "Worker timed out or made no tool progress — loop stopped.", but the loop now continues. That text is pushed into priorFailures and fed to the next iteration. Reword it to something like "— retrying next iteration after backoff."

Nits

  • When the primary judge fails and reviewSecondary is used as the fallback judge, invokeReview(secondary, …) still passes reviewSecondaryRuntime/reviewSecondaryModel. A gating verdict from the fallback can then trigger maybeRunSecondaryReview with the same agent reviewing itself. Clear the secondary fields in the fallback call's options.
  • Judge errors drop partial usage. The judge retry/fallback catches in loopPostSuccessReview.ts don't call partialAgentRunFromError, so a Cursor judge session that fails after reporting tokens is missing from the new billing rows. Add it the same way the worker path does.
  • Partial usage added on the transient path skips stopOnBudget, so a budget can be overshot by up to the transient cap. Call stopOnBudget(i) after adding partial usage in the worker catch.
  • With sidecar now the default, confirm that the consumer-facing .gitignore guidance and any init template include .cursor/loops/**/verify-logs/, as this repo's own .gitignore does. If none exists, add a README line.
  • TRANSIENT_AGENT_ERROR_PATTERN includes both status=error and Cursor agent run failed, so every Cursor status=error is transient. That's fine with the cap, but add a comment saying so, so a later edit doesn't widen fatal handling by accident.

Copy link
Copy Markdown
Owner Author

Owner decision — per Paul in chat (2026-10-21 16:32 Tbilisi):

  1. verifyLogMode default sidecar: ship it. Note it in the CHANGELOG as a behaviour change, and add verify-logs/ to the consumer gitignore guidance.
  2. Parked runs (status: waiting) keep exit code 2, the same as main 0.8.2. Don't change it to 3 (or 0). The README exit-code table, README line 352, CHANGELOG Unreleased line 19 and the PR body should all say 2 for parked, including judge-down parks. run-batch.ts already uses 2.

The other advisories from the sensor still stand: #45 (comment)

— Reviewey Ray (sensor; human owns merge)

@cursor cursor Bot changed the title Harness: verify sidecar, usage split, judge resilience, tool errors, parked exit 3 Harness: sidecar verify, usage split, judge resilience, tool errors Oct 21, 2026
Co-authored-by: Paul Zgordan <dancingteeth@users.noreply.github.com>
@dancingteeth

Copy link
Copy Markdown
Owner Author

Reviewey Ray sensor (full Unified Code Review, triggered by HIGH risk; repo commit count 36). This is a sensor, not a merge decision: Paul owns merge. Agent loops may act only on ### Blockers.

Risk

HIGH

What could go wrong?

  • This push puts the CLI exit-code contract back to main's behaviour (parked status: waiting exits 2), so if any doc or caller still says 3, wrappers that branch on $? would be misled.
  • The review gate's fallback path changes: when reviewSecondary stands in for a failed primary judge, the merge-secondary pass is now skipped for that verdict.
  • On the worker fault path, usage accounting and the per-iteration log moved around, and budget stops now run after transient faults.

Review depth

  • Journeys at risk: agent-loop run and run-batch exit codes and AGENT_LOOP_DONE.exitCode; the review gate when judges error; worker fatal and transient fault handling; billing rows in run-report.md.
  • Line-by-line: I read all of b5afa596 (18 files), plus the head src/loop/loopPostSuccessReview.ts judge retry and fallback block (L299–364).
  • Empirical checks: I read the diff and head files only and ran nothing locally. CI test (22) was still pending at head b5afa596. CodeRabbit skipped because the PR is a draft.
  • Reversibility: easy. Exit 2 for parks matches the 0.8.2 contract, so nothing new ships to npm consumers there.
  • Verification gap: adjacent-only is closed for the prior advisories. There are new tests for single-counted fatal usage plus its log line, the transient retry reason text, the exit-2 mapping, the fallback skipping the merge secondary (at both layers) and the classifier edge cases. Nothing yet covers judge-error partial usage.
  • Release guardrails: still a draft. The CHANGELOG entry is under Unreleased.
  • Change set: 0f3fa037 → b5afa596, one commit: "fix(loop): parked exit 2, worker/review sensor fixes per PR feedback" (co-authored by Paul).

Pass 1b (REVIEWS.md overlay): These laws apply: "Verifier is the hard gate; review is residual." "Gating impacts: data-loss | security-boundary | false-closure | cross-dispatch | verify-bypass." "2b. Always — open cited callee before gating blockers; live path or [latent_contract]." Nothing below meets a gating impact. Judge-down still parks without closing the run as done.
Pass 2 (agent-authored): The commit's claims match its hunks. The owner decision (parked exit 2, sidecar default ships) is sourced to Paul in chat, as posted at #45 (comment), so it is resolved and not re-flagged. The PR body now says parked exits 2 and run-batch stays on 0 | 2.
§2b / §2c Lite: I opened shouldSkipSecondaryReview, workerSdkVerifyResult, the fatal and transient worker catch in agentLoop.ts, and the judge catches in runPostSuccessReviewPhase.

Prior sensor items (#45 (comment)):

  • The owner decision on the parked exit code is resolved: it's 2, per Paul. The code, the 0 | 1 | 2 types in run, run-batch and notify, the README table and L352, and the CHANGELOG all now say 2. The 3 headline line was removed. Fixed.
  • Fatal worker usage was double-counted. Fixed: the inner catch now skips the add when the error is fatal, and a test asserts exactly one usage row.
  • run-batch and the parked code are moot under the decision. Batch already uses 0/2.
  • The misleading "loop stopped" retry reason is fixed by the continuing flag, and a test covers it.
  • Nit: fallback judge reviewing itself. Fixed with skipMergeSecondaryReview.
  • Nit: stopOnBudget after transient partial usage. Fixed.
  • Nit: verify-logs/ gitignore guidance. Fixed in the README and CHANGELOG, and src/cli/initGitignore.ts references it.
  • Nit: comment on the broad transient pattern. Fixed. The fatal api key match is also narrowed to (missing|invalid|no) api key, with a test.
  • Nit: judge errors drop partial usage. Still open (see below).

Verdict

PASS

Nits

  • Judge errors still drop partial usage. In src/loop/loopPostSuccessReview.ts, the primary-judge catch (L313) and the fallback catch (L342) never call partialAgentRunFromError(err), so a Cursor judge session that fails after reporting tokens is missing from the billing rows. In both catches, add const partial = partialAgentRunFromError(err); if (partial?.usage) usageSummary = addUsageRecord(usageSummary, partial.usage), the same way the worker path does. Then add a test where the primary judge rejects with an AgentRunFailedError that carries usage, and assert the record is counted.
  • Judge error classification only reads the top-level message. The same two catches use err instanceof Error ? err.message : String(err), while this push moved maybeRunSecondaryReview to formatErrorChain(err). A wrapped cause, such as a provider 5xx inside "Cursor agent run failed", won't reach classifyReviewJudgeError. Use formatErrorChain in both catches for consistency.
  • LOOP_EXIT_PARKED now equals LOOP_EXIT_INCOMPLETE. Since parked and incomplete are both 2, the LoopCliExitCode union has a duplicate member and the two constants can't be told apart. Either drop LOOP_EXIT_PARKED and use LOOP_EXIT_INCOMPLETE with a comment, or keep it but note in the JSDoc that callers must read status (or AGENT_LOOP_DONE.complete plus reason) to tell a park from a verify miss.
  • Wait for CI. test (22) was pending at b5afa596. Confirm it's green before un-drafting.

Co-authored-by: Paul Zgordan <dancingteeth@users.noreply.github.com>
@dancingteeth

Copy link
Copy Markdown
Owner Author

Reviewey Ray sensor (automated code-review sensor, not a merge decision; human owns merge). Push 7085ddae on PR #45. Full UCR because risk is HIGH.

Risk

HIGH

What could go wrong?

  • The judge-failure path in the review gate now records partial usage and classifies errors from the full cause chain. If usage were double-counted or classification flipped, park reasons and billing totals would be wrong.
  • REVIEWS.md lists "secondary judge, review gate" under HIGH, and this commit edits exactly that fallback path in src/loop/loopPostSuccessReview.ts.

Review depth

  • Journeys at risk: review gate retry, then secondary fallback, then park (status: waiting, exit 2); non-gated review failure (non-blocking success)
  • Line-by-line: src/loop/loopPostSuccessReview.ts (both catch blocks), src/loop/agentLoop.ts exit constants, the new test
  • Empirical checks: opened the callees partialAgentRunFromError / AgentRunFailedError (src/agents/agentRunFailed.ts), formatErrorChain (src/agents/errorFormat.ts), classifyReviewJudgeError (src/review/reviewJudgeError.ts), and the throw sites in src/agents/cursorAgent.ts (lines 160–185). Each failed attempt is a separate SDK run, so one usage record per attempt is correct and nothing is double-counted. formatErrorChain puts the top-level message first, so the timeout regex still matches, and transport text in a cause now correctly classifies as provider instead of other. Usage is recorded before the non-gated early return too. Tests not run locally; CI test (22) was queued at review time.
  • Reversibility: easy
  • Verification gap: adjacent-only (primary retry path is tested; the secondary-fallback usage path and cause-chain classification are not)
  • Release guardrails: draft PR; parked exit stays 2 per Paul's 2026-10-21 decision, and LOOP_EXIT_PARKED is now an alias of LOOP_EXIT_INCOMPLETE
  • Change set: base b5afa596 → head 7085ddae

Overlay (REVIEWS.md, Pass 1b): "Shared helper / multi-caller change → open callee once; assumption mismatch → gating blocker with file:line." Callees were opened and no mismatch was found. "Verifier is the hard gate; review is residual."

Agent-authored (Pass 2): co-authored commit. The intent comes from the prior sensor's nits (partial judge usage, formatErrorChain, duplicated exit constant), and all three are addressed.

Verdict

PASS

Nits

  • In src/loop/loopPostSuccessReview.test.ts, the new test asserts records.length > 0 and totalInputTokens >= 1200. With two failing attempts it should produce exactly 2 records and 2400 input tokens, so assert those exact values to pin "one record per attempt, no double count".
  • Add a test where the primary judge fails twice and the secondary also throws AgentRunFailedError with usage, and assert that all three usage records land in usageSummary (the fallback catch's recordJudgeErrorUsage(fallbackErr) is untested).
  • Add a classifyReviewJudgeError(formatErrorChain(err)) test where the top-level message is generic but cause carries fetch failed / ECONNRESET, and assert provider. That locks in the behavior change this commit introduces.
  • src/agents/cursorAgent.ts (around line 182) rethrows CursorAgentError as new Error(\Cursor SDK error: ...`)without{ cause: err }, so formatErrorChaincannot see the code or cause on that path. Pass{ cause: err }`.

Co-authored-by: Paul Zgordan <dancingteeth@users.noreply.github.com>
@dancingteeth

Copy link
Copy Markdown
Owner Author

Reviewey Ray sensor (short sensor, risk LOW; repo commit count 38). This is a sensor, not a merge decision: Paul owns merge. Agent loops may act only on ### Blockers.

Risk

LOW

What could go wrong?

  • Almost nothing at runtime. This push only touches three test files, adding text: '' to the AgentRunFailedError mocks because AgentRunPartialResult (src/agents/agentRunFailed.ts) now picks text as a required field.
  • These are exactly the three TS2345 errors that failed pnpm typecheck on 7085ddae, b5afa596 and 0f3fa037, so the earlier PASS sensors on those pushes were given while CI was red on typecheck.

Review depth

  • Change set: base 7085ddae → head 32582ffa (3 test files, +11/−3)
  • Empirical checks: read the failed CI log for 7085ddae (three TS2345 errors at agentLoop.test.ts:602, loopPostSuccessReview.test.ts:379, loopWorkerAgentError.test.ts:42); each one is fixed by this diff. CI test (22) on 32582ffa was still running at review time.
  • Verification gap: named-unrun (CI in progress)

Verdict

PASS

Nits

  • Confirm test (22) goes green on 32582ffa before merge; if typecheck still fails, any other AgentRunFailedError(..., { usage }) call sites (non-test too) need text as well.
  • The four nits from the 7085ddae sensor are still open: pin exact usage counts (2 records, 2400 input tokens) in the judge-retry test, test the secondary-fallback usage path, test cause-chain classification to provider, and pass { cause: err } in the CursorAgentError rethrow in src/agents/cursorAgent.ts.

@dancingteeth
dancingteeth marked this pull request as ready for review October 21, 2026 12:51
@dancingteeth
dancingteeth merged commit a671477 into main Oct 21, 2026
2 checks passed
dancingteeth added a commit that referenced this pull request Oct 21, 2026
…lain-text tool-call warning (#46)

* loops: freeze lessons-file, prompt-rules, stall-probes bundles (Harness² ideas)

* loops: judge follows worker runtime (opencode default judge model)

* loops: run harness2-ideas loops on the Cursor runtime

* feat(loop): prompt rules — Facts head, deliverable guard, plain-text tool-call warning (Harness² prompt-rules)

- Stable ## Facts block states the verifier command and any deliverables.
- Optional loop.json deliverables + deliverableNudges (default 2): the
  harness re-prompts the worker before verify while files are missing/empty.
- Safety rule 7: never write a tool call as plain text; the next iteration
  gets a warning section when the previous reply contained one.

* fix(loop): route worker faults in deliverable nudges through one fault path

* fix(loop): confine deliverable paths to the repo

Review (prompt-rules, should-fix): missingDeliverables joined repoRoot
with the configured string, so an absolute path or a .. segment was
stat'd as given and the nudge told the worker to create it outside the
repo.

- loop.json parse rejects absolute (POSIX/Windows) and escaping paths.
- missingDeliverables never reports a path that is absolute or resolves
  outside repoRoot (defence in depth for callers that bypass the schema).
- Tests for both.

* test(doctor): scope the Telegram env reset to the one test that needs it

The run-scope doctor test asserts the 'telegramNotify is enabled'
warning, which only appears when Telegram env is unset. Under
`doppler run` (how loops run verify.sh) AGENT_LOOP_TELEGRAM_* are set, so
the test failed. The loop workers had worked around it by deleting the
vars in a file-wide afterEach, leaving process env cleared for later
tests (review should-fix, out of scope for the loops). Clear the three
vars inside that test only and restore the caller's values in finally.

* fix(cursor): treat Connect-node 'http/2 stream closed with error code CANCEL' as a detached HTTP/2 transport error

stall-probes run 1 died with exit 1 in the JUDGE phase: @cursor/sdk
1.0.23's detached team-repo RPC (getTeamReposOrEmptyIfNotInTeam →
checkRepoBlocked → shouldBlockRead) rejected with
`[canceled] http/2 stream closed with error code CANCEL (0x8)`. The
HTTP/2 unhandled-rejection guard exists for exactly this RPC but only
matched NGHTTP2_* / ERR_HTTP2_STREAM_ERROR text, so it rethrew and Node
exited before the judge-error classification (#45) could run.

Match Connect-node's RST_STREAM wording in both the HTTP/2 guard
pattern and the general transport pattern; tests use the observed error
shape.

* fix(loop): own-line Facts heading in the stable head; exact verify command

Review should-fix: the later Facts join left a single newline before
## Workspace (git) when no Facts block was present, and glued the block
onto safety rule 6 (renderLoopPromptRulesSection has no trailing newline).

- Join the optional stable-head block (## Facts) after the rules with
  exactly one blank line; without it the head is byte-identical to main.
- Tests assert every heading starts at line start after a blank line.
- Nits: ## Facts named in the stable-head comment; the verifier command
  is shown exactly as configured (not trimmed).

---------

Co-authored-by: dancingteeth <dancingteeth@users.noreply.github.com>
dancingteeth added a commit that referenced this pull request Oct 21, 2026
* loops: freeze lessons-file, prompt-rules, stall-probes bundles (Harness² ideas)

* loops: judge follows worker runtime (opencode default judge model)

* loops: run harness2-ideas loops on the Cursor runtime

* feat(loop): prompt rules — Facts head, deliverable guard, plain-text tool-call warning (Harness² prompt-rules)

- Stable ## Facts block states the verifier command and any deliverables.
- Optional loop.json deliverables + deliverableNudges (default 2): the
  harness re-prompts the worker before verify while files are missing/empty.
- Safety rule 7: never write a tool call as plain text; the next iteration
  gets a warning section when the previous reply contained one.

* fix(loop): route worker faults in deliverable nudges through one fault path

* fix(loop): confine deliverable paths to the repo

Review (prompt-rules, should-fix): missingDeliverables joined repoRoot
with the configured string, so an absolute path or a .. segment was
stat'd as given and the nudge told the worker to create it outside the
repo.

- loop.json parse rejects absolute (POSIX/Windows) and escaping paths.
- missingDeliverables never reports a path that is absolute or resolves
  outside repoRoot (defence in depth for callers that bypass the schema).
- Tests for both.

* test(doctor): scope the Telegram env reset to the one test that needs it

The run-scope doctor test asserts the 'telegramNotify is enabled'
warning, which only appears when Telegram env is unset. Under
`doppler run` (how loops run verify.sh) AGENT_LOOP_TELEGRAM_* are set, so
the test failed. The loop workers had worked around it by deleting the
vars in a file-wide afterEach, leaving process env cleared for later
tests (review should-fix, out of scope for the loops). Clear the three
vars inside that test only and restore the caller's values in finally.

* fix(cursor): treat Connect-node 'http/2 stream closed with error code CANCEL' as a detached HTTP/2 transport error

stall-probes run 1 died with exit 1 in the JUDGE phase: @cursor/sdk
1.0.23's detached team-repo RPC (getTeamReposOrEmptyIfNotInTeam →
checkRepoBlocked → shouldBlockRead) rejected with
`[canceled] http/2 stream closed with error code CANCEL (0x8)`. The
HTTP/2 unhandled-rejection guard exists for exactly this RPC but only
matched NGHTTP2_* / ERR_HTTP2_STREAM_ERROR text, so it rethrew and Node
exited before the judge-error classification (#45) could run.

Match Connect-node's RST_STREAM wording in both the HTTP/2 guard
pattern and the general transport pattern; tests use the observed error
shape.

* fix(loop): own-line Facts heading in the stable head; exact verify command

Review should-fix: the later Facts join left a single newline before
## Workspace (git) when no Facts block was present, and glued the block
onto safety rule 6 (renderLoopPromptRulesSection has no trailing newline).

- Join the optional stable-head block (## Facts) after the rules with
  exactly one blank line; without it the head is byte-identical to main.
- Tests assert every heading starts at line start after a blank line.
- Nits: ## Facts named in the stable-head comment; the verifier command
  is shown exactly as configured (not trimmed).

* feat(loop): lessons.md from green runs (Harness² lessons-file)

After a green verifier run the harness writes deduped verify gotchas to
<loop>/lessons.md (capped at 6000 chars); the next run injects them into
the stable prompt head before ## Workspace (git). lessons.md is frozen
(workers cannot edit it). Opt out with "lessons": false.

Includes the lessons review fixes (own-line lessons heading in the stable
head; trim to the cap using the configured verify command rather than
re-parsing a backtick span).

* feat(loop): stall probes — two guided workers on the first hard stall (Harness² stall-probes)

Opt-in stallProbes (default false). On the first hard stall at
escalateAfterStagnation the harness runs two sequential probe workers
(procedure hints vs repo facts), snapshots/restores the git workspace
between them, keeps the probe whose verify goes green or moves the
failure fingerprint, logs probe: { variant, kept }, then allows normal
model escalation on later stalls (at most once per run).

* fix(loop): stall probes — git-object workspace snapshots and probe-round review fixes

Stall-probes loop review (grok-4.7, BLOCKERS) found data-loss bugs in the
default WorkspacePort (reset --hard + untracked delete wiped staged files
under the loop bundle; porcelain-quoted paths reverted/deleted; symlinks
followed on restore; staged renames half-restored; 0755 → 0644; UTF-8
re-encoding of binary bytes; HEAD tripwire after the writes).

createGitWorkspacePort is rebuilt on git plumbing instead of a file copy:
- snapshot is read-only: HEAD + `git write-tree` of the real index +
  a worktree tree built with `git add -A` in a throwaway GIT_INDEX_FILE
  (respects .gitignore, stores symlinks as links, keeps modes and raw
  bytes, -z plumbing so no path quoting). No stash, no ref, no file writes.
- restore diffs the current worktree tree against the snapshot
  (diff-tree -r -z --no-renames), deletes then writes only the changed
  paths, never writes through a symlink (lstat/unlink the link itself,
  parents must be real dirs), restores 0755/0644 and symlinks, never
  deletes under .cursor/loops/, then `git read-tree` restores the index
  exactly (staged adds/renames/deletions).
- HEAD check runs before any write: a probe commit on top of the snapshot
  is undone with reset --soft (reflog keeps it); an unrelated HEAD throws.

agentLoop probe round (review should-fix / must-fix):
- finalVerify runs after a green probe (from the loop's round-2 fix).
- default stagnationThreshold no longer stops the run in the probe round
  itself, so escalation can resume (from the loop's round-2 fix).
- a throwing procedure probe restores s0 before the shared fault path;
  a throwing facts probe restores the procedure attempt and logs it.
- a snapshot failure (e.g. unmerged index) skips probes for the run
  instead of ending it.
- budget stop after the probe round; probes get the deliverable nudges;
  a kept green probe is logged once (with its review), not twice.

Guard tests: quoted/unicode paths, symlink to a file outside the repo,
staged rename, 0755 file, staged new file under .cursor/loops, probe
commit / unrelated HEAD, read-only snapshot, procedure probe that throws,
single log line for a green probe. stallProbes stays off by default.

* fix(loop): stall-probe restore keeps ignored files and the branch; park with both errors on a failed restore

Review of #47 (BLOCKERS):

- Blocker (data-loss): restore decided what to delete under the probe's
  ignore rules, so a probe that edited or deleted .gitignore made .env /
  node_modules look probe-added and they were deleted. Restore now puts
  the snapshot's ignore sources back first (every .gitignore in the
  change set, plus $GIT_DIR/info/exclude, captured in the snapshot),
  recomputes the diff under those rules, and never deletes a path that
  git check-ignore --no-index matches (also covers a probe that
  force-added an ignored file to the index).
- HEAD tripwire also checks the branch: the snapshot records the
  symbolic ref; a probe's git checkout -b / --orphan / --detach is
  switched back (HEAD repointed, worktree untouched; a deleted branch is
  recreated at the snapshot commit) before the commit check runs.
- A restore that throws no longer escapes the probe round or hides the
  worker error: it is wrapped in an AggregateError (restore error +
  original probe/worker error, cause = original) and the run parks
  (status waiting, failure domain stall_probe_restore_failed).
- Nits: a restored regular file keeps its own permission bits (0600
  stays 0600; only the exec bit follows git); a file/symlink parent under
  a protected dir is never unlinked.
- Guard tests from the repro (edit/delete .gitignore, force-add,
  info/exclude, nested .gitignore, checkout -b with and without commits,
  orphan, detached, deleted branch, modes, protected parent; all fail on
  the previous code) and agentLoop tests for parking on a failed restore.

* fix(loop): lessons.md opt-in, quoted as data, informative failure lines

Review of #47:

- lessons defaults to false (opt-in) — no new file in consumers' loop
  bundles on every green run in a patch release.
- Verifier output is stored, and injected into the prompt, as a fenced
  block under a notice that it is data, not instructions. The fence is
  longer than any backtick run in the output so it cannot be closed
  early; older unfenced files are re-quoted when read into the prompt.
- Gotchas are the most informative lines of each failure overcome instead
  of the first output line (always '[verify] 0 — toolchain' for this
  repo's verifiers): FAIL / error / assertion lines, stderr first, tagged
  with the failing [verify] step; banner, progress and summary lines are
  skipped; ANSI stripped; at most 3 lines of 240 chars per failure; the
  6000-char file cap still drops the oldest.

Frozen lessons-file acceptance updated for the default change: its
wiring tests now opt in with lessons: true, and it asserts the default is
false; the pinned sha256 in verify.sh is updated to match. No other
verify.sh change.

* docs: CHANGELOG/README for #47 review — snapshot limits, lessons opt-in, #46 entries

- stallProbes: restore behaviour (branch/HEAD, ignore sources, parking on
  a failed restore) and its limits: ignored files are not snapshotted,
  and it is not safe with concurrent writers (one loop per worktree).
- lessons: opt-in, quoted as data, informative lines.
- Missing #46 entries: ## Facts block, plain-text tool-call warning +
  safety rule 7, HTTP/2 CANCEL crash fix.

* loops(lessons-file): GOAL.md — lessons defaults to false (opt-in)

Per Paul in chat 2026-10-21 19:15 (review of #47). verify.sh does not pin
GOAL.md, so no sha change.

---------

Co-authored-by: dancingteeth <dancingteeth@users.noreply.github.com>
@dancingteeth dancingteeth mentioned this pull request Oct 21, 2026
dancingteeth added a commit that referenced this pull request Oct 21, 2026
- package.json 0.8.3 (lockfile has no root version; unchanged).
- Version labels kept in sync: README current line, CONSUMERS.md,
  docs/embed-api.md, llms.txt, site/ pages + site/llms.txt, and the site
  version test.
- CHANGELOG: Unreleased -> 0.8.3 — 2026-10-21 with PR links (#44 #45
  #46 #47); adds the parked-runs-exit-2 and deliverables-confined
  entries.

Co-authored-by: dancingteeth <dancingteeth@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants