Repository navigation
Harness² ideas (2/2): lessons.md + stall probes (stacked on #46) - #47
Merged
Merged
Conversation
…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.
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.
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.
… 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.
…mmand 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).
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).
… (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).
…und 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.
Owner
Author
Reviewey Ray sensor: #47 (Harness² 2/2, lessons-file + stall-probes), full UCRRiskHIGH What could go wrong?
Review depth
VerdictBLOCKERS Blockers
Advisory
Nits
|
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
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 |
…rk 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.
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.
…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.
Owner
Author
Reviewey Ray sensor: #47 re-review at
|
| Probe action | Ignored files kept | Other |
|---|---|---|
rewrite / delete root .gitignore |
✅ | .gitignore restored |
| user edits during the window | ✅ | reverted (documented limit) |
checkout -b / --orphan / --detach / branch -m |
✅ | back on refs/heads/main at s0 |
orphan + rm -rf + commit |
✅ | tracked files back, probe file gone |
| commit on top | ✅ | soft-reset to s0 |
| mode 0600 file edited | — | stays 0600, content restored |
edit / delete info/exclude |
✅ | restored |
git add -f .env node_modules |
✅ | content intact, index clean |
delete nested .gitignore; add ! negation in nested or new .gitignore |
✅ | — |
create info/exclude with !.env when the snapshot had none (.env ignored via core.excludesFile) |
❌ .env deleted |
— |
git config core.excludesFile /dev/null |
❌ .env deleted |
— |
reset --hard HEAD~1 with dirty work |
— | parks (refuses); dirty work only in snapshot tree |
- Reversibility: hard/one-way for the two ❌ cases. Easy for the code.
- Verification gap: adjacent-only. The guard tests cover in-tree ignore sources, but not ignore sources outside the tree or a park-recovery pointer.
- Release guardrails:
stallProbesandlessonsare both opt-in now. Ships in 0.8.3 per Paul in chat. The PR is still a draft. - Change set: base
c9dc3fb(main, Harness² ideas (1/2): prompt-rules — Facts head, deliverable guard, plain-text tool-call warning #46 squash) → headc24c7b9. Commits since the last sensor:10df420(merge of main),23fdcd2,ee9e9d4,c24c7b9. No force-push. - Overlay (REVIEWS.md): there's no frozen-file law in REVIEWS.md or AGENTS.md beyond "Do not edit
GOAL.mdmid-loop. The harness restoresGOAL.md,verify.sh,loop.json… if a worker edits them" (AGENTS.md), which is about workers during a run. The binding law is in the loop's frozenGOAL.md: "FAILURE POLICY: … Never editacceptance.ts.txt,verify.sh,GOAL.mdorloop.json…" and "HUMAN GATE: … Paul reviews the branch."
Verdict
ADVISORY
Advisory
- Frozen-spec edit needs Paul's sign-off.
ee9e9d4addslessons: trueto the spec'sbundle()defaults, addsit('is opt-in: lessons defaults to false'), and re-pinsACC_SHA(now447ea4b5…, which matches the file). I diffed it againstda02b88: purely additive, with no assertion removed or loosened. Every wiring test still exercises the feature, and thelessons: falseoverride test still works because overrides spread last. It isn't averify-bypass, but the lessons-fileGOAL.mdfrozen policy forbids the edit and its human gate is Paul. The default flip came from a review advisory, not from Paul.GOAL.mdstill says "(defaulttrue)" (L16, L21), so goal and spec now disagree. Get Paul's yes, then updateGOAL.mdto match in the same PR, or say it stays as a historical record. - Ignore sources outside the tree (reproduced, the two ❌ rows above).
info/excludeis only put back when the snapshot had one, so a probe-created exclude file stays. Changes tocore.excludesFilearen't covered at all. The robust fix is mechanism-independent: at snapshot time recordgit ls-files -o -i --exclude-standard -z(plus ignored paths that are force-added), and never delete a path that was ignored at snapshot time, whatever the rules on disk say at restore. Also delete a probe-createdinfo/excludewhensnap.infoExclude === null. Narrow cases (a probe has to tamper with git's own config), so I'm not gating on them. - Park recovery pointer. On
stall_probe_restore_failed, the user's uncommitted work exists only insnap.worktreeTreeandsnap.indexTree. Those are unreferenced objects, not in the reflog, andgccan prune them. Include the oids in the park message and failure domain, or pin them under a ref such asrefs/agent-loop/probe-snapshot, sogit read-tree/checkout <tree> -- .can recover. Optionally soft-reset tosnap.headeven when HEAD moved to an ancestor, since the snapshot commit is still reachable. - Unguarded mid-round snapshot.
const sA = await workspacePort.snapshot()(agentLoop.ts~L880) isn't wrapped. If the procedure probe leaves unmerged entries,write-treethrows out ofrunAgentLoopinstead of parking. Route it through the same park path. Found by reading the code, not reproduced. - prompt-rules scoreboard on main. After merge,
.cursor/loops/prompt-rules/verify.shstep 8 stays red for good, because the frozen out-of-scope check now seesloopStagnationPolicy.tschanged. Mark that loop finished or archived so nobody re-runs it expecting green. - Leftovers: the PR body still says "Stacked on Harness² ideas (1/2): prompt-rules — Facts head, deliverable guard, plain-text tool-call warning #46", still says "
lessonsdefaults totrue", and still lists gates atda02b88. No concurrent-writer lock exists; it's documented as a limit in the CHANGELOG/README, which is acceptable for an opt-in feature. The judge sandbox (grok-4.7 ran git in the live repo) remains a follow-up issue. It isn't touched here, so it isn't gating. - Not re-judged: this sensor stands in for the skipped judged loop run on the restore redesign.
Nits
- The
applyChangescomment says "A .gitignore the probe added … can only ignore more". A nested!negation can un-ignore. The behaviour is still safe, because probe-added ignore files are removed before the recompute (reproduced), but the comment is wrong. - The CHANGELOG
## Factsentry says "without either, the head is byte-identical".verifyis required, so Facts is always present. - CHANGELOG coverage for 0.8.3 is complete: Harness: sidecar verify, usage split, judge resilience, tool errors #45 (sidecar, usage, judge park, tool errors, transient faults, secondary review), fix: lazy-load optional runtime SDK peers (0.8.3) #44 (lazy peers, pack smoke), Harness² ideas (1/2): prompt-rules — Facts head, deliverable guard, plain-text tool-call warning #46 (deliverables, Facts, rule 7, HTTP/2 CANCEL), and Harness² ideas (2/2): lessons.md + stall probes (stacked on #46) #47 (stallProbes with limits, lessons opt-in).
Owner
Author
|
Owner decision, per Paul in chat (2026-10-21 19:15 Tbilisi): approved. The edit to the frozen lessons-file acceptance spec and its Before merge:
The remaining advisories can be follow-ups. They're in the re-review: ignore rules from outside the repo tree, the recovery pointer when a run parks, the unguarded mid-round snapshot, and marking the prompt-rules loop finished. — Reviewey Ray |
Per Paul in chat 2026-10-21 19:15 (review of #47). verify.sh does not pin GOAL.md, so no sha change.
This was referenced Oct 21, 2026
Owner
Author
|
Reviewey Ray sensor: 083cc73 (delta from c24c7b9) Pass 0: one commit, BlockersNone. AdvisoriesNo new ones. The earlier advisories are now tracked in #48 to #52. Verdict: PASS This is a review result, not a merge decision. Paul approved merging #47 once this push is green, so I'm merging it now. |
dancingteeth
marked this pull request as ready for review
October 21, 2026 15:21
Merged
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>
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
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.
Harness² ideas, part 2 of 2: lessons-file + stall-probes.
Based on
main, not stacked. Part 1 (#46) was squash-merged; this branch mergedorigin/mainin (no rebase, no force-push), andgit diff origin/main...shows only this PR's changes.What's in it
feat(loop)), opt-in:lessonsdefaults tofalse(per Paul in chat 2026-10-21 19:15). The frozen lessons-file acceptance, its sha pin and GOAL.md are updated to match.[verify]step. Banner, progress and summary lines are skipped. The caps are 3 lines × 240 chars per failure and 6000 chars per file.feat(loop)), off by default: on the first hard stall, two differently guided probe workers run in sequence on one workspace before the model escalates. The better workspace is kept.info/exclude;checkout -b, orphan or detach is undone, and a deleted branch is recreated. A probe's own commits are soft-reset, and an unrelated HEAD is refused;.gitignoreor force-adds.env;.cursor/loopsis deleted;status: waiting, failure domainstall_probe_restore_failed) with an AggregateError that keeps both the restore error and the original probe/worker error.## Facts, the plain-text tool-call warning + safety rule 7, and the HTTP/2 CANCEL crash fix.Gates (clean committed head
083cc73)bash .cursor/loops/lessons-file/verify.sh: exit 0bash .cursor/loops/stall-probes/verify.sh: exit 0pnpm build,pnpm typecheck: passpnpm test: 136 files, 1258 tests passed, plus the node test scriptsexp.mjs):.envandnode_modulesare kept after a.gitignoreedit or delete, the branch is restored aftercheckout -b, and mode 0600 is kept. The two concurrent-writer cases still revert, as documented.verify.shstep 8 was gated on Harness² ideas (1/2): prompt-rules — Facts head, deliverable guard, plain-text tool-call warning #46. This PR changesloopStagnationPolicy.tson purpose.Open items
core.excludesFile), and recording ignored files at snapshot time: stall-probes: respect ignore rules from outside the repo tree; record ignored files at snapshot time #48sA): stall-probes: guard the mid-round snapshot (sA) #50