spec: amend Phase 2 to three workflows; defer the fix loop - #142
Conversation
Records the operator's ruling at the review cap, in the artifact that is supposed to carry it: - R4 (bounded fix loop) is DEFERRED to its own intent. It needed a third claude[bot] trigger edge where R5 allows two, and it was the one job holding write credentials, running PR-authored code, and reading untrusted PR text at once — that earns a clean design, not patches. - R5 is scoped to this phase's three workflows and its single open bot edge (review-of-agent-PRs). - The Design section no longer builds fix-on-review.yml or the address-review skill, and the exit test proves R1-R3. This amendment REDUCES what the lane does. Widening a spec so an agent can have more power is the amendment this project refuses; recording less delivery is the operator's call, taken at the cap. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Deploying ystack with
|
| Latest commit: |
aa2e856
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0d6c752c.fabrica-6yx.pages.dev |
| Branch Preview URL: | https://ystack-spec-v2-phase-2-amend.fabrica-6yx.pages.dev |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05a085599a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| From the accepted intent: the chain must advance without a live session — merged | ||
| intent → spec PR, merged spec → implementation PR, every PR reviewed with a bounded | ||
| fix loop; the operator's only actions are the gate merges. | ||
| intent → spec PR, merged spec → implementation PR, every PR reviewed; the operator's | ||
| only actions are the gate merges. |
There was a problem hiding this comment.
When any automated review produces findings, the amendment explicitly makes the operator address them in a live session (lines 17–18), so the operator's actions are no longer limited to gate merges as claimed here. The same contradiction appears in the R1–R3 exit criterion, allowing a clean smoke PR to pass without testing the manual handoff that now interrupts the autonomous lane; update the outcome and exit criterion to describe and verify the operator remediation step.
Useful? React with 👍 / 👎.
| recorded in `plan.md` under "Deferred: fix-on-review": the fix stage needed a third | ||
| `claude[bot]` trigger edge where R5 allows two, and it was the one job holding write | ||
| credentials, running PR-authored code, and reading untrusted PR text at once — it |
There was a problem hiding this comment.
In this commit, work/v2-phase-2/plan.md contains neither the referenced "Deferred: fix-on-review" section nor a credential-split design; it still instructs PR B to build fix-on-review and address-review. Consequently the amendment's rationale cannot be audited and its later instruction to start the next intent from that credential split points at nonexistent content, while the repository's ordering document continues to prescribe the unsafe scope being deferred. Include the plan amendment in the reviewed state or reference an artifact that already contains the design.
Useful? React with 👍 / 👎.
| label before acting; at `round-3` it applies `needs-human`, posts the productive-cap | ||
| comment, and stops. That design is superseded — the next intent starts from the | ||
| credential split in `plan.md`, not from this text. | ||
| - **R5 — safety invariants (all three workflows in this phase).** One global `claude-quota` |
There was a problem hiding this comment.
After redefining R5 as the safety requirements for the three workflows that remain, the same requirement still says that "the fix stage never pushes to a PR that already has an approval" (lines 52–54). Because this amendment explicitly removes that stage, the active requirement is impossible to exercise or verify and can lead an implementer to restore part of the deferred workflow merely to satisfy R5; move this condition into the deferred R4 history instead.
Useful? React with 👍 / 👎.
Codex on this PR, all three adopted: - the opening claim said the operator's only actions are gate merges, while the amendment gives them review findings to answer; it now says both - the deferral cited a plan section that lands in a later PR, so the rationale could not be audited from this branch alone. The reasons and the credential-split direction are stated here, in the spec - R5 still required "the fix stage never pushes to an approved PR" — a rule about a stage this phase does not build Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The spec amendment took three review findings, so its blob moved and the plan's recorded hash went stale. Chain discipline: the plan names the exact spec it answers, or the helpers report it stale and refuse to build on it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fac77d4d17
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| **Amended 2026-08-27 (operator ruling at the review cap).** This phase ships three | ||
| workflows, not four: the fix loop (R4) is deferred to its own intent. Two reasons, |
There was a problem hiding this comment.
Compliance: After this amendment reduces the lane to three workflows, R6 still requires the plumbing probe to run before “the four workflows are finalized” (line 63). Since the fourth workflow is now explicitly deferred, that prerequisite no longer describes a reachable phase milestone and leaves the implementation plan ambiguous about whether another workflow must exist before R6 can pass; change the remaining count to three.
Useful? React with 👍 / 👎.
Codex: the probe prerequisite still said 'before the four workflows are finalized', describing a phase this amendment no longer reaches. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbbd65e794
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Each requirement is verifiable; R1–R3 are jointly proven by the phase exit test | ||
| (one real change flows through the lane with the operator only merging). R4 is | ||
| deferred with this phase's fix stage — see the amendment note above. |
There was a problem hiding this comment.
When the review workflow reports a finding, the fresh amendment now explicitly requires the operator to resolve it in a live session, but this changed exit criterion still declares R1–R3 proven by a smoke run where the operator only merges. A clean smoke PR can therefore pass without exercising the newly introduced review-to-operator handoff; update the exit test to include a finding and its manual remediation.
Useful? React with 👍 / 👎.
| comment, and stops. That design is superseded — the next intent starts from the | ||
| credential split in `plan.md`, not from this text. |
There was a problem hiding this comment.
The newly added sentence points the next intent at a credential split in plan.md, but the plan in this target tree still has the old spec-blob and directs PR B to build four workflows, including fix-on-review, plus address-review. Thus the supposedly superseded, unsafe fix design remains the referenced source of truth; either amend the plan or point this sentence to the credential split actually described in the amendment above.
Useful? React with 👍 / 👎.
| **Amended 2026-08-27 (operator ruling at the review cap).** This phase ships three | ||
| workflows, not four: the fix loop (R4) is deferred to its own intent. Two reasons, |
There was a problem hiding this comment.
After this amendment removes the fix workflow, the quota concern later in this same spec still says that “all four jobs” share the subscription window. That stale count makes the sizing question and the intended quota-preflight.sh default ambiguous—especially because the separate plumbing probe is a permanent diagnostic—so state whether the budget covers the three lane jobs or those jobs plus the probe.
Useful? React with 👍 / 👎.
| (mechanical enforcement arrives with Phase 3 hooks). (The "never push to an | ||
| approved PR" rule went with the deferred fix stage — it belongs to that intent, | ||
| which is the only thing that pushes to an open PR.) |
There was a problem hiding this comment.
On a re-trigger with an existing spec or implementation PR, R1 explicitly requires the stage to open “or update” that PR, so the deferred fix stage is not the only component that pushes to an open PR. Removing the approval-safety discussion under this false premise leaves it unspecified whether a producer stage may rewrite an already approved PR; either apply the guard to every updating stage or document the distinct safety mechanism for producer updates.
Useful? React with 👍 / 👎.
Codex round 3 on this amendment: - the exit test declared R1-R3 proven while the operator "only merges", but this amendment also makes them answer findings. The run now counts only if the review found something and the operator resolved it — otherwise the smoke test proves the happy path, not the handoff the phase depends on - the deferral pointed the next intent at plan.md for its design, but any plan written for the four-workflow shape is exactly what was superseded; the direction lives in this spec - quota sizing still counted four jobs - I removed the "never push to an approved PR" rule claiming only the fix stage pushes to open PRs. False: R1 and R2 let a re-triggered producer stage update an existing PR. The rule is restored and now covers every stage — an approval means the operator read that diff Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Round 3 — four folded, and one was a rule I cut on a false premise. I removed "never push to a PR the operator already approved" on the grounds that only the deferred fix stage pushes to open PRs. That was wrong: R1 and R2 let a re-triggered producer stage update an existing PR. So the rule is restored and now covers every stage — an approval means the operator read that diff, and a later push silently moves what they approved. Deleting a safety rule because of a wrong assumption about who can trip it is a good way to lose one. Also: the exit test declared R1–R3 proven while "the operator only merges", even though this amendment gives them findings to answer — so the run now counts only if the review found something and the operator resolved it; a smoke change clean enough to find nothing proves the happy path, not the handoff the phase now depends on. Plus: the deferral pointed the next intent at a plan written for the four-workflow shape (the very thing superseded), and quota sizing still counted four jobs. This is round 3 for this PR — the cap. Whatever comes next is the operator's ruling, not another automatic fold. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43a28a7add
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| (mechanical enforcement arrives with Phase 3 hooks). **No stage pushes to a PR | ||
| the operator has already approved** — R1 and R2 let a re-triggered producer | ||
| stage update an existing PR, so this is not only the deferred fix stage's | ||
| concern: an approval means the operator read that diff, and a later push would | ||
| silently move what they approved. |
There was a problem hiding this comment.
When an existing spec or implementation PR has an approval and a later intent/spec merge makes that PR stale, R1/R2 require the deterministic PR to be updated, while this new invariant forbids the producer from pushing that update. Because the same slug cannot use a second PR, these requirements leave the chain unable to publish the fresh artifact unless an unstated manual step first removes the approval or closes the PR; specify that recovery behavior or permit a safe update that invalidates the old approval.
Useful? React with 👍 / 👎.
My restored rule created a deadlock: R1/R2 have a re-triggered producer update an existing PR, while the new invariant forbids pushing to an approved one. Resolution: the stage does not push. It labels the PR stale, comments once, and stops — the operator merges, dismisses the approval, or closes it. An approved PR belongs to the operator, so the lane never edits one behind them. Completing the previous fold rather than a new round: the deadlock was introduced by that fold. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Adopted — and this was a defect in my own fold, not a new round: restoring "never push to an approved PR" collided with R1/R2's "open or update" and deadlocked the case where the upstream artifact moves while a PR sits approved and unmerged. Resolved: the stage does not push — it labels the PR |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b76d303fa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| unmerged — it does not push. It labels the PR `stale`, says so in one comment, | ||
| and stops. The operator then merges it, dismisses the approval, or closes it; |
There was a problem hiding this comment.
When the upstream intent moves while an approved spec PR is open, the newly specified recovery explicitly permits merging that stale PR. This lands a spec.md whose intent-blob mismatches main; the spec workflow will not rerun because the merge changes the spec rather than the intent, while pending-impl.sh rejects the mismatch, so the chain remains stranded. Require dismissal followed by a producer rerun/rebuild (or another mechanism that restores freshness) rather than presenting merge as a valid recovery.
Useful? React with 👍 / 👎.
| stated here so this spec stands on its own: the fix stage needed a third | ||
| `claude[bot]` trigger edge where R5 allows two, and it was the one job holding write |
There was a problem hiding this comment.
The amendment says the deferred fix stage would require a third claude[bot] trigger edge, but R5 later says this phase opens one edge and that the second allowed edge served that same fix stage. In the described chain, fix-on-review-comment is the second edge and a fix push reuses review-on-agent-PRs; no third distinct actor-gate edge is identified. This makes a stated security reason for the deferral internally unauditable, so either identify the third workflow transition or correct the count and rationale.
Useful? React with 👍 / 👎.
Codex on the current head, both adopted: - my stale-PR recovery offered merging as a way out. That lands an artifact whose recorded upstream hash no longer matches main, and no stage rebuilds it — merging changes the artifact, not the upstream that moved. The way out is to close it, or dismiss the approval and let the stage rebuild from current upstream. - the amendment said the fix stage needed a third edge while R5 said the second allowed edge already served it. Both are true and the text now says so: R5 allows two, the fix stage needed both plus a third (the dispatch re-running review after a push-back that changes no code) — which is precisely why it was deferred. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Splitting the amendment into #142 left an older copy of it committed here. Merging this after #142 would have quietly reverted the four rounds of fixes #142 has taken since — a stale artifact reintroduced by merge order, which is exactly what the chain's hashes exist to prevent and exactly what a file-level revert sneaks past them. This PR now carries no spec change at all: the plan, the rules, the docs sweep and the hook fix. The spec is #142's alone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Both findings folded: a stale PR is rebuilt, never merged (merging it would land an artifact whose recorded upstream hash no longer matches main, and no stage would rebuild it — the merge changes the artifact, not the upstream that moved), and the edge count now reads honestly (R5 allows two; the fix stage needed both plus a third for the push-back re-review, which is precisely why it was deferred). @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f89e9240ad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| upstream that moved. The operator closes the stale PR (or dismisses the approval | ||
| and lets the stage rebuild it from the current upstream). An approved PR belongs |
There was a problem hiding this comment.
In the stale-approved-PR scenario, neither recovery action schedules the producer again: the Design gives the spec and implementation workflows only push-to-main path triggers (lines 101–103), while dismissing an approval or closing a PR emits no such event. Because the upstream merge that found the stale PR has already been consumed and the stage explicitly stopped, the artifact remains stale or closed until an unrelated merge or manual rerun; require an explicit rerun/dispatch after either operator action so the promised rebuild occurs. This is fresh evidence from the newly added recovery path, rather than the earlier merge-versus-rebuild concern.
Useful? React with 👍 / 👎.
Codex: closing a stale PR or dismissing an approval fires no event, so push-triggered stages never wake and the slug sits stranded — my recovery told the operator to do something the lane could not act on. Both stage workflows now carry workflow_dispatch beside their push trigger, gated to the operator, and the recovery says to use it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Adopted. Closing a stale PR or dismissing an approval fires no event, so the push-triggered stages would never wake — my recovery told you to do something the lane could not act on. Both stage workflows now carry That is three consequences-of-a-fix in a row on this one rule: the rule, the deadlock it created, and now the dead end its resolution left. Each was real, and the chain is shorter each time — worth watching for the point where a finding is no longer a consequence of the last fix, which is what settling looks like. @codex review |
|
@codex review |
Codex [Security]: adding workflow_dispatch to the stage workflows re-opened the rerun hole the probe already taught us — github.actor stays the ORIGINAL dispatcher on a re-run, so any write-capable collaborator can re-run an operator's dispatch and spend their subscription. R5 now requires github.triggering_actor to match too, as plumbing-test.yml already does. The lesson existed in the plan and in a live workflow; the spec never carried it, so a new dispatch path inherited the old hole. Rules learned in one artifact have to reach the artifact that governs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6563156f3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - **R4 — bounded fix loop. DEFERRED** to its own intent (see the amendment note | ||
| above). It described: review findings trigger a fix pass that bumps the `round-N` | ||
| label before acting; at `round-3` it applies `needs-human`, posts the productive-cap | ||
| comment, and stops. That design is superseded — the next intent starts from the | ||
| credential split stated in the amendment note above, not from this text and not |
There was a problem hiding this comment.
After deferring the entire bounded fix loop, the active Helpers section still prescribes scripts/v2/round-cap.sh and a hermetic test for it (lines 119–123), even though none of the three remaining workflows consumes round labels and address-review was removed. This leaves fix-loop-only machinery in the phase scope despite the amendment saying that design is superseded; move the helper and its test into the deferred intent or document a remaining in-scope consumer.
Useful? React with 👍 / 👎.
yihanzhu
added
the
needs-human
Codex: the Helpers section still prescribed round-cap.sh and its test while nothing in the three workflows consumes round labels — fix-loop machinery left in an active design reads as work to do. It is already merged, so the spec now says it stays and sleeps until the deferred intent wakes it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44966e34a4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| the two stage jobs **plus `workflow_dispatch` on each, so the operator can | ||
| restart a stage after clearing a stale PR** (see the approved-PR rule in R5), |
There was a problem hiding this comment.
When a non-operator collaborator manually dispatches either producer workflow, the actor gate skips the agent job, but the workflow run is still recorded. scripts/v2/quota-preflight.sh lines 15–22 explicitly assumes these workflows can never have skipped runs, while lines 66–74 count every run for each workflow without filtering by event, status, or triggering user; gh run list --help confirms that --event, --status, and --user are the available filters. Adding workflow_dispatch here therefore lets 20 cost-free dispatches trip the backstop and block legitimate lane work for the five-hour window, so the quota helper or recovery trigger design must be adjusted.
Useful? React with 👍 / 👎.
Codex: adding workflow_dispatch to the producer stages broke the invariant quota-preflight.sh depends on — that a counted workflow's runs always mean the agent ran. A dispatch that fails the actor gate still records a skipped run, so a collaborator could spam dispatches until the brake trips and starves the lane. Counting --event push restores the invariant: the ruleset means only an operator merge causes those. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
yihanzhu
removed
the
needs-human
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98e7b6ce57
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| this reason — or `allowed_bots: claude[bot]` only on deliberately | ||
| opened bot edges — this phase opens exactly one, review-of-agent-PRs. R5's second | ||
| allowed edge, the fix-on-review comment, went with the deferred stage, and the | ||
| third edge that stage would also have needed is precisely why it was deferred); |
There was a problem hiding this comment.
Security: When a write-capable collaborator reruns a review run originally triggered by claude[bot], GitHub preserves the bot as github.actor and records the collaborator only in github.triggering_actor—the behavior already documented in .github/workflows/plumbing-test.yml lines 30–33. The operator alternative now checks both identities, but the allowed_bots: claude[bot] alternative places no restriction on triggering_actor, so such a rerun can still pass the bot edge and spend the subscription or post duplicate reviews. Require an authorized triggering actor on the bot alternative as well.
Useful? React with 👍 / 👎.
yihanzhu
added
the
needs-human
Codex found the actor/triggering_actor hole for the third time, at a third location: a collaborator can re-run a bot-triggered review, and github.actor stays claude[bot] while only triggering_actor names them. Patching a third edge would have invited a fourth. R5 now states it as one invariant covering every job in the lane — operator gates, dispatches, and the bot edge alike — instead of a note attached to whichever gate was last found wanting. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
yihanzhu
removed
the
needs-human
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5e0916c81
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| requires the triggering actor to be the operator (or, on the bot edge, the bot | ||
| itself), alongside whatever `github.actor` or `allowed_bots` allows. The probe |
There was a problem hiding this comment.
When a claude[bot]-originated review run fails transiently and the operator clicks re-run, github.actor remains the bot while github.triggering_actor becomes the operator (the behavior documented in .github/workflows/plumbing-test.yml lines 30–33). Requiring the triggering actor to be the bot itself makes both the bot alternative and the operator alternative fail, so no authorized human can rerun that review and R3 may remain unsatisfied without manufacturing another PR event. Allow the operator as an authorized triggering actor on the bot edge while continuing to reject other collaborators.
Useful? React with 👍 / 👎.
yihanzhu
added
the
needs-human
My own both-actors rule was written tightly enough to lock the operator out of re-running a review that failed transiently — the triggering actor on such a re-run is the operator, not the bot. The rule now names the two acceptable triggering actors explicitly: the operator, or the bot itself. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
yihanzhu
removed
the
needs-human
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22f1d3307e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| upstream that moved. The operator closes the stale PR, then **re-runs the stage | ||
| by dispatch** — closing a PR or dismissing an approval fires no event, so the |
There was a problem hiding this comment.
The fresh close-and-dispatch recovery still has no compliant outcome when the stale PR was already approved: reusing and reopening that PR requires pushing new content to a PR the operator approved, while opening a replacement violates the repository's rule in work/README.md lines 18–20 that reruns update the existing PR and never open a second one for the slug. Thus an upstream change can still strand an approved spec or implementation PR; specify a recovery that clears the approval and safely reuses the PR, or explicitly revise the single-PR invariant.
Useful? React with 👍 / 👎.
yihanzhu
added
the
needs-human
Codex: closing a stale PR and re-dispatching leaves no legal move if the one-PR-per-slug rule counts closed PRs — reopening means pushing into an approval, and a new PR looks like a second one. The rule is about concurrency: one OPEN PR per slug. A closed PR is history. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
yihanzhu
removed
the
needs-human
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* plan: record the probe verdict and the rules it cost us R6 answered — app events cascade, so the lane uses the direct path. The five probe rules and the three reviewer-loop rules are now written into the plan, so an implementer who never saw the conversation gets them: allowlist by command prefix, writes behind a deterministic wrapper, assert side effects, verify secrets via API, dispatch only from the default branch; fence verdicts to the head, let them settle, bump the round label before acting. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * AGENTS.md: the repo's rules, in the file other agents read Codex reads AGENTS.md, not CLAUDE.md — so pointing it at this folder meant it never saw the conventions, the review policy, or the chain. Thin bridge that points at the real sources instead of duplicating them (a copy would drift), plus the handful of rules that bite. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * AGENTS.md: the repo's rules, in the file other agents read Codex reads AGENTS.md, not CLAUDE.md — so pointing it at this folder meant it never saw the conventions, the review policy, or the chain. Thin bridge that points at the real sources instead of duplicating them (a copy would drift), plus the handful of rules that bite. Registered in the manifest. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * One rules file: AGENTS.md canonical, CLAUDE.md imports it Two files describing how to work here would drift — the same class of bug this repo keeps catching. AGENTS.md is now the single source (the vendor-neutral name Codex and most tools read); CLAUDE.md is an import so Claude Code loads the same text with no copy. Pointers in REVIEW.md and the stage skills follow. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * AGENTS.md: drop the framing the merge duplicated Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * plan: fold six review findings — including three of my own rules Codex on the brief, all adopted: - assert the side effect THIS stage should have; "a PR exists" is vacuous for review and fix jobs, which run on an existing PR - bind verdicts to the reviewed head SHA, not a timestamp: a review of head A can land after head B is pushed and pass a time fence - wait for a completion marker, not a fixed settle window — a slow review wakes the fix job early, a late comment starts a duplicate - load write wrappers from the default branch: a same-repo PR can edit its own wrapper and the job would run the PR's version - corrected my dispatch rule: --ref works once a workflow exists on the default branch; my earlier claim was inferred from a run that had actually failed on the allowlist - marked PR A and the probe done and retired the fallback in the steps and risks, which still told the implementer to do obsolete work Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Fold five more findings: guard the rules file, retire a stale permission Codex round 2 on the brief, all adopted: - SECURITY: AGENTS.md became the canonical rules file but no constitution list protected it — an unattended PR could have rewritten the safety rails directly. Added to REVIEW.md, proposals/README.md, and its own stage rule. - The old permission for the manager to auto-merge low-risk PRs survived into the canonical file, contradicting "no agent may merge" three lines below. Retired explicitly: merging is the operator's, always. - PR B cannot review itself — a pull_request workflow only fires when its file is already on the default branch — so its proof is the existing harness plus the operator's read; the smoke test proves the lane fires. - The reviewed-head marker has to come from THIS phase's reviewer, which is Claude: the review skill emits `reviewed-head: <sha>` and the fix job parses it (the same parser reads Codex's body later). - Human docs pointed at a stub GitHub does not expand; they now point at AGENTS.md, and the stub carries a real link. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Retire the agent merge path everywhere it was still promised Codex round 3 found the live persona still telling yshifu to run merge-pr.sh — a permission the branch ruleset retired hours earlier. Chasing it turned up the same stale promise across the product: README's team table, loop diagram, design decisions and rollout; QUICKSTART's adopter walkthrough; the routines; both reviewer runbooks; the repo-setup template; and RESTORE, the disaster-recovery runbook — the worst place to carry a rail that no longer exists. Now, in one voice: yshifu labels a reviewed-clean head merge-ready and hands the PR to the operator, who merges. merge-pr.sh stays for the operator's own use. The deferred "unattended auto-merge" promise is gone with it — there is no agent merge path, present or planned. Also folded (same review round): constitution list completed with CLAUDE.md and REVIEW.md; review and fix jobs load stage SKILLS as well as wrappers from the trusted default branch; the reviewed-head marker is stamped by a deterministic step after the review step, never written by the model (an injected diff could otherwise forge a verdict); the marker carries the verdict so a clean review no longer burns a fix round; and a push-back re-triggers review so a standoff can still reach its cap. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Fix two regressions the post-cap review found in live files Both are mine, from removing merge authority earlier: - RESTORE's branch-protection checklist still said merges go through "yshifu or the human" — opposite instructions in the same runbook - rewriting the merge-ready rule kept only the head check; the retired harness compared reviewed head AND base, and when main advances the head SHA is unchanged while the reviewed diff is gone. The label now goes void on either. The brief-level findings from this round go to the operator: the cap holds. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * plan: the lane ships three workflows; fix-on-review is deferred Operator's ruling at the cap. The fix stage is cut from this phase and comes back as its own intent with its own spec. Two reasons, both recorded in the plan: the fix stage needed a third claude[bot] trigger edge while the approved spec allows two, and a plan that reaches past its spec gets shrunk rather than the spec amended; and every security finding of the last two review rounds landed on that one job, the only one that holds write credentials, runs PR-authored code, and reads untrusted PR text at once. That earns a clean design, not four patches. Cut with it: the stamped verdict marker, marker polling, the push-back re-review path, and the round-label brake — all of them existed to serve the fix job. What review-on-pr still needs is kept and now scoped to it: PR text is data, the review skill loads from the trusted default branch, the skill posts nothing and a plain step posts its file, so the job needs no write access at all. Until the fix stage exists, review findings are the operator's to handle in a session — as they were while this phase was built. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Amend the spec to match the ruling, and fix the label it still promised Codex, from the other direction: a plan may not exceed its spec, and it may not silently drop one either. The deferral left R4 in the approved spec with nothing building it, so the spec now records the scope reduction the operator ruled: R4 deferred to its own intent, R5 scoped to this phase's three workflows and its one open bot edge, exit test proving R1-R3. The plan's spec-blob is rebaselined to the amended spec, so the chain reads fresh. Note the direction of this amendment: it REDUCES what the lane does. The earlier refusal was to amend a spec so an agent could have a third trigger edge — widening power to fit a plan. Recording less delivery is the operator's call; widening rails is not. Also live: setup-target-repo.sh still installed a merge-ready label saying low-risk PRs are "auto-merged in-session", and it force-updates labels on every run — so every target repo kept getting the retired promise. Now: "waiting on YOUR merge (no agent merges; void once new commits land)", and this repo's live label is reconciled. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Close the merge-helper bypass; point the plan at the amended spec Codex [Security]: the guard hook explicitly exempted scripts/merge-pr.sh. That exemption was written when the manager could still merge in-session; once that permission was retired the exemption became a live bypass — in a clone with no ruleset, or under an account that can merge, an agent could merge through the helper, making this repo's "no agent has a merge path" claim false. The hook now blocks it: the helper is the operator's tool, run by a human. The spec amendment moves to its own PR (Codex [Compliance]: a spec change earns its own G2 gate, not a ride along with the plan that answers it). This branch keeps the plan, whose spec-blob points at that amended spec — so the chain reads fresh once the spec PR lands, and stale before it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * plan: rebaseline onto the folded spec (#142) The spec amendment took three review findings, so its blob moved and the plan's recorded hash went stale. Chain discipline: the plan names the exact spec it answers, or the helpers report it stale and refuse to build on it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * plan: rebaseline onto the spec's R6 fix (#142) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * plan: rebaseline onto the spec's round-3 fold (#142) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * plan: rebaseline (#142 deadlock fix) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * plan: rebaseline onto the spec's stale-PR and edge-count fixes (#142) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * plan PR: drop the stale spec copy it was still carrying Splitting the amendment into #142 left an older copy of it committed here. Merging this after #142 would have quietly reverted the four rounds of fixes #142 has taken since — a stale artifact reintroduced by merge order, which is exactly what the chain's hashes exist to prevent and exactly what a file-level revert sneaks past them. This PR now carries no spec change at all: the plan, the rules, the docs sweep and the hook fix. The spec is #142's alone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * plan: rebaseline onto the spec's dispatch fix (#142) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * plan: rebaseline onto the current spec (#142) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * plan: rebaseline onto the settled spec (#142) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: ci <ci@example.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 participant
G2 — the spec amendment, on its own gate. Codex was right that folding this into #141 collapsed the gate: a spec change earns its own approval, separate from the plan that answers it.
What changes: R4 (bounded fix loop) deferred to its own intent; R5 scoped to three workflows and one open bot edge; the Design section stops building
fix-on-review.ymlandaddress-review; the exit test proves R1–R3.Direction matters: this amendment reduces what the lane does — a delivery decision you ruled at the cap. The amendment this project refuses is the opposite one: widening a spec so an agent can have more power (which is exactly what the deferred fix stage would have needed).
Merge order: this first, then #141 — whose plan already records this spec's hash, so the chain reads fresh the moment this lands.
🤖 Generated with Claude Code