docs(plan): restore Phase 3d's BEFORE ordering; correct only its rationale - #295
Merged
Conversation
Follow-up to #294. That PR corrected two real defects but overreached on one: it replaced Phase 3d's "App access extends BEFORE the manifest PR merges" with a tightest-window instruction, on the premise that exposure is symmetric. That premise does not survive re-derivation, so the ordering is restored here and only the rationale stays corrected. ORDERING RESTORED. The failure MODE is symmetric; the trigger coupling is not. The manifest PR merge is itself a push to standards main, and the caller passes no `standards-ref`, so the reusable's `main` default plans the POST-merge manifest — merge-first self-triggers and wedges with certainty, while grant-first wedges only if an unrelated push, the weekly cron, or a dispatch lands inside the window. BEFORE is the safer order and is restored in 3d, approval item 5, the [GATE] line, and the user-approval-gate line. What was genuinely wrong stays corrected: the original rationale said attestation "compares the FULL target set both directions", but the operative gate is a cardinality check that aborts before any set-diff. Grant-first's real cost — the App holds write access to two non-target repos for the window — is now stated rather than implied. RECOVERY CLAIM FIXED. "Recovery waits for that cron" was false: sync.yml also carries workflow_dispatch with a settable `dry-run` input, so recovery is immediate. 3C INVARIANT COMPLETED. It pinned the property condition but not `target` or `conditions.ref_name`, each of which is a single-field disarm that leaves every pinned field byte-identical. Both are now pinned. PASS STATUSES ATTRIBUTED. `success`/`skipped`/`neutral` is enumerated for classic branch protection; the rulesets docs never enumerate conclusions, and this gate is a ruleset. Recorded as the best-documented proxy, not as gospel. ORG-OWNER QUALIFIER DOWNGRADED. The docs require only repo admin; org-owner gating is undocumented. The three summary lines now carry it as a precaution, matching how the body already frames it. SANITY CHECK FIXED. The Phase 3 Sanity Check told the operator to verify the gate with `gh api repos/melodic-software/claude-code-plugins/rulesets`. That endpoint returns only {id, name, source_type, target, enforcement} — the context string never appears, so the recipe was unexecutable. It is the same defect #294 corrected, expressed as a command. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 2m 45s —— View job Review: docs(plan): restore Phase 3d's BEFORE ordering; correct only its rationale
Scope: single file, Verification performed:
Not independently run: Findings: none. The rationale correction is well-derived and matches the actual |
![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: 58d2b88dff
ℹ️ 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".
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
… list
Independent verification found the enumerated invariant unsound and two
supporting details wrong.
WHOLE-OBJECT COMPARE. The previous form listed the fields to check and asserted
that list was complete. It was not: `conditions.repository_property.exclude` is
a documented single-field disarm — "The condition will not pass if any of these
properties match" — so excluding any property claude-code-plugins carries
(`requires-signing == "true"`, for instance; it carries that, `requires-ci`, and
`requires-security-review`) disarms the gate while every enumerated field stays
byte-identical. An implementer running the check would have reported an already
disarmed gate as intact, which is the exact failure 3c exists to catch. The
criterion is now a canonicalized-JSON compare of the entire `conditions` and
`rules` objects, with the disarm examples kept as motivation rather than as a
closed list.
SANITY CHECK FIELD LIST CORRECTED. It claimed the repo-level `rulesets` listing
returns only `{id, name, source_type, target, enforcement}`. It also returns
`source`, `node_id`, `_links`, `created_at`, and `updated_at`. The load-bearing
point is unchanged and simpler stated: the listing returns neither `rules` nor
`conditions`, so it cannot verify the gate.
PROPERTY-SET RECIPE ADDED. The Sanity Check named the property-set criterion
without giving a way to regenerate it. It now carries a `gh api` + `jq` command,
run and confirmed to return exactly `claude-code-plugins`.
RECOVERY ASYMMETRY STATED. Recovery is asymmetric in the same direction as the
wedge: under grant-first the manifest merge is itself the clean recovery run,
while merge-first needs a manual `workflow_dispatch` with `dry-run: false` —
the dispatch default is `true` and both `attest` and `sync` gate on
`!inputs.dry-run`, so a default dispatch syncs nothing. That is a stronger
operational argument for the BEFORE ordering than the one already stated.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…y claim Review finding, verified against the tree and correct on both halves. DRAIN PRECONDITION ADDED. The grant-first window was described as wedging only if a NEW trigger lands inside it. That misses already-triggered work: the reusable serializes on `concurrency: standards-sync` with `cancel-in-progress: false`, so runs queue rather than cancel, and a queued run resolves the moving `main` ref only when its `plan` job finally executes. A run triggered before the grant can therefore plan the pre-merge manifest (8) against the post-grant installation (10) and wedge. The sequence is now drain, grant, merge, with an explicit check that no standards-sync run is in progress or queued. CERTAINTY CLAIM DROPPED. "Merge-first wedges with CERTAINTY" was too strong: if the grant completes before the merge-triggered run reaches `attest`, it passes. Restated as merge-first MANUFACTURING its own triggering run and wedging unless the grant wins a seconds-to-minutes race — which is not something to plan around, and leaves the ordering conclusion intact. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
kyle-sexton
added a commit
that referenced
this pull request
Jul 29, 2026
Follow-up to #295, which landed the drain precondition as prose. It told the operator to confirm no sync run is active or queued without saying how, and it checked only once. The sequence is now drain, grant, RE-CHECK, merge. The second check is the one that matters: the window between grant and merge is exactly where a newly triggered run would land, and the pre-grant check cannot see it. The command negates `completed` rather than enumerating statuses, so `queued`, `in_progress`, and `waiting` are all caught by construction. Run against standards, it currently returns `[]`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
kyle-sexton
added a commit
that referenced
this pull request
Jul 29, 2026
…tion a command (#300) ## Summary Follow-up to #295. Independent verification of the merged text found the Phase 3c gate invariant **unable to detect two of the disarms its own prose names**, and the Phase 3d drain precondition stated without a command. Both fixed here, plus four smaller accuracy defects. ### 1. The 3c invariant could sign off on a disarmed gate (the important one) #295 pinned the ruleset's `conditions` + `rules` objects. But `target` and `bypass_actors` are top-level **siblings** of those keys, not nested inside them. The live object's keys are: ``` bypass_actors, conditions, created_at, enforcement, id, name, node_id, rules, source, source_type, target, updated_at, _links ``` So a gate silently retargeted to `tag`, or handed a bypass actor, passed the check **byte-for-byte** — while the very same sentence named both as sufficient single-field disarms. An approved spec executed verbatim would have reported an already-disarmed security gate as intact, which is precisely the failure 3c exists to catch. The fix inverts the enumeration. Rather than list the fields to keep (an allow-list is what fails here, and fails silently when GitHub adds a field), it lists only the fields that provably **cannot** disarm and compares the whole remainder: ``` gh api orgs/melodic-software/rulesets/19388547 \ --jq 'del(.id,.name,.node_id,.source,.source_type,.created_at,.updated_at,._links)' ``` The Phase 3 Sanity Check restated the invariant **weaker still** — `conditions` + `rules` only, dropping even `enforcement`. It now carries the same deny-list compare, on one line so it is copy-pasteable rather than reassembled from wrapped code spans. ### 2. The drain precondition now has a command, a re-check, and no blind spot #295 landed the precondition as prose: confirm no sync run is active or queued, without saying how, checked once. The sequence is now **drain, grant, re-check, merge**. The second check is the one that matters — the window between grant and merge is exactly where a newly triggered run lands, and the pre-grant check cannot see it. The command paginates the full run history rather than sampling a page: ``` gh api "repos/melodic-software/standards/actions/workflows/sync.yml/runs?per_page=100" --paginate --slurp \ | jq -c '[.[].workflow_runs[]|select(.status!="completed")]' ``` Three deliberate choices, each closing a real hole found in review: - **Negates `completed`** rather than enumerating pending states — GitHub has at least five (`queued`, `in_progress`, `waiting`, `requested`, `pending`) and may add more. - **Paginates instead of sampling.** `gh run list` defaults to `--limit 20`, and raising the limit still rests on gh's newest-first ordering, which neither gh nor the REST docs document. A pending run outside the sample would read as a drained queue. - **Pipes to `jq` rather than using `--jq`.** `--slurp` is rejected alongside `--jq`, and this matters beyond syntax: `gh api --paginate --jq` applies the filter **per page**, emitting one array per page instead of the single `[]` the criterion asks for. The text now also says plainly that this **reduces** the window rather than closing it — a push between the re-check and the merge still wedges, which is why the fail-closed property is what makes the procedure safe, not the checks. ### 3. Four smaller accuracy fixes - **The repo-listing trap is named.** The bare `gh api repos/.../rulesets` call returns the 4 org-sourced rulesets and reads as though the repo owned them; `?includes_parents=false` returns `[]`. An operator told "zero repo-local rulesets" would otherwise see 4 and doubt the spec. - The pending-state parenthetical no longer reads as exhaustive. - Decision-log item 5 now carries the drain step, matching 3d. - Both Sanity Check commands are single-line and copy-pasteable. ## Test plan Documentation-only; no runtime behavior. **Every `gh` command in the spec was extracted from the file as a reader would copy it (a wrapped backtick span renders as one line) and executed** — not merely written: | Command | Result | | --- | --- | | 3c bullet ruleset compare | exit 0 | | Sanity Check ruleset compare | exit 0 | | Drain check | exit 0, returns `[]`, inspects all 176 runs | | Property-set regeneration | exit 0, returns exactly `claude-code-plugins` | The one command that does not run is pre-existing and carries a `<repo>` placeholder by design. Also verified: - `markdownlint-cli2` — 0 errors. - Zero U+FFFD, zero CR; UTF-8/LF preserved. - Phase-tag inventories byte-identical to `origin/main` — no tag advanced. - The 3d bullet is re-wrapped as a whole; text after "Wave 1:" proven **word-identical** to `origin/main` by token comparison. - The only added lines over 81 chars are the copy-pasteable commands themselves, matching the Sanity Check block's existing precedent. - The ruleset key structure, the `repository_property.exclude` semantics, and the `gh run list` default limit were each re-derived by command before being written. ## Related No linked issue — corrects the specification document for the in-flight claude-review-lanes effort. Follows up #295. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #294, which auto-merged via this repo's loop-lane merge automation
while this correction was still in review. #294 fixed two real defects but
overreached on one of them, and the overreach is on
mainright now.What #294 got wrong. It replaced Phase 3d's approved instruction — "App
access extends BEFORE the manifest PR merges" — with a tightest-window
instruction, justified by "Exposure is SYMMETRIC — neither order is the safe
one." That premise does not survive re-derivation.
The failure mode is symmetric: either order trips the same cardinality check.
The trigger coupling is not. The manifest PR merge is itself a push to
standards
main, and the standards caller passes nostandards-ref, so thereusable's
maindefault plans the post-merge manifest(
standards-sync.yml,planjob checkout). So:attestsees8 installed against 10 expected, and the wedge is certain and immediate.
weekly cron, or a
workflow_dispatchlands inside the window.BEFOREis the lower-risk order. It is restored verbatim in the 3d pre-step,approval-record item 5, the
[GATE]line, and the user-approval-gate line —which means the approved decision is left standing, not overturned.
What stays corrected, because it was genuinely wrong: the original rationale
claimed attestation "compares the FULL target set both directions". It does not
get that far — the operative gate is a cardinality check that aborts before
any set-diff, and
missing/excessis only reachable on an equal-cardinalitysubstitution. Grant-first's real cost — the App holds write access to two repos
that are not yet manifest targets, for the window — is now stated rather than
implied. The REST-selection mechanism, installation and repository ids, and the
selection-vs-permissions distinction from #294 are unchanged.
Four further corrections in the same pass:
for that cron" is wrong —
sync.ymlalso carriesworkflow_dispatchwith asettable
dry-runinput, so recovery is immediate.repository_propertyconditionbut not
targetorconditions.ref_name. Each of those is a single-fielddisarm that leaves every pinned field byte-identical: retarget the ruleset, or
move
~DEFAULT_BRANCHfromincludetoexclude, and docs(plan): correct Phase 3c security-gate scope and 3d ordering #294's check stillgoes green on a disarmed gate. Both are now pinned.
success/skipped/neutralisenumerated for classic branch protection; the rulesets docs never enumerate
conclusions, and gate
19388547is a ruleset. Now recorded as thebest-documented proxy rather than as documented fact for this surface.
the repository"; org-owner gating is flagged UNDOCUMENTED in the body. The
three summary lines now carry it as a precaution, matching the body.
One edit outside the sites #294 touched, called out deliberately: the Phase 3
Sanity Check told the operator to verify the gate with
gh api repos/melodic-software/claude-code-plugins/rulesets"still listssecurity-review / security-review". That endpoint returns only{id, name, source_type, target, enforcement}— the context string neverappears, so the recipe is unexecutable, and #294's own thesis makes it obsolete.
It is the same defect expressed as a command, so leaving it would have it
contradict 3c a hundred lines below.
Test plan
Documentation-only; no runtime behavior. Verified mechanically on this branch:
markdownlint-cli2— 0 errors.origin/main, all in added lines).[DOING],[DONE],[TODO],[GATE],[USER-RESERVED],[USER-APPROVAL GATE],[EXEC-SHAPE]) byte-identical toorigin/main— no tag advanced.word-identical to
origin/mainby token-level comparison, so the reflowchanged wrapping only.
Load-bearing facts re-derived by command before writing:
standards-ref'smaindefault and theplanjob checkout (the trigger-coupling discriminator);sync.yml'spush/workflow_dispatch/scheduletriggers; ruleset19388547'starget,conditions.ref_name,bypass_actors, and context string; therequires-security-reviewschema and its regenerated true-set; therepo-level
rulesetsresponse shape; and the GitHub REST documentation for theinstallation-repository endpoints.
Two independent fresh-context verifiers audited this work with rationale
withheld. The second one is what surfaced the ordering error and four of the
five other findings; every one of its blocking findings is addressed here.
Related
No linked issue — corrects the specification document for the in-flight
claude-review-lanes effort. Follows up #294.