Skip to content

docs(plan): order the engine re-pin ahead of the 3d window as pre-step (0) - #301

Merged
kyle-sexton merged 2 commits into
mainfrom
docs/plan-3d-repin-prestep
Jul 29, 2026
Merged

docs(plan): order the engine re-pin ahead of the 3d window as pre-step (0)#301
kyle-sexton merged 2 commits into
mainfrom
docs/plan-3d-repin-prestep

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Summary

Phase 3d could not be executed safely as written. Two ordering defects, both
verified against live state, both blocking.

Pre-step (0): re-pin the engine before the window opens

standards sync.yml pins ci-workflows at ac223bb, where auto-merge arming is
gated on:

if: steps.cpr.outputs.pull-request-operation == 'created' && matrix.automerge

Why that gate is unsound for this phase. create-pull-request at the pinned
5f6978f (v8.1.1) always attempts pulls.create first and returns
created: true on success; it reaches the update path only when that call is
rejected with A pull request already exists for. So the discriminator is an
open PR — not the existence of the chore/standards-sync branch.

Phase 3d manufactures exactly that state: the window holds sync PRs open by
design
. So any target whose PR is already open when arming would fire is
silently not armed, and restoring automerge: true while wave PRs are still
open leaves precisely those PRs unarmed, to be merged by hand.

It is neither permanent nor fleet-wide — once a PR merges, the branch is
deleted and the next delta reports created again. That is what makes it easy
to miss, and it is the honest case for ordering the re-pin first.

One pin satisfies both requirements, not two. I established the target SHA
rather than assuming it:

  • git log -S "pull-request-number != ''" shows 8202e03 is where the fixed
    gate was introduced — the same commit that adds the never-armed watchdog
    (fix(standards-sync-stuck-automerge-alert): detect sync PRs that were never armed #291). The arming fix and the watchdog are not separate pins.
  • git diff --name-only 8202e03 origin/main returns only PLAN.md, so
    nothing functional has landed since and no later SHA is required.
  • The new gate pull-request-number != '' is non-empty on both the create
    and update paths, so it arms in the state the old gate missed.

Target: 8202e03f30dd0c0189c862052d5f242b9a496798.

Pre-step (i): marked landed, and ordered

It read as an open, unordered to-do. It landed in standards#290 across 8/8
targets, so it is a state to confirm by reading the manifest, not a step to
perform.

Only the re-pin writes; confirming is a read. Because it writes to standards
main, it must precede the drain, or it becomes the very trigger the drain
exists to exclude. The restore of automerge: true is named as a second write
belonging after the window closes. The whole sequence is now stated once:

re-pin → confirm automerge: false → drain → grant → re-check → merge

Per-target counts are deliberately kept out of the spec — they decay. The
rule is stated instead, with the commands to regenerate current state.

Test plan

Documentation-only; no runtime behavior. Every load-bearing fact re-derived by
command before it was written:

Claim How verified
standards pins ac223bb live contents API on standards main
ac223bb gates arming on 'created' git show ac223bb:.github/workflows/standards-sync.yml
created vs updated keys on an open PR, not a branch read github-helper.ts at the pinned 5f6978fpulls.create first, ERROR_PR_ALREADY_EXISTS fallthrough
The branch is not universal live per-target branch probe: present on exactly the targets with an open sync PR, absent elsewhere (delete_branch_on_merge)
8202e03 gates on pull-request-number != '' git show 8202e03:… — in the "Arm auto-merge on the sync PR" step
That commit introduced the fix git log -S "pull-request-number != ''" returns exactly 8202e03
Same commit carries the watchdog git show --stat 8202e03 (#291)
Nothing functional landed since git diff --name-only 8202e03 origin/main → only PLAN.md
Pre-step (i) already landed, 8 targets regenerated from the manifest on standards main (standards#290)

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 one long line is the pre-existing drain command, kept on one line so it
    stays copy-pasteable.
  • No open PR touches PLAN.md, so no conflict with sibling work in flight —
    re-checked before pushing.

Correction history, stated plainly: the first commit on this branch
justified pre-step (0) with a false premise — that the branch already existed
fleet-wide, so runs always reported updated. An independent verifier caught
it; I re-derived the mechanism from the pinned action's source and corrected it
in the second commit. The prescribed ordering never changed, only its
justification, which is now narrower and true.

Numbering note: the new step is (0) rather than renumbering (i)/(ii),
because nothing else in the document cross-references those labels (checked)
and renumbering would churn text this PR has no reason to touch.

Related

No linked issue — corrects the specification document for the in-flight
claude-review-lanes effort. Follows up #300.

…p (0)

Phase 3d could not be executed safely as written. Two ordering defects, both
verified against live state.

RE-PIN IS NOW PRE-STEP (0). standards `sync.yml` pins ci-workflows at
`ac223bb`, where auto-merge arming is gated on
`pull-request-operation == 'created'`. Every target's `chore/standards-sync`
branch already exists, so subsequent runs report `updated`, never `created`,
and arming is skipped — which makes pre-step (i)'s "restore after" a permanent
no-op at that pin, leaving every sync PR silently unarmed. A warning would have
documented the trap; ordering the re-pin ahead of the window removes it.

The target SHA is `8202e03f30dd0c0189c862052d5f242b9a496798` (#291), and ONE
pin satisfies both requirements rather than two: `git log -S` shows that commit
is where `pull-request-number != ''` replaced the `'created'` gate, and it adds
the never-armed watchdog in the same commit. `git diff --name-only
8202e03 origin/main` returns only PLAN.md, so nothing functional has landed
since and no later SHA is needed.

PRE-STEP (i) IS MARKED LANDED AND ORDERED. It read as an open, unordered
to-do; it landed in standards#290 across 8/8 targets, so it is a state to
confirm rather than a step to perform. It is also itself a push to standards
main — as is the re-pin — so both must precede the drain, or they become the
very trigger the drain exists to exclude. The whole ordered sequence is now
stated once: re-pin, confirm `automerge: false`, drain, grant, re-check, merge.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 4m 48s —— View job


Review: docs(plan) — order the engine re-pin ahead of the 3d window

  • Gather context (PR body, diff, CLAUDE.md)
  • Understand the request (docs-only review of PLAN.md)
  • Review diff against CLAUDE.md guidance and internal consistency
  • Post findings

Scope note: this PR only touches docs/topics/claude-review-lanes/PLAN.md. It's not a workflow file, so the CLAUDE.md security-lane rules (SHA-pinning, pull_request_target tripwires, least-privilege tokens, log hygiene) don't directly apply here — I focused on factual correctness and internal consistency of the plan text instead, since this document is the operational runbook for a live, security-sensitive rollout.

Verification performed

I independently re-derived the load-bearing technical claims rather than taking them on faith:

  • ac223bb (current pin): confirmed if: steps.cpr.outputs.pull-request-operation == 'created' && matrix.automerge at .github/workflows/standards-sync.yml:475.
  • 8202e03 (target pin): confirmed if: steps.cpr.outputs.pull-request-number != '' && matrix.automerge at line 487, and confirmed via git show --stat that the same commit touches standards-sync-stuck-automerge-alert.yml (the watchdog) alongside standards-sync.yml — the "one pin satisfies both" claim holds.
  • Confirmed no other (i)/(ii) cross-references exist elsewhere in the doc, so the new (0) label doesn't collide with anything (per the PR's own numbering note).
  • Confirmed every newly added line in the diff stays ≤81 chars, and the one pre-existing >81-char line (the gh api ... | jq ... drain command, line 697) is unchanged context in the diff, not newly added — matches the stated test plan.
  • No trailing whitespace introduced.
  • Attempted to verify "nothing but PLAN.md has changed since 8202e03" via git diff --name-only 8202e03 origin/main, which returned only PLAN.md — consistent with the claim. (Caveat: my checkout is shallow, so I couldn't fully confirm ancestry via merge-base --is-ancestor; the diff result is corroborating but not a substitute for the live-API check the author already ran.)

No correctness defects found. The added text is logically consistent with the pre-existing 3d prose it's interleaved with (e.g., the "restore automerge after the window" note now correctly depends on step (0) having landed first, since restoring automerge: true at the old pin would still silently fail to re-arm).

One minor, non-blocking suggestion

docs/topics/claude-review-lanes/PLAN.md#L662 — the claim "nothing but docs has landed on ci-workflows main since [8202e03]" is a live-state fact with no re-verification command or dated anchor attached, unlike comparable time-sensitive preconditions elsewhere in this same document (e.g. the explicit gh api/jq drain-check command a few lines below, or the "at lock time" framing used for the action pin near the top of the doc). Since this is a runbook meant to be executed potentially days or weeks after being written, and this specific fact is what justifies skipping a re-derivation of the target SHA, it'd be more consistent with the doc's own convention to give the operator the exact command to re-run before acting on step (0) — e.g. git diff --name-only 8202e03f30dd0c0189c862052d5f242b9a496798 origin/main. Not a defect in the current PR, just a small robustness improvement to consider (could be folded into a future edit rather than blocking this one).

@github-actions

Copy link
Copy Markdown

Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count.

…tence

Independent verification falsified the justification I wrote for pre-step (0).
The step is still right; the reason given for it was not. Corrected from the
pinned action's own source rather than from inference.

WRONG PREMISE. I wrote that every target's `chore/standards-sync` branch
already exists, so runs report `updated` and never `created`. Both halves fail.
`create-pull-request` at the pinned `5f6978f` (v8.1.1) ALWAYS attempts
`pulls.create` first and returns `created: true` on success; it falls through to
the update path only when that call is rejected with "A pull request already
exists for". The discriminator is an OPEN PR, not a branch. And the branch is
not universal anyway — targets merge their sync PRs and `delete_branch_on_merge`
removes it, so live state today has the branch on only half the fleet, exactly
matching which targets have an open PR.

CORRECTED, AND NARROWER. At `ac223bb`, arming is skipped for any target whose
sync PR is already OPEN when arming would fire. That is precisely the state
Phase 3d manufactures, because the window holds PRs open by design — so
restoring `automerge: true` while wave PRs are still open leaves exactly those
PRs unarmed, to be merged by hand. It is neither permanent nor fleet-wide: once
a PR merges its branch goes away and the next delta reports `created` again.
That is what makes it easy to miss, and it is the honest case for doing the
re-pin first rather than a claim of universal breakage.

The re-pin target is unchanged and still verified: `8202e03f` changes the gate
to `pull-request-number != ''`, which is non-empty on BOTH the create and update
paths, and adds the never-armed watchdog in the same commit.

ALSO FIXED. The sequence sentence said "the first two are pushes to standards
main" while (i) had just been described as a state to confirm. Only the re-pin
writes; confirming is a read. The restore of `automerge: true` is named as a
second write belonging after the window. Volatile per-target counts are
deliberately kept OUT of the spec — the rule is stated instead, with the
commands to regenerate current state.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@kyle-sexton
kyle-sexton merged commit 70e11d9 into main Jul 29, 2026
38 checks passed
@kyle-sexton
kyle-sexton deleted the docs/plan-3d-repin-prestep branch July 29, 2026 18:35
kyle-sexton added a commit that referenced this pull request Jul 29, 2026
## Summary

Pre-step (0) (merged in #301) states that re-pinning the standards sync
engine
to `8202e03f` also activates the never-armed watchdog, "so ONE pin
satisfies
both." The commit does carry both halves; **standards does not consume
it that
way**, so the guidance is wrong for the operator who follows it.

Verified against the live repos:

| Claim | Reality |
|---|---|
| One pin activates arming + watchdog | Two separate callers, two
separate pins |
| Watchdog rides `sync.yml`'s pin | Watchdog is
`standards-sync-stuck-automerge-alert.yml`, a different reusable |

```
standards/.github/workflows/sync.yml:33
  → standards-sync.yml@8202e03f…                      (standards#293, in flight)
standards/.github/workflows/standards-sync-stuck-automerge-alert.yml:19
  → standards-sync-stuck-automerge-alert.yml@43bc8d0f  # 43bc8d0 2026-07-22
```

Re-pinning `sync.yml` therefore activates the **arming half only**.

`43bc8d0` predates **two** commits, not one — `git log --oneline
43bc8d0..8202e03 --
.github/workflows/standards-sync-stuck-automerge-alert.yml`:

- `8202e03` #291 — never-armed detection
- `42329ef` #234 — split the stuck-PR scan into a cheap page fetch +
per-candidate merge-state probe with retry

**The deadline the old text implied was already met.** The never-armed
scan
selects targets with `jq '[.include[] | select(.automerge) |
.repo_name]'` and
gates candidates on `automergeRepoNames.has(repo)`, so it is inert while
the
rollout window holds all 8 targets at `automerge: false` — and becomes
load-bearing at the exact moment `automerge: true` is restored. The
watchdog
re-pin must land **before** that restore.

**One nuance the correction records**, because it cuts against reading
the
stale pin as wholly dormant: the *armed-but-stuck* scan is **not**
automerge-gated. It loops `repoNames` (all 8 targets) at `:299` and
pushes
`isArmedCandidate` matches at `:315` with no automerge check — only
`:316`'s
never-armed branch is gated. It sweeps every target on the hourly cron
today,
and is quiet only because no sync PR is currently armed (checked:
dotfiles#361,
provisioning#231, github-iac#244, medley#1676 all report
`autoMergeRequest:
null`).

No phase tag advances. Scope is the pre-step (0) paragraph only.

## Test plan

- `markdownlint-cli2 docs/topics/claude-review-lanes/PLAN.md` →
`Summary: 0 error(s)`
- `git diff -U0 | grep -E "^[+-].*\[(DONE|DOING|TODO)\]"` → no matches
(no phase tag touched)
- Every factual claim above regenerated from a command against the live
repos,
  on a **full** clone — the local clone was shallow, which can make
  `git diff <sha>^ <sha>` error in a way that reads as a false positive;
`git fetch --unshallow` was run and every finding re-derived. All held.
- Independently re-verified by a fresh-context verifier (verdict in the
PR thread).

## Related

No linked issue. Corrects text merged in #301; describes work delivered
by #291
and #234. The paired standards-side watchdog re-pin ships as its own PR.

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant