plan: probe verdict + the rules Stack B must obey - #141
Conversation
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>
Deploying ystack with
|
| Latest commit: |
99fdbd1
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://01b72886.fabrica-6yx.pages.dev |
| Branch Preview URL: | https://ystack-impl-v2-phase-2b.fabrica-6yx.pages.dev |
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>
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>
|
Added Footnote: the rename gate rejected my first draft of that file for spelling the old names while explaining the rule against them. Reworded. |
![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: c3411418b0
ℹ️ 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".
| 3. **Assert side effects.** A denied write still reports `is_error: false`, so | ||
| every job ends with a step that checks the PR/branch really exists and | ||
| fails loudly when it does not. |
There was a problem hiding this comment.
For review-on-pr and fix-on-review, the PR and branch necessarily already exist, so this prescribed final check succeeds even when a denied comment or push reports is_error: false; the lane can therefore skip the review or fix while leaving a green job. Require stage-specific assertions instead—for example, a review marker bound to the expected head for review jobs and the expected new head or response for fix jobs.
Useful? React with 👍 / 👎.
| - Fence every reviewer verdict to the head it judged: a verdict whose | ||
| timestamp precedes the push is about the previous code. |
There was a problem hiding this comment.
If a review of head A starts, head B is pushed, and the verdict for A is posted afterward, that stale verdict's timestamp is later than the push and passes this fence. The fix loop can then modify B in response to findings for A, contrary to the plan's own exact-commit proof rule; stamp each verdict with the reviewed head SHA and require it to equal the current PR head before acting.
Useful? React with 👍 / 👎.
| 5. **`workflow_dispatch` executes only from the default branch** — a | ||
| dispatch-triggered change cannot be tested before it merges. |
There was a problem hiding this comment.
This is only true when the workflow is brand new and therefore absent from the default branch; once its path is registered there, a branch version can be run before merge. The installed gh workflow run --help documents --ref as the branch or tag containing the workflow version and gives gh workflow run triage.yml --ref my-branch, so this blanket rule would unnecessarily prevent pre-merge testing of subsequent workflow changes.
Useful? React with 👍 / 👎.
| 2. **Put writes behind a deterministic wrapper and allowlist only that.** See | ||
| `scripts/v2/probe-publish.sh`: it fixes the branch name, the files it | ||
| stages, and the refspec, and refuses arguments that are not this run's. | ||
| Each stage gets its own wrapper in the same shape. |
There was a problem hiding this comment.
For review-on-pr or fix-on-review, a same-repo PR can itself change a wrapper under scripts/v2/; if the workflow checks out the PR head and then allowlists that path, the changed script—not the reviewed deterministic wrapper—runs with the job's write credentials and can bypass all of the stated staging and refspec restrictions. Require these wrappers to be loaded or verified from the trusted default-branch revision before execution.
Useful? React with 👍 / 👎.
| - Let a verdict settle (~8 minutes) before acting on it — the reviewer | ||
| acknowledges a push within seconds and posts real findings minutes later. |
There was a problem hiding this comment.
A fixed eight-minute delay is not a synchronization mechanism: when a review takes longer under queue, API, or model latency, the fix job wakes before the findings exist and may consume a round or exit without acting; a later comment can also start a duplicate pass. Gate the fix job on an explicit completed-verdict marker for the expected head, polling only up to a bounded timeout rather than assuming the reviewer finishes within the observed delay.
Useful? React with 👍 / 👎.
| **PR B — the lane** (PR A merged; the plumbing test PASSED — run 33088639117: | ||
| the agent published a branch and opened PR #140 as `app/claude` with zero | ||
| denials, and that PR triggered CI by itself. App events cascade, so the lane | ||
| uses the direct path and the spec's PR-creation fallback is retired.) |
There was a problem hiding this comment.
This now declares PR A merged, the plumbing probe passed, and the fallback retired, but the same document still tells the implementer to build and merge PR A, run the probe, and choose that fallback if it fails, while the Risks section still treats the probe as pending. An implementer using this plan as the delegated source of truth can repeat obsolete work or follow a retired path; update the order, risks, and proof sections to distinguish completed evidence from remaining PR B work.
Useful? React with 👍 / 👎.
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>
|
Consolidated per the operator: one rules file. |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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>
|
All six adopted — and four of them killed rules I had just written. Worth recording plainly, because the brief is what a delegated implementer will trust:
Two of the fixes are "do what v1 already does" — trust-anchored config reads and reviewed-head pinning. The crown jewels keep being the right answer. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8fd3fe69ae
ℹ️ 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 implement stage never touches `intent.md`/`spec.md`; unattended agents never write | ||
| `.github/**` or `.claude/**` — such changes land as patches under `proposals/` that the |
There was a problem hiding this comment.
[Security] Once AGENTS.md becomes the canonical home of the safety rails, an unattended implementation can edit it directly: both REVIEW.md's constitution list and proposals/README.md still protect .github/**, .claude/**, CLAUDE.md, and REVIEW.md, but omit AGENTS.md, and this stage rule omits it too. That lets a PR rewrite the real rules without using a proposal, so add AGENTS.md to every constitution and write-protection list before moving the rules.
AGENTS.md reference: AGENTS.md:L65-L73
Useful? React with 👍 / 👎.
| - **Bind a verdict to the head SHA it reviewed, never to a timestamp.** A | ||
| review of head A can land after head B is pushed, so a timestamp fence | ||
| accepts it and the fix loop then edits B in response to findings about A. | ||
| Codex's own review body names the commit it reviewed; parse that SHA and |
There was a problem hiding this comment.
[Bugs] Fresh evidence in this revision is that the new fence says to parse “Codex's own review body,” but this phase's review-on-pr design runs the Claude review-pr skill with claude_code_oauth_token; the spec explicitly leaves Codex cloud review out of scope. If the plan is followed literally, a Claude verdict has no promised Codex marker, so fix-on-review times out or escalates instead of fixing findings. Define a marker that review-on-pr itself emits and parse that marker.
AGENTS.md reference: AGENTS.md:L10-L14
Useful? React with 👍 / 👎.
| 5. **A brand-new workflow can only be dispatched once its file is on the | ||
| default branch.** After that, `gh workflow run <file> --ref <branch>` runs |
There was a problem hiding this comment.
[Bugs] A pull_request workflow triggers only when its workflow file already exists on the default branch, so PR B cannot fire the brand-new review-on-pr workflow that it introduces. This new caveat correctly says the file is not registered until merge, but the Proof section still requires the lane to review its own PR, making the promised pre-merge proof impossible. Use the existing trusted review harness for PR B, then prove the new lane on the post-merge smoke PR.
AGENTS.md reference: AGENTS.md:L10-L14
Useful? React with 👍 / 👎.
| The working rules live in one file so every agent reads the same thing, | ||
| whatever vendor. Claude Code loads it through this import: | ||
|
|
||
| ## What lives here | ||
| - `manager/CLAUDE.md` — yshifu's role (the manager persona). | ||
| - `routines/*.md` — the coder's baseline instructions yshifu reads to brief a spawned | ||
| coder subagent (`coder.md` / `coder-revision.md`) plus the per-task `brief.md`. | ||
| - `reviewer/codex-review.md` — the doc for the Codex reviewer harness (`scripts/codex-review.sh`). | ||
| - `templates/*` — drop-in files for target repos. | ||
| - `scripts/*.sh` — the shipped tooling: `install.sh` (generates the `/yshifu` command), | ||
| `setup-target-repo.sh` (bootstraps a target repo's loop labels), and `codex-review.sh` | ||
| (runs the Codex reviewer against a PR). | ||
| @AGENTS.md |
There was a problem hiding this comment.
[Compliance] After this bridge replaces the full document, README.md still labels CLAUDE.md as the repository conventions and safety rails, while QUICKSTART.md and RESTORE.md send humans there to read those rules. GitHub and ordinary Markdown readers do not expand Claude Code's @AGENTS.md import, so those documented entry points now show only this stub without a usable link to the canonical file. Update the repository map and human-facing links to point at AGENTS.md, and describe CLAUDE.md as the Claude bridge.
AGENTS.md reference: AGENTS.md:L51-L51
Useful? React with 👍 / 👎.
| read-only / comments-only; merge stays gated (yshifu may merge only CI-green + Codex-clean | ||
| + low-risk PRs under standing authorization, and must bring safety-rail / north-star / | ||
| ambiguous / high-risk PRs to the human); the rounds cap and `needs-human` escalation stay |
There was a problem hiding this comment.
[Compliance] This canonical rule still authorizes yshifu to merge CI-green, Codex-clean low-risk PRs, while the new lines 98–99 say no agent may merge and the artifact chain makes the operator's merge the G3 gate; work/v2-phase-2/spec.md also marks the old auto-merge claim as retired. In a yshifu session these instructions give opposite answers at the actual merge gate, and following this earlier clause bypasses G3. Remove the stale standing authorization and sync the manager and generated instructions so only the operator merges.
AGENTS.md reference: AGENTS.md:L75-L79
Useful? React with 👍 / 👎.
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>
|
Round 2: five findings, all adopted. Two are worth naming plainly. The security one is mine. Making A stale permission survived the migration: the old clause letting the manager auto-merge CI-green low-risk PRs came across into the canonical file and sat three lines above "no agent may merge." In a live session those give opposite answers at the merge gate. Retired explicitly — merging is the operator's, always. Also fixed: PR B can't review itself (a Round 3 is the cap for this PR. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d36c1dce24
ℹ️ 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".
| read-only / comments-only; **merging is the operator's, always** — the in-session | ||
| auto-merge v1 allowed was retired when the branch ruleset landed, and no agent has a | ||
| merge path any more; the rounds cap and `needs-human` escalation stay |
There was a problem hiding this comment.
[Compliance] Fresh evidence in this revision is that the new canonical rule forbids every agent merge, but the live sources were not synced: manager/CLAUDE.md:547-559 and templates/yshifu-command.md:25-28 still tell yshifu to run merge-pr.sh without confirmation, and scripts/install.sh:41-58 installs that template. A restored or regenerated /yshifu therefore still has a merge path despite this rule; update the manager, command template, and related restore guidance so only the operator merges.
AGENTS.md reference: AGENTS.md:L58-L64
Useful? React with 👍 / 👎.
| 6. **Load write wrappers from a trusted revision.** A same-repo PR can edit | ||
| `scripts/v2/*.sh`; a job that checks out the PR head and then allowlists | ||
| that path runs the PR's script, not the audited one, with the job's write | ||
| credentials. Review and fix jobs read their wrappers from the default | ||
| branch — the same trust-anchor rule the v1 review harness already uses for |
There was a problem hiding this comment.
[Security] When a same-repo PR changes .claude/skills/review-pr/SKILL.md or address-review/SKILL.md, checking out that PR head still gives the agent an untrusted control prompt even though the wrapper comes from the default branch. The altered review skill can use the allowed wrapper to post a forged completion marker instead of reviewing the diff, while the altered fix skill can steer credentialed edits. Load every stage skill and REVIEW.md from the trusted default-branch revision too, and treat only the PR tree as input data.
AGENTS.md reference: AGENTS.md:L65-L68
Useful? React with 👍 / 👎.
| **The reviewer must emit the marker** — this phase's `review-on-pr` runs | ||
| Claude, so its skill ends every review comment with a machine-readable | ||
| `reviewed-head: <sha>` line, and `fix-on-review` parses that and refuses to | ||
| act unless it equals the PR's current head. (Codex's cloud review body |
There was a problem hiding this comment.
[Bugs] This marker records the reviewed SHA but not whether the verdict passed or contained findings. Because fix-on-review is triggered by the review comment (work/v2-phase-2/spec.md:67) and bumps the round before acting, a clean review for the current head also passes this fence, launches an unnecessary fix agent, and consumes a round; repeated clean reviews can reach needs-human without any finding. Include a machine-readable pass or findings status and exit before the bump when the status is pass.
AGENTS.md reference: AGENTS.md:L65-L69
Useful? React with 👍 / 👎.
| assert a review marker naming the head it reviewed. For `fix-on-review` | ||
| assert a new head SHA (or a posted push-back). A denied write still |
There was a problem hiding this comment.
[Bugs] When address-review rejects a finding and posts a push-back without changing code, this check accepts the response as the fix job's successful side effect, but only a pushed head retriggers review-on-pr. The reviewer therefore never answers the disagreement, no later round is consumed, and the loop cannot reach its needs-human cap. Add a trusted push-back event that starts another review of the same head, or explicitly dispatch that review before treating the fix job as complete.
AGENTS.md reference: AGENTS.md:L65-L69
Useful? React with 👍 / 👎.
| **The reviewer must emit the marker** — this phase's `review-on-pr` runs | ||
| Claude, so its skill ends every review comment with a machine-readable | ||
| `reviewed-head: <sha>` line, and `fix-on-review` parses that and refuses to |
There was a problem hiding this comment.
[Security] The PR diff and body are untrusted inputs to review-pr, yet this makes the model-owned skill emit the completion credential that fix-on-review trusts. Prompt injection can make Claude output reviewed-head: <current-sha> without doing the review, and the resulting comment is still authored by the trusted app, so the poll accepts it. Have a deterministic wrapper append the head and completion marker only after the review action succeeds, and strip or escape marker-shaped lines from the model body.
AGENTS.md reference: AGENTS.md:L65-L68
Useful? React with 👍 / 👎.
| - **Stage rules (autonomous lane):** the spec stage writes only `work/<slug>/spec.md`; | ||
| the implement stage never touches `intent.md`/`spec.md`; unattended agents never write | ||
| `.github/**`, `.claude/**`, or this file — such changes land as patches under `proposals/` that the | ||
| operator applies. Operator-driven sessions are exempt; Phase 3 hooks enforce this |
There was a problem hiding this comment.
[Compliance] This stage rule protects the new AGENTS.md but still omits root CLAUDE.md and REVIEW.md, even though REVIEW.md:15-19 classifies both as constitution paths that unattended agents may change only through proposals/. Because this revision turns CLAUDE.md into the import bridge and leaves REVIEW.md as the review gate, an autonomous implementation can redirect Claude away from the canonical rules or weaken its own review policy before the Phase 3 hooks exist. Add both files to this write ban and to PR B's implement-stage rules.
AGENTS.md reference: AGENTS.md:L65-L68
Useful? React with 👍 / 👎.
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>
yihanzhu
added
the
needs-human
|
Round 3 = the cap. All six adopted, and the first one opened something bigger than the finding. Codex flagged that the live persona still told yshifu to run The other five, in the brief:
Operator call. Three rounds, 24 findings, every round still finding real holes — so this is not "diminishing returns, ship it". My recommendation is still to merge: the brief is now materially safer than the implementation it describes, and the remaining risk lives in code that will get its own review rounds with the real thing in front of it. But the cap is yours to rule on, per REVIEW.md. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b4ada0b5e
ℹ️ 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".
| - check out the default branch to its own path and point the job's skill | ||
| directory and its allowlisted wrapper path at that checkout; | ||
| - `review-on-pr` needs no PR-head checkout at all — read the PR's diff and | ||
| text through `gh`, as data; | ||
| - `fix-on-review` does need the head, because it edits those files. Check it | ||
| out to a separate working path: files to edit, never files to run. |
There was a problem hiding this comment.
[Security] Fresh evidence in this revision is that the fix job separates trusted files from the PR checkout, but it does not stop the fix agent's required Write/Edit tools from changing the sibling trusted checkout by absolute path. A same-repo PR that prompt-injects address-review can therefore rewrite the allowlisted wrapper before calling it with the job's write token, restoring the bypass this rule is meant to close. Make the trusted artifacts immutable or constrain writes mechanically and verify the wrapper immediately before execution.
AGENTS.md reference: AGENTS.md:L65-L68
Useful? React with 👍 / 👎.
| yshifu **never writes code or opens PRs** and **never approves on your behalf** — it opens | ||
| issues and orchestrates the loop. yshifu **does** merge clean, low-risk PRs (CI green + | ||
| Codex review passed) under your standing authorization, and brings you anything needing | ||
| human review (safety-rail changes, north-star / goal drift, high-risk back-look). The | ||
| issues and orchestrates the loop. yshifu **never merges either**: when a PR is CI-green and | ||
| Codex passed that head, it labels the PR `merge-ready` and hands it to you, naming the risk | ||
| when there is one (safety-rail changes, north-star / goal drift, high-risk back-look). The |
There was a problem hiding this comment.
[Compliance] In a restored setup, this paragraph now says yshifu never merges, but the branch-protection checklist at RESTORE.md:139-140 still says merges go through “yshifu or the human.” An operator following the runbook therefore receives opposite instructions about a safety rail and may restore the retired agent merge path; update that later checklist so only the operator merges.
AGENTS.md reference: AGENTS.md:L65-L68
Useful? React with 👍 / 👎.
| - **`merge-ready` is void the moment new commits land.** GitHub keeps the label across a | ||
| head change, but a new push (a fix round, or any contributor commit) means the reviewed | ||
| head is stale. Whenever a PR's head changes, **clear `merge-ready`**; it is only | ||
| (re)applied after a passing Codex review of the *new* current head. Never merge on a | ||
| `merge-ready` label whose review predates the current head — re-run `codex-review.sh` | ||
| on the new head first. | ||
| (re)applied after a passing Codex review of the *new* current head. Never leave the | ||
| label standing when its review predates the current head — the operator merges on the |
There was a problem hiding this comment.
[Bugs] When the base branch advances after Codex reviews a PR, the head SHA does not change, so this rule leaves merge-ready in place even though the reviewed diff is stale. The retired merge flow explicitly compared both Reviewed-head and Reviewed-base; without that check, a status scan can hand an operator a false-green PR in targets that do not require branches to be up to date. Track the stamped base SHA too, and clear and re-review whenever either SHA moves.
AGENTS.md reference: AGENTS.md:L65-L68
Useful? React with 👍 / 👎.
| - `review-on-pr` then runs a plain step, only if the review step succeeded. | ||
| That step posts the comment: the body from the file, plus the marker line | ||
| it builds itself. The SHA comes from the event | ||
| (`github.event.pull_request.head.sha`), never from model output. |
There was a problem hiding this comment.
[Bugs] Fresh evidence in this revision is the new push-back path that dispatches review-on-pr, but a workflow_dispatch event has no github.event.pull_request, so this stated SHA source is empty on exactly that path. The re-review therefore cannot produce a valid head-bound marker, leaving the disagreement loop stalled after the round was bumped. Pass and validate an expected SHA input, or fetch and freeze the PR head before reviewing and stamping.
AGENTS.md reference: AGENTS.md:L65-L68
Useful? React with 👍 / 👎.
| - `review-on-pr` then runs a plain step, only if the review step succeeded. | ||
| That step posts the comment: the body from the file, plus the marker line | ||
| it builds itself. The SHA comes from the event |
There was a problem hiding this comment.
[Bugs] This plain post-review step is now responsible for creating the comment that triggers fix-on-review, but the plan gives it no Claude GitHub App credential; the probe only proved that writes made from inside claude-code-action use that app identity. Using ${{ github.token }} here would author the comment as github-actions[bot] and GitHub would not start another workflow from that event, so Important findings never reach the fix loop. Explicitly mint and pass an app token to the deterministic step, or perform the stamped write in trusted action code that retains the app authentication.
AGENTS.md reference: AGENTS.md:L65-L68
Useful? React with 👍 / 👎.
| - **Proof is tied to the code:** verify output names the exact commit it ran | ||
| on. New commits make old proof stale. | ||
| - **Each change type has its own proof:** shell → shellcheck + tests; |
There was a problem hiding this comment.
[Security] In fix-on-review, these required tests and shell commands come from the editable PR checkout, so a PR can change a package script, Make target, or test to read and exfiltrate the Claude OAuth or GitHub App credential available to the same action that later pushes. Making the trusted wrapper immutable does not contain code deliberately executed as verification. Run PR-controlled checks in a separate credential-free job or scrub every write/OAuth credential before invoking them.
AGENTS.md reference: AGENTS.md:L65-L68
Useful? React with 👍 / 👎.
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>
|
Post-cap round: 6 findings (5 P1). Two were regressions in live files — fixed now (
The remaining four are design questions on the brief, and they cluster on one stage —
What I read in that cluster: every one of them is the same structural problem — one job holds write credentials, executes PR-authored code, and reads untrusted PR content. Patching them individually will keep producing findings. The structural fix is to split those: the agent that edits PR code gets no credentials and runs nothing from the PR; verification happens in CI on the pushed branch (where it already runs, sandboxed and credential-free); a deterministic step with the app credential does the push and the stamping, and never executes anything the PR authored. That's a real design change to the brief, not a patch — which is why it's yours to call, per the cap. Options: (a) merge as-is and let the implementation PR carry these as known work; (b) I fold the structural split into the brief first (one more pass, and it makes |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5bc74b3673
ℹ️ 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".
| every review comment the fix job may act on ends with one machine-readable | ||
| line, and `fix-on-review` refuses to act unless the SHA on that line equals | ||
| the PR's current head. Same discipline as the v1 merge harness, which pins to |
There was a problem hiding this comment.
[Bugs] Fresh evidence in this revision is that the in-session merge-ready path now treats a base move as stale, but the autonomous marker still records and checks only reviewed-head. If main advances while a review of unchanged head H is running, or before fix-on-review consumes the verdict, this head-only check still passes and the fixer edits H using findings for the old diff. Stamp the reviewed base SHA and refuse or re-run the review when either SHA differs.
AGENTS.md reference: AGENTS.md:L102-L103
Useful? React with 👍 / 👎.
| third deliberately-opened bot edge on top of the two the spec's R5 names, | ||
| so `review-on-pr`'s dispatch trigger gets its own actor gate: the app or | ||
| the operator, nobody else. |
There was a problem hiding this comment.
[Compliance] This explicitly adds a third app-triggered edge, but the approved spec's R5 permits claude[bot] only on the two named edges. Building PR B from this plan would therefore widen the unattended trigger surface beyond the design approved at G2. Route the re-review through an approved edge or revise and approve the spec before implementation.
AGENTS.md reference: AGENTS.md:L76-L79
Useful? React with 👍 / 👎.
|
Two more P1s — and the governance one changes my recommendation to (c). Verified against the merged spec: R5 permits The other finding is the same base-move bug I just fixed in the in-session path, still present in the autonomous marker (a review of unchanged head H is stale once Updated recommendation: (c) — ship Stack B without
Concretely that means: drop the fix stage and the push-back path from PR B's file list, keep three workflows, and the marker requirement dissolves with them (it existed to trigger the fix job). The brief then sits inside its approved spec again. Still your call at the cap — (a) merge as-is, (b) fold the structural split and amend the spec, or (c) as above. |
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>
yihanzhu
removed
the
needs-human
|
Ruling taken: (c). The brief now describes a three-workflow lane — Cut with the fix stage: the stamped verdict marker, marker polling, the push-back re-review path, and the round-label brake — every one of them existed to serve that job. What The plan is inside its approved spec again, and it uses one of R5's two allowed bot edges, not a third. One mismatch worth naming rather than hiding: the spec's R4 (bounded fix loop) and R5's "all four workflows" now describe more than this plan builds. That is a delivery decision — shipping less than was specced, recorded in the plan's Deferred section — not a weakening of any rail, so the spec stays as the record of what was designed. When the phase closes it will read: R1–R3 and R5–R7 delivered, R4 deferred with reasons. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bba368d38b
ℹ️ 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 lane ships three workflows. The fix stage is cut from this phase** and | ||
| comes back later as its own intent, with its own spec. Two reasons, both worth |
There was a problem hiding this comment.
[Compliance] The new deferral removes the approved spec's R4 (work/v2-phase-2/spec.md:32-35) and changes the phase exit from “operator only merges” (spec.md:8-15) to a human manually handling findings (plan.md:174-176). Following this plan therefore cannot implement or prove the accepted Phase 2 scope; keep fix-on-review in PR B, or revise and re-approve the spec before treating this as the Phase 2 implementation plan.
AGENTS.md reference: AGENTS.md:L76-L83
Useful? React with 👍 / 👎.
| - **Never weaken the safety rails without explicit human sign-off:** reviewer stays | ||
| read-only / comments-only; **merging is the operator's, always** — the in-session | ||
| auto-merge v1 allowed was retired when the branch ruleset landed, and no agent has a | ||
| merge path any more; the rounds cap and `needs-human` escalation stay |
There was a problem hiding this comment.
[Compliance] This new never-merge rule leaves scripts/setup-target-repo.sh:108 unchanged, so every setup or reconciliation still installs a merge-ready description saying low-risk PRs are “auto-merged in-session.” Because that script force-updates the canonical labels in target repos, restored and newly configured teams continue to advertise the retired agent authority; update the shipped label definition to say the operator merges.
AGENTS.md reference: AGENTS.md:L65-L68
Useful? React with 👍 / 👎.
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>
|
Both adopted — and the first one is the chain enforcing itself from the other side. Earlier this reviewer stopped the plan from exceeding its spec (a third bot edge R5 never granted). This time it stopped the plan from silently dropping one: deferring the fix stage left R4 sitting in the approved spec with nothing building it, so the phase could not have proven the spec it answers to. Same principle both times — the plan is subordinate to the spec, in both directions. So the spec is amended to record the scope reduction you 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 Worth being explicit about the direction of this amendment, since I refused one earlier today: this one reduces what the lane does, which is a delivery decision the operator makes. The one I refused would have widened agent power to fit my plan. Reduce by ruling, never widen by convenience. Second finding, live artifact: Merging this PR is therefore both gates at once — G2 for the spec amendment and the plan's approval — which matches the single decision you already made. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c22703a6ec
ℹ️ 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".
| - **Never weaken the safety rails without explicit human sign-off:** reviewer stays | ||
| read-only / comments-only; **merging is the operator's, always** — the in-session | ||
| auto-merge v1 allowed was retired when the branch ruleset landed, and no agent has a | ||
| merge path any more; the rounds cap and `needs-human` escalation stay |
There was a problem hiding this comment.
[Security] In clones without branch protection, or when the session account can merge, the new claim that agents have no merge path is false: .claude/hooks/no-merge-guard.sh:8-11 explicitly permits scripts/merge-pr.sh, and lines 25-27 direct the agent to that bypass. The helper then performs the merge with the same session credentials. Block the helper from agent tool calls while leaving it available to an operator outside Claude.
AGENTS.md reference: AGENTS.md:L65-L68
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.
[Compliance] Fresh evidence in this revision is that the accepted spec is now amended in the same PR that carries its updated plan and other implementation-facing instructions. That collapses the required G2 approval into the later work instead of landing the changed spec first in its own spec: PR, so PR B would be delegated from a scope the operator never approved at the separate spec gate. Land and merge the amended spec separately, then regenerate the plan from that main revision.
AGENTS.md reference: AGENTS.md:L76-L83
Useful? React with 👍 / 👎.
| - **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 in `plan.md`, not from this text. |
There was a problem hiding this comment.
[Compliance] Fresh evidence in this revision is that R4 now says the fix loop is deferred, but the same spec's Design section at lines 75-86 still directs the implementer to create fix-on-review.yml, its issue_comment trigger, and the address-review skill; R5 and the Areas of concern also retain fix-stage and four-job wording. Because this artifact is the source for planning PR B, it gives two incompatible deliverable lists and can still cause the deferred credentialed workflow to ship. Reconcile the full spec with the three-workflow ruling.
AGENTS.md reference: AGENTS.md:L10-L14
Useful? React with 👍 / 👎.
| "round-3|1f6fc0|Review-loop counter: revision 3 (cap)" | ||
| "needs-human|d93f0b|Escalation: round cap hit, ambiguous spec, oversized PR, or failure" | ||
| "merge-ready|5319e7|Current head passed Codex review; auto-merged in-session if low-risk, else awaiting your merge" | ||
| "merge-ready|5319e7|This head passed review; waiting on YOUR merge (no agent merges; void once new commits land)" |
There was a problem hiding this comment.
[Bugs] Fresh evidence in the final label change is that installed target labels still say merge-ready becomes void only after new commits. When the base branch advances, the PR head is unchanged but the reviewed diff is stale, which manager/CLAUDE.md:416-425 now correctly recognizes; an operator reading this force-installed label description can therefore merge a false-green PR. State that either the head or base moving invalidates the label, and keep the human-facing docs consistent.
Useful? React with 👍 / 👎.
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>
|
All three adopted. The first was a live hole of mine. [Security] The guard hook exempted [Compliance] The spec amendment now has its own gate: #142. Folding it in here collapsed G2 into the plan's approval, and the answer is one extra PR, not an exception. Merge order: #142 first, then this one — the plan here already records the amended spec's hash, so the chain reads fresh the moment #142 lands (and correctly reads stale before it, which is the discipline working). [Compliance] The amendment was incomplete — R4 said deferred while the Design section still built |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d24a3bc6d
ℹ️ 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".
| @@ -1,5 +1,5 @@ | |||
| --- | |||
| spec-blob: 2efc5c5d6182e9f72c2e1f50e150d0d51faa4faf | |||
| spec-blob: 7e61bccc7b7ec1f2bc488bc13c6ca790694bade1 | |||
There was a problem hiding this comment.
[Compliance] At this commit, git hash-object work/v2-phase-2/spec.md returns f6500899eec771a94faf90912f706c7e82f66b2c, but the plan records 7e61bccc7b7ec1f2bc488bc13c6ca790694bade1. The required freshness check will therefore mark PR B stale and stop instead of implementing this plan. Regenerate the plan after the separately approved spec lands, using the exact blob that is present on main.
AGENTS.md reference: AGENTS.md:L81-L83
Useful? React with 👍 / 👎.
| - **Never weaken the safety rails without explicit human sign-off:** reviewer stays | ||
| read-only / comments-only; **merging is the operator's, always** — the in-session | ||
| auto-merge v1 allowed was retired when the branch ruleset landed, and no agent has a | ||
| merge path any more; the rounds cap and `needs-human` escalation stay |
There was a problem hiding this comment.
[Compliance] In the final tree, shipped guidance still contradicts this new rule: scripts/merge-pr.sh:10-13 says yshifu invokes the helper, while website/index.html:314-316 tells adopters that yshifu merges clean PRs. Leaving those entry points unchanged means operators or future agents can restore the retired authority even though the canonical file now forbids it; update them as part of the same documentation sync.
AGENTS.md reference: AGENTS.md:L49-L55
Useful? React with 👍 / 👎.
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>
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: 8c6576fada
ℹ️ 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".
| if printf '%s' "$cmd" | grep -Eq 'merge-pr\.sh'; then | ||
| echo "Blocked by ystack guard: merge-pr.sh is the operator's own tool. Apply merge-ready and hand the PR over." >&2 | ||
| exit 2 |
There was a problem hiding this comment.
In the normal installed path, /yshifu runs from the target clone, so Claude loads that target's .claude/settings.json, not the control-plane repo's settings; scripts/install.sh installs only the command file and never installs this hook. As a result, this new check is active only when Claude is launched inside ystack itself, and an agent in a target without branch protection can still invoke the absolute scripts/merge-pr.sh path with its session credentials. Fresh evidence after the earlier hook comment is that the helper block was added only to this repo-local hook; install a global/target hook or enforce the agent ban inside the helper.
AGENTS.md reference: AGENTS.md:L65-L68
Useful? React with 👍 / 👎.
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: 678eab844b
ℹ️ 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".
| supposed to have didn't land. `review-on-pr` runs on agent-opened PRs, which | ||
| is one of the two bot edges the spec's R5 allows; the lane opens no other. |
There was a problem hiding this comment.
[Compliance] When a collaborator other than the configured operator opens a same-repo PR, this agent-only plan and its actor gate skip the review, even though the approved spec's R3 requires every same-repo PR to receive one. Make review-on-pr accept all trusted same-repo authors while still rejecting fork PRs.
AGENTS.md reference: AGENTS.md:L76-L79
Useful? React with 👍 / 👎.
| - Three workflows: `spec-on-intent`, `implement-on-spec`, `review-on-pr`. | ||
| Every job: one agent at a time (the global `claude-quota` group), an actor | ||
| gate, hard time and turn limits, loud failure if the side effect it was |
There was a problem hiding this comment.
[Bugs] When two stage or review events queue while another Claude run is active, this shared concurrency group loses work: GitHub keeps at most one running and one pending run per group, and a newer run cancels the older pending one, as described in GitHub's concurrency documentation. An intent, spec, or PR can therefore remain unprocessed with no automatic retry; use a durable queue or locking design that retains every event while still running only one agent at a time.
Useful? React with 👍 / 👎.
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>
|
My own review of this PR found a defect worth naming: it was still carrying a stale copy of the spec. When I split the amendment out to #142, I restored Fixed: this PR now carries no spec change at all. Its scope is the plan, the rules, the docs sweep, and the hook that closes the merge-helper bypass. The spec belongs to #142 alone. Also rebaselined onto #142's current spec hash, and re-verified locally: rename gate clean, all suites green. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54ec34d8c0
ℹ️ 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".
| echo "Blocked by ystack guard: agents never merge PRs. The operator merges at the gate." >&2 | ||
| exit 2 | ||
| fi | ||
| if printf '%s' "$cmd" | grep -Eq 'merge-pr\.sh'; then |
There was a problem hiding this comment.
This pattern blocks every Bash command that merely mentions the file, not just commands that run it. For example, cat scripts/merge-pr.sh, git diff -- scripts/merge-pr.sh, and shellcheck scripts/merge-pr.sh all exit 2, so an agent cannot inspect or directly verify the helper during legitimate work. Narrow the match to command-position execution while still enforcing the rule that agents never merge.
AGENTS.md reference: AGENTS.md:L100-L101
Useful? React with 👍 / 👎.
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: 74276d3e70
ℹ️ 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".
| @@ -1,5 +1,5 @@ | |||
| --- | |||
| spec-blob: 2efc5c5d6182e9f72c2e1f50e150d0d51faa4faf | |||
| spec-blob: 804431e5d7d388dacb9f0293e6ddfe95fc04740f | |||
There was a problem hiding this comment.
[Compliance] Fresh evidence in da6885e is that this line was changed again to 804431e5d7d388dacb9f0293e6ddfe95fc04740f, while git rev-parse da6885e:work/v2-phase-2/spec.md returns 2efc5c5d6182e9f72c2e1f50e150d0d51faa4faf. The required freshness check will therefore mark the next implementation stale and stop it; regenerate the plan from the approved spec that is actually on main.
AGENTS.md reference: AGENTS.md:L81-L83
Useful? React with 👍 / 👎.
yihanzhu
added
the
merge-ready
|
merge-ready — but merge #142 first. This PR's Order: #142 → then I rebaseline this plan's one hash line and re-verify → then this. The rebaseline is a single line and doesn't touch anything reviewed here. |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Records R6's answer (app events cascade → direct path, fallback retired) and writes the eight hard-won rules into the plan so an implementer who never saw this conversation inherits them — which is the point of the artifact chain, and the precondition for delegating Stack B's build to Codex.
🤖 Generated with Claude Code