From 255a6272aa3836d7812696b86dd3214e238537b7 Mon Sep 17 00:00:00 2001 From: Andy Stark Date: Fri, 10 Jul 2026 11:27:52 +0100 Subject: [PATCH 01/13] DOC-6830 Add /park and /pickup skills for parked-PR lifecycle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two skills that bracket the existing /reflect -> /finalize pipeline for docs written against an unreleased or still-changing external source. /park freezes a branch as a PR carrying a machine-diffable manifest and defers the durable squash; /pickup thaws it once the source ships, diffing current source state against the park-time snapshot. Format lives in _shared/park-manifest.md, mirroring how _shared/commit-trailers.md is shared between reflect and finalize. Built against a real artifact first: the manifest in PR #3611 (DOC-6829) was hand-authored, then the shared spec was written to match it, so the format and a real instance are in sync. The load-bearing design choice is that the manifest snapshots the source's state (merge status, head SHA, milestone, observed API shape) rather than just pointing at it — that snapshot is what makes /pickup a diff instead of a blind re-read. Learned: park/pickup bracket reflect->finalize; the manifest snapshot is what makes pickup a diff, not a re-read Constraint: never run /finalize while a PR is parked — it squashes the Recheck/Gaps trailers that /pickup depends on Rejected: in-repo .claude/parked/ manifest file | chose a PR-body block so gh can read and list it with no merge-time cleanup Gaps: only the authoring side has been exercised; the /pickup path is unrun until an upstream trigger fires (SDR#3390 is the shakedown) Ticket: DOC-6830 Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/skills/_shared/park-manifest.md | 85 ++++++++++++++++++ .claude/skills/park/SKILL.md | 111 ++++++++++++++++++++++++ .claude/skills/pickup/SKILL.md | 106 ++++++++++++++++++++++ 3 files changed, 302 insertions(+) create mode 100644 .claude/skills/_shared/park-manifest.md create mode 100644 .claude/skills/park/SKILL.md create mode 100644 .claude/skills/pickup/SKILL.md diff --git a/.claude/skills/_shared/park-manifest.md b/.claude/skills/_shared/park-manifest.md new file mode 100644 index 0000000000..0ff3c992b0 --- /dev/null +++ b/.claude/skills/_shared/park-manifest.md @@ -0,0 +1,85 @@ +# Park manifest — format & conventions (shared) + +Canonical definition of the **park manifest**: the pickup contract embedded in a parked PR. +Shared by **`/park`** (which *writes* it) and **`/pickup`** (which *reads and diffs against* +it), so the format can't drift between the two. Change it here, once. + +> Referenced by `.claude/skills/park/SKILL.md` and `.claude/skills/pickup/SKILL.md`. Not a +> skill itself (no `SKILL.md`), so it won't be invoked directly. + +## What a parked PR is + +A PR whose docs are **written but deliberately not mergeable yet** because they track an +external event that hasn't happened — an upstream release, a feature reaching GA, an API +being finalised. The work is done to the extent it *can* be; what remains is **re-checking it +against the source once the source stops moving.** + +The enemy is forgetting: which upstream thing this tracked, what state it was in when we +wrote against it, and what we already knew was likely to change. The manifest fixes all +three in a place a future agent can find with `gh` alone. + +## Where it lives + +In the **PR body**, as a markdown block delimited by exact marker lines so `/pickup` can +extract it deterministically: + +``` + +## Park manifest +... + +``` + +PR-body (not an in-repo file) on purpose: `gh` can read/write it and list parked PRs by +label, with nothing to clean up at merge. The block is plain markdown — human-readable in the +PR, machine-locatable by its markers. + +## Required sections + +1. **Header fields** — flat `Key: value` lines: + - `Ticket:` — `DOC-NNNN`. + - `Parked at:` — date (pass it in; agents can't read the clock). + - `Trigger to pick up:` — the *condition*, concrete enough to test mechanically, e.g. + "upstream PR X merged **and** in a released version". + - `Labels:` — the labels applied (`parked`, `do not merge yet`). + +2. **Pinned sources (state observed at park time)** — a table, one row per external source. + Each row records the URL **and a snapshot of its state at park time**: for a GitHub PR, + `state` / `merged` / `head SHA` / `base` / `milestone` / `updated`. **The snapshot is what + makes pickup a diff rather than a re-read** — without it, `/pickup` sees "now" with nothing + to compare against. Include the exact command to re-fetch each source. + +3. **Observed shape the page assumes** — a **confidence-tagged** summary of the API/behaviour + the docs commit to. Tag it (`LOW`/`MEDIUM`/`HIGH`) — preemptive docs written against an + unmerged diff are usually LOW, and saying so tells pickup how hard to scrutinise. + +4. **Re-check checklist** — GitHub task-list (`- [ ]`) of everything to verify or finish on + pickup. **Seed it from the branch's commit trailers** — harvest `Recheck:` / `Gaps:` / + `Directive:` from `main..HEAD` (see [`commit-trailers.md`](./commit-trailers.md)); those + *are* the loose ends. Add anything predicted-to-change not already captured. Mark the + highest-risk items. + +5. **On pickup, then** — the closing steps: reconcile docs, remove the `bannerText` warning, + drop the labels, run `/reflect` then `/finalize`. + +## Relationship to commit trailers + +No double-bookkeeping. Commit `Recheck:` / `Gaps:` trailers are the *per-change, +location-bound* record (`git log`-reachable). The manifest is the *consolidated, source-pinned* +contract for the PR as a whole. `/park` **harvests** the trailers into the checklist; it does +not restate them as trailers. + +## Finalize is deferred while parked + +Critical: **do not run `/finalize` at park time.** Finalize squashes away the episodic +`Recheck:` / `Gaps:` trailers — exactly what `/pickup` needs. The durable distill happens +*after* pickup, once the source has settled. The manifest's "On pickup, then" section records +this so it isn't forgotten. + +## Labels + +- `parked` — "PR speculatively added based on pre-release info. Check validity when release + goes ahead." +- `do not merge yet` — the merge guard. + +`/pickup` finds parked PRs with `gh pr list --label parked`. diff --git a/.claude/skills/park/SKILL.md b/.claude/skills/park/SKILL.md new file mode 100644 index 0000000000..1a38932e38 --- /dev/null +++ b/.claude/skills/park/SKILL.md @@ -0,0 +1,111 @@ +--- +name: park +description: "Freeze a release-ready-pending-upstream branch as a parked PR — open (or update) the PR with a machine-diffable pickup manifest, apply the parked / do-not-merge labels, and deliberately defer /finalize so the episodic re-check notes survive. Use when docs are written against an unreleased or still-changing external source (an upstream PR, a preview feature) and must wait for it to ship before merging. Pairs with /pickup." +--- + +# Park — freeze a PR that's waiting on the world + +Some docs are done as far as they *can* be, but can't merge yet: they're written against an +upstream PR that hasn't merged, a feature that hasn't GA'd, an API still in review. Left as a +bare open PR, the context rots — six weeks later nobody remembers what it tracked or what was +known to be shaky. This skill **freezes that context into the PR** so it can be picked up +cleanly when the source settles. + +Park is the pipeline's **pause button between `/reflect` and `/finalize`** — it preserves the +episodic layer and adds a forward-looking re-check contract, instead of distilling and +merging. + +The manifest format is defined once in +[`../_shared/park-manifest.md`](../_shared/park-manifest.md) — **read it first**, and +[`../_shared/commit-trailers.md`](../_shared/commit-trailers.md) for the trailer vocabulary +you'll harvest. This file is *how to build and land the manifest*. + +## Where this sits + +``` +/reflect → WIP commit messages [episodic, provisional] + ↓ THIS SKILL — when the work can't merge yet +/park → parked PR + manifest [frozen; /finalize DEFERRED] + ↓ (wait for the upstream trigger) +/pickup → reconcile docs vs source [thaw + delta] + ↓ +/reflect+/finalize → durable squash commit [resume the normal pipeline] +``` + +## The one principle: snapshot the source, not just a pointer + +A note that says "watch upstream PR X" is nearly useless on pickup — it tells you where to +look but not *what changed*. The manifest must capture the source's **state at park time** +(merge status, head SHA, milestone, the observed API shape) so pickup is a **diff**, not a +fresh read. Recording the snapshot is the whole value of parking; everything else is +plumbing. + +## Step 1 — Confirm it should be parked (not merged, not abandoned) + +Park only when **the docs are as complete as the source allows** and the sole blocker is an +external event. If the work is merely unfinished, keep working. If the source may never ship, +don't park — say so. There must be a concrete, testable **trigger condition**. + +## Step 2 — Harvest the loose ends already on the branch + +The branch's own commits hold the re-check items, from `/reflect`: + +``` +git log --reverse main..HEAD --format='%(trailers:only,unfold)' +``` + +Pull the `Recheck:`, `Gaps:`, and `Directive:` trailers — these seed the checklist. Read the +commit bodies too, for the *why*. Don't restate them as trailers on a new commit; they belong +in the manifest checklist now (see [`../_shared/park-manifest.md`](../_shared/park-manifest.md), +"Relationship to commit trailers"). + +## Step 3 — Snapshot each pinned source + +Identify the external sources the docs depend on (URLs in the ticket, commit bodies, the page's +`bannerText`). For each, capture its live state. For a GitHub PR/issue: + +``` +gh api repos///pulls/ \ + --jq '{state, merged, head_sha: .head.sha, base: .base.ref, milestone: .milestone.title, updated_at}' +``` + +Record URL + snapshot in the sources table, and the re-fetch command for pickup. Add a +**confidence-tagged** summary of the API/behaviour the page assumes (LOW for docs written +against an unmerged diff — and say *why* it's low, e.g. "signatures differed between two reads +of the diff"). + +## Step 4 — Compose the manifest & PR body + +Build the PR body: a short human summary, a prominent **do-not-merge warning** linking the +source, then the ``…`` block with all five +required sections (per the shared spec). The page itself should already carry a `bannerText` +"not yet released / subject to change" note — if it doesn't, add one before parking. + +## Step 5 — Land it + +- Push the branch. +- Open the PR (or update an existing one — replace any prior manifest block in place): + ``` + gh pr create --title "DOC-XXXX [PARKED]" --body-file \ + --label parked --label "do not merge yet" --base main + ``` +- Verify the labels stuck (`gh pr view --json labels`). +- **Do not run `/finalize`.** State explicitly that it's deferred until pickup — the manifest + records this, but say it in your handoff too. + +Present the PR URL, the trigger condition, and the checklist, then stop. Opening a PR is +outward-facing — if the branch isn't pushed or the user hasn't asked, confirm first. + +## Limits (read honestly) + +- **A stale snapshot is worse than none** — if you record a head SHA or API shape you didn't + actually verify, pickup diffs against fiction. Snapshot only what you checked; leave the + rest out and flag it in the checklist. +- Park **cannot judge whether the source will ship.** It records a trigger; it doesn't predict + the future. A parked PR that never triggers is dead weight — `/pickup`'s scan mode is how you + find and close those. +- It **defers, never distills.** If you're tempted to "just finalize while it's fresh," don't: + you'll squash away the very notes pickup needs. Freshness is preserved *in the manifest*, not + in a premature durable commit. +- The manifest is only as good as the checklist. If Step 2's harvest is skipped, the loose ends + live only in scattered trailers and the pickup starts blind. diff --git a/.claude/skills/pickup/SKILL.md b/.claude/skills/pickup/SKILL.md new file mode 100644 index 0000000000..5f07ce5050 --- /dev/null +++ b/.claude/skills/pickup/SKILL.md @@ -0,0 +1,106 @@ +--- +name: pickup +description: "Thaw a parked PR — re-fetch each pinned source, diff it against the manifest snapshot taken at park time, report what changed vs what was predicted, then reconcile the docs and resume the normal pipeline (/reflect → /finalize) toward merge. Run with a PR number, or with no argument to scan all parked PRs and report which triggers have fired. Pairs with /park." +--- + +# Pickup — thaw a parked PR against its moved source + +`/park` froze a PR with a snapshot of the source it tracks. Time passed; the source moved — +the upstream PR merged, the API changed, a version shipped. This skill **thaws the PR**: it +diffs the source's *now* against the *then* recorded in the manifest, tells you exactly what +changed, reconciles the docs, and hands back to the normal merge pipeline. + +The manifest format is defined in +[`../_shared/park-manifest.md`](../_shared/park-manifest.md) — **read it first**. Trailer +vocabulary for the reflect/finalize handoff is in +[`../_shared/commit-trailers.md`](../_shared/commit-trailers.md). + +## Where this sits + +``` +/park → parked PR + manifest snapshot [frozen] + ↓ (the trigger fires) +/pickup → diff source now vs snapshot [THIS SKILL: thaw + reconcile] + ↓ +/reflect+/finalize → durable squash commit [resume: predicted-vs-actual, distill] +``` + +## The one principle: diff, then reconcile — don't rewrite from scratch + +The manifest's value is the snapshot. Use it: fetch the source now, **compare field-by-field** +against what park recorded, and let the *delta* drive the edits. Rewriting the page blind +throws away the park-time reasoning and re-introduces whatever was already got right. Report +the delta first; edit second. + +## Mode A — scan (no argument) + +List every parked PR and report, for each, whether its trigger has fired — the standing answer +to "what's waiting, and is any of it ready?" + +``` +gh pr list --label parked --json number,title,url,updatedAt +``` + +For each, read the manifest's `Trigger to pick up:` and test it against the source's current +state (Step 1 below, snapshot only). Output a table: PR / trigger / **fired?** / one-line +delta. Recommend which to pick up, which are still waiting, and which look abandoned (source +closed-unmerged, or long dead). Then stop — picking one up is Mode B. + +## Mode B — pick up one PR (PR number given) + +### Step 1 — Extract the manifest & re-fetch sources + +Read the PR body, slice out the ``…`` block. For +each pinned source, run its recorded re-fetch command and capture current state. + +### Step 2 — Diff against the snapshot + +Compare current vs park-time, field by field. Has `merged` flipped true? Has `head SHA` moved +(the diff may have changed under review)? Is a `milestone` / version now set? For a merged PR, +read the **released** source at the merged tag, not the PR branch. Confirm whether the trigger +condition is genuinely met — **if it isn't, stop and report**; picking up early re-freezes bad +info. + +### Step 3 — Walk the checklist + +Take each `- [ ]` item in the manifest and resolve it against the current source: **confirmed +as-written**, **changed → here's the correction**, or **still open**. This is the core output — +a per-item verdict, highest-risk items first. Verify against the actual released source, never +the park-time assumption (which was often LOW-confidence by design). + +### Step 4 — Reconcile the docs + +Apply the corrections: fix signatures/config, fill version placeholders, add sections for +pieces that landed after park (e.g. a deferred API), convert provisional inline examples to +tested examples where the tooling now allows. Then clear the staleness: + +- Remove the page's `bannerText` "not yet released" warning. +- Rebase / merge `main`; re-run link checks; confirm `relref` targets still resolve. + +### Step 5 — Resume the pipeline & hand off + +Pickup **reconciles; it does not merge.** + +- Run **`/reflect`** on the reconciliation commit — record *predicted-vs-actual*: what the park + snapshot got right, what the source changed. That's a high-value note (it closes the loop and + calibrates future preemptive docs). +- Run **`/finalize`** — the durable squash deferred at park time, now that the source has + settled. It reconciles the whole arc (park notes + pickup findings + any review). +- Remove the `parked` and `do not merge yet` labels; strip the manifest block from the PR body + (or mark it resolved). + +Present: the delta report, the doc changes, the finalize output + `gh pr merge … --body-file` +command, then stop. The human triggers the merge. + +## Limits (read honestly) + +- **A met trigger is not a settled source.** An upstream PR can merge and *still* churn via + follow-ups before release. If `milestone`/version is unset, treat the API as not-yet-final and + lean toward staying parked. Report the ambiguity; don't force it. +- It **trusts the snapshot's honesty.** If `/park` recorded an unverified shape, the diff is + against fiction — so re-verify high-risk checklist items from source regardless of what the + snapshot claimed, especially anything tagged LOW. +- It **can't recover a checklist that was never written.** With a thin manifest, pickup + degrades to a blind re-review — still useful, but the park-time context is gone. +- Reconciliation edits are real doc changes: drive/verify them like any other, don't assume the + released API matches even a HIGH-confidence snapshot. From d5b6c86fda02043d9dfe5c8247d43987242a1042 Mon Sep 17 00:00:00 2001 From: Andy Stark Date: Fri, 10 Jul 2026 11:44:49 +0100 Subject: [PATCH 02/13] DOC-6830 Fix cross-file consistency issues in park/pickup skills MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address three Cursor Bugbot findings on the skill spec: reorder the manifest's "On pickup, then" so labels drop only after /reflect and /finalize (matching pickup Step 5), correct scan mode to apply Step 2's trigger-met check rather than citing Step 1, and document the `gh pr edit` re-park path alongside `gh pr create`. The fourth finding (a High claiming the REST `merged` field does not exist) was a false positive — the single-PR endpoint returns it — and is left unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/skills/_shared/park-manifest.md | 6 ++++-- .claude/skills/park/SKILL.md | 7 ++++++- .claude/skills/pickup/SKILL.md | 6 +++--- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/.claude/skills/_shared/park-manifest.md b/.claude/skills/_shared/park-manifest.md index 0ff3c992b0..bcac99b9fe 100644 --- a/.claude/skills/_shared/park-manifest.md +++ b/.claude/skills/_shared/park-manifest.md @@ -59,8 +59,10 @@ PR, machine-locatable by its markers. *are* the loose ends. Add anything predicted-to-change not already captured. Mark the highest-risk items. -5. **On pickup, then** — the closing steps: reconcile docs, remove the `bannerText` warning, - drop the labels, run `/reflect` then `/finalize`. +5. **On pickup, then** — the closing steps, in order: reconcile docs, remove the `bannerText` + warning, run `/reflect` then `/finalize`, and **only then** drop the labels. The + `do not merge yet` guard must hold until the durable commit is done — never lift it before + `/finalize` completes (this ordering is authoritative; it matches `/pickup` Step 5). ## Relationship to commit trailers diff --git a/.claude/skills/park/SKILL.md b/.claude/skills/park/SKILL.md index 1a38932e38..9193ad487c 100644 --- a/.claude/skills/park/SKILL.md +++ b/.claude/skills/park/SKILL.md @@ -84,11 +84,16 @@ required sections (per the shared spec). The page itself should already carry a ## Step 5 — Land it - Push the branch. -- Open the PR (or update an existing one — replace any prior manifest block in place): +- Open the PR, or update an existing one in place. For a new PR: ``` gh pr create --title "DOC-XXXX [PARKED]" --body-file \ --label parked --label "do not merge yet" --base main ``` + When the branch already has a PR (re-parking — `gh pr create` would fail), replace the body + and ensure the labels instead: + ``` + gh pr edit --body-file --add-label parked --add-label "do not merge yet" + ``` - Verify the labels stuck (`gh pr view --json labels`). - **Do not run `/finalize`.** State explicitly that it's deferred until pickup — the manifest records this, but say it in your handoff too. diff --git a/.claude/skills/pickup/SKILL.md b/.claude/skills/pickup/SKILL.md index 5f07ce5050..f95ccd100a 100644 --- a/.claude/skills/pickup/SKILL.md +++ b/.claude/skills/pickup/SKILL.md @@ -41,9 +41,9 @@ to "what's waiting, and is any of it ready?" gh pr list --label parked --json number,title,url,updatedAt ``` -For each, read the manifest's `Trigger to pick up:` and test it against the source's current -state (Step 1 below, snapshot only). Output a table: PR / trigger / **fired?** / one-line -delta. Recommend which to pick up, which are still waiting, and which look abandoned (source +For each, read the manifest's `Trigger to pick up:`, snapshot the current source state (Step 1's +re-fetch only — don't do the full reconcile), then apply Step 2's trigger-met check to decide +**fired?**. Output a table: PR / trigger / **fired?** / one-line delta. Recommend which to pick up, which are still waiting, and which look abandoned (source closed-unmerged, or long dead). Then stop — picking one up is Mode B. ## Mode B — pick up one PR (PR number given) From 7044aefc117204f9bf2f88fc2bcf117b4e70130b Mon Sep 17 00:00:00 2001 From: Andy Stark Date: Fri, 10 Jul 2026 12:37:30 +0100 Subject: [PATCH 03/13] DOC-6830 Consolidate pickup closing sequence to require a commit before reflect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address Cursor Bugbot High 3558314964: pickup Step 4 reconciled and rebased but never committed, while Step 5 ran /reflect and /finalize, which see committed history only — so an uncommitted reconcile would be silently dropped from the squash merge. Rather than bolt on a lone commit step (this is the second finding on the pickup closing sequence — subsystem churn), specify the whole sequence once as an explicit ordered list in Step 5 (commit -> /reflect -> /finalize -> drop labels) and make the manifest summary defer to it as authoritative. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/skills/_shared/park-manifest.md | 7 ++++--- .claude/skills/pickup/SKILL.md | 22 +++++++++++++--------- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/.claude/skills/_shared/park-manifest.md b/.claude/skills/_shared/park-manifest.md index bcac99b9fe..e354439ca6 100644 --- a/.claude/skills/_shared/park-manifest.md +++ b/.claude/skills/_shared/park-manifest.md @@ -60,9 +60,10 @@ PR, machine-locatable by its markers. highest-risk items. 5. **On pickup, then** — the closing steps, in order: reconcile docs, remove the `bannerText` - warning, run `/reflect` then `/finalize`, and **only then** drop the labels. The - `do not merge yet` guard must hold until the durable commit is done — never lift it before - `/finalize` completes (this ordering is authoritative; it matches `/pickup` Step 5). + warning, **commit the reconciliation**, run `/reflect` then `/finalize`, and **only then** + drop the labels. The `do not merge yet` guard must hold until the durable commit is done — + never lift it before `/finalize` completes. This is a summary; **`/pickup` Steps 4–5 are the + authoritative sequence** (keep this list in step with them). ## Relationship to commit trailers diff --git a/.claude/skills/pickup/SKILL.md b/.claude/skills/pickup/SKILL.md index f95ccd100a..a987baf8a1 100644 --- a/.claude/skills/pickup/SKILL.md +++ b/.claude/skills/pickup/SKILL.md @@ -79,15 +79,19 @@ tested examples where the tooling now allows. Then clear the staleness: ### Step 5 — Resume the pipeline & hand off -Pickup **reconciles; it does not merge.** - -- Run **`/reflect`** on the reconciliation commit — record *predicted-vs-actual*: what the park - snapshot got right, what the source changed. That's a high-value note (it closes the loop and - calibrates future preemptive docs). -- Run **`/finalize`** — the durable squash deferred at park time, now that the source has - settled. It reconciles the whole arc (park notes + pickup findings + any review). -- Remove the `parked` and `do not merge yet` labels; strip the manifest block from the PR body - (or mark it resolved). +Pickup **reconciles; it does not merge.** Run the closing sequence in **exactly this order** — +each step depends on the previous one, and `/reflect` / `/finalize` act on **committed history +only**, so anything left uncommitted is silently excluded from the squash: + +1. **Commit the reconciliation.** The Step 4 edits must be in branch history before anything + downstream runs — an uncommitted working tree never reaches the squash merge. +2. **`/reflect`** on that commit — record *predicted-vs-actual*: what the park snapshot got + right, what the source changed. High-value (it closes the loop and calibrates future + preemptive docs). `/reflect` may fold its note into the commit from step 1 or amend it. +3. **`/finalize`** — the durable squash deferred at park time, now that the source has settled. + It reconciles the whole arc (park notes + pickup findings + any review). +4. **Only now, remove** the `parked` and `do not merge yet` labels and strip the manifest block + from the PR body (or mark it resolved). The merge guard holds until `/finalize` is done. Present: the delta report, the doc changes, the finalize output + `gh pr merge … --body-file` command, then stop. The human triggers the merge. From d7152902dc728cc8a87e2fd97db081f0670ffca0 Mon Sep 17 00:00:00 2001 From: Andy Stark Date: Fri, 10 Jul 2026 13:09:37 +0100 Subject: [PATCH 04/13] DOC-6830 Rewrite pickup closing sequence around a remote-head invariant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Widen the fix for the recurring pickup closing-sequence churn instead of adding another step. Step 5 now leads with the invariant it enforces — before the `gh pr merge` hand-off the pushed remote PR head must contain everything that must land (reconciliation commit, Step 4's rebased/merged-main history, and any `/reflect` amend), because the squash sees only what is pushed — then orders the steps commit -> /reflect -> push -> /finalize -> drop labels, with a remote-matches-local check before hand-off. This addresses Bugbot High 3558614316 (missing push before merge), the third finding on this sequence. Also realign the manifest's "On pickup, then" summary to that exact order (commit -> /reflect -> push -> /finalize), per Codex review: the summary is copied verbatim into every parked PR body, and its previous "commit and push ... then /reflect" wording would push before the /reflect amend and re-open the gap. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/skills/_shared/park-manifest.md | 12 +++++---- .claude/skills/pickup/SKILL.md | 36 ++++++++++++++++--------- 2 files changed, 31 insertions(+), 17 deletions(-) diff --git a/.claude/skills/_shared/park-manifest.md b/.claude/skills/_shared/park-manifest.md index e354439ca6..8a40ed6f1c 100644 --- a/.claude/skills/_shared/park-manifest.md +++ b/.claude/skills/_shared/park-manifest.md @@ -59,11 +59,13 @@ PR, machine-locatable by its markers. *are* the loose ends. Add anything predicted-to-change not already captured. Mark the highest-risk items. -5. **On pickup, then** — the closing steps, in order: reconcile docs, remove the `bannerText` - warning, **commit the reconciliation**, run `/reflect` then `/finalize`, and **only then** - drop the labels. The `do not merge yet` guard must hold until the durable commit is done — - never lift it before `/finalize` completes. This is a summary; **`/pickup` Steps 4–5 are the - authoritative sequence** (keep this list in step with them). +5. **On pickup, then** — the closing steps, in the exact order `/pickup` Step 5 defines: + reconcile docs, remove the `bannerText` warning, **commit** the reconciliation, run + **`/reflect`**, **push** (so the remote head carries the final history, including any + `/reflect` amend — the squash merges only what has been pushed), run **`/finalize`**, and + **only then** drop the labels. The `do not merge yet` guard must hold until `/finalize` + completes. This is a summary; **`/pickup` Steps 4–5 are the authoritative sequence** — keep + it in that order (commit → `/reflect` → push → `/finalize` → drop labels). ## Relationship to commit trailers diff --git a/.claude/skills/pickup/SKILL.md b/.claude/skills/pickup/SKILL.md index a987baf8a1..afb05168c0 100644 --- a/.claude/skills/pickup/SKILL.md +++ b/.claude/skills/pickup/SKILL.md @@ -79,22 +79,34 @@ tested examples where the tooling now allows. Then clear the staleness: ### Step 5 — Resume the pipeline & hand off -Pickup **reconciles; it does not merge.** Run the closing sequence in **exactly this order** — -each step depends on the previous one, and `/reflect` / `/finalize` act on **committed history -only**, so anything left uncommitted is silently excluded from the squash: +Pickup **reconciles; it does not merge.** `/finalize` prepares a squash that operates on the +**pushed remote PR head**, so the invariant governing this whole step is: -1. **Commit the reconciliation.** The Step 4 edits must be in branch history before anything - downstream runs — an uncommitted working tree never reaches the squash merge. +> Before the `gh pr merge` hand-off, the remote PR branch must already contain **everything +> that must land** — the reconciliation commit, Step 4's rebased/merged-`main` history, and any +> later amend. The squash sees only what has been pushed; anything left local is silently +> dropped from the merge. + +Everything below just drives the branch to that end state, re-pushing whenever local history +changes: + +1. **Commit the reconciliation.** `/reflect` and `/finalize` act on committed history only — an + uncommitted working tree never reaches the merge. 2. **`/reflect`** on that commit — record *predicted-vs-actual*: what the park snapshot got - right, what the source changed. High-value (it closes the loop and calibrates future - preemptive docs). `/reflect` may fold its note into the commit from step 1 or amend it. -3. **`/finalize`** — the durable squash deferred at park time, now that the source has settled. - It reconciles the whole arc (park notes + pickup findings + any review). -4. **Only now, remove** the `parked` and `do not merge yet` labels and strip the manifest block + right, what the source changed (it closes the loop and calibrates future preemptive docs). + `/reflect` may fold its note into the commit or amend it. +3. **Push the branch** — and re-push after any amend in step 2. This is the step that makes the + remote head match local; skip it and the squash merges *without* the reconciliation (and + without Step 4's rebase), which is the gap that keeps reappearing here. +4. **`/finalize`** — the durable squash deferred at park time, now that the source has settled. + It reconciles the whole arc (park notes + pickup findings + any review) and prepares the + `gh pr merge … --body-file` command. +5. **Only now, remove** the `parked` and `do not merge yet` labels and strip the manifest block from the PR body (or mark it resolved). The merge guard holds until `/finalize` is done. -Present: the delta report, the doc changes, the finalize output + `gh pr merge … --body-file` -command, then stop. The human triggers the merge. +Before handing off, **confirm the remote head matches local** (working tree clean, branch not +ahead of `origin`). Then present: the delta report, the doc changes, the finalize output + the +`gh pr merge … --body-file` command, and stop. The human triggers the merge. ## Limits (read honestly) From 42afebcd99d6ccf28471c84052e21c91a71528ea Mon Sep 17 00:00:00 2001 From: Andy Stark Date: Fri, 10 Jul 2026 13:24:55 +0100 Subject: [PATCH 05/13] DOC-6830 Update assess-comments coverage ledger from #3612 review cycle Record the park/pickup review rounds on #3612: a new bugbot FP flavour (nonexistent 'merged' field, refuted empirically), an internal doc-vs-doc contradiction, and the 3-round pickup closing-sequence churn with the 'consolidation scoped too narrowly' recurrence; calibration 5 valid / 1 FP. Also correct two pre-existing off-by-one encounter counts (GraphQL, fix-quality) caught by Codex review. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/state/assess-comments.coverage.md | 150 ++++++++++++++++++++-- 1 file changed, 137 insertions(+), 13 deletions(-) diff --git a/.claude/state/assess-comments.coverage.md b/.claude/state/assess-comments.coverage.md index d4c3ffc3b1..78e2ad54df 100644 --- a/.claude/state/assess-comments.coverage.md +++ b/.claude/state/assess-comments.coverage.md @@ -27,23 +27,23 @@ whether to commit the change. | Capability | Confidence | Real encounters | Last verified | Evidence | |---|---|---|---|---| -| Branch/PR identification + arg handling | 🟢 corroborated | 8 | 2026-07-02 | #3415, #3507, #3374, #3536, #3542, #3543, #3573, #2531 | -| Multi-source collection (inline + top-level + reviews) | 🟢 corroborated | 9 | 2026-07-02 | #3415, #3507, #3510, #3374, #3536, #3542, #3543, #3573, #2531 | -| GraphQL thread-resolution pull (`isResolved`/`isOutdated`) | 🟢 corroborated | 7 | 2026-07-02 | #3510 (12/12 resolved), #3374 (15/15), #3536 (2/2 open), #3542 (3 resolved/1 open), #3543 (1/1 open), #3573 (2/2 open), #2531 (r1: 11 open/2 res-outdated/1 open; r2: 11 of 14 outdated after fixes + APPROVED) | -| Source-role tagging (bugbot/security/history/summary/ci/human) | 🟢 corroborated | 8 | 2026-07-02 | #3415, #3507, #3374, #3536, #3542, #3543, #3573, #2531 | -| Open/resolved split | 🟢 corroborated | 7 | 2026-07-02 | #3510, #3374, #3536 (0 resolved/2 open), #3542 (3 resolved/1 open), #3543, #3573 (r1/r2 mixed), #2531 (11 open / 3 resolved) | -| Fix-quality spot-check (genuinely fixed vs silenced) | 🟢 corroborated | 5 | 2026-07-02 | #3510 (term removals landed), #3374 (`num_docs`, dropIndex landed), #3542 (xargs+guard, narrowed exclude, SHA pin landed), #3573 (relpath + `--add` dedup landed), #2531 (r1 found decimal thread 2619563419 reverted; r2 doc fixes landed + engineer ZdravkoDonev **APPROVED**) | +| Branch/PR identification + arg handling | 🟢 corroborated | 11 | 2026-07-10 | #3415, #3507, #3374, #3536, #3542, #3543, #3573, #2531, #3585, #3604, #3612 | +| Multi-source collection (inline + top-level + reviews) | 🟢 corroborated | 12 | 2026-07-10 | #3415, #3507, #3510, #3374, #3536, #3542, #3543, #3573, #2531, #3585, #3604, #3612 | +| GraphQL thread-resolution pull (`isResolved`/`isOutdated`) | 🟢 corroborated | 10 | 2026-07-10 | #3510 (12/12 resolved), #3374 (15/15), #3536 (2/2 open), #3542 (3 resolved/1 open), #3543 (1/1 open), #3573 (2/2 open), #2531 (r1: 11 open/2 res-outdated/1 open; r2: 11 of 14 outdated after fixes + APPROVED), #3585 (r1 2 open; r2 3 open + 2 resolved — one outdated, one resolved-not-outdated); #3604 (r1 2/2 open; r2 4/4 open; r3 5 threads = 2 resolved [one outdated, one resolved-not-outdated] / 3 open, hasNextPage:false); #3612 (r1 4/4 open, hasNextPage:false) | +| Source-role tagging (bugbot/security/history/summary/ci/human) | 🟢 corroborated | 11 | 2026-07-10 | #3415, #3507, #3374, #3536, #3542, #3543, #3573, #2531, #3585, #3604, #3612 (bugbot inline + github-actions Jira-link ci; no human/summary this PR) | +| Open/resolved split | 🟢 corroborated | 10 | 2026-07-10 | #3510, #3374, #3536 (0 resolved/2 open), #3542 (3 resolved/1 open), #3543, #3573 (r1/r2 mixed), #2531 (11 open / 3 resolved), #3585 (r1 2 open/0 res; r2 3 open/2 resolved), #3604 (r1 0 res/2 open; r2 0 res/4 open; r3 2 res/3 open), #3612 (0 res/4 open) | +| Fix-quality spot-check (genuinely fixed vs silenced) | 🟢 corroborated | 8 | 2026-07-10 | #3510 (term removals landed), #3374 (`num_docs`, dropIndex landed), #3542 (xargs+guard, narrowed exclude, SHA pin landed), #3573 (relpath + `--add` dedup landed), #2531 (r1 found decimal thread 2619563419 reverted; r2 doc fixes landed + engineer ZdravkoDonev **APPROVED**), #3585 r2 (both r1 findings genuinely fixed — byId multimap + isError wrapping present in code, not silenced); #3604 r3 (reactive race 3544441638 resolved+outdated by 6735cf9cc — sequential `.then()` chain present + harness PASS; smismember 3545172677 resolved-not-outdated, genuinely fixed in client sources not the anchored sets.md line); #3612 (r1's 3 doc-consistency fixes + r2 commit-gap all resolved; r2 3558314964 resolved+**outdated** by 7044aefc1 — consolidation genuinely present in Step 5, not silenced) | | "Resolved ≠ fixed" flag — **legitimate deferral** variant | 🟡 seen once | 1 | 2026-06-23 | #3510 (TS.BGET:122 left pending eng) | | "Resolved ≠ fixed" flag — **still-broken / reverted** variant | 🟡 seen once | 1 | 2026-06-30 | #2531 (resolved+outdated thread 2619563419 said decimal default=`string`; a later rewrite reverted current code to `precise`, so the resolved fix is no longer in the code — engineer re-raised it as 3496835587). Regression flavour; see worked examples | | Cross-tool **agreement** | 🟡 seen once | 1 | 2026-06-23 | #3374 (Claude + bugbot independently on `num_docs`) | -| **Contradiction** detection | 🟢 corroborated | 2 | 2026-07-02 | #3415 (approval vs open bugbot finding); #2531 (RDI engineer's repo ground truth contradicts the page's Debezium-docs claims on ≥4 points — version, decimal default, temporal pass-through, MariaDB connector — **and** engineer-vs-existing-doc on temporal normalization). *(#3507 was an off-branch manual demo — not counted.)* | -| **Ping-pong loop** detection | ❓ untested | 0 | 2026-07-02 | still no true tool A↔B loop across #3536 (4 rounds), #3542 (r2 "empty-scope"), #3573 (r1 fixed point; r2 independent), or #2531. #3542/#3573 were churn not loops; #2531's nearest reopened-concern was the decimal regression (resolved Dec → reverted by a June rewrite → re-raised) — a regression across one rewrite, not a cycle | -| **Subsystem churn** detection (repeated findings on one patched area) | 🟢 corroborated | 3 PRs | 2026-07-02 | 3 distinct PRs. #3536 — 3 instances (review-handling / churn-feature / cap↔report contract). #3542 — 2 instances on the extraction *fail-loud-on-empty* contract (r1 ARG_MAX silent-green → r2 sibling zero-files `exit 0`). #3573 — 2 instances on the `--add` virtual-merge mechanism (r1 dup-vs-disk → r2 dropped-under-collapse). Worked examples below | -| Approval-over-open-finding cross-check | 🟢 corroborated | 6 | 2026-07-02 | #3415 (dwdougherty), #3374 (low-confidence over open HIGH), #3536 (high-confidence over 2 open Mediums: benign), #3542 (paoloredis "yep go ahead" 7 min after open Medium #3498159511; unacknowledged), #3573 (dwdougherty "Sure, why not?" APPROVED 13:41 over open findings; 2 bot findings landed 13:49 after), #2531 (run1 correct **negative** — no approval; run2 **positive** — ZdravkoDonev APPROVED 13:13 then bugbot finding 3499796857 landed 15:08, and he approved over 2-3 of his own still-open findings incl. the temporal one) | +| **Contradiction** detection | 🟢 corroborated | 3 | 2026-07-10 | #3415 (approval vs open bugbot finding); #2531 (RDI engineer's repo ground truth contradicts the page's Debezium-docs claims on ≥4 points — version, decimal default, temporal pass-through, MariaDB connector — **and** engineer-vs-existing-doc on temporal normalization); #3612 (**new flavour: internal doc-vs-doc within one PR's own files** — bugbot 3558223727: `park-manifest.md` "On pickup, then" drops labels *before* `/reflect`+`/finalize` while `pickup` Step 5 drops them *after*; `/park` copies the manifest into every PR body so the guard could lift early). *(#3507 was an off-branch manual demo — not counted.)* | +| **Ping-pong loop** detection | ❓ untested | 0 | 2026-07-09 | still no true tool A↔B loop across #3536 (4 rounds), #3542 (r2 "empty-scope"), #3573 (r1 fixed point; r2 independent), #2531, #3585, or #3604 (r2: SMISMEMBER finding fixed by 9bf49b2f4 with no re-flag; node-dep FP independent; r3: new `Double.MIN_VALUE` finding in `SortedSetExample.java`, a file *untouched* by the 6735cf9cc push that triggered the re-scan — new-independent-in-different-file, textbook normal iteration). #3542/#3573/#3585 were churn not loops; #3585 r2 was the closest yet (the r1 fix *caused* r2's findings) but still not a loop — new independent findings + no fix undone, no A↔B cycle. #2531's nearest was the decimal regression (resolved → reverted by a rewrite → re-raised), a regression not a cycle | +| **Subsystem churn** detection (repeated findings on one patched area) | 🟢 corroborated | 5 PRs | 2026-07-10 | 5 distinct PRs. #3536 — 3 instances (review-handling / churn-feature / cap↔report contract). #3542 — 2 instances on the extraction *fail-loud-on-empty* contract. #3573 — 2 instances on the `--add` virtual-merge mechanism. #3585 — get_page/search **resolution** subsystem: r1 fix (dup-id → byId multimap + matchByUrlSuffix + ambiguous-url early-return) directly caused 2 of r2's 3 findings (suffix path-boundary High, url-blocks-id-fallback) plus adjacent version-default gap. Cleanest "fix-caused-its-own-next-round" churn yet. #3612 — pickup **closing sequence** (Step 4→5), **3 rounds**: r1 label-drop ordering (3558223727) + r2 commit-before-reflect High (3558314964) + r3 push-before-merge High (3558614316). The r2 "consolidation" (explicit ordered commit→reflect→finalize→drop-labels) was **scoped too narrowly** — it threaded *commit* but not the full local→remote→squash dependency, so r3 landed on that very commit. 2nd PR to show the #3536-r6 "consolidation boundary too narrow" meta-pattern. Worked examples below | +| Approval-over-open-finding cross-check | 🟢 corroborated | 7 | 2026-07-08 | #3415 (dwdougherty), #3374 (low-confidence over open HIGH), #3536 (high-confidence over 2 open Mediums: benign), #3542 (paoloredis "yep go ahead" 7 min after open Medium #3498159511; unacknowledged), #3573 (dwdougherty "Sure, why not?" APPROVED 13:41 over open findings; 2 bot findings landed 13:49 after), #2531 (run1 correct **negative** — no approval; run2 **positive** — ZdravkoDonev APPROVED 13:13 then bugbot finding 3499796857 landed 15:08, and he approved over 2-3 of his own still-open findings incl. the temporal one), #3585 (correct **negative** both rounds — open bugbot findings, only bot COMMENTED verdicts, no human approval); #3604 (correct **negative** all 3 rounds — r3 3 open bugbot findings, only cursor COMMENTED verdicts, no human approval); #3612 (correct **negative** — 4 open bugbot findings, only cursor COMMENTED verdict, no human approval) | | Depth cap / prioritisation under load | 🟢 corroborated | 2 | 2026-07-02 | #3374 (19 candidate findings → 4 deep-verified); #2531 (r1: 14 threads → 5 deep-verified, 6 deferred). *(#3542/#3573 were under cap — not load tests)* | -| Mandatory deep-verify of resolved+not-outdated HIGH | 🟡 seen once | 1 | 2026-07-02 | #3542 #3467309496 (High "Grep failure skips link check", resolved + isOutdated:false) — deep-verified against current code: xargs+guard genuinely present, so legitimately fixed (not still-broken). First real firing of the rule | -| Bot calibration (fixed-vs-dismissed ratio) | 🟢 corroborated | 6 | 2026-07-02 | #3374 (bugbot mostly accepted); #3536 (5/5 valid); #3542 (3/3 valid — 2 fixed, 1 open); #3543 (1/1 valid; Jit 0); #3573 (4/4 valid; Jit 0); #2531 (r1 bugbot 0 findings; r2 bugbot 1/1 valid — caught the ledger duplicate-rows defect 3499796857; Jit 0) | -| Codex second-opinion availability gate | 🟢 corroborated | 6 | 2026-07-02 | #3415, #3374 (CLI on PATH; #3374 had a real Codex review), #3542, #3543, #3573, #2531 (codex on PATH) | +| Mandatory deep-verify of resolved+not-outdated HIGH | 🟢 corroborated | 2 | 2026-07-02 | #3542 #3467309496 (High "Grep failure skips link check", resolved + isOutdated:false) — deep-verified: xargs+guard genuinely present, legitimately fixed. #3585 #3513924736 (High "Duplicate page IDs break get_page", resolved + isOutdated:false) — deep-verified against current code: byId multimap + url-first resolution present, genuinely fixed. 2nd distinct PR → corroborated | +| Bot calibration (fixed-vs-dismissed ratio) | 🟢 corroborated | 8 | 2026-07-08 | #3374 (bugbot mostly accepted); #3536 (5/5 valid); #3542 (3/3 valid — 2 fixed, 1 open); #3543 (1/1 valid; Jit 0); #3573 (4/4 valid; Jit 0); #2531 (r1 bugbot 0 findings; r2 bugbot 1/1 valid — caught the ledger duplicate-rows defect 3499796857; Jit 0); #3585 (bugbot **5/5 valid** across 2 rounds); #3604 (across 3 rounds **3 valid / 2 FP**: valid = reactive `Mono.when` race 3544441638 + SMISMEMBER client-tab parity 3545172677 [fixed by 9bf49b2f4] + `Double.MIN_VALUE`≠−∞ range bug 3550287458 [r3, a language-semantics catch]; FP = async `allOf` 3544581411 + node harness `@redis/time-series` "missing dep" 3545172685. **Calibration axis**: bugbot is strong on API/language semantics [`Double.MIN_VALUE` is smallest-positive] but both FPs mis-model runtime dependency/ordering it can't see from the diff [async fire-on-call; npm transitive hoisting]. Also found 2 of 3 identical `Double.MIN_VALUE` instances — correctly skipped the jedis one, pre-existing on main / outside the PR diff); #3612 (**5 valid / 1 FP across 3 rounds** — r1 valid = internal doc-vs-doc label-order contradiction 3558223727 + scan-mode wrong-step-ref 3558223733 + park update-path-only-shows-`gh pr create` 3558223738; r2 valid = commit-before-reflect High 3558314964; r3 valid = push-before-merge High 3558614316 [both pickup closing-sequence churn]; FP = r1 High "`merged` field doesn't exist" 3558223722, refuted empirically — single-PR REST endpoint returns top-level `merged`. **Calibration axis holds**: all 5 valid catches are cross-file doc-consistency / procedure-completeness [bugbot's strength], and it effectively ran the churn detection one seam per round; the 1 FP is again an **unseen API mechanic** [list-endpoint vs single-PR-endpoint response shape] — same root as #3604's async/npm FPs) | +| Codex second-opinion availability gate | 🟢 corroborated | 9 | 2026-07-10 | #3415, #3374 (CLI on PATH; #3374 had a real Codex review), #3542, #3543, #3573, #2531, #3585, #3604, #3612 (codex on PATH at /Users/andrew.stark/bin/codex) | | Ledger self-integrity after `main` merge (no duplicate rows) | 🟡 seen twice | 2 | 2026-07-02 | #2531 r2 — bugbot 3499796857 caught the shared ledger gaining duplicate rows when `main` (carrying a #3573-era ledger) merged in and git kept both blocks. **2026-07-02**: merging `main` again produced a real conflict as #3542/#3573 edited the same rows — union-merged per capability. Recurring shared-file hazard; see worked examples + step-11 refinement | ## Worked examples library @@ -123,6 +123,19 @@ A↔B cycle) and a never-fixed thread (it *was* fixed, then un-fixed). *the author's own verified claims and another shipped doc* — the reconciler's job is to surface that the Debezium-docs basis was the weaker source all along. +**Internal doc-vs-doc within one PR's own files — #3612, 2026-07-10.** A new contradiction +flavour: not human-vs-doc or tool-vs-intent, but two files *in the same PR* that must stay +consistent by construction. Bugbot Medium `3558223727` caught `_shared/park-manifest.md`'s +"On pickup, then" listing *"drop the labels, run `/reflect` then `/finalize`"* (guard removed +**before** the durable commit) while `pickup/SKILL.md` Step 5 removes labels **after** reflect ++ finalize. The bite: `/park` copies the manifest section verbatim into every parked PR body, +so an agent following the PR-body copy could lift the `do not merge yet` guard before +reconciliation finishes. Signature: a shared/duplicated spec fragment whose ordering disagrees +with the authoritative procedure that consumes it. Extra irony — these very skills exist to stop +`/park` and `/pickup` drifting, and the seam bugbot found is a drift between them. Lesson for the +detector: when one PR ships a spec split across N files, diff the *duplicated* fragments against +their canonical procedure; contradictions hide in the copies. + **Deep-verify came back CLEAN — #3542, 2026-07-01.** First real firing of the "mandatory deep-verify of resolved+not-outdated High" rule: thread #3467309496 was `isResolved:true, isOutdated:false` (a green checkmark on an unchanged-looking @@ -188,7 +201,118 @@ rule fires and distinguishes fixed from still-broken. (only 2 instances, niche feature), but the cross-round, same-mechanism shape is the early churn signal. +- **#3585 get_page/search resolution, 2026-07-02 (fix-caused-its-own-next-round — the purest signature).** + Round 1 bugbot raised a High (duplicate page ids break get_page, `3513924736`) and a + Medium (missing page returns success, `3513924746`). Both were fixed in `ea1b97254`: + `byId` became a multimap, a new `matchByUrlSuffix` returned all suffix matches, an + ambiguous `url` returned an error early, and a `toolResult` wrapper set `isError`. + Round-2 re-scan then raised THREE new findings, **two of them defects created by that very + fix**: `matchByUrlSuffix` uses `endsWith` with no path boundary so `url:"get"` matches 19 + pages incl. `arget`/`config-get` (High `3514124777`, confirmed empirically); the ambiguous-url + early-return skips the `id` fallback (Medium `3514124798`); plus the adjacent pre-existing + version-default gap (Medium `3514124788`). Not ping-pong (no fix undone, no A↔B cycle) — + it's churn: the get_page/search *resolution* logic is under-specified, and each patch exposes + the next adjacent gap. Distinct from #3536/#3542/#3573 in that here the *fix itself* introduced + the next round's findings, making the "consolidate, don't patch" call unambiguous. Recommended + consolidation: one resolver with explicit handle precedence (exact url → unique id → + boundary-anchored suffix url, collecting candidates across handles), path-segment-anchored + suffix matching, and version semantics decided once. + ### Cross-tool agreement - **#3374 `num_docs`** — Claude (Critical #2, top-level review) and bugbot ("Wrong FT.INFO document count field", inline, 06-17) independently flagged the same `info.numDocs` → `num_docs` bug. Verified fixed in current code. + +### Bot false positives (calibration signatures) +- **#3604 async fire-on-call mis-modeled, 2026-07-08 (first adjudicated bugbot FP).** + Bugbot raised two Medium concurrency findings on new Lettuce doc examples, same + heuristic ("concurrent Redis ops on a shared key"): (a) `3544441638` reactive + `StringExample` — `Mono.when(setAndGet, setnx, setxx, mset, incrby).block()` + subscribes cold publishers ~concurrently → **REAL** interleaving race on `bike:1` + (corroborated: the sibling reactive `SortedSetExample` blocks per-step, and the + async `StringExample` uses per-step `join()` — the `Mono.when` file is the outlier); + (b) `3544581411` async `HashExample` — `CompletableFuture.allOf(delResult, hIncrBy, + incrByGetMget)` claimed to race `del bike:1:stats` against the stats increments → + **FALSE POSITIVE**: `delResult = asyncCommands.del(...)` fires *on call* (line 26), + and a single Lettuce connection executes commands in write order, so the delete + completes before `incrByGetMget`'s increments are dispatched (line ~120); `allOf` + only awaits. Corroborated by upstream `redis/lettuce` shipping the identical async + structure under CI. Lesson for the detector: bugbot's concurrency heuristic is sound + for reactive **cold-publisher** subscription (`Mono.when`) but over-fires on **async + fire-on-call** commands, which are already ordered by call site on one connection. + When a "race" finding lands on Lettuce **async** code, check dispatch order before + trusting it. +- **#3604 node "missing dependency" mis-modeled npm hoisting, 2026-07-08 (2nd bugbot FP on same PR).** + Bugbot Medium `3545172685` claimed `run_node` installs only `redis` while the time-series + node example `import`s `@redis/time-series`, so `./run.sh time_series_tutorial node` "fails + at module load." **FALSE POSITIVE**: the `redis` npm metapackage (v6.1.0) declares + `@redis/time-series` (and `@redis/bloom`/`json`/`search`/`client`) as **direct dependencies**; + after `npm i redis` they sit hoisted-flat at `node_modules/@redis/time-series`, so the bare + import resolves. Refuted empirically: `node_modules/@redis/` contains `time-series`, and the + existing `results/time_series_tutorial_node.log` shows the module **loaded** and failed later + on a `deepEqual` assertion (out-of-scope `.rules` staleness), not at import. Lesson: bugbot's + "missing dependency / fails at module load" findings don't account for metapackage transitive + hoisting — check `node_modules` (or the metapackage's `dependencies`) before trusting them. + Same root as the async-`allOf` FP: bugbot mis-models a **dependency/ordering mechanic** it + can't see from the diff alone. + +- **#3612 pickup closing-sequence churn, 2026-07-10 (5th churn PR; docs/spec target).** Round 1 + flagged the label-drop *ordering* between `park-manifest.md` and `pickup` Step 5 (3558223727); + the fix reordered Step 5. Round 2 (on the fix commit `d5b6c86fd`) then raised a High, + `3558314964`: Step 4 reconciles + rebases but never *commits*, and Step 5 runs `/reflect` on + "the reconciliation commit" + `/finalize` — both of which see committed history only, so an + uncommitted reconcile is silently dropped from the squash. New, independent finding (not + ping-pong — no fix undone, no A↔B cycle) but the **same under-specified subsystem**: the + pickup closing sequence (reconcile → commit → reflect → finalize → drop-labels). The r1 edit + sat right in that area, so it's a "fix-caused-its-own-next-round" flavour like #3585. Consolidated (r2 fix `7044aefc1`): + an explicit ordered 4-step closing sequence in `pickup` Step 5 with the commit as step 1, and + the manifest summary deferring to it as authoritative. **But r3 (2026-07-10) proved that + consolidation was scoped too narrowly** — bugbot High `3558614316` "Missing push before merge": + Step 5 commits locally but never pushes, and the squash merges the *remote* head, so the + reconciliation commit **and** Step 4's rebased/merged-`main` history stay local and can be + omitted from the merge. Landed on the r2 commit itself. This is the **2nd PR** exhibiting the + #3536-round-6 meta-pattern ("when round N+1 finds another gap in an area you *just* + consolidated, your boundary was wrong — widen it"): the true subsystem is the + **local→remote→squash** dependency, not "is it committed." Recommended r4 fix is not another + step but an **invariant**: before the `gh pr merge` hand-off the remote PR head must contain + everything that must land (commit + push + Step 4 history + re-push after any `/reflect` + amend). Calibration note: bugbot effectively performed the churn detection one seam per round + — strong on procedure-completeness, which is why the fix must widen the boundary, not chase + the next seam. Meta-flag: if even the invariant rewrite draws an r4, the closing sequence + likely needs to leave prose for an actual checklist/script the skill references. +- **#3604 `@Test`/`[Fact]` "leak" ignores the build's marker stripping, 2026-07-09 (3rd FP flavour).** + Bugbot Low `3552548353` flagged a Lettuce override for leaving `@Test` on `run()` *outside* + any `REMOVE_START`/`REMOVE_END` region, concluding it would "expose JUnit markup in the client + tab." The *observation* is correct (the `@Test` is unwrapped) but the *impact is wrong*: + `build/components/example.py:185` strips any line matching the language's test marker + (`@Test` for java/java-async/java-reactive, `[Fact]` for c#) during extraction, independent + of REMOVE markers. Verified empirically — the rendered file has **zero** `@Test`. Lesson: + when bugbot claims a test annotation (`@Test`/`[Fact]`/`[SkippableFact]`) "leaks" into a + rendered client tab, check `build/components/example.py`'s `TEST_MARKER` stripping and the + actual rendered `examples/...` output before trusting it — the extractor already removes them. + (User flagged this one as a likely FP before I confirmed it — trust that instinct.) + +- **#3612 "`merged` field doesn't exist" — list-vs-single-PR endpoint confusion, 2026-07-10 (4th bugbot FP flavour; docs/spec target, not code).** + Bugbot High `3558223722` claimed the park/pickup snapshot command `gh api repos///pulls/ --jq '{merged, ...}'` + selects a nonexistent top-level `merged` field (should use `merged_at`), so merge-state diffs "silently fail." + **FALSE POSITIVE**: the **single-PR** REST endpoint (`GET /pulls/{n}`) — which the skills document — returns a + top-level `merged` boolean. Refuted empirically twice: `gh api repos/redis/docs/pulls/3612 --jq '{merged}'` → + `{"merged":false}`, and the DOC-6829 park run got `"merged":false` for spring-data-redis#3390 via the same path. + Root cause: the **list** endpoint (`GET /pulls`) omits `merged`, so the field is context-dependent; bugbot generalised + the list-shape to the single-PR call it can't distinguish from the diff. Same family as #3604's async/npm FPs — an + **API/runtime mechanic bugbot can't see from the diff**. Lesson: when bugbot says "this field/API won't work", run the + documented call before trusting it, especially where list-vs-item endpoint response shapes differ. + +### Bot true positives (calibration — what bugbot is good at) +- **#3604 `Double.MIN_VALUE` ≠ −∞, 2026-07-09 (language-semantics strength).** Bugbot Low + `3550287458` flagged `Range.create(Double.MIN_VALUE, 10)` in the Lettuce `SortedSetExample` + async+reactive overrides: `Double.MIN_VALUE` is Java's smallest *positive* value (~4.9e-324), + not −∞, so the range is ~0→10 not −∞→10 (the CLI uses `-inf`). Correct, and correctly scoped + as "produces right output on this all-positive dataset but teaches a copy-paste-wrong pattern." + This is the **complement** to the two dependency/ordering FPs on the same PR: bugbot is reliable + on **pure language/API semantics** (values it can evaluate from the source alone) and unreliable + on **runtime mechanics** it can't see (dispatch order, transitive deps). Reconciler value-add: + the *identical* bug also sits in `tmp/datatypes/sorted-sets/SortedSetsExample.java` (jedis, from + DOC-6057 `2569d4bc2`) — bugbot correctly did **not** flag it because it's pre-existing on `main`, + outside the PR diff. So "bugbot found N instances" ≠ "N instances exist"; grep the whole tree for + the pattern when a semantics finding lands. From 23f60254f8b1f3b5aef9ad75229d1171e6b661a0 Mon Sep 17 00:00:00 2001 From: Andy Stark Date: Fri, 10 Jul 2026 13:24:55 +0100 Subject: [PATCH 06/13] DOC-6830 Ignore in-repo .claude/projects/ memory store The Claude Code per-machine memory store belongs in ~/.claude and must never be committed; ignore any repo-root .claude/projects/ so it can't be added by mistake. Co-Authored-By: Claude Opus 4.8 (1M context) --- .gitignore | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.gitignore b/.gitignore index df31141b38..0c7268678d 100644 --- a/.gitignore +++ b/.gitignore @@ -35,3 +35,6 @@ package-lock.json # Link checker (link_check workflow) working files /external-urls.txt /.lycheecache + +# Claude Code per-machine memory store (belongs in ~/.claude, never committed) +/.claude/projects/ From ffd1868666a574d2bee49dfb7aa7fbd45b60f333 Mon Sep 17 00:00:00 2001 From: Andy Stark Date: Fri, 10 Jul 2026 13:44:47 +0100 Subject: [PATCH 07/13] DOC-6830 Collapse manifest pickup summary to a pointer, ending the drift MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fifth review finding on the pickup closing sequence (bugbot 3558867168, 'manifest omits rebase step') — the third on manifest-summary-vs-pickup drift. Root cause: the manifest kept a hand-maintained copy of pickup's sequence, and a copy drifts from its source every round. Rather than patch in the missing rebase (inviting a sixth finding), remove the duplication: manifest section 5 now defers entirely to /pickup Steps 4-5 as the single authoritative sequence and restates no steps, so there is nothing left to drift. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/skills/_shared/park-manifest.md | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/.claude/skills/_shared/park-manifest.md b/.claude/skills/_shared/park-manifest.md index 8a40ed6f1c..548b5f8b01 100644 --- a/.claude/skills/_shared/park-manifest.md +++ b/.claude/skills/_shared/park-manifest.md @@ -59,13 +59,15 @@ PR, machine-locatable by its markers. *are* the loose ends. Add anything predicted-to-change not already captured. Mark the highest-risk items. -5. **On pickup, then** — the closing steps, in the exact order `/pickup` Step 5 defines: - reconcile docs, remove the `bannerText` warning, **commit** the reconciliation, run - **`/reflect`**, **push** (so the remote head carries the final history, including any - `/reflect` amend — the squash merges only what has been pushed), run **`/finalize`**, and - **only then** drop the labels. The `do not merge yet` guard must hold until `/finalize` - completes. This is a summary; **`/pickup` Steps 4–5 are the authoritative sequence** — keep - it in that order (commit → `/reflect` → push → `/finalize` → drop labels). +5. **On pickup, then** — when the trigger fires, run `/pickup `. It reconciles the docs + against the now-settled source and takes the PR through the normal `/reflect` → `/finalize` + pipeline to merge, lifting the `parked` / `do not merge yet` labels only after `/finalize`. + **The exact closing sequence — reconcile, rebase, commit, push, and their ordering — lives + in `/pickup` Steps 4–5, which are authoritative; this manifest deliberately does not restate + it.** (A hand-copied step list here drifted from `/pickup` on every review round — label + order, push-before-`/reflect`, missing rebase — so it was replaced by this pointer. Don't + re-add one.) The one guarantee this section makes: the `do not merge yet` guard holds until + `/finalize` completes. ## Relationship to commit trailers From 03ec1e1f59f5bd28cc85ff88001164e96d816c77 Mon Sep 17 00:00:00 2001 From: Andy Stark Date: Fri, 10 Jul 2026 13:44:48 +0100 Subject: [PATCH 08/13] DOC-6830 Record churn resolution in assess-comments coverage ledger Capture the decisive round of the #3612 pickup closing-sequence churn: the fifth finding forced the structural fix (eliminate the duplicated manifest summary), generalising the earlier 'widen the boundary' rule to 'when a duplicate of a procedure keeps drawing drift findings, stop duplicating and point at the single source'. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/state/assess-comments.coverage.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/.claude/state/assess-comments.coverage.md b/.claude/state/assess-comments.coverage.md index 78e2ad54df..f354edee3d 100644 --- a/.claude/state/assess-comments.coverage.md +++ b/.claude/state/assess-comments.coverage.md @@ -280,6 +280,17 @@ rule fires and distinguishes fixed from still-broken. — strong on procedure-completeness, which is why the fix must widen the boundary, not chase the next seam. Meta-flag: if even the invariant rewrite draws an r4, the closing sequence likely needs to leave prose for an actual checklist/script the skill references. + **It did (r5/bugbot-r4, 2026-07-10): `3558867168` "Manifest omits rebase step"** — the + *third* finding on the manifest-summary-vs-`pickup` drift (after r1 label-order and Codex's + push-order P2). Root cause finally clear: the manifest kept a hand-maintained **copy** of + pickup's closing sequence, and a copy drifts from its source every round — each fix matched + one more element, the next round found the next omission (rebase, this time). Ended not by + patching in "rebase" but by **eliminating the duplication**: manifest §5 now defers entirely + to `/pickup` Steps 4–5 (the single authoritative sequence) and restates *no* steps, so there + is nothing left to drift. Lesson (generalises the #3536-r6 "widen the boundary" rule): when a + *summary/duplicate* of a procedure keeps drawing drift findings, the boundary to widen is + "stop duplicating" — collapse to one source and point at it, don't keep re-syncing the copy. + The 5-round arc (3 of them manifest-copy drift) is the textbook case for it. - **#3604 `@Test`/`[Fact]` "leak" ignores the build's marker stripping, 2026-07-09 (3rd FP flavour).** Bugbot Low `3552548353` flagged a Lettuce override for leaving `@Test` on `run()` *outside* any `REMOVE_START`/`REMOVE_END` region, concluding it would "expose JUnit markup in the client From 7e593583cbb065347c016caf8b615b6a0a998c94 Mon Sep 17 00:00:00 2001 From: Andy Stark Date: Fri, 10 Jul 2026 14:12:19 +0100 Subject: [PATCH 09/13] DOC-6830 Consolidate pickup git sequence and split park PR/issue snapshots MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review-driven skill fixes. Pickup: merge Steps 4-5 into one ordered git sequence (commit -> rebase/merge main + link-checks -> /reflect -> push -> /finalize -> drop labels), fixing bugbot 3558994646 — the old order ran 'git rebase' on a dirty tree before the reconciliation commit. Park: split Step 3's source snapshot into PR (pulls/) and issue (issues/) variants per Codex review, since the PR-only merged/head/base fields don't exist for issue-driven parks. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/skills/park/SKILL.md | 12 ++++++++++- .claude/skills/pickup/SKILL.md | 39 ++++++++++++++++++---------------- 2 files changed, 32 insertions(+), 19 deletions(-) diff --git a/.claude/skills/park/SKILL.md b/.claude/skills/park/SKILL.md index 9193ad487c..32c6ae8589 100644 --- a/.claude/skills/park/SKILL.md +++ b/.claude/skills/park/SKILL.md @@ -62,13 +62,23 @@ in the manifest checklist now (see [`../_shared/park-manifest.md`](../_shared/pa ## Step 3 — Snapshot each pinned source Identify the external sources the docs depend on (URLs in the ticket, commit bodies, the page's -`bannerText`). For each, capture its live state. For a GitHub PR/issue: +`bannerText`). For each, capture its live state with the endpoint that matches the source type. + +For a GitHub **pull request**: ``` gh api repos///pulls/ \ --jq '{state, merged, head_sha: .head.sha, base: .base.ref, milestone: .milestone.title, updated_at}' ``` +For a GitHub **issue** — a feature-request or GA-tracking issue, say — the `pulls` endpoint and +its `merged` / `head` / `base` fields don't exist, so use `issues/`: + +``` +gh api repos///issues/ \ + --jq '{state, state_reason, milestone: .milestone.title, updated_at, closed_at}' +``` + Record URL + snapshot in the sources table, and the re-fetch command for pickup. Add a **confidence-tagged** summary of the API/behaviour the page assumes (LOW for docs written against an unmerged diff — and say *why* it's low, e.g. "signatures differed between two reads diff --git a/.claude/skills/pickup/SKILL.md b/.claude/skills/pickup/SKILL.md index afb05168c0..424ba03b07 100644 --- a/.claude/skills/pickup/SKILL.md +++ b/.claude/skills/pickup/SKILL.md @@ -72,10 +72,9 @@ the park-time assumption (which was often LOW-confidence by design). Apply the corrections: fix signatures/config, fill version placeholders, add sections for pieces that landed after park (e.g. a deferred API), convert provisional inline examples to -tested examples where the tooling now allows. Then clear the staleness: - -- Remove the page's `bannerText` "not yet released" warning. -- Rebase / merge `main`; re-run link checks; confirm `relref` targets still resolve. +tested examples where the tooling now allows, and remove the page's `bannerText` "not yet +released" warning. This step is only *what to change* — the git operations that make those +changes land (including the rebase and link checks) come next, in order. ### Step 5 — Resume the pipeline & hand off @@ -83,25 +82,29 @@ Pickup **reconciles; it does not merge.** `/finalize` prepares a squash that ope **pushed remote PR head**, so the invariant governing this whole step is: > Before the `gh pr merge` hand-off, the remote PR branch must already contain **everything -> that must land** — the reconciliation commit, Step 4's rebased/merged-`main` history, and any +> that must land** — the reconciliation commit, the rebased/merged-`main` history, and any > later amend. The squash sees only what has been pushed; anything left local is silently > dropped from the merge. -Everything below just drives the branch to that end state, re-pushing whenever local history -changes: - -1. **Commit the reconciliation.** `/reflect` and `/finalize` act on committed history only — an - uncommitted working tree never reaches the merge. -2. **`/reflect`** on that commit — record *predicted-vs-actual*: what the park snapshot got - right, what the source changed (it closes the loop and calibrates future preemptive docs). - `/reflect` may fold its note into the commit or amend it. -3. **Push the branch** — and re-push after any amend in step 2. This is the step that makes the - remote head match local; skip it and the squash merges *without* the reconciliation (and - without Step 4's rebase), which is the gap that keeps reappearing here. -4. **`/finalize`** — the durable squash deferred at park time, now that the source has settled. +Run these steps in **exactly this order** — the ordering is load-bearing (notably `git rebase` +refuses to run on a dirty tree, so the commit must precede it), re-pushing whenever local +history changes: + +1. **Commit** the Step 4 reconciliation. Nothing downstream sees an uncommitted tree, and the + rebase in step 2 needs a clean one. +2. **Rebase / merge `main`** into the branch, then re-run link checks and confirm `relref` + targets still resolve. The tree is clean now, so the rebase proceeds; resolve any conflicts + here. +3. **`/reflect`** on the reconciliation commit — record *predicted-vs-actual*: what the park + snapshot got right, what the source changed (it closes the loop and calibrates future + preemptive docs). `/reflect` may fold its note into the commit or amend it. +4. **Push** the branch — and re-push after any amend in step 3. This makes the remote head match + local; skip it and the squash merges *without* the reconciliation and rebase — the gap that + kept reappearing here. +5. **`/finalize`** — the durable squash deferred at park time, now that the source has settled. It reconciles the whole arc (park notes + pickup findings + any review) and prepares the `gh pr merge … --body-file` command. -5. **Only now, remove** the `parked` and `do not merge yet` labels and strip the manifest block +6. **Only now, remove** the `parked` and `do not merge yet` labels and strip the manifest block from the PR body (or mark it resolved). The merge guard holds until `/finalize` is done. Before handing off, **confirm the remote head matches local** (working tree clean, branch not From 91d0c20837f5d2f629deb3500289234d25300719 Mon Sep 17 00:00:00 2001 From: Andy Stark Date: Fri, 10 Jul 2026 14:12:19 +0100 Subject: [PATCH 10/13] DOC-6830 Record r6 churn resolution and fix calibration count in ledger Capture the #3612 review cycle's decisive rounds: de-duplication validated (manifest copy-drift class closed) and a distinct pickup git-op ordering thread surfaced and fixed; correct the #3612 calibration header to 6 valid / 1 FP to match its enumerated findings (Codex P3). Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/state/assess-comments.coverage.md | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/.claude/state/assess-comments.coverage.md b/.claude/state/assess-comments.coverage.md index f354edee3d..e08f222203 100644 --- a/.claude/state/assess-comments.coverage.md +++ b/.claude/state/assess-comments.coverage.md @@ -32,7 +32,7 @@ whether to commit the change. | GraphQL thread-resolution pull (`isResolved`/`isOutdated`) | 🟢 corroborated | 10 | 2026-07-10 | #3510 (12/12 resolved), #3374 (15/15), #3536 (2/2 open), #3542 (3 resolved/1 open), #3543 (1/1 open), #3573 (2/2 open), #2531 (r1: 11 open/2 res-outdated/1 open; r2: 11 of 14 outdated after fixes + APPROVED), #3585 (r1 2 open; r2 3 open + 2 resolved — one outdated, one resolved-not-outdated); #3604 (r1 2/2 open; r2 4/4 open; r3 5 threads = 2 resolved [one outdated, one resolved-not-outdated] / 3 open, hasNextPage:false); #3612 (r1 4/4 open, hasNextPage:false) | | Source-role tagging (bugbot/security/history/summary/ci/human) | 🟢 corroborated | 11 | 2026-07-10 | #3415, #3507, #3374, #3536, #3542, #3543, #3573, #2531, #3585, #3604, #3612 (bugbot inline + github-actions Jira-link ci; no human/summary this PR) | | Open/resolved split | 🟢 corroborated | 10 | 2026-07-10 | #3510, #3374, #3536 (0 resolved/2 open), #3542 (3 resolved/1 open), #3543, #3573 (r1/r2 mixed), #2531 (11 open / 3 resolved), #3585 (r1 2 open/0 res; r2 3 open/2 resolved), #3604 (r1 0 res/2 open; r2 0 res/4 open; r3 2 res/3 open), #3612 (0 res/4 open) | -| Fix-quality spot-check (genuinely fixed vs silenced) | 🟢 corroborated | 8 | 2026-07-10 | #3510 (term removals landed), #3374 (`num_docs`, dropIndex landed), #3542 (xargs+guard, narrowed exclude, SHA pin landed), #3573 (relpath + `--add` dedup landed), #2531 (r1 found decimal thread 2619563419 reverted; r2 doc fixes landed + engineer ZdravkoDonev **APPROVED**), #3585 r2 (both r1 findings genuinely fixed — byId multimap + isError wrapping present in code, not silenced); #3604 r3 (reactive race 3544441638 resolved+outdated by 6735cf9cc — sequential `.then()` chain present + harness PASS; smismember 3545172677 resolved-not-outdated, genuinely fixed in client sources not the anchored sets.md line); #3612 (r1's 3 doc-consistency fixes + r2 commit-gap all resolved; r2 3558314964 resolved+**outdated** by 7044aefc1 — consolidation genuinely present in Step 5, not silenced) | +| Fix-quality spot-check (genuinely fixed vs silenced) | 🟢 corroborated | 8 | 2026-07-10 | #3510 (term removals landed), #3374 (`num_docs`, dropIndex landed), #3542 (xargs+guard, narrowed exclude, SHA pin landed), #3573 (relpath + `--add` dedup landed), #2531 (r1 found decimal thread 2619563419 reverted; r2 doc fixes landed + engineer ZdravkoDonev **APPROVED**), #3585 r2 (both r1 findings genuinely fixed — byId multimap + isError wrapping present in code, not silenced); #3604 r3 (reactive race 3544441638 resolved+outdated by 6735cf9cc — sequential `.then()` chain present + harness PASS; smismember 3545172677 resolved-not-outdated, genuinely fixed in client sources not the anchored sets.md line); #3612 (r1's 3 doc-consistency fixes + r2 commit-gap all resolved; r2 3558314964 resolved+**outdated** by 7044aefc1 — consolidation genuinely present in Step 5, not silenced; **structural de-dup confirmed**: bugbot re-scan of the collapsed manifest §5 (`03ec1e1f5`) raised nothing on it — the copy-drift class is genuinely closed, not merely silenced) | | "Resolved ≠ fixed" flag — **legitimate deferral** variant | 🟡 seen once | 1 | 2026-06-23 | #3510 (TS.BGET:122 left pending eng) | | "Resolved ≠ fixed" flag — **still-broken / reverted** variant | 🟡 seen once | 1 | 2026-06-30 | #2531 (resolved+outdated thread 2619563419 said decimal default=`string`; a later rewrite reverted current code to `precise`, so the resolved fix is no longer in the code — engineer re-raised it as 3496835587). Regression flavour; see worked examples | | Cross-tool **agreement** | 🟡 seen once | 1 | 2026-06-23 | #3374 (Claude + bugbot independently on `num_docs`) | @@ -42,7 +42,7 @@ whether to commit the change. | Approval-over-open-finding cross-check | 🟢 corroborated | 7 | 2026-07-08 | #3415 (dwdougherty), #3374 (low-confidence over open HIGH), #3536 (high-confidence over 2 open Mediums: benign), #3542 (paoloredis "yep go ahead" 7 min after open Medium #3498159511; unacknowledged), #3573 (dwdougherty "Sure, why not?" APPROVED 13:41 over open findings; 2 bot findings landed 13:49 after), #2531 (run1 correct **negative** — no approval; run2 **positive** — ZdravkoDonev APPROVED 13:13 then bugbot finding 3499796857 landed 15:08, and he approved over 2-3 of his own still-open findings incl. the temporal one), #3585 (correct **negative** both rounds — open bugbot findings, only bot COMMENTED verdicts, no human approval); #3604 (correct **negative** all 3 rounds — r3 3 open bugbot findings, only cursor COMMENTED verdicts, no human approval); #3612 (correct **negative** — 4 open bugbot findings, only cursor COMMENTED verdict, no human approval) | | Depth cap / prioritisation under load | 🟢 corroborated | 2 | 2026-07-02 | #3374 (19 candidate findings → 4 deep-verified); #2531 (r1: 14 threads → 5 deep-verified, 6 deferred). *(#3542/#3573 were under cap — not load tests)* | | Mandatory deep-verify of resolved+not-outdated HIGH | 🟢 corroborated | 2 | 2026-07-02 | #3542 #3467309496 (High "Grep failure skips link check", resolved + isOutdated:false) — deep-verified: xargs+guard genuinely present, legitimately fixed. #3585 #3513924736 (High "Duplicate page IDs break get_page", resolved + isOutdated:false) — deep-verified against current code: byId multimap + url-first resolution present, genuinely fixed. 2nd distinct PR → corroborated | -| Bot calibration (fixed-vs-dismissed ratio) | 🟢 corroborated | 8 | 2026-07-08 | #3374 (bugbot mostly accepted); #3536 (5/5 valid); #3542 (3/3 valid — 2 fixed, 1 open); #3543 (1/1 valid; Jit 0); #3573 (4/4 valid; Jit 0); #2531 (r1 bugbot 0 findings; r2 bugbot 1/1 valid — caught the ledger duplicate-rows defect 3499796857; Jit 0); #3585 (bugbot **5/5 valid** across 2 rounds); #3604 (across 3 rounds **3 valid / 2 FP**: valid = reactive `Mono.when` race 3544441638 + SMISMEMBER client-tab parity 3545172677 [fixed by 9bf49b2f4] + `Double.MIN_VALUE`≠−∞ range bug 3550287458 [r3, a language-semantics catch]; FP = async `allOf` 3544581411 + node harness `@redis/time-series` "missing dep" 3545172685. **Calibration axis**: bugbot is strong on API/language semantics [`Double.MIN_VALUE` is smallest-positive] but both FPs mis-model runtime dependency/ordering it can't see from the diff [async fire-on-call; npm transitive hoisting]. Also found 2 of 3 identical `Double.MIN_VALUE` instances — correctly skipped the jedis one, pre-existing on main / outside the PR diff); #3612 (**5 valid / 1 FP across 3 rounds** — r1 valid = internal doc-vs-doc label-order contradiction 3558223727 + scan-mode wrong-step-ref 3558223733 + park update-path-only-shows-`gh pr create` 3558223738; r2 valid = commit-before-reflect High 3558314964; r3 valid = push-before-merge High 3558614316 [both pickup closing-sequence churn]; FP = r1 High "`merged` field doesn't exist" 3558223722, refuted empirically — single-PR REST endpoint returns top-level `merged`. **Calibration axis holds**: all 5 valid catches are cross-file doc-consistency / procedure-completeness [bugbot's strength], and it effectively ran the churn detection one seam per round; the 1 FP is again an **unseen API mechanic** [list-endpoint vs single-PR-endpoint response shape] — same root as #3604's async/npm FPs) | +| Bot calibration (fixed-vs-dismissed ratio) | 🟢 corroborated | 8 | 2026-07-08 | #3374 (bugbot mostly accepted); #3536 (5/5 valid); #3542 (3/3 valid — 2 fixed, 1 open); #3543 (1/1 valid; Jit 0); #3573 (4/4 valid; Jit 0); #2531 (r1 bugbot 0 findings; r2 bugbot 1/1 valid — caught the ledger duplicate-rows defect 3499796857; Jit 0); #3585 (bugbot **5/5 valid** across 2 rounds); #3604 (across 3 rounds **3 valid / 2 FP**: valid = reactive `Mono.when` race 3544441638 + SMISMEMBER client-tab parity 3545172677 [fixed by 9bf49b2f4] + `Double.MIN_VALUE`≠−∞ range bug 3550287458 [r3, a language-semantics catch]; FP = async `allOf` 3544581411 + node harness `@redis/time-series` "missing dep" 3545172685. **Calibration axis**: bugbot is strong on API/language semantics [`Double.MIN_VALUE` is smallest-positive] but both FPs mis-model runtime dependency/ordering it can't see from the diff [async fire-on-call; npm transitive hoisting]. Also found 2 of 3 identical `Double.MIN_VALUE` instances — correctly skipped the jedis one, pre-existing on main / outside the PR diff); #3612 (**6 valid / 1 FP across the review cycle** — r1 valid = internal doc-vs-doc label-order contradiction 3558223727 + scan-mode wrong-step-ref 3558223733 + park update-path-only-shows-`gh pr create` 3558223738; r2 valid = commit-before-reflect High 3558314964; r3 valid = push-before-merge High 3558614316; r5 valid = manifest-omits-rebase 3558867168 [manifest copy-drift]; r6 valid = rebase-before-commit 3558994646 [pickup git-op ordering]; FP = r1 High "`merged` field doesn't exist" 3558223722, refuted empirically — single-PR REST endpoint returns top-level `merged`. **Calibration axis holds**: all 6 valid catches are cross-file doc-consistency / procedure-completeness [bugbot's strength], and it effectively ran the churn detection one seam per round across ~6 rounds; the 1 FP is again an **unseen API mechanic** [list-endpoint vs single-PR-endpoint response shape] — same root as #3604's async/npm FPs) | | Codex second-opinion availability gate | 🟢 corroborated | 9 | 2026-07-10 | #3415, #3374 (CLI on PATH; #3374 had a real Codex review), #3542, #3543, #3573, #2531, #3585, #3604, #3612 (codex on PATH at /Users/andrew.stark/bin/codex) | | Ledger self-integrity after `main` merge (no duplicate rows) | 🟡 seen twice | 2 | 2026-07-02 | #2531 r2 — bugbot 3499796857 caught the shared ledger gaining duplicate rows when `main` (carrying a #3573-era ledger) merged in and git kept both blocks. **2026-07-02**: merging `main` again produced a real conflict as #3542/#3573 edited the same rows — union-merged per capability. Recurring shared-file hazard; see worked examples + step-11 refinement | @@ -291,6 +291,19 @@ rule fires and distinguishes fixed from still-broken. *summary/duplicate* of a procedure keeps drawing drift findings, the boundary to widen is "stop duplicating" — collapse to one source and point at it, don't keep re-syncing the copy. The 5-round arc (3 of them manifest-copy drift) is the textbook case for it. + **Validated + a 2nd thread exposed (r6/bugbot re-scan of `03ec1e1f5`, 2026-07-10):** the + re-scan of the collapsed manifest raised **nothing** on it — the copy-drift class is + structurally closed (fix-quality confirmed). But a *distinct* churn thread surfaced underneath + on the authoritative source: `3558994646` "Rebase before reconciliation commit" + (pickup/SKILL.md) — Step 4 applies edits then `git rebase`, which fails on a dirty tree, while + Step 5 commits only afterwards. This is thread B: the **git-operation ordering** in the + closing sequence (commit r2 → push r3 → rebase-vs-commit r6) — three genuine ordering gaps in + pickup itself, separate from the manifest-copy thread. Lesson refinement: de-duplication kills + *copy* drift but not an under-specified *procedure*; six prose rounds is the signal to stop + describing a precise git sequence in prose and make it one explicit ordered block/checklist + (commit → rebase → link-check → /reflect → push → /finalize → drop-labels). Also: **longest + clean churn-not-loop case on record (6 rounds, still no ping-pong)** — new independent finding + each round, no fix undone, no A↔B cycle. - **#3604 `@Test`/`[Fact]` "leak" ignores the build's marker stripping, 2026-07-09 (3rd FP flavour).** Bugbot Low `3552548353` flagged a Lettuce override for leaving `@Test` on `run()` *outside* any `REMOVE_START`/`REMOVE_END` region, concluding it would "expose JUnit markup in the client From 46dca5cf1cbe739c73647528b97fcc1d7871d1ab Mon Sep 17 00:00:00 2001 From: Andy Stark Date: Fri, 10 Jul 2026 14:31:05 +0100 Subject: [PATCH 11/13] DOC-6830 Guard park's clean tree and make the manifest a pure pointer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two Codex review residuals. Park Step 5: commit working-tree edits (notably a bannerText added in Step 4) and confirm a clean tree before pushing, so a parked PR can't get its manifest and labels while a required edit lingers local. Manifest section 5: strip the residual 'reconcile, rebase, commit, push' step list (which also implied the stale rebase-before-commit order) so it is a pure pointer to /pickup Steps 4-5 — finishing the de-duplication. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/skills/_shared/park-manifest.md | 9 ++++----- .claude/skills/park/SKILL.md | 3 +++ 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/.claude/skills/_shared/park-manifest.md b/.claude/skills/_shared/park-manifest.md index 548b5f8b01..5e3ab2d61d 100644 --- a/.claude/skills/_shared/park-manifest.md +++ b/.claude/skills/_shared/park-manifest.md @@ -62,11 +62,10 @@ PR, machine-locatable by its markers. 5. **On pickup, then** — when the trigger fires, run `/pickup `. It reconciles the docs against the now-settled source and takes the PR through the normal `/reflect` → `/finalize` pipeline to merge, lifting the `parked` / `do not merge yet` labels only after `/finalize`. - **The exact closing sequence — reconcile, rebase, commit, push, and their ordering — lives - in `/pickup` Steps 4–5, which are authoritative; this manifest deliberately does not restate - it.** (A hand-copied step list here drifted from `/pickup` on every review round — label - order, push-before-`/reflect`, missing rebase — so it was replaced by this pointer. Don't - re-add one.) The one guarantee this section makes: the `do not merge yet` guard holds until + **The closing sequence and its ordering live solely in `/pickup` Steps 4–5, which are + authoritative; this manifest deliberately does not restate them — not even as a list.** (A + hand-copied step list here drifted from `/pickup` on every review round, so it was replaced by + this pointer. Don't re-add one, in any order.) The one guarantee this section makes: the `do not merge yet` guard holds until `/finalize` completes. ## Relationship to commit trailers diff --git a/.claude/skills/park/SKILL.md b/.claude/skills/park/SKILL.md index 32c6ae8589..ff801734ac 100644 --- a/.claude/skills/park/SKILL.md +++ b/.claude/skills/park/SKILL.md @@ -93,6 +93,9 @@ required sections (per the shared spec). The page itself should already carry a ## Step 5 — Land it +- **Commit any working-tree edits first** — including a `bannerText` you added in Step 4 — and + confirm a clean tree before pushing. The parked PR must not get its manifest and labels while a + required edit lingers only in the local working tree. - Push the branch. - Open the PR, or update an existing one in place. For a new PR: ``` From 847af35b296bd38c2052ae026f5f1c47092776d7 Mon Sep 17 00:00:00 2001 From: Andy Stark Date: Fri, 10 Jul 2026 14:31:05 +0100 Subject: [PATCH 12/13] DOC-6830 Record pickup git-sequence de-prescription in coverage ledger MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Capture the r7 finding: the pickup git sequence hit its 4th churn round (force-with-lease), and the endgame shifted from consolidation to de-prescription — assert the invariant + intent checklist, delegate git mechanics; an invariant can't be incomplete the way a command list can. Calibration 8 valid / 1 FP. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/state/assess-comments.coverage.md | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/.claude/state/assess-comments.coverage.md b/.claude/state/assess-comments.coverage.md index e08f222203..51840fe494 100644 --- a/.claude/state/assess-comments.coverage.md +++ b/.claude/state/assess-comments.coverage.md @@ -42,7 +42,7 @@ whether to commit the change. | Approval-over-open-finding cross-check | 🟢 corroborated | 7 | 2026-07-08 | #3415 (dwdougherty), #3374 (low-confidence over open HIGH), #3536 (high-confidence over 2 open Mediums: benign), #3542 (paoloredis "yep go ahead" 7 min after open Medium #3498159511; unacknowledged), #3573 (dwdougherty "Sure, why not?" APPROVED 13:41 over open findings; 2 bot findings landed 13:49 after), #2531 (run1 correct **negative** — no approval; run2 **positive** — ZdravkoDonev APPROVED 13:13 then bugbot finding 3499796857 landed 15:08, and he approved over 2-3 of his own still-open findings incl. the temporal one), #3585 (correct **negative** both rounds — open bugbot findings, only bot COMMENTED verdicts, no human approval); #3604 (correct **negative** all 3 rounds — r3 3 open bugbot findings, only cursor COMMENTED verdicts, no human approval); #3612 (correct **negative** — 4 open bugbot findings, only cursor COMMENTED verdict, no human approval) | | Depth cap / prioritisation under load | 🟢 corroborated | 2 | 2026-07-02 | #3374 (19 candidate findings → 4 deep-verified); #2531 (r1: 14 threads → 5 deep-verified, 6 deferred). *(#3542/#3573 were under cap — not load tests)* | | Mandatory deep-verify of resolved+not-outdated HIGH | 🟢 corroborated | 2 | 2026-07-02 | #3542 #3467309496 (High "Grep failure skips link check", resolved + isOutdated:false) — deep-verified: xargs+guard genuinely present, legitimately fixed. #3585 #3513924736 (High "Duplicate page IDs break get_page", resolved + isOutdated:false) — deep-verified against current code: byId multimap + url-first resolution present, genuinely fixed. 2nd distinct PR → corroborated | -| Bot calibration (fixed-vs-dismissed ratio) | 🟢 corroborated | 8 | 2026-07-08 | #3374 (bugbot mostly accepted); #3536 (5/5 valid); #3542 (3/3 valid — 2 fixed, 1 open); #3543 (1/1 valid; Jit 0); #3573 (4/4 valid; Jit 0); #2531 (r1 bugbot 0 findings; r2 bugbot 1/1 valid — caught the ledger duplicate-rows defect 3499796857; Jit 0); #3585 (bugbot **5/5 valid** across 2 rounds); #3604 (across 3 rounds **3 valid / 2 FP**: valid = reactive `Mono.when` race 3544441638 + SMISMEMBER client-tab parity 3545172677 [fixed by 9bf49b2f4] + `Double.MIN_VALUE`≠−∞ range bug 3550287458 [r3, a language-semantics catch]; FP = async `allOf` 3544581411 + node harness `@redis/time-series` "missing dep" 3545172685. **Calibration axis**: bugbot is strong on API/language semantics [`Double.MIN_VALUE` is smallest-positive] but both FPs mis-model runtime dependency/ordering it can't see from the diff [async fire-on-call; npm transitive hoisting]. Also found 2 of 3 identical `Double.MIN_VALUE` instances — correctly skipped the jedis one, pre-existing on main / outside the PR diff); #3612 (**6 valid / 1 FP across the review cycle** — r1 valid = internal doc-vs-doc label-order contradiction 3558223727 + scan-mode wrong-step-ref 3558223733 + park update-path-only-shows-`gh pr create` 3558223738; r2 valid = commit-before-reflect High 3558314964; r3 valid = push-before-merge High 3558614316; r5 valid = manifest-omits-rebase 3558867168 [manifest copy-drift]; r6 valid = rebase-before-commit 3558994646 [pickup git-op ordering]; FP = r1 High "`merged` field doesn't exist" 3558223722, refuted empirically — single-PR REST endpoint returns top-level `merged`. **Calibration axis holds**: all 6 valid catches are cross-file doc-consistency / procedure-completeness [bugbot's strength], and it effectively ran the churn detection one seam per round across ~6 rounds; the 1 FP is again an **unseen API mechanic** [list-endpoint vs single-PR-endpoint response shape] — same root as #3604's async/npm FPs) | +| Bot calibration (fixed-vs-dismissed ratio) | 🟢 corroborated | 8 | 2026-07-08 | #3374 (bugbot mostly accepted); #3536 (5/5 valid); #3542 (3/3 valid — 2 fixed, 1 open); #3543 (1/1 valid; Jit 0); #3573 (4/4 valid; Jit 0); #2531 (r1 bugbot 0 findings; r2 bugbot 1/1 valid — caught the ledger duplicate-rows defect 3499796857; Jit 0); #3585 (bugbot **5/5 valid** across 2 rounds); #3604 (across 3 rounds **3 valid / 2 FP**: valid = reactive `Mono.when` race 3544441638 + SMISMEMBER client-tab parity 3545172677 [fixed by 9bf49b2f4] + `Double.MIN_VALUE`≠−∞ range bug 3550287458 [r3, a language-semantics catch]; FP = async `allOf` 3544581411 + node harness `@redis/time-series` "missing dep" 3545172685. **Calibration axis**: bugbot is strong on API/language semantics [`Double.MIN_VALUE` is smallest-positive] but both FPs mis-model runtime dependency/ordering it can't see from the diff [async fire-on-call; npm transitive hoisting]. Also found 2 of 3 identical `Double.MIN_VALUE` instances — correctly skipped the jedis one, pre-existing on main / outside the PR diff); #3612 (**8 valid / 1 FP across the review cycle** [r7 added 3559171169 rebase-force-lease + 3559171157 scan-mode-stop-rule, both pickup] — r1 valid = internal doc-vs-doc label-order contradiction 3558223727 + scan-mode wrong-step-ref 3558223733 + park update-path-only-shows-`gh pr create` 3558223738; r2 valid = commit-before-reflect High 3558314964; r3 valid = push-before-merge High 3558614316; r5 valid = manifest-omits-rebase 3558867168 [manifest copy-drift]; r6 valid = rebase-before-commit 3558994646 [pickup git-op ordering]; FP = r1 High "`merged` field doesn't exist" 3558223722, refuted empirically — single-PR REST endpoint returns top-level `merged`. **Calibration axis holds**: all 8 valid catches are cross-file doc-consistency / procedure-completeness / git-correctness [bugbot's strength], and it effectively ran the churn detection one seam per round across 7 rounds; the 1 FP is again an **unseen API mechanic** [list-endpoint vs single-PR-endpoint response shape] — same root as #3604's async/npm FPs) | | Codex second-opinion availability gate | 🟢 corroborated | 9 | 2026-07-10 | #3415, #3374 (CLI on PATH; #3374 had a real Codex review), #3542, #3543, #3573, #2531, #3585, #3604, #3612 (codex on PATH at /Users/andrew.stark/bin/codex) | | Ledger self-integrity after `main` merge (no duplicate rows) | 🟡 seen twice | 2 | 2026-07-02 | #2531 r2 — bugbot 3499796857 caught the shared ledger gaining duplicate rows when `main` (carrying a #3573-era ledger) merged in and git kept both blocks. **2026-07-02**: merging `main` again produced a real conflict as #3542/#3573 edited the same rows — union-merged per capability. Recurring shared-file hazard; see worked examples + step-11 refinement | @@ -304,6 +304,24 @@ rule fires and distinguishes fixed from still-broken. (commit → rebase → link-check → /reflect → push → /finalize → drop-labels). Also: **longest clean churn-not-loop case on record (6 rounds, still no ping-pong)** — new independent finding each round, no fix undone, no A↔B cycle. + **7th round (bugbot re-scan of `91d0c2083`, 2026-07-10): churn is now 4-deep on the git + operations specifically.** After the r6 consolidation, bugbot raised `3559171169` "Rebase push + omits force lease" (a rebased parked branch was already pushed at `/park`, so a plain `git + push` is non-fast-forward — needs `--force-with-lease`) plus `3559171157` "Scan mode inherits + stop rule" (Mode A borrows Step 2's trigger check, which includes *stop-if-not-met*, so a scan + would abort on the first not-ready PR instead of recording `fired?=no` and continuing). The + git-op count is now commit (r2) → push (r3) → rebase-order (r6) → force-lease (r8): every time + the sequence spells out one more git command, the next git edge case surfaces, and **two of + these were my own prior fixes exposing the next** (rebase step → force-lease; Mode-A→Step-2 + reference → stop-rule import). Terminal diagnosis: the skill **over-specifies git plumbing** + that is context-dependent and standard operator competence. Recommended endgame is NOT another + consolidation but **de-prescription**: reduce Step 5 to the *invariant* (remote head must + contain everything before hand-off) + an ordered checklist of *intent* (reconcile → commit → + sync main → /reflect → push → /finalize → drop labels), delegating the exact commands + (force-with-lease after rebase, dirty-tree ordering) to the operator — an invariant can't be + "incomplete" the way a command list can. Generalises the churn rule one step further: when + *enumerated mechanics* keep drawing correct-but-incremental findings, stop enumerating and + assert the invariant instead. - **#3604 `@Test`/`[Fact]` "leak" ignores the build's marker stripping, 2026-07-09 (3rd FP flavour).** Bugbot Low `3552548353` flagged a Lettuce override for leaving `@Test` on `run()` *outside* any `REMOVE_START`/`REMOVE_END` region, concluding it would "expose JUnit markup in the client From d9263a62d2b6f47e4f88ba20db8591cf09f4570f Mon Sep 17 00:00:00 2001 From: Andy Stark Date: Fri, 10 Jul 2026 14:31:39 +0100 Subject: [PATCH 13/13] DOC-6830 De-prescribe pickup Step 5 git mechanics; decouple scan-mode trigger check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the 4th pickup git-sequence churn round (force-with-lease, bugbot 3559171169) not by patching another command but by de-prescribing: Step 5 is now the invariant + an ordered intent checklist (committed & synced -> /reflect -> remote==local -> /finalize -> labels last), delegating the exact git commands (rebase clean-tree, force-with-lease on rewritten history) to normal git — an invariant can't be incomplete the way a command list can. Also decouple Mode A's trigger check from Step 2's stop-and-report rule (bugbot 3559171157) so a scan records fired?=no and continues instead of aborting on the first not-ready PR. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/skills/pickup/SKILL.md | 43 +++++++++++++++++----------------- 1 file changed, 21 insertions(+), 22 deletions(-) diff --git a/.claude/skills/pickup/SKILL.md b/.claude/skills/pickup/SKILL.md index 424ba03b07..44240ae048 100644 --- a/.claude/skills/pickup/SKILL.md +++ b/.claude/skills/pickup/SKILL.md @@ -42,8 +42,10 @@ gh pr list --label parked --json number,title,url,updatedAt ``` For each, read the manifest's `Trigger to pick up:`, snapshot the current source state (Step 1's -re-fetch only — don't do the full reconcile), then apply Step 2's trigger-met check to decide -**fired?**. Output a table: PR / trigger / **fired?** / one-line delta. Recommend which to pick up, which are still waiting, and which look abandoned (source +re-fetch only — don't do the full reconcile), then compare that state to the trigger condition to +decide **fired?**. Use only Step 2's *comparison* — **not** its stop-and-report rule: in scan mode +a not-met trigger is recorded as `fired?=no` and you continue to the next PR, never aborting the +loop. Output a table: PR / trigger / **fired?** / one-line delta. Recommend which to pick up, which are still waiting, and which look abandoned (source closed-unmerged, or long dead). Then stop — picking one up is Mode B. ## Mode B — pick up one PR (PR number given) @@ -86,26 +88,23 @@ Pickup **reconciles; it does not merge.** `/finalize` prepares a squash that ope > later amend. The squash sees only what has been pushed; anything left local is silently > dropped from the merge. -Run these steps in **exactly this order** — the ordering is load-bearing (notably `git rebase` -refuses to run on a dirty tree, so the commit must precede it), re-pushing whenever local -history changes: - -1. **Commit** the Step 4 reconciliation. Nothing downstream sees an uncommitted tree, and the - rebase in step 2 needs a clean one. -2. **Rebase / merge `main`** into the branch, then re-run link checks and confirm `relref` - targets still resolve. The tree is clean now, so the rebase proceeds; resolve any conflicts - here. -3. **`/reflect`** on the reconciliation commit — record *predicted-vs-actual*: what the park - snapshot got right, what the source changed (it closes the loop and calibrates future - preemptive docs). `/reflect` may fold its note into the commit or amend it. -4. **Push** the branch — and re-push after any amend in step 3. This makes the remote head match - local; skip it and the squash merges *without* the reconciliation and rebase — the gap that - kept reappearing here. -5. **`/finalize`** — the durable squash deferred at park time, now that the source has settled. - It reconciles the whole arc (park notes + pickup findings + any review) and prepares the - `gh pr merge … --body-file` command. -6. **Only now, remove** the `parked` and `do not merge yet` labels and strip the manifest block - from the PR body (or mark it resolved). The merge guard holds until `/finalize` is done. +The checklist below is the **intent** — *what must be true*, in this order — before the +`gh pr merge` hand-off. It is **not** a command script: achieve each state with normal git, minding +the edge cases of an already-pushed, rebased branch (a rebase needs a clean tree; pushing +rewritten history needs `--force-with-lease`). Stating intent rather than commands is deliberate — +enumerating the exact git steps here drifted out of date every review round. + +1. **Reconciliation committed**, the branch **up to date with `main`**, link checks and `relref` + targets re-verified. +2. **`/reflect`** recorded on that commit — *predicted-vs-actual*: what the park snapshot got + right, what the source changed (closes the loop; calibrates future preemptive docs). +3. **Remote head == local** — everything above pushed. This is the invariant made true; nothing + reaches the squash until it is. +4. **`/finalize`** run — the durable squash deferred at park time, now that the source has + settled; it reconciles the whole arc (park notes + pickup findings + any review) and prepares + the `gh pr merge … --body-file` command. +5. **Labels dropped last** — remove `parked` / `do not merge yet` and strip the manifest block + only after `/finalize`. The merge guard holds until then. Before handing off, **confirm the remote head matches local** (working tree clean, branch not ahead of `origin`). Then present: the delta report, the doc changes, the finalize output + the