9559e7d4 - Build the error-fix fixer: drive an error.fix conclusion to a draft PR unattended - #56
Draft
Danswar wants to merge 23 commits into
Draft
9559e7d4 - Build the error-fix fixer: drive an error.fix conclusion to a draft PR unattended#56Danswar wants to merge 23 commits into
Danswar wants to merge 23 commits into
Conversation
…o a draft PR unattended Adds agent watch error-fix-work, a script-only driver (no Claude session, no human) that takes an error.fix conclusion's brief, authors the implement spec, runs the implementer/reviewer/PR-reviewer round loop via a hardcoded STATUS+FINDINGS pass/fail/retry rule, and pushes to a draft PR. Narrow, payload-scoped carve-out lets spec_written be script-authored only for error-fix-originated tasks; every other implement task is untouched. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017TZHqRN57WvdUXvYdmLVZe
Reviewer and PR-reviewer lanes previously received the implementer's spec file verbatim instead of a real review instruction, so the mechanical STATUS+FINDINGS verdict rule had never actually been exercised. Fixes that, plus a no-upstream first-push failure, a round-cap regression that leaked into every interactive agent run task on this device, a failed pr.open insert being mistaken for an existing PR, a stray working-agent row on vendor-CLI-unavailable, and the deviation_declared/deviation_granted gate that blocked every error-fix task from ever reaching done. Known remaining issue, next round: store.py's second-resolution timestamps make several "latest gate/check/agent" lookups unreliable when multiple rows land in the same wall-clock second. Two of four known instances are fixed here; test_fixer_pr_gate_rejection_clears_head_for_new_push and test_fixer_pr_gate_rejection_clears_head_before_next_step still fail against the remaining two, in main.py's _latest_gates/_latest_checks. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017TZHqRN57WvdUXvYdmLVZe
store.py's second-resolution timestamps made several latest-gate/check/agent lookups unreliable when rows land in the same wall-clock second. Stamps a real per-device monotonic sequence into gate/check/agent row payloads inside the write transaction itself (reusing the value already computed there, insert-only so an update can never reshuffle sort order), instead of a pre-write peek that had its own race. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017TZHqRN57WvdUXvYdmLVZe
Real PR-review gates (grok-pr quality + logic, run properly for the first time) found genuine gaps across several rounds: a persistently failing PR create silently never blocking task completion, an OSError leaking a working-agent row on both the first and retry lane launch, a stale-head gate resolution across scan/process boundaries after a rejection and re-push, an untested privilege-bypass predicate, and several smaller robustness/consistency issues. All fixed with tests; 731 tests pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017TZHqRN57WvdUXvYdmLVZe
Two narrower-scope recurrences of previously-fixed bug classes: the FINDINGS-body terminator allowlist still let REASON/SCOPE/DIMENSION/STATUS truncate a real finding (same auto-pass-bypass risk as the original ERROR: case, smaller trigger set), and has_fresh didn't use last-wins like _artifact_ok, deadlocking a pass-then-fail local-check sequence. Plus three real test-coverage gaps (an untested privilege-relevant spec generator, an untested new --head validator, a legacy-row test that wasn't actually testing what it claimed) and process/doc cleanup. 743 tests pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017TZHqRN57WvdUXvYdmLVZe
The auto-first-push path had no check that the branch being pushed actually matched the task it belongs to, gated only by the develop/main/master protected-branch list. For a pipeline with no human between implement and push, that's a real control gap: verify the current branch against the task's expected error-fix-<id> branch before pushing, fail loud otherwise. Also closes an empty-review-diff gate (a reviewer could previously report against nothing) and a template-echo parsing edge case, plus doc/README sync. 749 tests pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017TZHqRN57WvdUXvYdmLVZe
The whitespace-strip fix landed at write time but the value that gets persisted and the value used for later comparisons could still diverge: the fixer's own pr.open branch-head derivation read the raw, unstripped error_id while the push-identity check used the stripped one. Also closes the existing-upstream push path's remote-name check (only the tracked branch ref was verified before, not the remote itself) and makes every error_id read path defensively normalize rather than trust that upstream writers got it right. 757 tests pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017TZHqRN57WvdUXvYdmLVZe
Three call sites still compared a caller-normalized error_id against a raw, unstripped persisted value: the fixer's brief lookup, the duplicate- conclusion guard, and the implement-task lookup. All three now use the same _nonempty_str normalization already applied elsewhere, closing the error_id normalization gap for good. An exhaustive grep confirms no remaining unnormalized comparison sites in this PR's scope. 760 tests pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017TZHqRN57WvdUXvYdmLVZe
The branch-identity check's fresh-branch path had no upstream configured turn into a silent auto-set-upstream push for every caller, not just the identity-checked error-fix flow -- weakening the original hard-fail behavior for ordinary agent run tasks. Gates the auto-push on a real expected_branch, restoring the original GitActError otherwise. Also swaps run_core's local strip reimplementation for the shared normalization helper. 761 tests pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017TZHqRN57WvdUXvYdmLVZe
The unattended-push identity check derived expected_branch from bare payload.error_id presence, not from the same error_fix_confirmed check used everywhere else this PR gates script privilege -- an unconfirmed error_id got the auto-push shortcut while being denied other carve-outs. Now fails loudly on that inconsistency instead of silently falling through. Also tried and reverted a stricter verdict-parser check: it would have rejected legitimate reasoning narration before a lane's report, not just contradictory prose, which was proven empirically against the review lane's own real output. Documented as an accepted, honest residual limitation instead of building a fragile heuristic. Consolidated 3 duplicate SHA regexes into one. 764 tests pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017TZHqRN57WvdUXvYdmLVZe
EmptyReviewDiffError's handler only marked the in-flight agent unavailable; nothing set task.state=failed, so the same error-fix task was re-selected by every subsequent scan and hit the identical empty diff again -- an unbounded retry loop with no round-cap applying (the cap is only consulted on rejection resets, a different path). The handler now also calls _check_record(result="fail"), the same mechanism the round-cap-exhaustion path already uses to reach a terminal state. Also, in priority order: - PR-gate rejection findings now get folded into a regenerated .spec.md for the next round instead of being silently dropped, so retries aren't blind repeats of an already-rejected diff. - template_pr_open_payload interpolates only the brief's first sentence into the EN/DE PR summary (full brief stays in <details>), so a multi-sentence brief can't push the summary past CONTRIBUTING.md's 4-sentence cap. - The auto-generated spec.md no longer claims the four PR-review gate keys can be n_a; only contributing_ok/deviation_* actually can be, per allow.py's N_A_ALLOWED. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017TZHqRN57WvdUXvYdmLVZe
… diff _collect_review_diff now reports whether every diff-producing git probe actually succeeded. A non-zero returncode (lock file, disk hiccup, bad cwd) no longer looks identical to "git ran fine, diff is empty" -- it raises a new ReviewDiffUnavailableError that leaves the task untouched for the next scan to retry, the same shape as the existing vendor_unavailable path. A genuinely empty diff still permanently fails the task via the prior round's EmptyReviewDiffError fix. Also, in priority order: - Rejection findings spliced into a regenerated .spec.md are now extracted from the FINDINGS: section (reusing the same parser count_findings already uses) and backtick-fenced, so untrusted rejection text can't reshape the spec's structure. - _contributing_ok_evidence now requires verdict==approved and a head_sha match before citing a gate as approved, and raises instead of writing a literal "missing" into the evidence text for a stale or absent gate. - Review prompt cites file:line only (drops the stray bilingual label). - is_error_fix_originated and _open_error_fix_tasks use the same _nonempty_str() normalization the rest of the error-fix path already applies to error_id. - First-sentence extraction for the PR summary no longer treats a period after a common abbreviation (Dr., e.g., etc.) as a sentence boundary. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017TZHqRN57WvdUXvYdmLVZe
…fs too. The prior commit's probes_ok tracking missed two cases: when no base candidate ref resolves at all, probes_ok stayed True (the rev-parse search is expected control flow, but a total failure to find any base is not) -- so a clean-but-unresolvable worktree wrongly hit the permanent-fail EmptyReviewDiffError path instead of the leave-for-retry ReviewDiffUnavailableError path. Separately, probes_ok was only consulted when the collected diff was empty, so a failed range-diff probe could be masked by non-empty dirty-worktree output from the supplemental diff calls, letting a wrong diff through unflagged. build_review_spec_file now checks probes_ok unconditionally, before the emptiness check. Also, in priority order: - write_error_fix_spec and template_pr_open_payload now fence `brief` the same way rejection_feedback is already fenced, closing the same spec/PR-body structure-injection class for the one field that wasn't covered yet. - _contributing_ok_evidence's StoreError (added for stale/missing PR gates) is now caught at all three call sites instead of leaking into an unbounded scan-error retry with no terminal state -- the same failure class the empty-diff fix targeted, reintroduced by that fix's own new check. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017TZHqRN57WvdUXvYdmLVZe
The task-summary fallback wrote the same English one-liner into both the English and German ledger-summary fields, so the PR-ready closing comment's DE: block was never actually German -- a real CONTRIBUTING.md conformance gap. It now builds a distinct German fallback with a genuine standalone sentence followed by a labeled quotation of the (inherently free-text, untranslatable) brief, mirroring the pattern template_pr_open_payload already uses for its own DE construction. Also closes a residual gap in the prior commit's probes_ok tracking: git merge-base succeeding with empty stdout was treated the same as a resolved base, silently skipping the base-range diff without flagging the probe as failed. And the PR body's brief_summary is now collapsed to its first line after sentence extraction, matching the same defensive pattern already applied to the PR title. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017TZHqRN57WvdUXvYdmLVZe
A comment pointed at a function name that never existed (_retry_launch_once); the real function implementing that retry shape is _lane_retry_then_fail. A separate test comment was left as unfinished drafting text. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017TZHqRN57WvdUXvYdmLVZe
.spec.md was written into the same directory the fixer clones and pushes from, so a real invocation would hit the pre-push dirty-check and fail every push -- or, if that check were ever bypassed, leak error_id/fingerprint/service/brief into the public PR. Never caught because every existing test mocked push_branch away entirely. The spec now lives under a sibling directory outside the checkout; the new regression test uses a real git repo, a real bare remote, and the real push_branch to prove the worktree stays clean. Also closes a second gap that would have silently defeated a real production run: the STATUS-text parser trusted a printed "STATUS: complete" over the process's actual exit code, so a lane that crashed or timed out right after printing its report still got auto-approved -- directly against CONTRIBUTING.md's "timeout is not zero findings" rule. A non-zero exit now overrides a stale-looking success claim. Smaller fixes, in priority order: bounded timeouts on the git/gh subprocess calls so a hung operation surfaces as a retryable failure instead of blocking the unattended scan forever; push destination now verified against the expected repo URL, not just the remote name; the opened PR's number is persisted so a PR-gate rejection reaches the actual PR as a review comment, not only the next round's regenerated spec; a docstring's overclaim about normalization coverage narrowed to what this PR actually touches; the review-diff prompt now fences its embedded copy dynamically instead of a fixed triple-backtick fence; and a cosmetic double-period fixed in the generated PR summary. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017TZHqRN57WvdUXvYdmLVZe
…ackfill. The prior commit's push-destination verification compared only the trailing org/repo path, stripping the host entirely -- a remote URL pointing at any other domain with a matching org/repo path (including a userinfo-confusion form like https://github.com@evil.com/org/app) would pass. The fixer's own clone step always targets github.com, so the host is now required to match it for both URL and SCP forms. Separately, the PR-number backfill sat inside the same guard that gates whether pr.open needs creating -- once that row reaches done the guard is permanently false, so a crash between the row finishing and the backfill write would skip the backfill forever, silently defeating the prior commit's own PR-gate-visibility fix in that window. The backfill is now its own independent check. Also: DESIGN.md's spec-path documentation corrected to match the prior commit's error-fix-specs move (it still described the old, leaky path); and PR resolution for error-fix tasks now goes strictly through payload.pr_number rather than letting a manually-set numeric task.ref (which has separate checkout-ref semantics for these tasks) take precedence. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017TZHqRN57WvdUXvYdmLVZe
…ions. Three straight rounds each patched a specific bypass of the push- destination host check (a hostile host, a query/fragment authority- parsing confusion, a host-less bare form) -- and this round's review found two more the parse-then-validate approach still admitted: an ext:: git transport-helper injection that smuggles an arbitrary shell command past the check by hiding it in a discarded "userinfo" prefix, and a missing scheme allowlist letting file:// through unchecked. Rather than patch a fifth bypass, the real remote URL now has to fullmatch one of exactly three known-good github.com shapes (https, SCP-style, ssh://) via anchored regex, with org/repo taken only from the match's own capture groups -- nothing else is accepted. This is structurally resistant to new bypass classes instead of chasing them one at a time; the trusted expected_repo config value keeps its existing host-less normalization, untouched. Also, in review: swapped $ for \Z in the three new patterns (defense- in-depth against re.fullmatch's trailing-newline allowance, already inert here due to double-stripping but cheap to close properly), and added the one missing accept-case test (SCP form with a .git suffix through the real push_branch path). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017TZHqRN57WvdUXvYdmLVZe
…ush check. expected_repo was computed from task.repo and passed straight to push_branch, but the destination-allowlist check only runs when expected_repo isn't None -- so a missing, malformed, or stale repo field silently skipped the entire check this PR just spent four rounds hardening, while the push still went ahead under the branch-only identity check. The two existing error_id guards right above this code already fail loudly for the equivalent stale-row case; this mirrors the same pattern for repo. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017TZHqRN57WvdUXvYdmLVZe
…e caller's. push_branch relied on its one caller to pass expected_repo whenever expected_branch was set; it now refuses outright if that invariant is violated, so a future caller (or a bug in the current one) can't silently drop the destination check. Separately, the three helpers that look up a task's pr.open activity matched on branch head alone -- an error_id hex-prefix collision or a reused error_id across sessions could let one task's PR-gate lookups resolve to a different task's PR, backfilling the wrong PR number for a possibly different repo. They now also match on repo. Also: the PR-number lookup returned None on the first row with a malformed result instead of continuing to scan, so a newer corrupted row could permanently shadow an older valid one; and the push- destination guard now resolves repo the same way _drive_one already does (payload first, falling back to the task field) instead of a narrower task-only read that could diverge from what actually opened the PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017TZHqRN57WvdUXvYdmLVZe
_task_pull_request still resolved repo from task.repo alone, while the
pr_number it returns for error-fix tasks comes from payload.pr_number
-- backfilled against the payload-resolved repo everywhere else in this
PR. A task.repo/payload.repo divergence would pair the wrong repo with
that PR number, and this function feeds the PR-gate-rejection comment
path, so a rejection could get posted against the wrong repo entirely.
Now resolves payload-first like run_core.py and fixer_act.py already do.
Also: two comments/docstrings reworded to drop session-relative
phrasing ("this PR touches", "pre-round-24") that reads as a
development note rather than a lasting invariant; insert_pr_open_and_scan
now has a direct unit test against a real Store instead of being
monkeypatched away in every caller; and a test that actually exercises
main._exec_argv moved out of the git_act-scoped test module it didn't
belong in.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017TZHqRN57WvdUXvYdmLVZe
…G.md requires.
grok_pr_quality/grok_pr_logic (and the codex pair) both become ready at
the same time, but the driver only ever processed one per iteration via
a blocking lane launch -- strictly sequential, never actually
concurrent. The two dimensions now launch together via a thread pool.
The concurrency is narrowly scoped: all store I/O for both dimensions
happens on the calling thread, before and after the concurrent phase --
worker threads only ever call the lane launch itself, never a store
method. This matters because the driver's whole scan already runs
inside a store-wide advisory lock (an RLock held for the scan's
duration); a worker thread touching the store from inside that window
would deadlock permanently, not just race.
Getting the finishing side of this right took three passes: the first
rejected dimension's own finish logic used to kick off the next round
immediately, before the sibling dimension's agent record was closed,
which the ledger correctly refuses ("round still has a working agent")
-- round-start is now deferred until both dimensions are finished.
Closing out an abandoned sibling was also reasserting its pre-rejection
head value, undoing the reset the first dimension's rejection had just
made in the same batch -- it no longer asserts a head at all.
Also: PR-gate rejection comments now carry only the extracted findings,
not the full raw lane transcript (STATUS:/REASON:/SCOPE: preamble and
all) that was leaking into the actual GitHub review comment; and a
redundant `git diff --cached` probe that duplicated every staged hunk
already covered by `git diff HEAD` is gone.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017TZHqRN57WvdUXvYdmLVZe
…esult. The prior commit's fix for parallel PR-review dispatch abandoned whichever dimension came second whenever the other hit any terminal outcome, including vendor_unavailable -- which carries no gate/reset side effects to conflict with. That discarded a real, already-obtained review result whenever it landed second: a genuine rejection's findings could vanish entirely if paired with an unrelated vendor hiccup on the other dimension. The actual problem was two different concerns sharing one mechanism: recording each dimension's own verdict (always safe -- checklist resets are idempotent, round-start already collapses to firing once, gate rows are independent per dimension) and deciding the task-level transition (a genuine batch-level decision). Both dimensions that launch are now always fully recorded; a new aggregation step combines every rejection's findings into one spec write and decides the continue-vs-message outcome only after every dimension has been recorded, independent of which order they resolved in. That in turn surfaced a real one: a task correctly failed by one dimension (round-cap or lane-retry exhaustion) could have its failure silently erased back to "implementing" by a sibling's rejection in the same batch -- an actual unbounded retry loop, reachable through two separate write paths (the deferred round-start, and gate recording's own auto-transition on a rejected verdict). Both paths now leave a failed task alone; starting a round on a failed task is refused outright, the same way it already refuses on a done one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017TZHqRN57WvdUXvYdmLVZe
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
1 participant
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.
EN: Adds
agent watch error-fix-work, a script-only driver (no Claude session, no human) that takes anerror.fixconclusion's brief and drives it through the implement chain to a pushed draft PR — spec authoring from the brief, a hardcoded STATUS+FINDINGS pass/fail/retry rule for reviewer/PR-reviewer lanes, and a 5-round cap. Full design/rationale in #53.DE: Fügt
agent watch error-fix-workhinzu, einen rein skriptgesteuerten Treiber (keine Claude-Session, kein Mensch), der eineerror.fix-Konklusion anhand ihres Briefs durch die Implement-Kette bis zum gepushten Draft-PR führt — Spec-Erstellung aus dem Brief, eine fest codierte STATUS+FINDINGS-Regel für Reviewer-/PR-Reviewer-Lanes, ein 5-Runden-Limit. Vollständige Begründung in #53.Full detail
Closes design work tracked in #53.
What this adds:
spec_writtenbecomes script-authorable, narrowly, only forimplementtasks whose payload carries anerror_id(i.e. tasksscan_error_fixcreates) — every other task keeps today's human-only gate.cmd_run's step executor is extracted into a sharedrun_core.execute_spine_step(), extended to interpret reviewer/PR-reviewer lane output (not just implementer) via a hardcoded rule:STATUS: complete+ an explicit zero-entryFINDINGS:section → pass; findings listed → fail/retry; anything unparseable (including a missingFINDINGS:header entirely) → retry once, then stop.fixer_act.py: the outer driver — spec authoring frompayload.briefonly (never raw log lines), a 5-round cap enforced by the driver itself (not the ledger, which has no cap), PR title/body templating, retry-safepr.openinsertion across scans, and graceful handling of a missing/misconfigured vendor CLI (task is left untouched for a later retry rather than permanently failed).agent watch error-fix-work, wired alongside the existingerror-fix/error-decidewatch modes. Not wired intoagent daemonin this PR, matching the precedent set forerror-decide.Known gap, flagged by review, not blocking: four test categories named in the original spec's Verification section were not written across the 4 implement/review rounds — a
spec_writtencarve-out accept/reject regression test, a realistic multi-section sample test for the findings parser, a round-cap-exceeded test, and a direct test of the PR title/body templating function. None indicate a logic bug (traced by hand, matches spec), but they're real untested surface area in exactly the places (round-cap enforcement, the parser gating pass/fail, the carve-out granting script authorship of a normally-human-only key) where a silent regression would be expensive to miss. Opening a fast-follow issue for these rather than extending this PR further.Review history: 4 implement/review rounds. Round 1: 8 findings (2 critical: no tests, tests would've hit a live vendor-CLI subprocess; 1 critical: PR-gate
head_shanever survived pastpushed, which would've failed every task before its first review gate). Round 2 fixed those 6 targeted findings but a full-pass review surfaced 3 more blocking ones (vendor-CLI-unavailable permanently failing instead of leaving the task for retry; a failedpr.openinsert never retried; retry-exhaustion leaving a stray working-agent row that blocks manual recovery). Round 3 fixed those. Round 4 closed the one remaining regression (an interactive-CLI dispatch gap for the new outcome kind round 3 introduced). Final review: no severe findings, whole-branch DoD 1-5 confirmed met, DoD 6 (tests) explicitly called safe-to-ship-and-track rather than blocking.688 tests pass locally (1 pre-existing, unrelated skip — missing
agent_corepackage).