fix(tooling): stop two silent false signals, in test selection and in bootstrap health - #3487
Conversation
…lure `claude plugin install --scope user -y` already leaves the plugin enabled, so the refresh chain's tail `plugin enable` exits 1 with "already enabled at user scope" on the healthy path. The three steps were joined with `&&`, so every successful refresh scored as a failure: the startup line read "65 failed" and "54 failed" for a registry that was verifiably installed, enabled and at HEAD. The failure count is the bootstrap's only health signal, and dozens of false alarms per session start buried it. The three subcommand exit statuses are now advisory. Verification instead reads the end state once per run, over every plugin `enabledPlugins` turns on, and counts a plugin failed only when it is absent at user scope, present there without `enabled: true`, or its own directory under `plugins/` changed between the recorded `gitCommitSha` and HEAD. Cases where the snapshot cannot be determined at all (no resolvable HEAD, no registry, absent or null recorded sha, a commit this clone lacks) fail closed with a named reason rather than reading as healthy. An unreadable `plugin list --json` fails the batch in one line instead of one warning per plugin. Verified live on this machine: 72 enabled, 0 newly installed, 0 refreshed, 0 failed, where the previous code reported every refresh as failed. Adds `.claude/hooks/cloud-bootstrap-plugins.test.sh`, 32 assertions driven by a stub CLI over synthetic registries, discovered by the existing `find plugins .claude/hooks` roots with no ci.yml change. It pins the five end states that previously reported "0 failed", the extraction anchors it depends on, and the block's exit status, since a nonzero exit there would abort the whole bootstrap under `set -e`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XGLX1xYgy27JiRqLjoiH8T
…d basename match R3 and R4 matched a changed file's basename as an unanchored substring of other files, so a file with no suite of its own could still select many suites and exit 0. That is not the safe over-selection the header describes, it is fail-open: the "a changed file that maps to zero suites is an error" contract silently did not apply. Measured on this tree, a new uncovered hook body (`utils.sh` under a plugin's hooks/) selected 131 suites at exit 0; it is now reported UNMAPPED at exit 1. The same hazard is what let `babysit_lease.py` report five suites, none of them its own, purely because its name is a substring of `manage_babysit_lease.py`. A file now names another only when the basename appears bounded on both sides by a character outside [A-Za-z0-9_.-]. `/` is deliberately outside that class, so path-qualified mentions, prose mentions and comment mentions all still count, and a trailing run of `.` is treated as sentence punctuation. A basename the rule cannot spell falls back to the old substring test rather than to zero coverage. Matching stays a basename rule because the cross-plugin copy fan-out depends on it. Exposing the false coverage revealed a real gap it had been masking: several Python files whose only suite is `<dir>/tests/test_<stem>.py` were never reachable by a name match, because those suites say `import <module>` and never spell the filename. R2 gained that path arm, and four files that were UNMAPPED now map to the suites that genuinely test them. Swept over every tracked file: 534 files select fewer suites (15.8% fewer selected-suite slots), 4 UNMAPPED files became mapped, nothing became unmapped. 25 files dropped to an empty selection, every one a markdown context file whose basename had been landing inside a longer one, and every one already covered by a class in scripts/affected-tests-no-suite.txt, so they report as no-suite at exit 0. Independently verified: all 190 dropped basename/enclosing-token pairs were read, only two name the real file (both keep their suites through other bounded mentions), and a direct-coverage invariant over all 3455 tracked files found no file missing a suite that genuinely names it. Selection also got faster, since the frontier is smaller. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XGLX1xYgy27JiRqLjoiH8T
|
Claude finished @kyle-sexton's task in 6m 42s —— View job CI code review — PR #3487
Scope reviewed: No blocking correctness or maintainability issues found. Notes from the review:
Verified by static reading, not execution: this lane's Bash access is restricted to |
|
Claude finished @kyle-sexton's task in 1m 41s —— View job Security review —
|
|
Last security-reviewed head: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cedd4cb241
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
The boundary rule put `-` inside the token class, so the shell default-value
operator glues onto the name it precedes: `"${TARGET:-<name>.sh}"` tokenizes to
`-<name>.sh`, and the reverse lookup never reaches the dependent that spells it
that way.
This one does not fail loud. When the referenced file has a co-located suite of
its own it stays MAPPED and the run exits 0, while the dependent's suite is
silently dropped, which is the same silent under-selection this change set
exists to remove. Found by a review bot on the pull request and reproduced
before fixing.
The existing leading-dot strip widens to `[-+=?.]`, covering the whole
`${V:-x}` / `${V:+x}` / `${V:=x}` / `${V:?x}` family. Only `-` is load-bearing:
the other three already fall outside the token class, so awk's split separates
them today. They are stripped anyway so the two sets cannot drift apart if the
class is ever widened. The strip runs after the exact and trailing-dot tests,
so it only ever adds matches, and every pair it admits is one the pre-token
substring rule already served. Measured on the corpus, it re-admits exactly two
pairs and changes no live selection.
The trailing side is unchanged and cannot be fixed the same way: in
`<name>.sh-shaped` the whole token carries the suffix, so no leading strip
reaches it. A file mentioned only that way still reports UNMAPPED at exit 1,
which is the loud direction. The header now states that asymmetry as forced
rather than chosen.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XGLX1xYgy27JiRqLjoiH8T
|
Claude finished @kyle-sexton's task in 2m 53s —— View job Code review — PR #3487
Scope of this pass: the lane's prior review already covered Verdict: the fix is correct. Traced the
Not independently executed: this lane's Bash access did not permit running No findings to report — no inline comments filed. |
|
Claude finished @kyle-sexton's task in 46s —— View job Security review —
|
2 participants
No linked issue
Summary
Two independent false signals found by an evidence-first improvement scan, both of the same shape: a check that reported healthy when it was not. Each is fixed and each now has a test that fails without the fix.
scripts/affected-tests.shclaimed coverage a file did not have, because R3/R4 matched a basename as an unanchored substring. A file with no suite of its own could select many suites and exit 0, so the repo's "a changed file that maps to zero suites is an error" contract silently did not apply. This is the gap class behind the kindle-dedrm firewall defect (fix(kindle-dedrm): compare firewall rule state against 'True', not truthiness #3396)..claude/cloud-bootstrap.shreported dozens of failures per session start for a perfectly healthy plugin registry, becauseclaude plugin install --scope user -yalready leaves a plugin enabled, so the chain's tailplugin enableexits 1 with "already enabled at user scope" and the three steps were joined with&&. The failure count is the bootstrap's only health signal, and the false alarms buried it.Fix
Test selection. A file now names another only when the basename appears bounded on both sides by a character outside
[A-Za-z0-9_.-]./is deliberately outside that class, so path-qualified, prose and comment mentions all still count; a trailing run of.is sentence punctuation; a basename the rule cannot spell falls back to the old substring test rather than to zero coverage. Matching stays a basename rule because the cross-plugin copy fan-out depends on it. Exposing the false coverage revealed a real gap it had been masking: several Python files whose only suite is<dir>/tests/test_<stem>.pywere unreachable by any name match, since those suitesimport <module>and never spell the filename, so R2 gained that path arm.Bootstrap health. The three subcommand exit statuses are now advisory. Verification reads the end state once per run, over every plugin
enabledPluginsturns on, and counts a plugin failed only when it is absent at user scope, present withoutenabled: true, or its own directory underplugins/changed between the recordedgitCommitShaand HEAD. Cases where the snapshot cannot be determined at all (no resolvable HEAD, no registry, absent or null recorded sha, a commit this clone lacks) fail closed with a named reason rather than reading as healthy. An unreadableplugin list --jsonfails the batch in one line instead of one warning per plugin.Verification
Measured, not asserted:
affected-tests-no-suite.txt, so they report as no-suite at exit 0.72 enabled, 0 newly installed, 0 refreshed, 0 failed, where the previous code reported every refresh as failed.affected-tests.test.sh56 assertions green (47 before); new.claude/hooks/cloud-bootstrap-plugins.test.sh32 assertions green, discovered by the existingfind plugins .claude/hooksroots with no ci.yml change.Both changes were reviewed by independent fresh-context verifiers that re-derived the evidence rather than trusting it. The selector verifier recomputed the full sweep itself, hand-read all 190 dropped basename/enclosing-token pairs (only two name the real file, and both keep their suites through other bounded mentions), proved a direct-coverage invariant over all 3,455 files with zero missing a suite that genuinely names it, and killed 8 of its own 10 mutations. The bootstrap verifier ran 19 fail-open probes (glob and space-bearing ids,
declare -Are-entry, duplicate ids, degenerate list JSON and registry shapes, no-git-repo,set -uinteraction) without constructing a fail-open, and its first pass rejected an earlier version that still printed0 failedfor five bad end states; those five are now pinned by tests, along with the extraction anchors and the block's exit status, since a nonzero exit there would abort the whole bootstrap underset -e.Related
wip/portability-perf-unverifiedwith the defect, the reproducer and the fix shape recorded in its commit message.🤖 Generated with Claude Code
https://claude.ai/code/session_01XGLX1xYgy27JiRqLjoiH8T
Generated by Claude Code