diff --git a/.claude/commands/docs/assess-comments.md b/.claude/commands/docs/assess-comments.md index 3c3d30b84a..c398a83d8d 100644 --- a/.claude/commands/docs/assess-comments.md +++ b/.claude/commands/docs/assess-comments.md @@ -4,43 +4,32 @@ argument-hint: "[commenter-login] (optional — defaults to all commenters)" --- You're going to read **all** the comments on the pull request for the current -branch — from automation (Cursor Bugbot, the security scanner, the history bot, -the AI PR summary, CI) *and* from humans — and produce a single **reconciliation -report**: what each comment raises, where the tools **agree**, where they -**contradict each other**, and where they're chasing each other in circles. This -is the synthesis layer the individual tools don't have. - -**This command assesses and reports. It does not edit PR content, commit, push, -or reply** — the one and only file it may write is its own coverage ledger -(step 11). Fixing is a separate step. Note that `/docs:bugbot` only triages and -fixes **Cursor Bugbot's own** comments — so it's the right hand-off *only* for -bugbot-sourced clusters. Safe fixes that came from humans, Codex, the security -or history bots, etc. won't be touched by `/docs:bugbot`; apply those directly -(or ask me to) once you've seen the report. - -It's also meant to be run **repeatedly** as the PR evolves: the bots tend to post -fresh comments after each round of fixes, so each run is a checkpoint — verify -the few highest-impact things deeply, leave a "story so far", and re-run to pick -up the rest. Don't try to settle the whole PR in one pass. - -The command **tracks its own test coverage** in -`.claude/state/assess-comments.coverage.md`. Read it at the start of every run -(step 1) and update it at the end (step 11) — it's how the command knows, -honestly, which of its checks have actually fired on real data and which are -still unproven (ping-pong loop detection, notably). It's a *coverage* record, -not a correctness proof. - -If `$ARGUMENTS` is given, treat it as a commenter login and assess **only** that -author's comments (match case-insensitively; `cursor` matches `cursor[bot]`). -With no arguments, assess **every** commenter — bots and humans alike, because -human intent ("this is a CTF example", "deliberate UK spelling in a quote") is -exactly what lets you adjudicate the tool findings. - -1. **Load your coverage ledger, then find the branch and its PR.** First read - `.claude/state/assess-comments.coverage.md` so you know which of your own - checks are proven vs unproven this run (it also tells you to watch hardest for - the untested ones — ping-pong loops especially). Then run - `git branch --show-current`, and: +branch — from automation (Cursor Bugbot, the security scanner, Redis repo +memory, the AI PR summary, CI) *and* from humans — and produce a single +**reconciliation report**: what each comment raises, where the tools **agree**, +where they **contradict each other**, and where they're chasing each other in +circles. This is the synthesis layer the individual tools don't have. + +**This command assesses and reports. It does not edit PR content, commit, +push, or reply, and it writes nothing to disk.** Fixing is a separate step. +Note that `/docs:bugbot` only triages and fixes **Cursor Bugbot's own** +comments — so it's the right hand-off *only* for bugbot-sourced clusters. Safe +fixes that came from humans, Codex, the security bot, etc. won't be touched by +`/docs:bugbot`; apply those directly (or ask me to) once you've seen the +report. + +It's also meant to be run **repeatedly** as the PR evolves: the bots tend to +post fresh comments after each round of fixes, so each run is a checkpoint — +verify the few highest-impact things deeply, leave a "story so far", and +re-run to pick up the rest. Don't try to settle the whole PR in one pass. + +If `$ARGUMENTS` is given, treat it as a commenter login and assess **only** +that author's comments (match case-insensitively; `cursor` matches +`cursor[bot]`). With no arguments, assess **every** commenter — bots and +humans alike, because human intent ("this is a CTF example", "deliberate UK +spelling in a quote") is exactly what lets you adjudicate the tool findings. + +1. **Find the branch and its PR.** Run `git branch --show-current`, and: ``` gh pr view --json number,url,title,headRefName,body @@ -56,7 +45,8 @@ exactly what lets you adjudicate the tool findings. ``` gh api repos/{owner}/{repo}/pulls//comments --paginate ``` - - **Top-level PR / issue comments** (summaries, bot posts, human replies): + - **Top-level PR / issue comments** (summaries, bot posts, human replies, + Redis repo memory): ``` gh api repos/{owner}/{repo}/issues//comments --paginate ``` @@ -66,18 +56,19 @@ exactly what lets you adjudicate the tool findings. ``` - **Thread resolution state** — *don't skip this.* The REST `comments` endpoint's `position`/outdated field is **not** a reliable "addressed" - signal (a resolved thread routinely still reports `position` non-null). The - authoritative signal is the GraphQL `reviewThreads`: + signal (a resolved thread routinely still reports `position` non-null). + The authoritative signal is the GraphQL `reviewThreads`: ``` gh api graphql -F query='query { repository(owner:"", name:"") { pullRequest(number:) { reviewThreads(first:100) { nodes { isResolved isOutdated comments(first:100){ nodes { databaseId author{login} path body } } } } } } }' ``` - `first:100` covers any normal PR, but unlike the `--paginate`d REST calls it - does **not** page — so if a PR genuinely has >100 review threads (or a thread - with >100 comments), check `pageInfo.hasNextPage` and follow the cursor, or - you'll silently miss resolution state and skew the open/resolved split. + `first:100` covers any normal PR, but unlike the `--paginate`d REST calls + it does **not** page — so if a PR genuinely has >100 review threads (or a + thread with >100 comments), check `pageInfo.hasNextPage` and follow the + cursor, or you'll silently miss resolution state and skew the + open/resolved split. Use `databaseId` to map each thread back to the inline comments from the REST pull, so you know which are **resolved** vs **open**. @@ -85,218 +76,284 @@ exactly what lets you adjudicate the tool findings. For each comment capture: author `login`, `created_at`, the body, and for inline comments the `path`, line, and its thread's `isResolved` / `isOutdated`. If `$ARGUMENTS` was given, it narrows which comments become - **findings you cluster and adjudicate** (steps 4–7) to the matching author — - but **don't discard the others**. Always retain every review verdict and the - full set of open findings as *context*, because step 8 (approval-over-open- - finding) needs to see approvals and bot findings together; filtering them out - would silently disable that safety check. In filtered mode, run step 8 - against the full comment set, not the filtered one. - - If no comment **or review verdict** matches `$ARGUMENTS`, stop and tell me — a - reviewer who left only an approve/request-changes verdict still counts as a - match. + **findings you cluster and adjudicate** (steps 4–7) to the matching author + — but **don't discard the others**. Always retain every review verdict and + the full set of open findings as *context*, because step 8 + (approval-over-open-finding) needs to see approvals and bot findings + together; filtering them out would silently disable that safety check. In + filtered mode, run step 8 against the full comment set, not the filtered + one. + + If no comment **or review verdict** matches `$ARGUMENTS`, stop and tell me + — a reviewer who left only an approve/request-changes verdict still counts + as a match. 3. **Tag each comment _and review verdict_ by its source role.** You're reconciling *roles*, not usernames. Reviews are first-class here: a **request-changes** (or any review whose body raises specific, actionable points) is a *finding* from its author's role, not just an approve/reject signal for step 8 — tag and carry it forward like any comment. (A bare - approval with no actionable body stays a step-8 signal only.) Classify each - author as one of: - - - **bugbot** — author `login` is `cursor` / `cursor[bot]`. **Tag by author, - not body text:** a *human* reply that quotes or mentions "Bugbot" is still a - human comment — don't tag it bugbot (that would mis-route it to - `/docs:bugbot`). Only treat a body mention of "Bugbot" as a signal when the - author is some other bot relaying bugbot's output. + approval with no actionable body stays a step-8 signal only.) Classify + each author as one of: + + - **bugbot** — author `login` is `cursor` / `cursor[bot]`. **Tag by + author, not body text:** a *human* reply that quotes or mentions + "Bugbot" is still a human comment — don't tag it bugbot (that would + mis-route it to `/docs:bugbot`). Only treat a body mention of "Bugbot" as + a signal when the author is some other bot relaying bugbot's output. - **security** — the security scanner - - **history** — the commit-history / semantic-similarity bot + - **repo-memory** — the Redis repo memory bot (`github-actions[bot]` + comment containing the `` marker and a "🧠 + Redis Memory" heading). See step 3a — this is a distinct role from + plain CI, because it's a live pointer into project history, not a + status check. - **summary** — the AI PR summary (usually the PR body or a bot comment) - - **ci** — `github-actions` / build / staging-link bots + - **ci** — `github-actions` / build / staging-link bots (everything else + posted by automation, including the Jira-link and staging-link comments) - **human** — everyone else - Treat **summary**, **ci**, and staging-build links as *context*, not - findings — they tell you what the author intended and whether the build is - healthy. Don't raise them as issues. + Treat **summary** and plain **ci** (staging links, build status) as + *context*, not findings — they tell you what the author intended and + whether the build is healthy. Don't raise them as issues. + +3a. **Mine repo memory for prior art, cheaply first.** Redis repo memory's + comment renders each related item as a one-line title echo — that + rendered line is not useful. But the same comment carries a hidden + `` block at the bottom: decode + it (`base64 -d` the payload, it's a JSON array) to get, per item, a + `score` (lower = closer match), a `sourceUrl`, and a `bodySummary` + truncated to roughly 500 characters. You already fetched this comment in + step 2 — decoding costs nothing. + + - Use `score` to rank the related items; don't treat the comment's own + display order as relevance. + - For a `push_summary`/commit-typed item, the `bodySummary` preview is a + genuine slice of the real `/reflect`+`/finalize` message (verified: it + correctly led with the actual "verified X against the source rather + than the PR description" finding, not a title echo). It's often + enough on its own to judge relevance, and sometimes enough to cite. + - It is **truncated**, though — a real distillation easily runs to + 1,500+ characters and the cut can land mid-thought, before a second or + third finding. Don't treat the preview as complete. + + For the items whose score and preview show real overlap with a cluster + you're about to adjudicate (steps 4–7) — same file/subsystem, or the + same bot raising a similar-shaped claim — pull the **full** message + before citing it as precedent: + + - For a PR: `gh pr view --json state,mergedAt,mergeCommit` — if + merged, the squash commit at `mergeCommit.oid` carries the full + distillation. Fetch it with `git log -1 --format=%B ` (or + `gh api repos/{owner}/{repo}/commits/{oid} --jq .commit.message` if + the commit isn't in your local history). + - For a bare commit link: the same `git log -1 --format=%B ` — it + may be a smaller, less distilled message, so weight it accordingly. + + Don't do the full fetch for every related item indiscriminately — it + costs a network round-trip per item, on top of decoding the blob. Reach + for it only once the cheap preview says it's worth it. + + Use what you find as **precedent, cited by PR number**, in step 7's + adjudication — e.g. "PR #3604's squash message documents that bugbot's + concurrency heuristic over-fires on async fire-on-call ordering; today's + finding has the same shape, so treat it as low-confidence on that + precedent" (see the calibration patterns in step 7 for the specific, + durable axes worth watching for). A precedent is supporting evidence for + your own verification, not a substitute for it — still open the current + file and check. 4. **Cluster findings by what they touch, not by who said them.** Group the - actual findings — from bugbot, security, history, humans, **and actionable + actual findings — from bugbot, security, humans, **and actionable review-verdict bodies (step 3)** — by file + line, or by shared theme when - they're not line-specific. One cluster may hold comments from several tools - pointing at the same code — that overlap is the whole point. + they're not line-specific. One cluster may hold comments from several + tools pointing at the same code — that overlap is the whole point. -5. **Split clusters into OPEN and RESOLVED — and treat them differently.** Using - the resolution state from step 2, mark each cluster open or resolved. An - active PR is almost always a mix; don't let resolved threads drown the open - ones. +5. **Split clusters into OPEN and RESOLVED — and treat them differently.** + Using the resolution state from step 2, mark each cluster open or + resolved. An active PR is almost always a mix; don't let resolved threads + drown the open ones. - **Open clusters** are your real work — they go through prioritise → adjudicate (the steps below). - **Resolved clusters** are *not* re-litigated as fresh findings. But they aren't worthless either — do two cheap, high-value things with them: - - **Fix-quality spot-check.** For a sample of resolved findings (favour the - higher-impact ones), check that the committed change *actually addressed* - the point, not just silenced the comment — e.g. if a reviewer asked to - remove a term or fix a command, confirm it's genuinely gone/correct in - the current file. Flag any "fixed in name only." - - **"Resolved ≠ fixed" flag.** Call out any thread that is **resolved but - has no corresponding code change** (resolved, `isOutdated` false, and no - intervening commit touched the line). These are often legitimate - deferrals ("leaving as-is pending eng decision before GA") — but that's a - future obligation hiding inside a green checkmark, so surface it. - **Mandatory deep-verify:** any such thread that the bot rated **High (or - Critical) severity** must be opened and verified against the current code - *this round*, regardless of the depth cap in step 6 — a serious bug must - never hide behind a green checkmark just because the run hit its budget. - If it's genuinely fixed, say so; if it's resolved-but-still-present, - that's a headline. - - Resolved threads also feed **bot calibration**: the ratio of a bot's - findings that humans accepted-and-fixed vs dismissed tells you how much to - trust that bot's next *solo* finding. Note it when it's informative. + - **Fix-quality spot-check.** For a sample of resolved findings (favour + the higher-impact ones), check that the committed change *actually + addressed* the point, not just silenced the comment — e.g. if a + reviewer asked to remove a term or fix a command, confirm it's + genuinely gone/correct in the current file. Flag any "fixed in name + only." + - **"Resolved ≠ fixed" flag.** Call out any thread that is **resolved + but has no corresponding code change** (resolved, `isOutdated` false, + and no intervening commit touched the line). These are often + legitimate deferrals ("leaving as-is pending eng decision before GA") + — but that's a future obligation hiding inside a green checkmark, so + surface it. **Mandatory deep-verify:** any such thread that the bot + rated **High (or Critical) severity** must be opened and verified + against the current code *this round*, regardless of the depth cap in + step 6 — a serious bug must never hide behind a green checkmark just + because the run hit its budget. If it's genuinely fixed, say so; if + it's resolved-but-still-present, that's a headline. + - Resolved threads also feed **bot calibration** (step 7): the ratio of a + bot's findings that humans accepted-and-fixed vs dismissed tells you how + much to trust that bot's next *solo* finding this round. Note it when + it's informative — you don't need to persist it anywhere; if it matters + again, repo memory will surface this PR next time something similar + comes up, and the squash message will carry the same ratio in context. 6. **Prioritise, and cap how deep you go.** This command is meant to be run **repeatedly** over a PR's life — the bots re-comment after each round of fixes, so a single run is never the whole story. Don't try to verify every cluster to the bottom. Rank the **open** clusters by impact — broken - `relref`/`image`/`embed-md` paths, wrong commands or code samples, incorrect - technical claims, and anything that breaks a feature the PR description calls - out, rank highest; pure style nits lowest — and pick **at most the top 3–5** - to verify in depth this round. The rest you list shallowly (see step 9) as - "not assessed this round" so the next run can pick them up. - - Three things always make the deep-verify set regardless of the cap, because - they're the failures a reconciler exists to catch: **contradictions**, - **ping-pong loops**, and **resolved-but-not-outdated High/Critical findings** - (step 5). Everything else competes for the remaining slots. - - One assessment runs *outside* the cap entirely: **subsystem churn** (step 7). - Churn is a pattern across findings — including ones you deferred this round - and ones from prior rounds — so judge it against the **full** open+recent set, - never just the capped deep set. If a deferred cluster turns out to be part of - a churn pattern, pull it into deep-verify; the cap must not hide churn. + `relref`/`image`/`embed-md` paths, wrong commands or code samples, + incorrect technical claims, and anything that breaks a feature the PR + description calls out, rank highest; pure style nits lowest — and pick **at + most the top 3–5** to verify in depth this round. The rest you list + shallowly (see step 9) as "not assessed this round" so the next run can + pick them up. + + Three things always make the deep-verify set regardless of the cap, + because they're the failures a reconciler exists to catch: + **contradictions**, **ping-pong loops**, and **resolved-but-not-outdated + High/Critical findings** (step 5). Everything else competes for the + remaining slots. + + One assessment runs *outside* the cap entirely: **subsystem churn** (step + 7). Churn is a pattern across findings — including ones you deferred this + round and ones from prior rounds — so judge it against the **full** + open+recent set, never just the capped deep set. If a deferred cluster + turns out to be part of a churn pattern, pull it into deep-verify; the cap + must not hide churn. 7. **Adjudicate the chosen clusters — and verify, don't trust.** For each - cluster in your deep set, open the file it points at (and the underlying data - it depends on) and judge for yourself; these tools have false positives, and - a single tool can be *correct about the code but wrong about impact*, or - *plausible but refuted once you read the data*. Label each: - - - **Agreement** — multiple sources flag the same thing → high confidence it's - real. - - **Contradiction** — sources disagree, *or* a finding conflicts with stated - human intent or the PR summary (e.g. security flags a sample that a human - said is a deliberate CTF example; bugbot wants a refactor the history bot - says was reverted before). Call these out loudest — they're where blind - auto-fixing does damage. - - **Ping-pong** — a genuine loop is narrower than it looks. Using `created_at` - order and the commits in between, flag it **only** when *the same concern is - reopened* (a finding you thought fixed comes back), or *a fix for tool A's - comment re-triggered tool B in a cycle* (A→fix→B→fix→A…). It is **not** - ping-pong when a fix round simply prompts a re-scan that surfaces **new, - independent findings** — even if they land in the region you just edited. - That's normal iteration; treat those as fresh clusters, not a loop. (Real - example of the non-loop case in the coverage ledger's near-miss log.) When - it *is* a true loop, flag it rather than extending it. - - **Subsystem churn** — distinct from ping-pong, and the *earlier* warning - sign. Look across fix rounds: if several findings (even new, independent - ones) keep landing on the **same area of code you keep patching**, the - symptom isn't any single finding — it's that the subsystem is - under-specified and each patch just exposes the next adjacent gap. - Incrementally fixing the latest one tends to trigger another round. When you - see this, **recommend a single consolidated redesign of that subsystem - rather than another patch** — that's the move that actually ends the churn. - (Worked example in the ledger: PR #3536's review-handling findings across - rounds 1 and 4.) - - **Solo** — a single source, no corroboration → judge it on its merits and - say how confident you are. A solo finding you've *verified against the code - and data* is high-confidence even with one source. - - **Stale / already-addressed** — the code no longer matches what the comment - describes; a later commit already handled it. - - US-vs-UK spelling counts as a real issue in this docs repo even though it's - small; pure style nits usually don't. + cluster in your deep set, open the file it points at (and the underlying + data it depends on) and judge for yourself; these tools have false + positives, and a single tool can be *correct about the code but wrong + about impact*, or *plausible but refuted once you read the data*. Label + each: + + - **Agreement** — multiple sources flag the same thing → high confidence + it's real. + - **Contradiction** — sources disagree, *or* a finding conflicts with + stated human intent or the PR summary (e.g. security flags a sample + that a human said is a deliberate CTF example). Call these out + loudest — they're where blind auto-fixing does damage. + - **Ping-pong** — a genuine loop is narrower than it looks. Using + `created_at` order and the commits in between, flag it **only** when + *the same concern is reopened* (a finding you thought fixed comes back), + or *a fix for tool A's comment re-triggered tool B in a cycle* + (A→fix→B→fix→A…). It is **not** ping-pong when a fix round simply + prompts a re-scan that surfaces **new, independent findings** — even if + they land in the region you just edited. That's normal iteration; treat + those as fresh clusters, not a loop. When it *is* a true loop, flag it + rather than extending it. If repo memory surfaces an earlier PR on the + same file, check whether its squash message already describes a loop or + a settled fixed-point — that's the fastest way to tell which one you're + in. + - **Subsystem churn** — distinct from ping-pong, and the *earlier* + warning sign. Look across fix rounds: if several findings (even new, + independent ones) keep landing on the **same area of code you keep + patching**, the symptom isn't any single finding — it's that the + subsystem is under-specified and each patch just exposes the next + adjacent gap. Incrementally fixing the latest one tends to trigger + another round. When you see this, **recommend a single consolidated + redesign of that subsystem rather than another patch** — that's the + move that actually ends the churn. If a repo-memory-linked PR's squash + message already names this subsystem's churn history, cite it — it + tells you how many rounds it took last time and what finally ended it. + - **Solo** — a single source, no corroboration → judge it on its merits + and say how confident you are. A solo finding you've *verified against + the code and data* is high-confidence even with one source. + - **Stale / already-addressed** — the code no longer matches what the + comment describes; a later commit already handled it. + + US-vs-UK spelling counts as a real issue in this docs repo even though + it's small; pure style nits usually don't. + + **Known bugbot calibration patterns** (durable across many PRs — not tied + to any one of them, so kept here rather than relying on repo memory to + resurface them): bugbot is strong on **pure language/API semantics** it + can evaluate from the source alone (e.g. `Double.MIN_VALUE` is the + smallest *positive* double, not `-Infinity`), and on **cross-file + doc-consistency / procedure-completeness** (two files in a PR that must + agree by construction). It's unreliable on **runtime mechanics invisible + in the diff**: async dispatch order (a `CompletableFuture`/`allOf` "race" + is often already ordered by call-site on one connection), npm/package + transitive-dependency hoisting (a metapackage's declared deps land + hoisted-flat, so "missing dependency" claims from import analysis alone + are often wrong), list-vs-single-item REST endpoint shape (a field present + on `GET /pulls/{n}` may be absent from the `GET /pulls` list shape bugbot + generalized from), and test-marker stripping the build performs + independently of `REMOVE` regions. It can also cite a **stale PR + description** as ground truth rather than checking whether the + description or the code is the side that's out of date. When a finding + falls in one of these blind spots, verify against the actual + runtime/build behavior before trusting it. 8. **Cross-check review verdicts against open findings.** Separately from the clusters, line up the PR's **review verdicts** (approve / request-changes) - against the findings still open. Flag the dangerous combination explicitly: - **a human approval (especially a low-confidence one — "skimmed it", "LGTM", - "didn't review in detail") sitting on top of an unresolved bot finding.** - Neither the reviewer nor the bot is wrong alone, but together they can let a - real issue merge — exactly the gap a reconciler should catch. Surface it even - when the PR is marked WIP / don't-merge. + against the findings still open. Flag the dangerous combination + explicitly: **a human approval (especially a low-confidence one — + "skimmed it", "LGTM", "didn't review in detail") sitting on top of an + unresolved bot finding.** Neither the reviewer nor the bot is wrong alone, + but together they can let a real issue merge — exactly the gap a + reconciler should catch. Surface it even when the PR is marked WIP / + don't-merge. 9. **Produce the reconciliation report.** One row per **open cluster you deep-verified this round** (step 7) — those are the only clusters with a - real verdict. Open clusters you deferred under the step-6 cap do **not** get a - table row or a verdict (you haven't verified them); they're listed, plainly - marked unverified, in the "story so far" below. The table never promises more - coverage than you actually did. + real verdict. Open clusters you deferred under the step-6 cap do **not** + get a table row or a verdict (you haven't verified them); they're listed, + plainly marked unverified, in the "story so far" below. The table never + promises more coverage than you actually did. | Cluster | File / line | Sources | Verdict | Recommended action | |---|---|---|---|---| - - **Verdict**: agreement / contradiction / ping-pong / churn / solo / stale. - - **Recommended action**: one of — *safe to fix* (uncontested and low-risk; - hand **bugbot-sourced** clusters to `/docs:bugbot`, but apply safe fixes - from any other source — human, Codex, security, history — directly, since - `/docs:bugbot` won't touch those); *needs your call* (contradiction, - approval-over-finding, or history-bot warning); *dismiss* (false positive — - say why); *already handled* (stale). + - **Verdict**: agreement / contradiction / ping-pong / churn / solo / + stale. Where a repo-memory-linked squash message informed the verdict, + cite the PR number inline (e.g. "solo, but matches #3604's precedent"). + - **Recommended action**: one of — *safe to fix* (uncontested and + low-risk; hand **bugbot-sourced** clusters to `/docs:bugbot`, but apply + safe fixes from any other source — human, Codex, security — directly, + since `/docs:bugbot` won't touch those); *needs your call* (contradiction, + approval-over-finding, or a serious repo-memory-sourced precedent); + *dismiss* (false positive — say why); *already handled* (stale). Then, in this order: - - the **headline** — contradictions, ping-pong loops, **subsystem churn (with - its consolidate-don't-patch recommendation)**, and any approval-over-open- - finding from step 8 (these need a human) first; + - the **headline** — contradictions, ping-pong loops, **subsystem churn + (with its consolidate-don't-patch recommendation)**, and any + approval-over-open-finding from step 8 (these need a human) first; - the **safe-to-fix** list; - - what you're **dismissing** and why, so I can sanity-check your reasoning; - - a short **resolved-threads** note (from step 5) — only what's worth saying: - any "fixed in name only" or "resolved ≠ fixed / deferred" flags, and a bot- - calibration line if it's informative. Don't list cleanly-resolved threads - one by one; a count is enough; - - a **"Story so far"** line: which open clusters you deep-verified this round, - how many you deferred as "not assessed this round" (list them by one-line - title), how many threads were already resolved, and a reminder that - re-running after the next round of fixes will pick up the deferred ones plus - any new comments. This is a checkpoint, not a verdict on the whole PR. - -10. **Offer a second opinion via Codex, only if it's available.** Check whether - the Codex CLI is installed — run `command -v codex`. If it is, end by - suggesting the user get an independent review of the same diff the bots saw: + - what you're **dismissing** and why, so I can sanity-check your + reasoning; + - a short **resolved-threads** note (from step 5) — only what's worth + saying: any "fixed in name only" or "resolved ≠ fixed / deferred" + flags, and a bot-calibration line if it's informative. Don't list + cleanly-resolved threads one by one; a count is enough; + - a **"Story so far"** line: which open clusters you deep-verified this + round, how many you deferred as "not assessed this round" (list them by + one-line title), how many threads were already resolved, which + repo-memory-linked PRs you actually opened and used as precedent (by + number, so I can follow the trail), and a reminder that re-running after + the next round of fixes will pick up the deferred ones plus any new + comments. This is a checkpoint, not a verdict on the whole PR. + +10. **Offer a second opinion via Codex, only if it's available.** Check + whether the Codex CLI is installed — run `command -v codex`. If it is, end + by suggesting the user get an independent review of the same diff the bots + saw: ``` /codex:review --base main --scope branch ``` - Frame it as optional and note it adds runtime, and that it's worth it mainly - for the contested or high-impact clusters — the ones where a second model - disagreeing (or agreeing) changes your confidence. Don't run it yourself: - `/codex:review` is user-invoked only, and this command is report-only. If - `command -v codex` finds nothing, say nothing about Codex. - -11. **Update your coverage ledger — automatically, but evidence-gated.** Open - `.claude/state/assess-comments.coverage.md` and update it to reflect what - *actually fired on real data this run*: - - - For each capability you genuinely exercised, bump its **encounters**, set - **last verified** to today, and add this PR (with a comment id or commit - SHA) to **Evidence**. Raise confidence per the scale — but **never raise a - confidence without a citable artifact.** No evidence, no bump. - - A capability counts as **🟢 corroborated** only on ≥2 *distinct* PRs, or - once with an independent `/codex:review` concurrence on the same finding. - - If you hit a **rare pattern for the first time** — a real ping-pong loop, a - resolved-but-still-broken thread — record its concrete signature in the - **Worked examples library** (tools involved, comment ids, commits). This is - how detection sharpens over time. - - Refresh ⏳ **decaying** entries you re-confirmed this run. - - If the run revealed that an *instruction* in this command should change - (a heuristic nearly missed something), **don't edit the command yourself** - — note the suggested refinement in your output and let me decide. - - The ledger update is the one file you *may* write. After writing it, **tell - me plainly that the command learned something and updated its own coverage - ledger**, name what changed in one line, and remind me I can include that - change in the PR or leave it out — my call. - -12. **Stop there.** Don't edit anything except the coverage ledger (step 11); - don't commit, push, or reply to the PR. Your output is the assessment — - I'll decide what to act on. + Frame it as optional and note it adds runtime, and that it's worth it + mainly for the contested or high-impact clusters — the ones where a + second model disagreeing (or agreeing) changes your confidence. Don't run + it yourself: `/codex:review` is user-invoked only, and this command is + report-only. If `command -v codex` finds nothing, say nothing about Codex. + +11. **Stop there.** Don't edit, write, or delete anything; don't commit, + push, or reply to the PR. Your output is the assessment — I'll decide + what to act on. diff --git a/.claude/state/assess-comments.coverage.md b/.claude/state/assess-comments.coverage.md deleted file mode 100644 index 6054089bb9..0000000000 --- a/.claude/state/assess-comments.coverage.md +++ /dev/null @@ -1,381 +0,0 @@ -# assess-comments — coverage ledger - -This file is the self-tracked **test coverage** of the `/docs:assess-comments` -command. It records, per capability, how confident we are that the logic has -actually *fired correctly on real data* — not how accurate its judgement is. - -**Read this honestly:** this is a *coverage* tracker, not a correctness proof. -It can only record capabilities it has *seen fire* (positive cases). It cannot -see false negatives — e.g. a ping-pong loop that was present but missed — so a -🟢 means "exercised on real PRs", never "guaranteed correct". - -The command reads this at the start of every run and updates it at the end -(step 1 and step 11). Updates are **automatic but evidence-gated**: no -confidence may rise without a citable artifact (PR number + comment id / commit -SHA). When the command updates this file it tells the user, who can choose -whether to commit the change. - -## Confidence scale -- ❓ **untested** — logic is written but has never fired on real data. -- 🟡 **seen once** — fired correctly on a single real PR. -- 🟢 **corroborated** — fired correctly on ≥2 *distinct* PRs, **or** once with an - independent `/codex:review` concurrence on the same finding. -- ⏳ **decaying** — was 🟡/🟢 but not re-confirmed in a long time (treat the API - shape, bot identities, and repo conventions as possibly drifted; re-verify). - -## Capabilities - -| Capability | Confidence | Real encounters | Last verified | Evidence | -|---|---|---|---|---| - -| Branch/PR identification + arg handling | 🟢 corroborated | 13 | 2026-07-23 | #3415, #3507, #3374, #3536, #3542, #3543, #3573, #2531, #3585, #3604, #3612, #3627, #3687 | -| Multi-source collection (inline + top-level + reviews) | 🟢 corroborated | 14 | 2026-07-23 | #3415, #3507, #3510, #3374, #3536, #3542, #3543, #3573, #2531, #3585, #3604, #3612, #3627, #3687 | -| 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). **#3687: GraphQL unavailable** (no `gh` CLI in this environment — `gh api graphql` requires the `gh` CLI, which is absent; curl returned 401 because no token was found). Fell back to context-based inference (CHANGES_REQUESTED + no post-review commits → all 9 threads open). Known failure mode: when `gh` is absent, resolution state is inferred, not confirmed. | -| Source-role tagging (bugbot/security/history/summary/ci/human) | 🟢 corroborated | 12 | 2026-07-23 | #3415, #3507, #3374, #3536, #3542, #3543, #3573, #2531, #3585, #3604, #3612, #3627, #3687 (yoavredis=human CHANGES_REQUESTED; github-actions=ci Jira+staging; no bots) | -| Open/resolved split | 🟢 corroborated | 11 | 2026-07-23 | #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), #3627 (0 res/1 open), #3687 (9 open/0 resolved — inferred from context, GraphQL unavailable) | - -| 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`) | -| **Contradiction** detection | 🟢 corroborated | 5 | 2026-07-28 | #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); #3627 (**new flavour: tool-finding vs verified-harness-reality + author design-intent** — bugbot 3587084676 "auth2 needs missing ACL user" claims the AUTH examples "break on the same environment the harness uses elsewhere"; verified `build/example-test-harness/run.sh` has **zero** `cmds_cnxmgmt` entries so the harness never runs them, and they are illustrative-by-design [matching the pre-existing shipped redis_py/node auth examples using the same `temp_pass`/`test-user` creds]. bugbot flagged only auth2 though auth1 has the identical property — a tell it's about call-shape not a runtime contract); #3714 (**new flavour: tool-finding vs harness-reality where the finding proposes the WRONG fix direction** — bugbot 3665551935 "Ruby incorrectly excluded from hexpire/hpexpire" argues redis-rb "supports HEXPIRE/HTTL/HPEXPIRE/HPTTL" so re-add it; harness proved released redis-rb 5.4.1 has NO native methods [`hexpire` → method_missing → no `FIELDS` → `ERR wrong number of arguments`], so exclusion is correct. bugbot also read the **stale PR body** [said only hexpireat omits Ruby] as ground truth. Correct reconciler move: fix the *description*, not the code). *(#3507 was an off-branch manual demo — not counted.)* | -| **Ping-pong loop** detection | ❓ untested | 0 | 2026-07-28 | 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. #3627 (r1) had a single bugbot finding on the PR's first review — no prior fix to undo, so no loop possible. #3714 (r1) same shape — single bugbot finding on first review, no prior fix to undo → no loop possible | -| **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 | 9 | 2026-07-23 | #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); #3627 (correct **negative** — 1 open bugbot finding, only cursor COMMENTED verdict, no human approval); #3687 (correct **negative** — CHANGES_REQUESTED only, no human approval over open findings) | - -| 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 | 10 | 2026-07-28 | #3714 (**0 valid / 1 FP-on-facts**: bugbot 3665551935 argued redis-rb "supports HEXPIRE/HTTL/HPEXPIRE/HPTTL" so Ruby should be re-added to the hexpire/hpexpire filters — refuted by the harness [released redis-rb 5.4.1 has no native methods; method_missing omits `FIELDS` → `ERR wrong number of arguments`]. **Same calibration axis** as #3604/#3612/#3627: bugbot mis-models runtime/environment it can't see from the diff [here: released-gem API surface] and trusted the **stale PR description** as ground truth); prior: #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); #3627 (**0 valid / 1 FP-on-impact**: bugbot 3587084676 correct on the literal [`AUTH test-user` errors on a bare Redis] but wrong on impact [claimed the harness runs these; it does not — no `cmds_cnxmgmt` in run.sh]. **Same calibration axis**: bugbot mis-models the runtime/environment it can't see from the diff — here that the auth set is illustrative-only and unwired from the harness, exactly the async/npm/endpoint-shape blind spot from #3604/#3612) | -| Codex second-opinion availability gate | 🟢 corroborated | 11 | 2026-07-28 | #3415, #3374 (CLI on PATH; #3374 had a real Codex review), #3542, #3543, #3573, #2531, #3585, #3604, #3612, #3627, #3714 (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 - -Concrete real-world signatures, so detection sharpens over time. Add to this -whenever a rare pattern is seen for the first time. - -### Ping-pong loops -*(no true loop observed yet — when the first real one appears, record the tool -sequence, the comment ids, and the commits between them here)* - -**Near-miss (NOT a loop) — #3536, 2026-06-23.** Round-1 bugbot findings (ids -3460160413, 3460160429 on commit `17517ed`) were fixed in commit `da7c04a`; -bugbot re-reviewed and raised 3 *new* findings (ids 3460820452/470/474). One new -finding (GraphQL pagination, `assess-comments.md:72-75`) sat in the same step I'd -just edited — superficially loop-shaped. But the new findings were independent, -pre-existing issues exposed by the file changing, **not** the same spot toggled -back and forth or a fix being undone. Lesson for the detector: "fix → re-scan → -new findings in an edited region" is normal iteration, **not** ping-pong. A true -loop needs the *same concern* reopened, or tool A's fix re-triggering tool B in a -cycle. - -*Confirmed convergence (round 3):* the fixes for those 3 new findings were -pushed, and bugbot's next re-scan came back **clean — no comments**. A real loop -would have spawned another round; this settled. So the "not a loop" judgement is -borne out by what happened next: assess → fix → re-scan reached a fixed point. - -**Near-miss (NOT a loop) — #3542, 2026-07-01.** Round-1 bugbot High #3467309496 -(ARG_MAX / `|| true` silent-green) was fixed (commit `21f079e1d`: xargs + -zero-URL `exit 1` guard). Round-2 re-scan raised Medium #3498159511 ("empty scope -exits successfully") — the *sibling* zero-files `exit 0` path the new guard didn't -cover. Superficially loop-shaped (same file, same "silent green" theme), but it's -a new adjacent gap, not the same concern reopened → correctly judged churn, not -ping-pong. - -### Ledger duplication / conflict after a `main` merge (shared-file hazard) -- **#2531 — 2026-06-30, and again 2026-07-02.** The coverage ledger is a single shared - file edited by *every* assess-comments run across all branches, so it repeatedly - collides on merges. (1) 2026-06-30: a #3573 run edited it on `main` while my #2531 run - edited the branch from the same #3543 ancestor; merging `main` (commit `3007dfb3`) - kept **both** blocks, duplicating nearly every capability with conflicting confidence. - Bugbot caught it (finding `3499796857`, Low). (2) 2026-07-02: merging `main` again - produced a real git **conflict** because #3542/#3573 runs had since edited the same - rows — resolved by union-merging evidence per capability. **Suggested step-11 - refinement:** reconcile *by capability name* (one row each; merge evidence) and - re-dedupe/re-merge after any `main` merge — concurrent runs on different branches - keep colliding in this shared file. - -### Resolved-but-still-broken -*(none of the classic "resolved but bug never fixed" variant confirmed yet)* - -**Reverted-after-resolve (regression) — #2531, 2026-06-30.** Thread `2619563419` -(ZdravkoDonev-redis, Dec 2025) stated RDI's decimal default is -`debezium.source.decimal.handling.mode=string`; it was **resolved + marked -outdated**. A June 2026 full rewrite of the page (commit `43e02c1d0`) then changed -the documented default to `precise` and added a `TODO` questioning it — i.e. it -**reverted the agreed, resolved fix**. The engineer had to re-raise the identical -point as a fresh finding (`3496835587`). Signature: a *resolved+outdated* thread -whose agreed resolution is no longer reflected in current code because a later -large edit overwrote it. Lesson for the detector: when spot-checking resolved -threads, don't assume "resolved+outdated" means the fix still holds — a big rewrite -can silently undo it. This is a **regression**, distinct from both ping-pong (no -A↔B cycle) and a never-fixed thread (it *was* fixed, then un-fixed). - -### Cross-source contradiction (authoritative human vs author's own prior work / other docs) -- **#2531 RDI engineer vs Debezium-docs basis, 2026-06-30.** The page's mappings - were built from the Debezium 3.0 reference because RDI internals were non-public. - RDI engineer ZdravkoDonev then supplied repo ground truth contradicting it on - ≥4 points: shipped Debezium is `3.5.0.Final-rdi.3` not 3.0.8 (`3496835571`); - decimal default is `string` not `precise` (`3496835587`); RDI *normalizes* - temporal logical types rather than passing Debezium values through (`3496835595`); - RDI maps both `mysql` and `mariadb` to `MySqlConnector`, no separate MariaDB - connector (`3496835604`). Additionally an **doc-vs-doc** contradiction: the - engineer's temporal-normalization examples disagree with the published - `formatting-date-and-time-values.md` (Debezium `Date` → ms per engineer vs → days - per the doc). Lesson: a finding can be authoritative-human-correct yet contradict - *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 -line). Opened the current file anyway — the xargs fix + `count==0 → exit 1` guard -are genuinely present (committed `21f079e1d`). So this was resolved-*and*-fixed, -not the still-broken variant. Recorded here as the negative case that proves the -rule fires and distinguishes fixed from still-broken. - -### Subsystem churn (not a loop, but the precursor) -- **#3536 review-handling, 2026-06-23.** Bugbot finding 429 (round 1) flagged the - `$ARGUMENTS` filter dropping cross-check data; it was patched. Two rounds later - bugbot raised 862 ("filter abort ignores reviews") and 874 ("review bodies skip - clustering") — *new, independent* findings (so not ping-pong), but all three - cluster on the **same under-specified subsystem**: how review verdicts flow - through collect → filter → tag → cluster → abort. Root cause: reviews were - second-class vs comments. Signature: each incremental patch exposed the next - adjacent gap. Response that ended it: one **consolidated** fix promoting reviews - to first-class across steps 2–4, rather than patching 862/874 individually. -- **#3536 churn-on-the-churn-feature (round 5), 2026-06-23.** The *fix* for the - case above added "subsystem churn" to step 7 — but only there. Round 5 bugbot - raised 442 ("churn detection capped incorrectly" — gated behind step 6's depth - cap) and 449 ("report verdict omits churn" — missing from step 9's verdict list - and headline). Same signature as any churn: a feature added in one place, - adjacent integration gaps exposed next round. Cured by threading churn through - steps 6 and 9 in one pass. Lesson: adding a capability *is* a subsystem edit — - wire it through every dependent step at once, or it becomes its own churn. -- **#3536 churn persisted — consolidation scoped too narrowly (round 6), - 2026-06-23.** The round-5 "consolidated" fix threaded the *churn feature* - through steps 6 & 9 but left the deeper contract under them unfixed, so round 6 - raised finding 3461052859 ("open table needs full verdicts": step 9 promised a - verdict per open cluster while step 6 only verifies 3–5). Same area a third - time. Real root: the **cap ↔ report-completeness contract**, not the churn - feature. Fixed by making the table cover only deep-verified clusters and - marking deferred ones unverified. Meta-lesson: when round N+1 finds another gap - in an area you *just* "consolidated", your consolidation boundary was wrong — - widen it to the true subsystem, don't re-patch the edge. -- **#3542 extraction "fail-loud-on-empty" contract, 2026-07-01.** 2nd distinct PR - showing the pattern (promotes churn detection to 🟢). Round-1 bugbot High - #3467309496 flagged `grep "${files[@]}"` + `|| true` as a silent-green path - (extraction failure → empty output → lychee passes checking nothing). Fixed - (commit `21f079e1d`) with xargs streaming + a `count==0 → exit 1` guard — but - the guard only covered the *zero-URLs-from-found-files* path. Round-2 bugbot - Medium #3498159511 then flagged the sibling *zero-files-found* branch, which - still does `: > "$OUTPUT_FILE"; exit 0` (silent green on a broken/mis-scoped - build). Same under-specified contract ("degenerate extraction must fail loudly"), - each patch exposing the adjacent path. Signature identical to #3536. Recommended - consolidation: enforce one invariant — any empty `external-urls.txt` in CI fails - — in a single place, rather than flipping just the zero-files branch and inviting - a round-3 finding on a third degenerate path. - -- **#3573 `--add` virtual-merge, 2026-06-30 (2nd PR → corroborates the pattern).** - Round 1 bugbot #3499137529 flagged `--add` duplicating a page that's also on - disk; fixed by an existence-check in `build_rows`. Round-2 re-scan then raised - #3499226329: `--add` into a `--collapse`d folder is silently dropped (virtual - entries merge only when `walk` recurses, which collapsed folders skip). *New, - independent finding* (not ping-pong) but the **same under-specified subsystem** - — how `--add` virtual entries reconcile with the on-disk walk and collapse - state. Signature again: each point-patch exposed the next adjacent gap. - Consolidation that would end it: resolve all `--add` entries against the final - tree in one place — validate the parent is a real, non-collapsed, walked dir - and **warn instead of silently dropping**, and dedupe vs disk there too — - rather than threading a `virtual` dict through the recursion. Borderline - (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. - -### Approval-over-open-finding — benign variants - -- **#3312 dwdougherty, 2026-05-12.** Review state `COMMENTED` (not `APPROVED`) - with body *"Apart from the issues already identified by Bugbot, LGTM."* sat - over 3 open bugbot findings. **Explicit carveout** — the human deliberately - excluded the bot's findings from the LGTM. Over the following months, 1 - finding (3228632453 raw-md → relref) was substantively fixed by the author and - 2 (3228632434 streaming.md empty page; 3228632444 manage-workspace.md empty - frontmatter) became *stale via the broader restructure* (streaming.md - deleted; manage-workspace.md fully populated with frontmatter). Pattern: the - approval-over-open-finding signal *did* fire, but the carveout language made - it benign and the open findings were later defanged by unrelated work. Distinct - from the dangerous variant where an "LGTM" silently rides over a real bug. - -### 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. - **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. - **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. - **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 - 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. - -- **#3714 "Ruby incorrectly excluded" — finding refuted by harness + reads stale PR body, 2026-07-28 (5th FP flavour; the "wrong fix direction" case).** - Bugbot Medium `3665551935` (hashes.md L164-165, L179-180): the `hexpire`/`hpexpire` `lang_filter`s exclude Ruby, but "per the PR description only `hexpireat` needs to omit Ruby" and "Ruby supports `HEXPIRE`/`HTTL`/`HPEXPIRE`/`HPTTL`, so it belongs in the first two filters" → re-add Ruby + add Ruby steps to `dt_hash.rb`. **Refuted:** the harness run this session proved released **redis-rb 5.4.1 has no native methods** for any of these — `r.hexpire(...)` falls through `Redis::Commands#method_missing` and emits `HEXPIRE key 60 f1 f2` *without* the mandatory `FIELDS ` token → `ERR wrong number of arguments`. Ruby was deliberately dropped from **all three** blocks in commit `a4528e2f4`. Two blind spots stacked: (1) bugbot trusted the **API surface** (the repo's own `data/command-api-mapping/HEXPIRE.json` even lists `redis_rb` — the aspirational-mapping trap), and (2) it treated the **stale PR body** (written pre-harness, said only hexpireat omits Ruby) as ground truth. Same root axis as #3604/#3612/#3627 — a runtime/environment fact invisible in the diff. **Distinctive reconciler lesson:** the finding *correctly detected* a description↔code inconsistency but proposed the **wrong resolution direction** (change the code). The right move is to **fix the stale PR description**, not the harness-verified code. When a finding cites "the PR description says…", check whether the description or the code is the stale side before acting. - -### 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.