docs(plan): correct Phase 3c security-gate scope and 3d ordering - #294
Conversation
Two factual corrections to the approved specification. No requirement is
added, removed, or re-scoped beyond the named defects; no phase tag moves
(tag inventory verified byte-identical against origin/main).
Phase 3c's security-gate check was under-scoped. claude-code-plugins carries
zero repo-local rulesets — the gate is org ruleset 19388547, whose
`repository_property` condition scopes it to repos where the org-actor
editable, false-by-default custom property `requires-security-review` is
"true". Flipping either that property on a repo or the condition's own
property_values arms or disarms the required check with the ruleset context
string byte-identical, so the old check would have passed cleanly against a
disarmed gate. Phase 3d, which distributes the security caller fleet-wide, is
exactly when that drifts. The check is now a multi-part invariant: ruleset
active, empty bypass_actors, exact context string, the condition still
including the property == "true", AND the true-property repo set still equal
to {claude-code-plugins}.
Phase 3c's queue-overflow criterion named an artifact that may not exist. A
run cancelled while pending appears to leave no check RUN, so overflow would
present as check absence rather than a `cancelled` conclusion. The criterion
now probes that discriminator directly and treats absence as a failure of
equal weight. The supporting observation is recorded with both of its limits:
it is one data point, and it came from pending eviction under
`cancel-in-progress: false` with no `queue:` key, which is adjacent to
`queue: max` overflow rather than the same mechanism. The check suite at that
commit does conclude `cancelled` with zero runs, so the signal survives at
suite level while the required check — which the ruleset evaluates by
context — is absent.
Phase 3d's ordering instruction was wrong and would have caused the outage it
was meant to prevent. Exposure is symmetric: the operative gate is a
cardinality check in standards-sync.yml's `attest` job, so granting first and
merging first each wedge, in mirrored directions. The mitigation is a tight
window, not an ordering. The mechanism is corrected too — repository
SELECTION is REST-addressable under a classic PAT with repo scope, so the
extension is an API call sequenceable to seconds rather than a hand-timed UI
click. Selection is not the same surface as installation PERMISSIONS; 3a0's
grant stays the org-owner action it is documented as. Whether org-owned
installations gate above repo-admin is undocumented, so the PAT holder should
be an org owner rather than assume repo admin suffices.
The 3d bullet is re-wrapped as a whole to keep the ~80-char convention; the
text after "Wave 1:" is proven word-identical to origin/main.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 4m 48s —— View job Review: Phase 3c/3d PLAN.md corrections
This is a documentation-only change to the approved I independently re-derived the mechanical claims in the test plan (tag Two findings from that pass: 1. The "org owner must hold the PAT" mitigation isn't propagated to all three restated lines (moderate) 3d's body (and the new But the PR description says three "consequential one-liners" were corrected
"Handoff to implementation" is exactly the section an implementer would 2. Minor grammar in the queue-overflow pass criterion (nit) PLAN.md:635-637: FYI, not a finding: the test plan's mechanical em-dash count Everything else — the security-gate multi-part invariant (3c), the symmetric |
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
Fix a malformed sentence in the queue-overflow pass criterion ("Pass requires
that required check reporting" did not parse) — it defines the acceptance
criterion, so it needs to be unambiguous.
Carry the "held by an org owner" qualifier into the other two restatements of
the Phase 3d gate (approval-record item 5 and the handoff user-approval-gate
line). It previously appeared only on the [GATE] line under Open questions,
but the handoff section is what an implementer reads at execution time, and
the qualifier exists specifically to mitigate a gating unknown the document
itself flags as UNDOCUMENTED.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…onale (#295) ## 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 `main` right 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 no `standards-ref`, so the reusable's `main` default plans the **post-merge** manifest (`standards-sync.yml`, `plan` job checkout). So: - **Merge-first** self-triggers: the merge is the triggering push, `attest` sees 8 installed against 10 expected, and the wedge is **certain and immediate**. - **Grant-first** triggers nothing. It wedges only if an unrelated push, the weekly cron, or a `workflow_dispatch` lands inside the window. `BEFORE` is 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`/`excess` is only reachable on an equal-cardinality substitution. 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:** - **False recovery claim.** "If the window closes with no push, recovery waits for that cron" is wrong — `sync.yml` also carries `workflow_dispatch` with a settable `dry-run` input, so recovery is immediate. - **3c invariant was incomplete.** It pinned the `repository_property` condition but not `target` or `conditions.ref_name`. Each of those is a single-field disarm that leaves every pinned field byte-identical: retarget the ruleset, or move `~DEFAULT_BRANCH` from `include` to `exclude`, and #294's check still goes green on a disarmed gate. Both are now pinned. - **Pass statuses were over-attributed.** `success`/`skipped`/`neutral` is enumerated for *classic branch protection*; the rulesets docs never enumerate conclusions, and gate `19388547` is a ruleset. Now recorded as the best-documented proxy rather than as documented fact for this surface. - **Org-owner qualifier was overstated.** The docs require only "admin access to 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 lists `security-review / security-review`". That endpoint returns only `{id, name, source_type, target, enforcement}` — the context string never appears, 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. - Encoding: zero U+FFFD, zero CR; em-dashes preserved as U+2014 (143 → 150 vs `origin/main`, all in added lines). - Phase-tag inventories (`[DOING]`, `[DONE]`, `[TODO]`, `[GATE]`, `[USER-RESERVED]`, `[USER-APPROVAL GATE]`, `[EXEC-SHAPE]`) byte-identical to `origin/main` — no tag advanced. - The 3d bullet is re-wrapped as a whole; the text after "Wave 1:" is proven **word-identical** to `origin/main` by token-level comparison, so the reflow changed wrapping only. - Wrap: no added line exceeds 81 chars, inside the file's existing distribution. Load-bearing facts re-derived by command before writing: `standards-ref`'s `main` default and the `plan` job checkout (the trigger-coupling discriminator); `sync.yml`'s `push`/`workflow_dispatch`/`schedule` triggers; ruleset `19388547`'s `target`, `conditions.ref_name`, `bypass_actors`, and context string; the `requires-security-review` schema and its regenerated true-set; the repo-level `rulesets` response shape; and the GitHub REST documentation for the installation-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. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Summary
Two factual corrections to
docs/topics/claude-review-lanes/PLAN.md, theapproved specification. Corrections only — no requirement is added, removed,
or re-scoped beyond the two named defects, and no phase tag moves (tag
inventory verified byte-identical against
origin/mainfor[DOING],[DONE],[TODO],[GATE],[USER-RESERVED],[USER-APPROVAL GATE],[FALLBACK …],[EXEC-SHAPE]).Defect A — Phase 3c's security-gate check was under-scoped. It asked only
that the claude-code-plugins ruleset string
security-review / security-reviewbe intact. But claude-code-plugins carries zero repo-local rulesets
(
gh api repos/…/rulesets?includes_parents=false→[]); the gate is orgruleset
19388547, whoserepository_propertycondition scopes it to reposwhere the custom property
requires-security-reviewis"true". That propertyis
org_actors-editable and defaults"false", and is currentlytrueonexactly one of fourteen repos. So flipping the property on a repo — or
inverting the condition's own
property_values— arms or disarms the requiredcheck with the context string byte-identical. The old check would have passed
cleanly against a disarmed gate, and Phase 3d, which distributes the security
caller fleet-wide, is exactly when that drifts. The check is now a multi-part
invariant.
Defect A′ — Phase 3c's queue-overflow criterion was unobservable as written.
It asked for an "overflowed-cancelled caller conclusion observed". A run
cancelled while pending appears to leave no check run at all, so overflow
would present as check absence, not a
cancelledconclusion. The criterionnow probes that discriminator directly, with pass = the required check
reporting a documented-pass status (
success,skipped,neutral) andabsence treated as a failure mode of equal weight. The supporting observation
is recorded with both of its limits: it is one data point, and it came from
pending eviction under
cancel-in-progress: falsewith noqueue:key, whichis adjacent to
queue: maxoverflow rather than the same mechanism.Defect B — Phase 3d's ordering instruction was wrong and would have caused
the outage it prevents. It said App access extends "BEFORE the manifest PR
merges". Exposure is symmetric: the operative gate is a cardinality check in
standards-sync.yml'sattestjob, which aborts before themissing/excessset-diff is ever reached. Granting first fails
installation reports 10 repositories; expected 8; merging first fails theinverse.
attestis aneeds:ofsync, so the whole matrix is skipped —fail-closed and self-clearing, nothing mutates, no target corrupted. The
mitigation is a tight window, not an ordering.
The 3d mechanism is corrected too: repository selection is
REST-addressable (
PUT /user/installations/{installation_id}/repositories/{repository_id}),so the extension is an API call sequenceable to seconds rather than a
hand-timed UI click. Selection is not the same surface as installation
permissions; 3a0's
workflows: writegrant stays the org-owner action it isdocumented as. Three consequential one-liners repeating the wrong "BEFORE"
ordering are corrected in step: approval-record item 5, the
[GATE]line, andthe user-approval-gate line.
Volatile line-number citations are deliberately not reintroduced (the old
text cited
standards-sync.yml:309-355); the new text citesstandards-sync.yml, attestand the error-message fragment instead, so itstays correct across the pending sync-engine re-pin.
Test plan
Documentation-only change; no runtime behavior. Verified mechanically:
markdownlint-cli2on the file — 0 errors.current base,
origin/main= 128 and this branch = 143, net +15, all of themin added lines; U+2013 unchanged at 7 both sides; file remains UTF-8 with LF
endings. (An earlier figure in this description was measured before the rebase
onto
origin/main; the net delta was the same, the absolute counts were not.)81–89 distribution for body prose; markdownlint MD013 is disabled here, so the
~80 wrap is convention rather than an enforced rule.
byte-identical to
origin/main.after "Wave 1:" is proven word-identical to
origin/mainby a token-levelcomparison, so the reflow changed wrapping only.
git merge-tree --write-treeagainstorigin/mainbeforeand after docs(plan): record Phase 5 equivalence + sweep evidence #287 landed — clean auto-merge both times. Branch is now rebased
onto
origin/mainat9416a0d.Every factual claim in the new text was re-derived from live state by command
before being written (org ruleset
19388547detail, the custom-property schemaand its regenerated true-set, run
30172462100and the check runs/suite atd26dac6, theattestjob source, the manifest-derived expected cardinalityof 8, installation
144867070'srepository_selection, both new-targetrepository ids, and the two quoted sentences against the current GitHub
OpenAPI description). An independent fresh-context verifier re-derived both
corrections with the rationale withheld; its findings are incorporated — the
three it raised as blocking (a cross-reference to a finding not present in
PLAN.md, an actor claim that outran what the docs support, and the
queue-overflow mechanism mismatch) are each fixed in the text above.
Related
No linked issue — this corrects the specification document for the in-flight
claude-review-lanes effort.