fix(context-guard): stop an uninstalled plugin's tee and the compounding sh -c wrap - #1844
Conversation
…ing sh -c wrap Two defects stranded on merged #1252, both re-verified present on main. The shim's resolve_tee() picked the newest tee by mtime, skipping only temp_* marketplace clones. `claude plugin uninstall` does not delete the version directory — the plugins reference documents that updating or uninstalling marks it orphaned and removes it automatically 14 days later — so scripts/statusline-tee.sh stayed on disk and kept being executed for that whole window, still writing snapshots, with no signal to the operator. A candidate whose version directory carries the orphan marker is now skipped. The marking is documented; the marker's on-disk spelling is not, so it is measured and recorded at the site (Claude Code 2.1.220), together with the fallback: a marker upstream renames or drops leaves resolution exactly as it is today — a stale tee, never a broken statusline. The undocumented installed_plugins.json the header previously rejected stays rejected, on both of its original grounds. setup's "Unwrap before you compose" stripped guard-shim prefixes but not the `sh -c '<escaped …>'` adapter the skill's own shell-syntax guard prints, so a rerun read that adapter as the renderer, found shell syntax in it, and wrapped it again — one layer per run. Unwrapping is now two rules applied until a pass strips nothing, so several layers from earlier reruns collapse rather than only the outermost. Regression coverage: two shim test cases (an uninstalled orphaned tee is not executed; an orphaned directory loses to an installed sibling even when it is newer by mtime) and eval 8 for the rerun-idempotence case. Both shim cases fail against the pre-fix shim and pass after. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 34s —— View job Security review complete
No security issues found. This PR is scoped to What changed, and why it doesn't open new surface:
No command/path/template injection, no secret/token handling, no authorization changes, and no GitHub Actions workflow modifications in this diff. |
|
Claude finished @kyle-sexton's task in 2m 50s —— View job Code review complete
This is a well-scoped, well-documented fix. The Test coverage: genuinely non-vacuous — case 16 in particular ( One nit and one open question:
Everything else — the |
![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: be492aa445
ℹ️ 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".
|
Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
Review found a real defect in the unwrap rule, not just the narrowing the PR body disclosed. Peeling any bare `sh -c '<string>'` breaks an operator whose genuine renderer needs a shell but carries no shell syntax: `sh -c 'ulimit -n'` peels to `ulimit -n`, the shell-syntax guard has nothing to re-wrap, and the shim ends up exec'ing a shell builtin with no shell — the statusline exits 127 instead of rendering. Rule 2 now peels only when the carried string ITSELF contains shell syntax. That is the provenance test rather than a heuristic: the skill emits the adapter only for a renderer carrying shell syntax, so an `sh -c` over a string carrying none cannot have been generated by it and is the operator's own. Both target cases still hold. The compounding case peels — every generated layer wraps something with shell syntax, so nested adapters still collapse to the innermost renderer. The genuine-builtin case does not, and is preserved verbatim. Eval 9 pins the preservation case alongside eval 8's rerun-idempotence case. Also fixes a garbled sentence in the 0.4.4 changelog entry describing the marker fallback. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 15s —— View job Security review complete
No security issues found. This PR is scoped entirely to Analysis by file:
No command/path/template injection, no secrets or token handling, no authorization changes, and no workflow modifications anywhere in this diff. |
|
Claude finished @kyle-sexton's task in 2m 17s —— View job Code review complete
This synchronize pushed a second commit (
Remaining open item from the prior pass — still unaddressed, not a blocker: the reinstall-same-version edge case ( Everything else checks out on static review (execute permissions aren't available in this sandbox, consistent with the prior pass, so I read rather than ran the suite):
|
…usline renderer (#1999) ## Summary `/context-guard:setup check` printed an operator's own `sh -c '<command>'` renderer back to them wrapped in a second one — `sh -c 'sh -c '\''ulimit -n'\'''` — adding one more shell on every statusline refresh. PR #1844 fixed the mechanism the finding named: unwrap rule 2 (`skills/setup/SKILL.md:143-153`) correctly declines to peel an `sh -c` the operator wrote themselves, because the string it carries holds no shell syntax. But the finding's complaint kept reproducing by a second path. The *preserved* renderer then reached the shell-syntax guard, which listed bare **quoting** among the triggers needing an adapter — so it matched on its own quote characters and was wrapped anyway. The escape hatch that was supposed to stop that had a true condition and a **false** rationale: it claimed such a command meant "rule 2 above having been skipped", when rule 2 had in fact run and declined. A finding is discharged when its complaint stops reproducing, not when the line it pointed at changes. Decisive proof it had not: a faithful implementation of the prose failed this skill's own **eval 9** (`skills/setup/evals/evals.json:107-118`), whose fourth expectation is "prints exactly one shim invocation and exactly one `sh -c` layer". ## What changed **Quoting is no longer a trigger, and the trigger test is scoped to top-level syntax.** The `statusLine` `command` field "runs in a shell" ([docs](https://code.claude.com/docs/en/statusline), fetched 2026-08-07), so that shell splits the line into words and consumes the quotes before `statusline-shim.sh` sees ARGV, and the shim `exec`s those words unchanged. A quoted argument therefore already survives the plain wrapped form intact — including an operator's `sh -c '<string>'`, where `sh` is the executable and `-c` and the carried string are two ordinary ARGV words. The guard now fires only on syntax no ARGV word can express — an inline env assignment, a pipe, `&&`, `||`, `;`, a trailing `&`, a redirection — **and only where it stands unquoted at the top level**. That second clause matters on its own: without it, "carries shell syntax" reads as a substring test, and an implementer meeting `sh -c 'a | b'` at the guard sees a pipe and wraps it, reproducing the double wrap by a third path. **Rule 2's provenance test is now three explicit branches, because scoping the guard silently rescoped it too.** Rule 2 cites the guard to define "carries shell syntax", so narrowing the guard narrowed rule 2 with it — and a wrap whose syntax sits inside inner quotes then looked operator-written. Two of the three branches are therefore keyed to the **shape** of the carried string, not the syntax in it, and are explicitly exempt from the top-level scoping: - **A — the carried string is itself an `sh -c '<string>'`.** Always a generated layer: an operator's renderer is at most one `sh -c` deep. Without this the peel stops one layer early and hands back the two-layer wrap it exists to collapse. - **B — the carried string begins with a guard-shim prefix.** This skill never puts a shim inside an adapter. Sealed there it is invisible to rule 1, which strips only *leading* prefixes, so the composed wiring named the sibling shim a **second** time and ran its tee twice per refresh — the `context → rate → rate → renderer` duplication this skill exists to prevent, reachable for anyone who wired `rate-limit-guard` first and ran the pre-0.4.8 guard. - **C — the carried string is a command the guard would wrap.** The only shape this skill's own adapter ever carries. Absent all three it is the operator's and is preserved. One shape stays ambiguous **by design** — a single `sh -c` over a merely-quoted command, which the buggy guard also emitted and which carries no evidence either way. It is preserved: one spurious shell per refresh is cheaper than a statusline broken by peeling on a guess. The prose says so rather than implying a re-run cleans it up. **The hatch's condition and rationale now agree**, because the hatch is gone and the guard's own condition does the work. The idempotency claim is scoped to what is actually true: for an input that is itself `sh -c '<string>'`, the two tests leave exactly one layer. That does **not** generalize to a layer count — a plain renderer takes none, and `sh -c 'ulimit -n' && echo ok` correctly takes **two**, since `&&` cannot be an ARGV word and peeling the inner `sh -c` would strand the builtin. What is invariant is that peel and wrap are inverses, which is what makes a re-run byte-identical at whatever count the renderer needs. **Three evals now pin this.** Eval 10 (`multiple-generated-layers-collapse-in-one-run`) was added earlier on this branch and deleted by `81c03090e7` with no rationale in the commit message or the CHANGELOG; it covers the nested-layer case exactly, and the scoping change broke it. It is restored with a fifth expectation pinning branch A. Eval 11 (`adapter-hiding-a-sibling-shim-is-peeled`) is new and pins branch B. **`check`'s shim-drift report stopped claiming a differing installed copy is harmless.** It said an older or hand-edited copy "still resolves the newest tee", which stopped being true when `# shim-revision: 3` added the orphan skip: a copy predating it picks by mtime alone, so it also resolves a tee left behind by an uninstalled plugin and keeps teeing for the whole ~14-day grace window. It now states which of the two behaviors the installed copy has. `0.4.7` → `0.4.8` with the matching CHANGELOG entry. ## Verification - Traced end to end for eval 9's input `sh -c 'ulimit -n'`: rule 1 finds no shim prefix; rule 2 declines on both branches (carried string is not an `sh -c` shape, and the guard would leave it alone); the guard finds no unquoted top-level trigger; the plain wrapped form is printed — one shim invocation, one `sh -c` layer. All four expectations met. - Eval 10's input `bash <shim> sh -c 'sh -c '\''THEME=dark my-statusline'\'''`: rule 1 strips the shim, rule 2's nested branch peels the outer layer, rule 2's guard branch peels the inner one, a third pass strips nothing, the guard fires once on `THEME=dark my-statusline`. One layer. - Eval 11's input (a `rate-limit-guard` shim sealed inside a preserved adapter): branch B peels the adapter, exposing the shim to rule 1, which strips it. The combined wiring names each shim exactly once. - Eval 8 is unaffected: rule 2 peels, the env assignment lands unquoted at top level, the guard fires once, output is byte-identical to the wiring already in settings.json. - No eval was loosened; one was restored and strengthened, one added. Evals 1-7 carry no renderer whose behavior the trigger-list change moves. - **A fresh-context verifier CHALLENGED this fix twice, and was right both times.** Round one caught that the guard scoping had silently regressed the nested-layer case; round two caught a false idempotency invariant and the sealed-sibling-shim duplication. Branches A and B, both restored and new evals, and the scoped invariant all come from those challenges. - Gates green from the worktree root: changelog parity (`--check`, `--check-order`, `--check-bump` against `origin/main`), `check-changed-skills.sh` (context-guard/setup PASS, 0 errors), orphaned-fixture gate, shell-portability, skill-portability, markdownlint, and the context-guard suite (shim 36, tee 47, zone 73 — 0 failures). - SKILL.md is 391/500 lines. ## Related - Reopens review finding `PRRT_kwDOTCGFQM6Tzj7l` on #1252, which a prior pass closed as already-fixed and a fresh-context verifier correctly challenged. - Builds on `3cbfaa89ae` (#1844), which fixed unwrap rule 2 and the shim's orphan skip. That behavior is untouched here. - Out of scope, filed for follow-up: `plugins/rate-limit-guard/skills/setup/SKILL.md` still lists bare quoting as a trigger and has no adapter-peel rule at all, so the sibling skill reproduces this same finding on its own surface. Not touched here. No linked issue --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…1866) Closes #1849 ## Summary Port the context-guard orphan-aware `resolve_tee()` fix into `rate-limit-guard` so an uninstalled plugin stops teeing through superseded version directories during the ~14-day orphan window. ## Fix `plugins/rate-limit-guard/scripts/statusline-shim.sh` now skips cache candidates whose version directory carries `.orphaned_at`, matching the supported uninstall/update marking documented for Claude Code (same shape as #1787 / PR #1844). Shim revision bumped to 3. Manifest `0.4.0` → `0.4.1` with a matching CHANGELOG entry. Two regression cases added: an orphaned-only install is not executed (statusline still renders), and an orphaned newer-mtime sibling loses to an installed one. Shims stay deliberately unregistered as a byte-identical cluster. ## Verification | Evidence | Result | | --- | --- | | `bash plugins/rate-limit-guard/scripts/statusline-shim.test.sh` | **36 passed, 0 failed** (includes new orphan cases) | | `shellcheck` (shim + test) | clean | | `scripts/check-shell-portability.sh origin/main` | PASS — 2 files | | `scripts/check-changelog-parity.sh --check-bump origin/main` | PASS | | `scripts/check-cross-plugin-source-drift.sh --check` | PASS (shims remain unregistered; deliberate) | ## Related - Refs #1787 — original context-guard defect this ports - Refs #1844 — sibling fix for context-guard --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Fixes #1787
Summary
Two findings that landed on #1252 after it merged, so the thread-resolution gate never saw them.
Both were re-verified present on
origin/mainbefore this branch started, and both are fixed herewith regression coverage.
resolve_tee()picked thenewest
scripts/statusline-tee.shby mtime, skipping onlytemp_*marketplace clones. It neverasked whether the plugin was still installed, so an uninstalled plugin kept teeing and kept
writing snapshots, with no signal to the operator.
setupadded onesh -clayer per run. "Unwrap before you compose" stripped guard-shimprefixes but not the
sh -c '<escaped …>'adapter the skill's own shell-syntax guard prints.Fix
The orphan marker, not
installed_plugins.jsonclaude plugin uninstalldoes not delete the version directory. The plugins reference, underPlugin caching and file resolution (fetched
2026-07-30), states: "When you update or uninstall a plugin, the previous version directory is
marked as orphaned and removed automatically 14 days later. The grace period lets concurrent Claude
Code sessions that already loaded the old version keep running without errors." Uninstall is named
explicitly, so
scripts/statusline-tee.shstays on disk — and stays executable by an mtime-onlyshim — for that whole window.
resolve_tee()now skips a candidate whose version directory carries the orphan marker.The issue asked for a supported signal, not the internal file the shim header deliberately
rejected. That rejection stands, on both of its original grounds:
installed_plugins.jsonisundocumented and reading it would put a
jqspawn on every statusline refresh. The orphan markerbeats it on both — the behavior it reports is documented, and the test is a shell builtin, so the
file's pure-builtins invariant is preserved (
[[ -e … ]], no subprocess).What is documented vs. what is measured, recorded at the site rather than assumed:
CLAUDE_CONFIG_DIR: an uninstall writes<version-dir>/.orphaned_atholding an epoch-ms stampand leaves
scripts/statusline-tee.shin place. Reproducible on any live cache — every supersededversion directory carries the marker and the currently installed one does not.
update), so absence of the marker is not a claim of installation. mtime still picks the winner
among unmarked candidates, exactly as before.
degrades to today's mtime-only behavior — a stale tee, never a broken statusline.
Unwrapping is now a fixpoint, not one pass
"Unwrap before you compose" became two rules applied until a pass strips nothing:
sh -c '<single-quoted string>'adapter with nothing after the closing quote andwhose carried string itself contains shell syntax — unescaped back by dropping
sh -cand theouter quotes and replacing
'\''with'.Iterating matters: an operator may already carry several layers from earlier reruns, and a single
peel over three layers leaves three. The shell-syntax-guard step now also states that it applies
only to the unwrapped renderer.
The second condition on rule 2 is provenance, and it is load-bearing. The first push of this
branch peeled any bare
sh -c '<string>', and disclosed that as a deliberate, behaviorallyequivalent narrowing. Review showed that claim was wrong, so it is now fixed rather than documented:
an operator whose genuine renderer is
sh -c 'ulimit -n'would have had the layer dropped, andsince
ulimit -ncarries no shell syntax the guard would not restore it — leaving the shimexec-ing a shell builtin with no shell, so the statusline exits 127 instead of rendering.Requiring shell syntax in the carried string is a provenance test, not a heuristic: this skill emits
the adapter only for a renderer that carries shell syntax, so an
sh -cover a string carryingnone cannot have come from it and is the operator's own. Both target cases still hold — every
generated layer wraps something with shell syntax, so nested generated layers collapse until what
remains is the operator's own, while the genuine-builtin case is preserved verbatim. A trailing word
(
sh -c '…' extra) marks a real command and is left alone.Stated precisely, because the distinction matters: the fixpoint does not always terminate at a bare
renderer. Over a hand-written
sh -c 'sh -c '\''my-statusline --flag'\'''the outer layer peels (itscarried string contains quoting) and the inner one does not (
my-statusline --flagcarries no shellsyntax), leaving exactly one layer. That is the correct outcome — the surviving layer is the
operator's own — and it is still idempotent and non-compounding, which is what the issue asked for.
context-guard0.4.3 → 0.4.4 with a matching## [0.4.4]CHANGELOG entry. (The branch originallybumped 0.4.2 → 0.4.3; the shared
hook-utils.shsync claimed 0.4.3 on main while this branch wasopen, so it was renumbered after fast-forwarding rather than co-owning a released version.)
Verification
Regression coverage per the acceptance criteria — and the shim cases are proven non-vacuous,
not merely green:
bash plugins/context-guard/scripts/statusline-shim.test.sh(post-fix)want [] got [TEE:uninstalled],[TEE:orphaned] does not contain [TEE:installed],[TEE:orphaned] unexpectedly contains [TEE:orphaned]context-guardversion dirs all carry.orphaned_at; the installed one does notTwo new shim cases: an uninstalled plugin's orphaned tee is not executed and the statusline still
renders with the wrapped exit code preserved; and an orphaned directory loses to an installed
sibling even when it is newer by mtime, so only the marker can decide the second case.
The P2 fix is a prose-instruction change, so its regressions live where instruction behavior is
graded — two evals, one per direction:
rerun-does-not-compound-the-sh-c-wrap— pins the exact rerun in the issue and assertsone
sh -clayer, never a nested one. Traced by hand:…shim.sh sh -c 'THEME=dark my-statusline --flag'→ rule 1 strips the shim → the carried stringhas an inline env assignment, so rule 2 recovers
THEME=dark my-statusline --flag→ second passstrips nothing → re-wrap reproduces the input byte-for-byte.
genuine-sh-c-renderer-is-not-peeled— the opposite direction, added in response toreview.
sh -c 'ulimit -n'carries no shell syntax, so rule 2 does not fire and the adaptersurvives into the printed wiring.
Review findings addressed on this branch
sh -crenderers" (sh -c 'ulimit -n'peeled to a brokenexec)Gates, all run from the worktree root against
origin/main:shellcheck(shim + test)scripts/check-shell-portability.sh origin/mainscripts/check-changed-skills.sh origin/mainscripts/check-changelog-parity.sh --checkscripts/check-changelog-parity.sh --check-bump origin/mainscripts/check-changelog-parity.sh --check-ordermarkdownlint-cli2 "plugins/context-guard/**/*.md"scripts/check-cross-plugin-source-drift.sh --checkscripts/check-silent-skips.shscripts/validate-plugins.shGate scoping notes, stated rather than assumed:
check-silent-skips.shreads onlyplugins/*/hooks/*.sh, so the new[[ -e … ]] && continueinscripts/statusline-shim.shisoutside its corpus by construction — it is not an unreported silent skip but a resolution filter in
a non-hook script, documented inline at the site.
check-cross-plugin-source-drift.shpassesbecause the two plugins' shims are already not byte-identical and are correctly unregistered.
Related
after this merges; recorded here as the remaining acceptance item rather than actioned on the
branch.
plugins/rate-limit-guard/scripts/statusline-shim.shcarries theidentical
resolve_tee()defect — same glob, sametemp_*-only filter, no orphan check — so anuninstalled
rate-limit-guardkeeps teeing for the same ~14-day window. It is out of scope forcontext-guard: an uninstalled plugin's tee keeps executing through the shim, plus a compounding sh -c wrap #1787, which is scoped to
context-guard, and porting it would pull a second plugin's versionbump, changelog, and test suite into this diff. The two shims are deliberately not a registered
byte-identical cluster (they differ in plugin name and header prose), so no drift gate will
surface it — filing it is the only thing that keeps it visible.