autofix: opt-in, one-shot fixing of the PR reviewer's feedback - #298
Conversation
… dispatched sub-agent outputs are missing (dispatch-conformance gate)
…ring; sentinel-gated job failure; per-line disclosure match (review feedback)
…ites so the rewrite always runs once blocked (re-review feedback)
…template coupling, and the Conformant summary branch (third-round nits)
…a deterministic pre-agent step (orchestrator slice 1)
… ++ content lines; paginate reviews; shared hunk splitter; feedback hardening
…s; warn on a missing scoped.diff; eval roster-shrink assertion (re-review feedback)
… to cache memory (the body stamp never survives gh-aw ingest)
…arrier' into jwies/review-dispatch-gate-local
…SON extraction; the gate reads out-files with the dispatcher's leniency (trial run 29893634730)
…patch-gate-local' into jwies/review-pre-agent-staging-local # Conflicts: # workflows/review/review.md
…ct mirrors stampSource (carrier-fix follow-through)
…te-limit 403 (Retry-After) in the staging fetch (re-review feedback)
🦋 Changeset detectedLatest commit: 7aed595 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| */ | ||
| export const runPlanCli = (fs: PlanCliFs, dir = AUTOFIX_DIR): AutofixPlan => { | ||
| const plan = buildPlan({ | ||
| labels: readJson<string[]>(fs, `${dir}/labels.json`, []), |
There was a problem hiding this comment.
suggestion (non-blocking): A missing or malformed labels.json degrades to [], producing a no-op plan with labelsToRemove: [] and the reason "no autofix label is present on this PR." Since the workflow only fires when an autofix: label was just added, that reason is false and the label is never removed — it reads as "still queued" indefinitely after a staging hiccup, and re-clicking is a no-op because the label is already applied. Consider treating an unreadable labels.json as a refusal whose labelsToRemove is the full allowed autofix label set, so the button invariant survives this failure mode.
| } | ||
| } | ||
|
|
||
| return {status: "current", divergence, stalePaths: stalePaths.sort()}; |
There was a problem hiding this comment.
question (non-blocking): stalePaths only iterates the current diff's signature, and plan.ts drops a work item only when its path is in stalePaths. A thread whose file has no hunks in the current diff (e.g. the author reverted that file to base but the anchored line text still exists) is never marked stale and stays actionable — the agent would then "fix" code that now matches base. Is a "path absent from the current diff" branch intended, or is this handled elsewhere?
| tools: | ||
| github: | ||
| lockdown: false | ||
| min-integrity: none |
There was a problem hiding this comment.
question (non-blocking): Step 1 stages every thread comment verbatim — including replies from arbitrary commenters on this public repo — into an agent that edits files and pushes with the bot token, under lockdown: false / min-integrity: none. The reviewer runs the same settings but only writes comments; here the identical config carries edit-and-push authority, so a crafted reply in a thread is a prompt-injection surface with a much larger blast radius. Is that injection risk acceptable given the deterministic guards don't cover thread content?
There was a problem hiding this comment.
Changes requested — see inline comments.
13 of 13 prior review threads are still unaddressed as of f188e83:
- issue (blocking)
workflows/autofix/lib/staleness.ts:101: Currency guard fails open when the staged diff is empty or header-less.computeHunkSignaturereturns{}for a diff w...
12 non-blocking threads still open
- thought (non-blocking)
workflows/autofix/README.md:60: The re-review is declared the only verification of a fix commit, and both the reviewer and autofix pinclaude-opus-4-8... - suggestion (non-blocking)
workflows/autofix/autofix.md:271: Step 1 has the agent fetch and transcribe five inputs before the deterministic plan runs, so the "determinism boundary"... - suggestion (non-blocking)
workflows/autofix/autofix.md:290: Step 1 tells the agent to buildpr.diffby "concatenating the per-file patches", butget_filespatches carry no `dif... - suggestion (non-blocking)
workflows/autofix/autofix.md:291:pr.diffis staged as the full, unstripped diff, butstaleness.tsandplan.tsdocumentdiffTextas the `full-stri... - suggestion (non-blocking)
workflows/autofix/autofix.md:386: Step 5 aborts when the live head SHA differs from the one Step 1 recorded, but Step 1 reads it from the API while edits... - question (non-blocking)
workflows/autofix/autofix.md:67: Step 1 stages every thread comment verbatim — including replies from arbitrary commenters on this public repo — into an... - suggestion (non-blocking)
workflows/autofix/lib/plan.ts:261: A missing or malformedlabels.jsondegrades to[], producing a no-op plan withlabelsToRemove: []and the reason "... - thought (non-blocking)
workflows/autofix/lib/staleness.ts:106:anchorHunksis the last fully-reviewed fingerprint, carried forward verbatim byfast/flip-gatedruns, but this m... - question (non-blocking)
workflows/autofix/lib/staleness.ts:112:stalePathsonly iterates the current diff's signature, andplan.tsdrops a work item only when its path is in `stale... - nitpick (non-blocking)
workflows/autofix/lib/trailer.ts:59:valueOfmatches trailer keys anywhere in the commit message via a multiline regex, not in the final-paragraph trailer... - note (non-blocking)
workflows/autofix/lib/worklist.ts:102: Thetypeof thread.line !== "number"check folds two of its own named cases — genuinely outdated threads and file-level... - suggestion (non-blocking)
workflows/autofix/lib/worklist.ts:78:buildWorkListreadsthread.comments?.[0]?.bodywithout checkingcomments[0].author, so the "reviewer feedback only...
| # trim more, but it is NOT set here because it has not been measured; add it | ||
| # only with a number behind it. | ||
| # ───────────────────────────────────────────────────────────────────────────── | ||
| env: |
There was a problem hiding this comment.
question (non-blocking): Has the gh-aw unbounded-fetch bug (push_to_pull_request_branch.cjs:935) been filed upstream? The WORKAROUND comment and the changeset cite the exact source line and say "remove once fixed upstream," but neither links an issue, so the removal condition isn't checkable. Since the override rides workflow-level env and reshapes checkout/blob-fetch in every job, filing the issue (or a one-line --depth/--filter PR) and referencing its URL here would bound its lifespan.
…changeset per PR Review feedback on #298, both blocking findings and one suggestion. assessReviewCurrency returned `current` with no stale paths and no degraded note whenever computeHunkSignature produced an empty signature, which happens for an empty diff and for raw get_files patches (no diff --git/---/+++ headers for splitUnifiedDiff to recognise). The guard reported a clean full check having performed none: the one failure direction this module must not have. An unreadable diff now degrades like any other unusable input. buildWorkList never checked who opened a thread, so the reviewer-feedback-only restriction lived solely in prose. A human thread whose first line quotes `**issue (blocking):**` would have become a work item an agent edits code for. Ownership is now enforced where the decision is made. Also collapses six changesets into one, which is what a PR should carry.
…r's feedback Label a PR `autofix: blocking` or `autofix: nits` (they union) and the run fixes the reviewer's open threads in that scope, pushes one commit, replies in each thread, posts a summary, and removes the label. One run per arming. The deterministic half (workflows/autofix/lib) decides everything before the agent edits anything: scope resolution, review-currency gating, the work list, and the commit trailer. The plan is final; the prompt may execute it or stop, never widen it. Currency is checked per file against the reviewer's own fingerprint stamp, so a PR whose author pushed one unrelated fix after the review still gets its other findings fixed rather than being refused wholesale. The push uses KHAN_ACTIONS_BOT_TOKEN, not GITHUB_TOKEN: GitHub creates no workflow runs for GITHUB_TOKEN-triggered events, and the re-review of the autofix commit is the only verification the fix gets.
…ngside the label Both surfaces resolve through one function over a shared token vocabulary, so a value cannot mean one thing as a label and another as a command. The axis model now describes tokens rather than labels. The trigger decides and the two never union: a stale `autofix: nits` label must not widen an explicit `/autofix blocking`. A command-armed run removes no labels, since a comment is self-clearing and any label present was not what armed it. The command gate is spelled out in the workflow's `if:` rather than using gh-aw's `slash_command` trigger, whose compiled gate misses a trailing CRLF and silently killed /review in Khan/webapp#40943.
…un exposed The bash allowlist used gh-aw's documented `"npx *"` form, which compiles to `Bash(npx)` and matches only a bare `npx` with no arguments, so the plan CLI was never executable and the run hand-simulated it instead. `"npx:*"` compiles to `Bash(npx:*)`, the form gh-aw's own defaults use. The prompt now treats a non-executable CLI as a hard stop. The currency guard collapsed "no reviews" with "reviews exist but carry no fingerprint", so a PR with real blocking feedback was told no feedback had been posted. They are now distinct, and the unstamped state degrades to the per-thread anchor check with a note in the summary rather than refusing.
…etch on large monorepos The safe-outputs job gets a depth-1 shallow clone of refs/pull/N/merge, then push_to_pull_request_branch.cjs:935 fetches the PR branch with no --depth. The branch tip is absent and its history never reaches the shallow boundary, so git walks it all the way back. Against Khan/webapp that ran >14 min and 5.6 GB before being cancelled, which is what killed the first trial push. Depth cannot be bounded from outside: git has no fetch.depth config, no gh-aw option touches the safe-outputs checkout, and no step can be injected into that job. Injecting a partial-clone filter via GIT_CONFIG_* does reach the fetch and removes the bulk: same command, 91 s and ~557 MB, exit 0. Composes with gh-aw's own GIT_CONFIG_* use, which appends rather than overwrites (git_helpers.cjs:64-76).
…n with a measurement Pin the autofixer at claude-opus-5, deliberately ahead of the reviewer's claude-opus-4-8: the reviewer's pin is tied to its eval calibration, autofix has no such tie, and writing the fix is the harder half of the pair. gh-aw v0.83.4's firewall (0.27.42) already knows Claude 5 pricing, so no sandbox.agent.version or models: override is needed. Do NOT add remote.origin.tagOpt=--no-tags. Measured back to back against Khan/webapp from identical checkouts: 85 s to completion without it, still transferring past 20 minutes with it. Also drop the em dash from the commit-message template, which is how one reached the bot's first real commit.
…-driven summary, fewer turns The commit subject must now name the change; `autofix: address reviewer feedback` was identical on every run and made git log useless. The body states the problem as a fact about the code, not a pointer to a thread a later reader cannot see. The summary comment is exception-driven. A clean run is already told by the thread reply, the commit, and the engine's own pushed-commit comment; a fourth notification saying the same thing trains people to ignore the bot. It still always posts a refusal, a no-op, anything left unfixed, or a degraded check. The first run spent 131 turns and $4.61 on a six-line fix while moving only ~93 KB of tool output, so the cost was turns, not payload: 7 turns to make one directory, 3 reading this workflow's own lib source, 5 hand-assembling staged JSON, 3 on --help. The prompt now closes each of those.
lib/stage.ts fetches everything the plan needs and writes it before the agent starts, so staging costs zero assistant turns and fails before any credits are spent. Follows the reviewer's orchestrator slice 1 (#280). Turns are what this workflow costs. The first live run: 131 turns, 460 AIC, ~93 KB of tool output total. 61% of the bill was cache reads at a 40.7:1 read-to-write ratio, so caching was already near-optimal and only the turn count could move. Staging was ~15 of those turns. Also records two findings from #287: gh-aw's ingest strips all HTML comments, so the reviewer's fingerprint never reaches a posted review (making autofix's degraded path the normal one, since cache memory is per-workflow and not reachable from here), and this workflow's own summary marker was being silently deleted too.
…changeset per PR Review feedback on #298, both blocking findings and one suggestion. assessReviewCurrency returned `current` with no stale paths and no degraded note whenever computeHunkSignature produced an empty signature, which happens for an empty diff and for raw get_files patches (no diff --git/---/+++ headers for splitUnifiedDiff to recognise). The guard reported a clean full check having performed none: the one failure direction this module must not have. An unreadable diff now degrades like any other unusable input. buildWorkList never checked who opened a thread, so the reviewer-feedback-only restriction lived solely in prose. A human thread whose first line quotes `**issue (blocking):**` would have become a work item an agent edits code for. Ownership is now enforced where the decision is made. Also collapses six changesets into one, which is what a PR should carry.
…ins across REST and GraphQL Both faults from the second trial run (Khan/webapp#41140, run 30416237794). The agent died at turn 1 with `API Error: 400 Model "claude-opus-5" has no AI credits pricing`. claude-opus-5 is in no gh-aw-firewall release's pricing table and no sandbox.agent.version pin can fix that, so the model needs `models.default-ai-credits-pricing` (gh-aw >= v0.83.0), exactly as #294 does for the reviewer roster. The comment this replaces asserted the opposite, reasoning that the firewall was past the release which priced Fable 5; Fable 5 being priced says nothing about Opus 5. Staging reported threadCount: 0 on a PR carrying five reviewer threads. REST spells an App's login `github-actions[bot]` and GraphQL spells the same actor `github-actions`, and staging reads threads over GraphQL but reviews over REST, so one configured spelling cannot match both. Comparison is now suffix-insensitive in staging and in the worklist's ownership guard. The unit tests could not have caught this: every fixture was written in the REST spelling.
…urs the pricing fallback Adding models.default-ai-credits-pricing was necessary but not sufficient: the proxy still 400'd claude-opus-5 for want of pricing. The staged awf-config.json from Khan/webapp#41140 run 30421726630 carried `apiProxy.defaultAiCreditsPricing: {input: 5, output: 25}` and the request was rejected anyway, so the field reaches the config and the compiler's default firewall (v0.27.42) ignores it. #294 verified awf v0.27.27 accepts and maps that field, which is why the reviewer pins it. Autofix inherited the default instead. Pinning to the version with evidence behind it.
…back does not work anywhere Three live runs on Khan/webapp#41140, each isolating one variable: 30416237794 no fallback configured 400 30421726630 fallback + firewall v0.27.42 (compiler default) 400 30422315631 fallback + firewall v0.27.27 (the version #294 names) 400 In both of the last two the staged awf-config.json carried `apiProxy.defaultAiCreditsPricing: {input: 5, output: 25}`, and for the third the job log confirms api-proxy:0.27.27 was the image pulled. v0.27.42 is the newest firewall release, so there is no version left to try. So `models.default-ai-credits-pricing` does not suppress the credits guard for claude-opus-5 on any currently available firewall, contrary to #294's description. Holding the model at claude-opus-4-8, which is what the first successful autofix run used, so the trial can produce the cost and comment data it exists to produce. The frontmatter records the evidence and says to restore the model, its models: block and the sandbox pin together. This blocks #294 the same way: its whole roster would 400.
| Addresses the PR reviewer's own feedback on demand. Opt in per PR with an | ||
| `autofix: blocking` or `autofix: nits` label; the run fixes the reviewer's | ||
| open threads in that scope, pushes one commit, replies in each thread, and | ||
| removes the label. One run per arming. |
There was a problem hiding this comment.
It seems like it would be better (if this is only to trigger a single run) to instead use the /fix or /autofix convention.
(Although, I'll admit that I'm not sure what value this is providing right now since I would imagine that if folks wanted this they could just have their harness handle this process for them.)
There was a problem hiding this comment.
Both the /autofix and label convention are supported.
Agreed that as-is this isn't particularly interesting at the moment, it's more of a base to build on. I imagine using this when I'm not at my machine, allowing me to address bot feedback from my phone. The useful bit comes next. I'm going to add:
- support for preemptively adding this autofix label, allowing the autofixer to immediately respond to bot feedback.
- per-user configuration, allowing each user to have a global preference.
- support for re-review cycles
I think the above will be a nice improvement to workflows. If the blocking bot feedback is consistently sound, we should generally be able to have bots automatically address it. Nits/questions are tougher, but if we can at least have a baseline "correctness assurance cycle" to harden PRs, we could reduce load on engineers and reviewers. Fewer bot comments to have to read and fewer issues in code for reviewers to deal with.
| # the command — never activates the workflow. That silently killed `/review` in | ||
| # Khan/webapp#40943. `scope.ts`'s parser tolerates the same shapes; keep the two | ||
| # in step. | ||
| if: "(github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository && startsWith(github.event.label.name, 'autofix: ') && !contains(github.event.pull_request.labels.*.name, 'skip-ai-review')) || (github.event_name == 'issue_comment' && github.event.issue.pull_request != null && (github.event.comment.body == '/autofix' || startsWith(github.event.comment.body, '/autofix ') || startsWith(github.event.comment.body, '/autofix\n') || startsWith(github.event.comment.body, '/autofix\r') || startsWith(github.event.comment.body, '/autofix\t')))" |
There was a problem hiding this comment.
It seems like this is handling /autofix comments but nothing is triggering when there is a comment like that left - I think this may need to be re-worked?
…stablished, not proven The previous note claimed the pricing fallback "does not work on any firewall release that currently exists". That overstates the evidence. What the three runs show is that it did not work in the configuration tried; the spec documents the mechanism, and config-mapper.ts maps the field in both v0.27.27 and v0.27.42, so version is not the variable and the pin has been dropped. Records the untried combination instead: those runs supplied the fallback AND a models.providers cost entry, and spec 10.7.1 applies the fallback only to a model that cannot be resolved, so the providers entry may be short-circuiting it. Also records that #294's own lock carries no claude-opus-5, so its verified claim is a reading of the spec rather than a run.
…to read the whole staging directory Step 1 said "Read these files" above a list of a dozen staged inputs, most of which only the sub-agents consume. Taken literally that pulls the whole change's diff into the orchestrator's context, which is the waste the same sentence is trying to prevent. Say which two files are the orchestrator's and leave the rest to the steps that use them.
… jwies/autofix-v1
…n' into jwies/review-dispatch-gate
…gate' into jwies/review-pre-agent-staging
There was a problem hiding this comment.
Changes requested — see inline comments.
8 of 16 prior review threads resolved; 8 still unaddressed as of ca109de:
8 non-blocking threads still open
- thought (non-blocking)
workflows/autofix/README.md:60: The re-review is declared the only verification of a fix commit, and both the reviewer and autofix pinclaude-opus-4-8... - question (non-blocking)
workflows/autofix/autofix.md:301: Has the gh-aw unbounded-fetch bug (push_to_pull_request_branch.cjs:935) been filed upstream? The WORKAROUND comment an... - suggestion (non-blocking)
workflows/autofix/autofix.md:463: Steps 4-5 go straight from edit to push with no check on the agent's own edits, thoughnpx:*/node:*are allowlisted.... - question (non-blocking)
workflows/autofix/autofix.md:67: Step 1 stages every thread comment verbatim — including replies from arbitrary commenters on this public repo — into an... - suggestion (non-blocking)
workflows/autofix/lib/plan.ts:261: A missing or malformedlabels.jsondegrades to[], producing a no-op plan withlabelsToRemove: []and the reason "... - thought (non-blocking)
workflows/autofix/lib/staleness.ts:137:anchorHunksis the last fully-reviewed fingerprint, carried forward verbatim byfast/flip-gatedruns, but this m... - question (non-blocking)
workflows/autofix/lib/staleness.ts:143:stalePathsonly iterates the current diff's signature, andplan.tsdrops a work item only when its path is in `stale... - note (non-blocking)
workflows/autofix/lib/worklist.ts:127: Thetypeof thread.line !== "number"check folds two of its own named cases — genuinely outdated threads and file-level...
| * so a stale generated path can never drop real work. Stripping would mean | ||
| * threading the router's `generatedFiles` through staging to buy nothing. | ||
| */ | ||
| export const assessReviewCurrency = ( |
There was a problem hiding this comment.
note (non-blocking): By this module's own header, gh-aw's sanitizer strips the reviewer's body stamp before any review posts, so assessReviewCurrency returns unverifiable on essentially every real run — which makes the current branch here, the stalePaths plumbing in plan.ts, and their test surface exercise a path production never reaches. It's acknowledged and degrades correctly to the anchor-only check, so this is a deliberate bet, not a defect; flagging only so you can weigh whether that much currently-untriggered machinery belongs in a "deliberately narrow first iteration."
| }) | ||
| .catch((error) => { | ||
| // eslint-disable-next-line no-console | ||
| console.error(`staging failed: ${error?.message ?? error}`); |
There was a problem hiding this comment.
note (non-blocking): The sibling pre-agent staging CLI surfaces failures as GitHub Actions annotations (workflows/review/lib/stage-pr.ts uses console.error("::error title=review staging::...")); this one logs a plain staging failed: ... with no ::error prefix, so an autofix staging failure lands only in raw step logs rather than the annotation surface. Matching the idiom (and typing the catch as (error: unknown) with an instanceof Error narrowing) would keep the two staging steps triageable the same way.
| return {status: "no-review"}; | ||
| } | ||
|
|
||
| const stamp = findLatestStamp(reviews); |
There was a problem hiding this comment.
note (non-blocking): findLatestStamp returns the newest stamped review, not the newest review, so if a newer review is unstamped while an older one carries a stamp, currency is checked against the older stamp and paths the newer review actually saw read as stale — dropped as stale-path with a summary that says they "changed since the last review" when the last review is exactly what flagged them. Can't fire today (all stamps are stripped), but worth guarding for whenever a stamp carrier lands — e.g. treat a stamp older than the newest bot review as unverifiable.
… jwies/autofix-v1
… path claimed Blocking finding on #298. The issue_comment branch of the workflow's `if:` carries neither the fork check nor the skip-ai-review check, because that event has no github.event.pull_request to read them from. Both this file's comment and the README said the checks "move into the plan"; the plan never implemented them, so a command-armed run reached the agent with neither. Staging now records whether the head is a fork, and plan.ts refuses a fork or a skip-ai-review PR before any work is planned. Enforced on every path rather than only the command one: a duplicated guard is cheap and a missing one authorises a code push. Unknown fork status refuses, so a staging gap cannot open the hole back up. Also records why nobody has been able to test /autofix: issue_comment is a repository-level event, so GitHub reads the workflow from the default branch and never from a PR head. Every run of the Khan/webapp#41140 trial was pull_request, and a reviewer's /autofix comment there did nothing. The command surface is only reachable once the workflow is on the consuming repo's default branch. Threads AUTOFIX_BOT_LOGIN through to the worklist's ownership guard, which was re-filtering against the hardcoded default while staging honoured the override.
…t the label jeresig on #298: for a single run the /autofix convention reads better than a label. The description named only the label, which both undersold the command surface and made it look like an afterthought. They are peers; the description now says so and leads with the command.
e3a22e5 to
1fe63af
Compare
1fe63af to
2de9c61
Compare
Retargets the PR from the jwies/review-pre-agent-staging staging branch to main, which now carries that branch's work squash-merged (#280, #282, #283, #284, #288, #296) plus the later slices this branch never received. The head branch carried pre-squash copies of the review stack, so every conflict was a stale duplicate rather than a real divergence. All seven resolve to main: - .github/aw/actions-lock.json: main's superset. The autofix compile at v0.83.4 had pruned the setup-cli entry; main is at v0.83.4 too now, so the version skew the PR description flagged between autofix.lock.yml (0.83.4) and review.lock.yml (0.81.6) is gone. - lib/dispatch-gate.ts, lib/dispatch-gate.test.ts, lib/stage-pr.ts, lib/stage-pr.test.ts: add/add against main's merged versions. - review.md, README.md: main supersedes the head copies (timeout 40, the scripted-dispatch SDK install, code-owned disciplines extraction, the retired firewall pin, the lens payload seam). No autofix commit ever touched workflows/review/, so nothing is lost taking main wholesale there: the merged review stack is byte-identical to main and the whole merge reduces to main plus the autofix package.
Cross-cutting patterns (post-approval, non-blocking)The individual findings are on the review; these are the recurring shapes underneath them, offered as themes rather than gates — approval stands.
None of this blocks the merge — the change is well-shaped and the guards fail closed. These are the threads worth pulling as v2 lands the cadence and source axes. |
There was a problem hiding this comment.
Review — APPROVE
A carefully built change. The new workflows/autofix/ package puts plan.ts up as an explicit determinism boundary, the guards fail closed in a deliberate order (fork → skip-ai-review → currency), and the label taxonomy, thread parser, and currency fingerprint are reused from review/lib rather than re-derived. Eight lenses (correctness, holistic, completeness, test-adequacy, first-principles, conventions, skill-audit, thread-reconcile) turned up no blocking defects — every surviving finding is non-blocking. Approving.
I left 5 inline comments; the rest are collected below. None gate the merge.
Re-review accountability
Since the last review the code addressed 4 open threads (resolving them now):
- plan.ts bot-login threading —
buildPlannow takesbotLoginand threads it throughbuildWorkList. - README "posts one summary comment" — now conditional ("a clean run stays quiet").
- no execution-based check between edit and push (two threads) — Step 4 gained a "verify what you can, be honest about what you cannot" section and Step 7 now reports an unverified fix.
Still open (kept, unchanged by this revision): labels.json→[] degradation; reduced-depth stalePaths anchoring; worklist file-level vs outdated-anchor conflation; stalePaths iterating only the current diff; the WORKAROUND :935 comment with no linked issue; the currency-machinery note; staging failed: logged without an ::error prefix; findLatestStamp not guarding a newer unstamped review. Several are re-surfaced as observations below.
Other non-blocking observations (below the inline confidence bar)
Nine lower-confidence notes — worth a glance, none blocking
- suggestion · autofix.md Step 5 (head-SHA source): "the live head SHA" is unspecified; if the agent reads it via
git rev-parse HEADin the static Actions checkout it equals the stagedhead-sha.txtby construction and the stale-head guard never fires. Name the GitHub API as the source for the live value. - suggestion · autofix.md Step 5 (head-moved guard): this is the one fail-closed guard enforced in prose rather than in
plan.ts, and it's TOCTOU-racy since the push runs in a later job. A staged-vs-live SHA check in code would move it to the layer the design says decisions belong in. - suggestion · plan.ts (closed/merged PR): no guard refuses a closed or merged PR on either arming surface;
state/mergedare already in the PR REST payloadcollectInputsfetches, so refusing in the same block as the fork guard is small. - nitpick · worklist.ts:130: file-level threads (
linenull,subjectType: FILE) are reported asoutdated-anchor, misstating why they were skipped; a distinctfile-levelreason reads straight. - note · stage.ts:396 (
restPaged): the 20-page/2000-item cap truncates silently despite the comment saying it doesn't; atruncated: truemarker would make the degradation observable, and files whosepatchthe API omits are invisible to the currency check. - note · plan.ts (unreadable labels.json): a label-armed run whose
labels.jsonis unreadable degrades to a no-op that leaves the arming label on (test-pinned at plan.test.ts:289). The plan can't know which labels to remove, but a distinct "could not read staged labels" reason would at least be honest rather than "nothing armed this run." - thought · autofix.md (re-review as verification): the same pinned model verifies its own fix and the fixer's reply posts under the reviewer's own login, so "the re-review IS the verification" is weaker than stated — CI plus a human approver is the real backstop, and the docs could say so.
- thought · autofix.md (command surface in v1): the
/autofixcommand path ships effectively untrialed (issue_commentreads the workflow from the default branch), carrying the weakest-gated arming surface and the plan-layer guards that exist only because it can't gate in theif:. A label-only v1 would be smaller; the sharedscope.tsvocabulary already prevents drift when the command lands. - note · PR description: says "64 unit tests"; the suite actually has ~130. Over-delivery — noting only because it was an explicit claim.
Verdict is mechanical: no blocking finding survived validation, so APPROVE. The one candidate raised as blocking (untested isFork derivation) was downgraded to a non-blocking todo because the derivation is currently correct and fails closed, as does the plan guard — a coverage gap, not a present defect.
| const baseLogin = (login: string): string => | ||
| login.endsWith("[bot]") ? login.slice(0, -"[bot]".length) : login; | ||
|
|
||
| const sameLogin = (a: string, b: string): boolean => |
There was a problem hiding this comment.
issue (non-blocking): sameLogin strips a [bot] suffix before comparing, and THREADS_QUERY fetches only author { login } with no __typename. GraphQL reports a GitHub App as github-actions (no suffix) — but it reports a regular user account's login the same way, so with a custom AUTOFIX_BOT_LOGIN (e.g. khan-actions-bot[bot]) any user who registers the base name khan-actions-bot becomes indistinguishable from the App here. That partially defeats the worklist.ts ownership guard (lines 91-98) whose whole job is to stop a human thread becoming a work item an agent then edits code for.
For the default github-actions[bot] the base name is GitHub-reserved, which contains but doesn't eliminate the class — hence non-blocking. Fetching author { __typename login } and requiring Bot before the suffix-insensitive compare would close it.
| const headRepo = isRecord(head["repo"]) ? head["repo"] : {}; | ||
| // Absent or unreadable repo data reads as a fork, so the guard fails closed. | ||
| const headRepoName = str(headRepo["full_name"]); | ||
| const isFork = headRepoName === "" || headRepoName !== `${owner}/${repo}`; |
There was a problem hiding this comment.
todo (non-blocking): The isFork derivation here — the security-critical input the plan's fork guard rests on — has no direct test. plan.test.ts covers the refusal by injecting isFork, but no collectInputs fixture in stage.test.ts sets head.repo.full_name (they all use head: {sha}), so none of the three cases is ever asserted: same-repo head → false (the push-authorizing case), fork head → true, missing/unreadable repo → true (the fail-closed case).
The derivation is currently correct and both it and the plan guard fail closed, so this is a coverage gap rather than a present defect — but it's the single input that authorizes an unattended bot-token push, and the package otherwise tests this layer exhaustively (~130 cases). Three collectInputs assertions would close it.
| isResolved | ||
| path | ||
| line | ||
| comments(first: 100) { |
There was a problem hiding this comment.
suggestion (non-blocking): THREADS_QUERY stages full reply chains (comments(first: 100)) into threads.json, but buildWorkList reads only comments[0] and plan.ts passes the threads nowhere else — so the reply chains have no consumer in autofix. Anyone with read access can reply in a reviewer thread, so this is untrusted text sitting verbatim in the workspace of an agent that has cat:* and push authority, guarded only by Step 1's prose ("you do not need to read most of these").
Consider staging only the bot opener (comments[0]) here, keeping the StagedThread shape, and adding the chains back if/when a consumer actually exists.
|
|
||
| /** Every autofix label present, so a label-armed refusal still clears the PR. */ | ||
| const autofixLabelsOn = (labels: readonly string[]): string[] => | ||
| labels.filter((label) => label.startsWith(AUTOFIX_LABEL_PREFIX)); |
There was a problem hiding this comment.
suggestion (non-blocking): autofixLabelsOn matches any autofix: prefix, but the workflow's remove-labels.allowed is a closed set of the five known labels. An autofix: <typo> label passes the if: gate, the plan refuses it as unrecognised and puts it in labelsToRemove — but the safe-output silently drops the removal as out-of-allowlist, so the label persists reading as "still queued", the exact state the design says must never happen (README:52-53, plan.ts:16-20).
Deriving remove-labels.allowed from the same token vocabulary, or clamping autofixLabelsOn to the known set, keeps the two in step.
| * for the caller's stop rule: an unreadable history under-reports work already | ||
| * done, so a cycle cap derived from it can only trip earlier, never later. | ||
| */ | ||
| export const summariseLedger = (messages: readonly string[]): Ledger => { |
There was a problem hiding this comment.
nitpick (non-blocking): summariseLedger uses British -ise, where the sibling review/lib (which this package imports from throughout) spells this morphology American — summarize, normalizeBody, serialize. Since it's an exported cross-package identifier, and its one caller lives in plan.ts, summarizeLedger would match the convention. (British spelling in the prose comments is fine as author voice — this is only about the identifier.)
The label stops the reviewer's next run; it does not withdraw a review already posted, so a labelled PR can still be carrying current findings. The reviewer even suggests the label from inside a review body it just posted, which makes "labelled, with live findings" a state the workflow steers people into rather than a corner case. Reading the label as "no AI may act on this PR" silently swallowed an explicit `autofix:` label from someone with write access, and that opt-in IS the authorisation while autofix only ever runs when a human arms it. Removed from the label path's `if:` (shared source, this repo's install, and the recompiled lock) and from `plan.ts`, where the refusal was justified as "with no review there is nothing to fix" while the guard that actually checks for a review sits ten lines below it and covers exactly that case. Tests pin the new behaviour on both surfaces: a labelled PR with a current review arms, and a labelled PR with no review still refuses on review currency. Revisit when autofix runs automatically rather than only when armed. That is when a push nobody asked for becomes possible, and autofix should get its own opt-out then rather than borrowing the reviewer's. Surfaced by the Khan/webapp#41177 docs trial: suppressing the repo's installed reviewer with the label also disarmed the autofix arm, so no trial could isolate one reviewer and exercise autofix at the same time.
There was a problem hiding this comment.
Requesting changes — one new blocking finding on this push: a fail-open in the thread-staging GraphQL port (stage.ts). GitHub returns 200-with-errors on rate-limits/partial failures; the port doesn't inspect errors, so a throttled run stages an empty threads.json, reports a clean no-op, and clears the arming label while blocking findings sit open — the opposite of the module's fail-closed contract. Details inline.
The rest of the package remains carefully built. Four smaller new observations were posted inline as non-blocking: the remove-labels allow-list can't clear an unrecognised autofix: * label; the intended quiet clean-run path is effectively unreachable (so every clean fix posts the "degraded" note); the Step 5 moved-head guard never says how to read the live head SHA (a local rev-parse makes it vacuous); and the README's "isLoopEligible is enforced in code" overstates a function with no production caller.
One lower-confidence note not posted inline: the machine-readable Autofix-Threads trailer is composed by the model in Step 5 rather than emitted verbatim by plan.ts, so a dropped/reordered thread id would silently corrupt the trial's ledger — cosmetic for v1 (nothing reads it back yet), worth a determinism-boundary pass before the cadence axis lands.
Re-review depth was full: no prior fingerprint stamp was readable (the sanitizer strips the body stamp), so the whole diff was re-examined. Dimensions: no specialist lenses applied (routing found no security/data-migration/API-federation/etc. surface); skill-auditor, completeness, and conventions found nothing new.
13 of 13 prior review threads are still unaddressed as of a0c17ac:
13 non-blocking threads still open
- question (non-blocking)
workflows/autofix/autofix.md:324: Has the gh-aw unbounded-fetch bug (push_to_pull_request_branch.cjs:935) been filed upstream? The WORKAROUND still cites no issue. - suggestion (non-blocking)
workflows/autofix/lib/plan.ts:296: A missing or malformedlabels.jsondegrades to[], producing a no-op plan withlabelsToRemove: []. - suggestion (non-blocking)
workflows/autofix/lib/plan.ts:97:autofixLabelsOnmatches anyautofix:prefix, butremove-labels.allowedis a closed set of five. (Re-surfaced this push from theautofix.mdallow-list side.) - issue (non-blocking)
workflows/autofix/lib/stage.ts:106:sameLoginstrips a[bot]suffix before comparing, andTHREADS_QUERYfetches onlyauthor { login }with no__typename. - suggestion (non-blocking)
workflows/autofix/lib/stage.ts:128:THREADS_QUERYstages full reply chains, butbuildWorkListreads onlycomments[0]. - todo (non-blocking)
workflows/autofix/lib/stage.ts:259: TheisForkderivation has no direct test. (Re-raised blocking by test-adequacy this push; held non-blocking — the derivation is correct and fails closed,plan.tsrefuses onisFork !== false, so the exposure is only a future regression.) - note (non-blocking)
workflows/autofix/lib/stage.ts:453: The catch logs a plainstaging failed:with no::errorannotation prefix and untypederror. - note (non-blocking)
workflows/autofix/lib/staleness.ts:112:findLatestStampreturns the newest stamped review, not the newest review. - thought (non-blocking)
workflows/autofix/lib/staleness.ts:137:anchorHunkstreats a path's absence from the carried-forward fingerprint as unreviewed. - question (non-blocking)
workflows/autofix/lib/staleness.ts:143:stalePathsonly iterates the current diff's signature. - note (non-blocking)
workflows/autofix/lib/staleness.ts:99: The sanitizer strips the reviewer's body stamp before any review posts, soassessReviewCurrencyis unverifiable in practice. - nitpick (non-blocking)
workflows/autofix/lib/trailer.ts:137:summariseLedgeruses British-ise, where the siblingreview/libspells it-ize. - note (non-blocking)
workflows/autofix/lib/worklist.ts:130: Thetypeof thread.line !== "number"check folds outdated and file-level threads into oneoutdated-anchorreason.
| # a refusal. A label left on after the run reads as "still queued" when | ||
| # nothing is, and re-arming is one click. | ||
| remove-labels: | ||
| allowed: |
There was a problem hiding this comment.
suggestion (non-blocking): An unrecognised autofix: <x> label can't be cleared and persists as apparent "still queued."
The workflow if: fires on any autofix: prefix, and plan.ts's prefix-matching autofixLabelsOn (plan.ts:97) puts an unknown token like autofix: everything into labelsToRemove even though resolveTokens refused it — but this allowed: list enumerates only the five known labels, so gh-aw won't remove it and the label sticks, contradicting the "removed on EVERY outcome" comment just above. (It doesn't itself re-fire — triggers are only pull_request:[labeled] and issue_comment:[created].) Consider deriving allowed: from the autofix: namespace, or having the plan only queue removal for recognised labels.
|
|
||
| ## Step 5: Push one commit | ||
|
|
||
| Compare the live head SHA against `/tmp/gh-aw/autofix/head-sha.txt`, which |
There was a problem hiding this comment.
question (non-blocking): How is "the live head SHA" meant to be obtained here?
head-sha.txt was captured from the job's checkout via git rev-parse HEAD, and the checkout's local HEAD does not move when the author pushes to the remote mid-run. Detecting a mid-run push needs a git fetch/git ls-remote or an API read, but Step 3 says "read from disk, not through the API" and nothing in this step names how to read the live value — so the cheapest reading is a local git rev-parse HEAD, which is tautologically equal to the staged file and makes the moved-head guard vacuous. Was a remote read (e.g. git ls-remote origin <branch>) intended here?
|
|
||
| ### One constraint that outlives v1: nits never loop | ||
|
|
||
| `isLoopEligible` is enforced in code rather than left to convention. |
There was a problem hiding this comment.
thought (non-blocking): "enforced in code rather than left to convention" overstates the current state — isLoopEligible has no production caller.
isLoopEligible (scope.ts) is referenced only by scope.test.ts and doc comments; nothing in the armed path consults it. What actually enforces nits-never-loop in v1 is the token table rejecting autofix: loop, not this function. As written, whoever adds the cadence axis could take "enforced in code" at face value and wire a loop that never calls it. Either soften the claim, or note that the enforcement point is "the loop implementer must call isLoopEligible" (itself a convention until a caller exists).
Blocking finding on #298. The GraphQL port threw only on `!res.ok`, but GraphQL does not signal failure by status: GitHub answers RATE_LIMITED, node-access failures, and partial field failures with HTTP 200 and an `errors` array. A throttled run therefore read as a successful one, `threadsConnectionOf` found no connection, `collectThreads` treated that as the end of the pages, and staging wrote an empty threads.json for a PR with open reviewer threads. Everything downstream then behaves correctly on false input, which is what makes it worth fixing rather than logging. Reviews come over REST on a separate quota, so the currency check still passes; the plan sees no actionable threads and emits a no-op; the no-op populates labelsToRemove. The label comes off and the author is told there is nothing to fix while the findings it was armed for sit open, with nothing left to re-arm from. That is the fail-open direction this module's contract rules out and the REST paths' own `throw` already rules out for them. Any `errors` entry is fatal, including alongside partial data: fixing whichever threads happened to arrive and then clearing the label is worse than refusing, because it reports success over the ones that did not. A body with no reviewThreads connection is likewise fatal rather than "no threads". Throwing fails the staging step before any AI spend and leaves the label in place. Guarded at both the transport and the reader. Duplicated on purpose, the same reasoning as the fork guard in plan.ts: the check is cheap and its absence clears an arming label on a PR with live findings. The reader is also the layer the unit tests can reach, since the real port is built inside the `require.main` block. The test harness defaulted to `{}` for unsupplied pages, which the old code read as "no threads"; it now defaults to a well-formed empty page, so the fail-closed tests are the only ones exercising the error paths. Verified the three new guards are load-bearing by reverting them: 3 fail, 27 pass.
There was a problem hiding this comment.
Re-review: APPROVE ✅ (was REQUEST_CHANGES)
The single blocking finding from the prior review — the GraphQL thread-fetch port failing open on HTTP‐200‐with‐errors (rate limits / node-access failures), which staged an empty threads.json and cleared the arming label on a false no-op — is fixed, and the fix is correct and complete. Commit 7816541 addresses it directly.
The fix (verified independently by the correctness, completeness, and test-adequacy passes)
assertNoGraphqlErrorsthrows on any non-emptyerrorsarray, guarding both the transport port (stage.ts:468) and the reader (stage.ts:219), and it runs inside thefor(;;)pagination loop so every page is checked — not just the first.- A missing
data.repository.pullRequestconnection now throws (stage.ts:221–226) instead of the oldbreak-returning-empty, so a throttled or partial fetch can no longer masquerade as "this PR has no threads" and clear the label. Non-record bodies fail closed the same way. - The four new
collectThreadstests (rate-limit-as-200, partial-data-with-errors, malformed body, well-formed-empty counterexample) plus fourassertNoGraphqlErrorsunit tests are load-bearing — reverting either guard fails at least one assertion, consistent with the "3 fail / 27 pass" claim. The harness default change (unsupplied pages →onePage([])) isolates the error-path tests without masking the "stops rather than looping" case.
Non-blocking (2 inline)
stage.ts:272— one malformed-page shape (hasNextPage:truewith a missing/emptyendCursor) stillbreaks and stages a subset instead of failing closed. GitHub's Relay contract guarantees a cursor whenhasNextPageis true, so there's no realistic trigger — but it's the one malformed shape left inconsistent with the fail-closed contract this commit establishes..changeset/autofix.md:11— the README's refusal contract (~lines 108–112) wasn't synced with the new fail-closed thread-fetch path.
Advisory — design/structure, no action required
- The reader-level guard duplicates the transport guard because the real port is built inside the untestable
require.main === moduleblock (which also holds!res.okhandling and the page cap). Extracting abuildRealPort/run*Clifactory — as the peerplan.ts(runPlanCli) and the reusedstage-pr.ts(runStagePrCli) do — would make the transport directly testable and give each invariant a single home. (first-principles + conventions) - All-
errors-fatal is right for the transient case (a rate-limited fetch retries via/autofix), but a persistent node-levelerrorsentry returned alongside complete data would make every staging run throw for that PR with no override path. Worth a thought only if that shape ever shows up in practice. (first-principles, low confidence)
The isFork-derivation coverage gap (stage.ts:298) remains an open non-blocking thread; held non-blocking again, consistent with prior runs (the derivation is correct and fails closed, and plan.ts refuses on isFork !== false).
17 of 17 prior review threads are still unaddressed as of 7816541:
17 non-blocking threads still open
- thought (non-blocking)
workflows/autofix/README.md:100: "enforced in code rather than left to convention" overstates the current state —isLoopEligiblehas no production caller. - suggestion (non-blocking)
workflows/autofix/autofix.md:160: An unrecognisedautofix: <x>label can't be cleared and persists as apparent "still queued." - question (non-blocking)
workflows/autofix/autofix.md:324: Has the gh-aw unbounded-fetch bug been filed upstream? - question (non-blocking)
workflows/autofix/autofix.md:488: How is "the live head SHA" meant to be obtained? Local HEAD doesn't move when the author pushes mid-run. - suggestion (non-blocking)
workflows/autofix/autofix.md:572: The quiet clean-run path is effectively unreachable, so every clean fix posts the degraded note. - suggestion (non-blocking)
workflows/autofix/lib/plan.ts:296: A missing/malformedlabels.jsondegrades to[], a no-op plan withlabelsToRemove: []. - suggestion (non-blocking)
workflows/autofix/lib/plan.ts:97:autofixLabelsOnmatches anyautofix:prefix, butremove-labels.allowedis a closed set of five. - issue (non-blocking)
workflows/autofix/lib/stage.ts:106:sameLoginstrips[bot]butTHREADS_QUERYfetches onlyauthor { login }with no__typename. - suggestion (non-blocking)
workflows/autofix/lib/stage.ts:128:THREADS_QUERYstages full reply chains, but onlycomments[0]is consumed. - todo (non-blocking)
workflows/autofix/lib/stage.ts:298: TheisForkderivation has no direct test. - note (non-blocking)
workflows/autofix/lib/stage.ts:499: The sibling staging CLI surfaces failures as GitHub Actions annotations; this one logs a plain message. - note (non-blocking)
workflows/autofix/lib/staleness.ts:112:findLatestStampreturns the newest stamped review, not the newest review. - thought (non-blocking)
workflows/autofix/lib/staleness.ts:137:anchorHunkscarried forward verbatim byfast/flip-gatedruns. - question (non-blocking)
workflows/autofix/lib/staleness.ts:143:stalePathsonly iterates the current diff's signature. - note (non-blocking)
workflows/autofix/lib/staleness.ts:99: The sanitizer strips the body stamp before any review posts. - nitpick (non-blocking)
workflows/autofix/lib/trailer.ts:137:summariseLedgeruses British-isewhere siblingreview/libspells it American. - note (non-blocking)
workflows/autofix/lib/worklist.ts:130: Thetypeof thread.line !== "number"check folds two named cases together.
| } | ||
| const next = pageInfo["endCursor"]; | ||
| if (typeof next !== "string" || next === "") { | ||
| break; |
There was a problem hiding this comment.
suggestion (non-blocking): one malformed-page shape still stages a subset instead of failing closed.
This commit makes every other malformed thread-fetch response fatal — an errors array, a non-record body, and a missing connection all throw. But a page that reports hasNextPage: true with a missing/empty endCursor still takes the break here and returns whatever was collected so far, which is the one outcome the changeset calls "worse than refusing" (a partial thread set → the plan fixes a subset and clears the arming label with later-page findings still open).
Per GitHub's Relay pagination endCursor is non-null whenever hasNextPage is true, so this only arises on a malformed response — the same class you now treat as fatal — which is why this is a consistency point, not a live bug. Making it uniform would be: throw here (as the missing-connection path does) and flip the stage.test.ts "stops rather than looping" case to expect a rejection.
|
|
||
| Everything except the code edit is deterministic. `lib/stage.ts` runs as a pre-agent step and fetches the inputs before the agent starts; `lib/plan.ts` then resolves scope, checks review currency, builds the work list, and renders the commit trailer. The plan is final: the prompt may execute it or stop, never widen it. | ||
|
|
||
| Guards fail closed. Currency is checked per file so one unrelated push doesn't refuse the whole run; unparseable labels, outdated anchors, threads a human opened, an unreadable diff, and a head that moves mid-run are all excluded. Refusal is reserved for a PR with no review at all, and for a thread fetch that fails: GitHub reports GraphQL rate limits and node-access failures as HTTP 200 with an `errors` array, so staging treats any `errors` entry or an unparseable body as fatal rather than as "this PR has no threads", which would clear the arming label while the findings it was armed for stayed open. |
There was a problem hiding this comment.
note (non-blocking): this changeset now documents the fail-closed thread-fetch refusal, but workflows/autofix/README.md (~lines 108–112) wasn't synced to match.
The README's refusal contract still says every refusal "clears any label that armed it" and frames a PR with no review as the only currency state that refuses. This commit's new refusal deliberately does the opposite — staging throws before the plan runs, so the arming label is intentionally retained for retry — and the README's "What it refuses to do" list omits the path entirely. Worth a one-paragraph README update so the prose doesn't contradict the shipped behavior.
…, and the summary says so The docs claimed the reviewer's re-review is the verification an autofix commit gets, in the indicative, and Step 7 made the summary comment promise it: "The reviewer will re-review this push." On Khan/webapp#41194 that promise was false. Autofix pushed ad8da8d4, the shim posted /review, and the re-review died in gh-aw's "Install AWF binary" step before the model ran. The commit sat unverified for 36 minutes until a human noticed and re-triggered. Nothing on the PR said so. That repo's reviewer is an issue_comment local override, so the run's head SHA is a default-branch merge commit and it never joins the PR's check suite; status-comment is false, so it posts nothing; and gh-aw's own fallback tried to file a failure issue and got "410 Issues has been disabled in this repository". The only trace was the eyes reaction the activation job had already added, which is indistinguishable from "still running". Of that repo's last 100 reviewer runs, 15 of the 53 that started ended in failure, so this is a shape to expect rather than a one-off. The gap is not really autofix's: where the reviewer is push-triggered, as the shared workflow ships it, a failed re-review is already a red X on the commit autofix pushed. What autofix owed the human re-arming loop was the truth, so the fix is prose in the four places that overstated it, plus one line of output. Step 7 now posts exactly one comment on every path, and states that nothing has checked the commit. The quiet-clean-run branch is retracted deliberately, and the section says so and says why: its rationale was that a clean run is already told in three other places, and all three record that something changed while none records that nothing has checked it. It was unreachable anyway, because it required an empty degradedNote and the reviewer's fingerprint stamp is stripped from every posted review. Both mandated lines have to survive the re-review landing, so neither is a conditional instruction: "if no review appears, comment /review" would leave every verified PR permanently carrying an instruction to go and trigger a review. The same objection is why verification state stays out of the commit trailer, where nothing in v1 could ever flip it. No new trigger, no detector, no trailer field. A detector would need a home that runs later than a one-shot run, and the README records the query it would ask so it need not be re-derived.
Follow-ups from the verification-gap investigationRecorded here rather than as issues, because issues are disabled on both this repo and Khan/webapp. Neither is in scope for this PR; 7aed595 covers only the prose and the one line of output. 1. Khan/webapp's reviewer:
|
There was a problem hiding this comment.
Re-review — APPROVE (unchanged from APPROVE at 7816541).
This push (7816541 → 7aed595) is documentation/prompt only — no lib/*.ts or tests changed; the only other diff is the regenerated autofix.lock.yml (body_hash re-synced, body runtime-imported, so not stale). The code was already reviewed and approved, so I re-checked doc-vs-code accuracy on the changed lines only.
The delta honestly reframes the autofix commit's re-review as best-effort verification (new README "Verification is best-effort" section, grounded in the Khan/webapp #41194 incident) and retracts the Step 7 "quiet clean-run" branch, so the run's summary now posts on every push and always says the commit is not yet verified. This is a net improvement in honesty and directly obviates one open thread.
One non-blocking finding (inline, README.md:68): the delta introduces a self-contradiction — the step overview (item 4, lines 48-49) still says "a clean run stays quiet," while the new prose and the retracted Step 7 say the summary posts on every push. Not a blocker; tidy in the same follow-up.
I ran a proportionate roster for a docs-only re-review of already-approved code (correctness, completeness, thread-reconciler) and shed the advisory lenses (holistic / first-principles / conventions / test-adequacy), since no code or tests changed.
1 of 19 prior review threads resolved; 18 still unaddressed as of 7aed595:
18 non-blocking threads still open
- note (non-blocking)
.changeset/autofix.md:11: this changeset now documents the fail-closed thread-fetch refusal, butworkflows/autofix/README.md(~lines 108-112) wasn't synced. - thought (non-blocking)
workflows/autofix/README.md:149: "enforced in code rather than left to convention" overstates the current state —isLoopEligible(scope.ts) has no production caller. - suggestion (non-blocking)
workflows/autofix/autofix.md:167: An unrecognisedautofix: <x>label can't be cleared and persists as apparent "still queued." - question (non-blocking)
workflows/autofix/autofix.md:331: Has the gh-aw unbounded-fetch bug (push_to_pull_request_branch.cjs:935) been filed upstream? - question (non-blocking)
workflows/autofix/autofix.md:495: How is "the live head SHA" meant to be obtained here (localgit rev-parse HEADwould make the moved-head guard vacuous)? - suggestion (non-blocking)
workflows/autofix/lib/plan.ts:296: A missing/malformedlabels.jsondegrades to[](no-op plan) rather than a full-label-set refusal. - suggestion (non-blocking)
workflows/autofix/lib/plan.ts:97:autofixLabelsOnmatches anyautofix:prefix, butremove-labels.allowedis a closed set of five, so a typo label persists. - issue (non-blocking)
workflows/autofix/lib/stage.ts:106:sameLogincompares onlyauthor { login }with no__typename, so a customAUTOFIX_BOT_LOGINis spoofable. - suggestion (non-blocking)
workflows/autofix/lib/stage.ts:128:THREADS_QUERYstages full reply chains butbuildWorkListreads onlycomments[0]— untrusted text with no consumer. - suggestion (non-blocking)
workflows/autofix/lib/stage.ts:272: a page withhasNextPage:true+ missing/emptyendCursorstillbreaks and stages a subset instead of failing closed. - todo (non-blocking)
workflows/autofix/lib/stage.ts:298: the security-criticalisForkderivation has no directcollectInputstest. - note (non-blocking)
workflows/autofix/lib/stage.ts:499: staging failures log a plain string with no::errorannotation, unlike the sibling CLI. - note (non-blocking)
workflows/autofix/lib/staleness.ts:112:findLatestStampreturns the newest stamped review, not the newest review. - thought (non-blocking)
workflows/autofix/lib/staleness.ts:137:anchorHunksabsence is treated as "unreviewed" without keying off the most recent full/scoped stamp. - question (non-blocking)
workflows/autofix/lib/staleness.ts:143: a thread whose file has no hunks in the current diff is never marked stale. - note (non-blocking)
workflows/autofix/lib/staleness.ts:99: the sanitizer strips the body stamp, soassessReviewCurrencyreturnsunverifiableon essentially every real run. - nitpick (non-blocking)
workflows/autofix/lib/trailer.ts:137: exportedsummariseLedgeruses British spelling against the sibling package's American convention. - note (non-blocking)
workflows/autofix/lib/worklist.ts:130: file-level and genuinely-outdated threads collapse into oneoutdated-anchorskip reason.
| ### Verification is best-effort | ||
|
|
||
| Nothing between the fix and the merge gate is guaranteed to check an autofix | ||
| commit, and the summary comment says so on every push. |
There was a problem hiding this comment.
issue (non-blocking): This new sentence — "the summary comment says so on every push" — contradicts the step overview above, which still reads "posts one summary comment, but only when it has something non-obvious to say (see below); a clean run stays quiet" (lines 48-49).
This push retracted the quiet clean-run branch — autofix.md Step 7 now mandates "Post exactly one add-comment, on every path through this workflow" — so item 4's "(see below)" cross-reference now points the reader here, to the opposite claim. Dropping "only when it has something non-obvious to say (see below); a clean run stays quiet" from item 4 (and letting it read "posts one summary comment") would make the overview match the shipped prompt and this section.
2 participants
Adds an opt-in
autofixworkflow that addresses the PR reviewer's own feedback on demand.Label a PR
autofix: blockingorautofix: nits(they union), and the run fixes the reviewer's open threads in that scope, pushes one commit, replies in each thread, posts a summary, and removes the label. One run per arming.Scope of this first iteration
Deliberately narrow, per the design discussion:
workflow_runchaining, the fail-closed cycle counter, the oscillation guard, and most of the governance surface.The label axis model
The namespace is flat; the semantics are not. Three axes exist and the README documents all three, because
autofix: nitsandautofix: loopread as peers and are not:blocking,nitsloop(absent = once)human,author(absent = bot)A label on an unimplemented axis is rejected, not ignored. Honouring the blocking half of
blocking + loopwould present as a loop that mysteriously stopped after one cycle.isLoopEligibleencodes the one constraint that outlives v1: nits never loop. Non-blocking findings have no fixed point, so a nits-scoped loop cannot converge. It is in code rather than convention because it is the rule most likely to be violated by whoever adds the cadence axis.Division of labour
lib/plan.tsis the determinism boundary, mirroring the reviewer's split. Code resolves the scope, checks currency, builds the work list, and renders the trailer, all before the agent is asked to change anything. The plan is final: the prompt executes it or stops, never widens it or re-classifies a skipped thread. Nothing inlib/composes a sentence about the code under review.Reuse rather than reimplementation: the label taxonomy comes from
render-comment.ts(BLOCKING_LABELS/NON_BLOCKING_LABELS), the thread label parser fromrereview.ts(parseLeadingLabel), and the currency fingerprint fromrereview-mode.ts.Guards
Every refusal fails closed: no action, label removed, reason stated.
rereview.ts, where an unparseable label is treated as blocking so the thread is kept. Here the risk is an agent editing code on the strength of a finding it could not classify.hunks=overflow), no review at all → refuse.Why the bot token is load-bearing
push-to-pull-request-branchusesKHAN_ACTIONS_BOT_TOKEN, notGITHUB_TOKEN. GitHub creates no workflow runs forGITHUB_TOKEN-triggered events, so a push made with it would emit nosynchronizeand the reviewer would never re-review. That re-review is the intended verification for an autofix commit, which is also why autofix never resolves its own threads.It is best-effort, not guaranteed: the chain has two links in a repo with the shared push-triggered reviewer and four in Khan/webapp, and the live trial lost one to a gh-aw setup failure that was invisible on the PR. The token is load-bearing for the stronger reason that
GITHUB_TOKENguarantees zero re-review while the bot token buys a best-effort one. The summary comment states the pending status on every push, and the human re-arming loop is the accepted backstop for v1; the README's "Verification is best-effort" section has the details and the numbers.The commit trailer
Every autofix commit carries
Autofix-Version/Autofix-Scope/Autofix-Cycle/Autofix-Threads. v1 never reads it back. It is written because the branch is the only cycle store that survives cache eviction and needs no external state (the same reasoning that put the reviewer's fingerprint in the review body rather than cache memory), and because diffingAutofix-Threadsagainst what the next review still reports open is how the trial scores whether a fix actually cleared a finding.Security review
gh aw compileflagged two new restricted secrets on this workflow. Both are already in use byreview.lock.ymlin this repo:ANTHROPIC_API_KEY— theclaudeengine, identical to the reviewer's use.KHAN_ACTIONS_BOT_TOKEN— the push, for the reason above. Already used by the reviewer forresolve-pull-request-review-threadandadd-reviewer.Two further secrets appear in the compiled manifest that I did not add; both are gh-aw v0.83.4 boilerplate and are unset in this repo, so they compile to empty:
COPILOT_GITHUB_TOKEN— read only by gh-aw's "Check for OAuth tokens" preflight step.GH_AW_CI_TRIGGER_TOKEN— gh-aw's optional CI-retrigger token on the safe-outputs job.No new actions beyond gh-aw's own pinned set. The agent job is
contents: read/pull-requests: read; all writes go through safe outputs.rolesrequires write access (deliberately not the reviewer'sroles: alloverride), forks are excluded, andskip-ai-reviewPRs never start.Things worth a second opinion
autofix.mdpinsautofix-v0.0.0, which does not exist as a tag yet. The version-sync script rewrites it toautofix-v0.1.0in the Version Packages commit, so the workflow is only runnable after the first release. Same shape as any new workflow package here, but worth knowing before trying to trigger it.autofix: blocking,autofix: nits) in whichever repo trials this first.Testing
64 unit tests across the five lib modules. Also smoke-tested the CLI end to end under
tsxexactly as the prompt invokes it, against staged fixtures, confirming the armed, stale-path, and no-review paths.Full CI locally: lint, test, typecheck, build all clean.