Skip to content

fix(rate-limit-guard): orphan-aware resolve_tee in statusline-shim - #1866

Merged
kyle-sexton merged 6 commits into
mainfrom
fix/rate-limit-guard-resolve-tee
Aug 9, 2026
Merged

fix(rate-limit-guard): orphan-aware resolve_tee in statusline-shim#1866
kyle-sexton merged 6 commits into
mainfrom
fix/rate-limit-guard-resolve-tee

Conversation

@kyle-sexton

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

Copy link
Copy Markdown
Contributor

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.00.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

Co-authored-by: Cursor <cursoragent@cursor.com>
@claude

claude Bot commented Jul 31, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 1m 13s —— View job


Security review complete

  • Gather context (PR diff, changed files)
  • Review statusline-shim.sh changes for logic/injection/trust-boundary issues
  • Review statusline-shim.test.sh changes
  • Review CHANGELOG.md changes
  • Post findings

Scope: the 3 files changed in this PR (plugins/rate-limit-guard/scripts/statusline-shim.sh, scripts/statusline-shim.test.sh, CHANGELOG.md). No GitHub Actions workflows are touched.

No security issues found.

Summary of what I checked:

  • Trust boundary / arbitrary code execution: resolve_tee() in statusline-shim.sh#L98-L123 globs the plugin cache and exec bash's whatever it resolves (L127-L128). That execution trust boundary is unchanged by this PR — it already existed pre-PR (mtime-only resolution also executed whatever tee it found). The new orphan check (L116-L118) only narrows the candidate set (skips version dirs marked orphaned), which shrinks the window an uninstalled/superseded tee can keep running — a net security improvement, not a regression.
  • Injection surfaces: PLUGIN_NAME is a hardcoded literal, not attacker-influenced input; the glob pattern and the .orphaned_at existence check ([[ -e ... ]]) take no externally-controlled data. No command/path injection.
  • TOCTOU / downgrade: an actor able to write into $CLAUDE_CONFIG_DIR/plugins/cache/... (e.g. to delete .orphaned_at between the check and the later exec, or to plant a malicious statusline-tee.sh) could already do so pre-PR to get code execution via the old mtime-only resolution — this requires local write access to the user's own Claude config directory, which is outside this shim's trust model and not a new capability introduced here.
  • Marker semantics: the comments correctly note the marker's absence is not treated as proof of "installed" — mtime still arbitrates among unmarked candidates exactly as before, so a staged/pending-update directory isn't wrongly trusted over an installed one.
  • Test file (statusline-shim.test.sh): all new fixtures (orphan_tee, cases 15–16) use fixed, test-authored strings/paths under a mktemp -d sandbox — no injection surface, no interaction with production paths.
  • CHANGELOG.md: documentation only, no secrets or executable content.

@claude

claude Bot commented Jul 31, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 3m 46s —— View job


Review complete

  • Gather context (PR diff, changed files, CLAUDE.md guidelines)
  • Understand the request (code review of orphan-aware resolve_tee port)
  • Review statusline-shim.sh changes
  • Review statusline-shim.test.sh changes
  • Review CHANGELOG.md changes
  • Compare against source implementation (context-guard) for consistency
  • Post final review

Note on tooling: the sandbox in this session refused to execute the new test suite (bash plugins/rate-limit-guard/scripts/statusline-shim.test.sh required approval that wasn't available), so the test-suite pass/fail below is from static review, not a live run. I compared it line-for-line against context-guard's already-landed analog instead.

Summary

This is a faithful, careful port. I diffed statusline-shim.sh against plugins/context-guard/scripts/statusline-shim.sh and the only differences are PLUGIN_NAME/header prose — the logic (including the new .orphaned_at skip at statusline-shim.sh#L116) is byte-identical, exactly as the changelog entry claims. I also fetched the live plugins-reference doc and confirmed the quoted orphan-marking text ("the previous version directory is marked as orphaned and removed automatically 14 days later...") is accurate, so the fresh-docs citation checks out. Test additions (#15 sole-orphan-skipped, #16 orphan-loses-to-installed-sibling-even-when-newer) mirror context-guard's equivalent cases and correctly isolate the marker check from the mtime comparison.

Findings

1. plugin.json version was not bumped to match the new CHANGELOG entry (plugin.json#L4)

CHANGELOG.md adds a ## [0.4.1] entry, but plugins/rate-limit-guard/.claude-plugin/plugin.json still reads "version": "0.4.0". Compare to context-guard's own port of this exact fix (#1787/#1844), where plugin.json was bumped to 0.4.4 alongside the matching changelog heading — this PR is the odd one out.

This isn't just a paperwork gap: CLAUDE.md's "Versioned" rule exists specifically so "consumers update on bumps." Without the version bump, an operator on cached rate-limit-guard 0.4.0 has no signal that anything changed, so the orphan-skip fix this PR ships never reaches an already-installed copy.

It also won't be caught by CI: scripts/check-changelog-parity.sh --check-bump only inspects plugins whose plugins/<name>/.claude-plugin/plugin.json actually appears in the PR diff (bumped_candidate is keyed off touched manifest paths). Since this PR never touches plugin.json, rate-limit-guard is skipped by that gate entirely — the mismatch is real and currently invisible to CI.

Suggested fix: bump plugin.json's version to 0.4.1 in this PR. Fix this →

Everything else — the resolve_tee() logic, the orphan-marker placement relative to the temp_* skip, and the test coverage — looks correct and consistent with the already-merged context-guard version of this fix.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 005823d037

ℹ️ 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".

Comment thread plugins/rate-limit-guard/CHANGELOG.md Outdated
Comment thread plugins/rate-limit-guard/scripts/statusline-shim.sh
@github-actions

github-actions Bot commented Jul 31, 2026

Copy link
Copy Markdown

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

Co-authored-by: Cursor <cursoragent@cursor.com>
@claude

claude Bot commented Jul 31, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 1m 26s —— View job


Security review complete

  • Gather context (git diff/PR diff, changed files)
  • Review statusline-shim.sh changes for logic/injection/trust-boundary issues
  • Review statusline-shim.test.sh changes
  • Review CHANGELOG.md / plugin.json changes
  • Post findings

Scope: the 4 files changed in this PR (plugins/rate-limit-guard/scripts/statusline-shim.sh, plugins/rate-limit-guard/scripts/statusline-shim.test.sh, plugins/rate-limit-guard/CHANGELOG.md, plugins/rate-limit-guard/.claude-plugin/plugin.json). No GitHub Actions workflows are touched.

No security issues found.

Re-confirming this PR at head 278ab38 (which adds the version bump commit on top of the previously-reviewed 005823d):

  • Trust boundary / execution: resolve_tee() still globs the plugin cache and exec bash's whatever it resolves (L127-L128). That trust boundary is unchanged and pre-existing — the new .orphaned_at check at L116-L118 only narrows the candidate set, shrinking the window an uninstalled/superseded tee can keep running. Net hardening, not a regression.
  • TOCTOU: the .orphaned_at existence check and the later exec are not atomic, but this requires an actor with local write access to the user's own $CLAUDE_CONFIG_DIR/plugins/cache/... — already sufficient to plant a malicious tee directly, pre- and post-PR alike. No new capability introduced.
  • Injection surfaces: PLUGIN_NAME is a hardcoded literal; the glob pattern and [[ -e ... ]] marker check take no externally-controlled input. No command/path injection.
  • Marker semantics: correctly treats the marker's absence as "unknown," not "installed" — mtime still arbitrates among unmarked candidates exactly as before, so a staged/pending-update directory can't be wrongly trusted over an installed one.
  • Test file: all new fixtures (orphan_tee, cases 15–16) use fixed, test-authored strings/paths under a mktemp -d sandbox — no injection surface, no interaction with production paths.
  • plugin.json / CHANGELOG.md: version bump (0.4.0 → 0.4.1) and documentation only; no secrets or executable content. This also resolves the version-parity gap flagged in the prior review round.

One item from the automated Codex review (manifest-bump P1) is already addressed by this PR's second commit. The other Codex item (migration path for already-setup apply'd installed shim copies) is a distribution/operational concern about setup apply/setup check, not a vulnerability introduced by the diff in this PR's 4 changed files, so it's out of scope for this security-only review.

@claude

claude Bot commented Jul 31, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 2m 7s —— View job


Code review complete

  • Gather context (PR diff, changed files, CLAUDE.md guidelines)
  • Review statusline-shim.sh changes
  • Review statusline-shim.test.sh changes
  • Review CHANGELOG.md / plugin.json changes
  • Post final review

Scope: the 4 files changed in this PR, reviewed for correctness/architecture/maintainability at head 278ab38 (security aspects are covered by the dedicated security lane, already posted above).

Summary

This is a clean, mechanical port and the previously-flagged issue is resolved. I diffed plugins/rate-limit-guard/scripts/statusline-shim.sh against plugins/context-guard/scripts/statusline-shim.sh (which already carries this fix, ported from #1787/#1844) and the logic is byte-identical modulo PLUGIN_NAME/header prose, exactly as the changelog claims. The new orphan check at statusline-shim.sh#L116-L118 is placed correctly relative to the existing temp_* skip and the mtime comparison, and correctly treats marker-absence as "unknown" rather than "installed" (so a staged/pending-update directory still loses to mtime arbitration among unmarked candidates, per the header comment at L69-72).

Version-parity gap from the previous review round is now fixed. plugin.json is bumped to 0.4.1, matching the ## [0.4.1] CHANGELOG entry — consistent with how context-guard's identical port was bumped.

Test coverage: the two new cases (#15 sole-orphan-skipped, #16 orphan-loses-to-installed-sibling-even-when-newer, at statusline-shim.test.sh#L255-L280) mirror context-guard's equivalent cases and correctly isolate the marker check from the mtime comparison — case 16 in particular gives the orphaned directory the newer mtime specifically so only the marker can decide the outcome, which is the right adversarial shape for this kind of regression test. The ci workflow on this head (278ab38) is green, confirming the suite (36 assertions) and shellcheck actually pass rather than relying on the PR description's claim.

Not a new finding, for context: Codex flagged that an operator who already ran setup apply won't get this fix until they re-run /rate-limit-guard:setup apply (a plugin update alone doesn't refresh the durable shim copy). That's accurate, but it's the same migration story as context-guard's already-merged, identical fix — the setup skill's existing check step 2 (skills/setup/SKILL.md#L71-L73) already detects a differing # shim-revision: and offers apply as the refresh — so this PR isn't introducing a new gap or regressing an existing migration path, it's exercising the same accepted mechanism (revision bumped 2 → 3). Not something this PR needs to newly solve.

No blocking issues found.
· branch fix/rate-limit-guard-resolve-tee

@claude

claude Bot commented Jul 31, 2026

Copy link
Copy Markdown

Claude encountered an error after 1s —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 31, 2026

Copy link
Copy Markdown

Claude encountered an error after 1s —— View job


I'll analyze this and get back to you.

kyle-sexton added a commit that referenced this pull request Aug 5, 2026
Thirteen of the sixteen provisional numbers already matched. Three move up to
clear versions claimed by open pull requests that "next patch above main" could
not see:

  bash-format       0.6.11 -> 0.6.12  (#1867 claims 0.6.11)
  rate-limit-guard  0.4.1  -> 0.4.2   (#1866 claims 0.4.1)
  source-control    0.45.2 -> 0.46.1  (#1830 claims 0.46.0)

Manifest and CHANGELOG heading move together; each entry keeps its position
above that plugin's newest shipped entry, and all sixteen remain strictly
greater than main.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
@claude

claude Bot commented Aug 6, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 1m 14s —— View job


Security review complete

  • Gather context (PR diff, changed files)
  • Review statusline-shim.sh changes for logic/injection/trust-boundary issues
  • Review statusline-shim.test.sh changes
  • Review CHANGELOG.md / plugin.json changes
  • Post findings

Scope: the 4 files changed in this PR (plugins/rate-limit-guard/scripts/statusline-shim.sh, plugins/rate-limit-guard/scripts/statusline-shim.test.sh, plugins/rate-limit-guard/CHANGELOG.md, plugins/rate-limit-guard/.claude-plugin/plugin.json). No GitHub Actions workflows are touched.

No security issues found.

Re-confirming at current head 67dd2da (a no-op merge of origin/main on top of 278ab38, which the earlier review rounds already covered — git show 67dd2da --stat confirms the merge touches only guardrails/knowledge/repo-fleet-hygiene files, not this PR's 4 files):

  • Trust boundary / execution: resolve_tee() still globs the plugin cache and exec bash's whatever it resolves (L127-L128). That trust boundary is pre-existing and unchanged by this PR — the new .orphaned_at check at L116-L118 only narrows the candidate set (skips version dirs marked orphaned), shrinking the window an uninstalled/superseded tee can keep running. Net hardening, not a regression.
  • TOCTOU: the .orphaned_at existence check ([[ -e ... ]]) and the later exec are not atomic, but exploiting that gap requires an actor with local write access to the user's own $CLAUDE_CONFIG_DIR/plugins/cache/... — already sufficient to plant a malicious tee directly, pre- and post-PR alike. No new capability introduced.
  • Injection surfaces: PLUGIN_NAME (L89) is a hardcoded literal; the glob pattern and the marker check take no externally-controlled input. No command/path injection.
  • Marker semantics: correctly treats the marker's absence as "unknown," not "installed" — mtime still arbitrates among unmarked candidates exactly as before, so a staged/pending-update directory can't be wrongly trusted over an installed one.
  • Test file: all new fixtures (orphan_tee, cases 15–16) use fixed, test-authored strings/paths under a mktemp -d sandbox — no injection surface, no interaction with production paths. Diffed against context-guard's equivalent, already-reviewed test file: logic is identical modulo plugin-name substitution.
  • plugin.json / CHANGELOG.md: version is 0.4.1, matching the ## [0.4.1] changelog entry — the version-parity gap from an earlier review round is confirmed still resolved. Documentation/manifest only; no secrets or executable content.

Diffed statusline-shim.sh against plugins/context-guard/scripts/statusline-shim.sh directly: the two are logic-identical, differing only in PLUGIN_NAME and header/comment prose. No new or narrowed trust boundary beyond what context-guard's already-merged analog established.

One item raised by the automated Codex review (migration path for already-setup apply'd installed shim copies) is a distribution/operational concern about the setup skill's update flow, not a vulnerability introduced by this diff — out of scope for this security-only review, and already addressed in the prior code-review round's findings.

Both sides claimed 0.4.1 for different content and main also shipped 0.4.2, so
this entry takes 0.4.3. Both changelog entries kept in full — the orphan-marker
fix and main's setup-escaping and hook-utils entries are unrelated.

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

cursor Bot commented Aug 9, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

@claude

claude Bot commented Aug 9, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 2m 13s —— View job


Code review complete

  • Gather context (PR diff, changed files, CLAUDE.md guidelines)
  • Review statusline-shim.sh changes
  • Review statusline-shim.test.sh changes
  • Review CHANGELOG.md / plugin.json changes
  • Post final review

Scope: the 4 files changed in this PR, reviewed for correctness/architecture/maintainability at head 8e4d64d (security aspects are covered by the dedicated security lane).

Summary

Re-reviewed after the origin/main merge (8e4d64d, merging d81b709). The merge only touched repo-wide tooling/docs files unrelated to this PR — git show 8e4d64d --stat confirms the 4 files this PR owns are not among the merge's own changes — and the merge commit message documents a manual conflict resolution: both origin/main and this branch had claimed 0.4.1 for different content, and main had also since shipped 0.4.2, so this fix's entry was renumbered to 0.4.3. I verified that renumbering is consistent: plugin.json reads 0.4.3 and matches the new ## [0.4.3] heading in CHANGELOG.md, and the pre-existing 0.4.1/0.4.2 entries from main (hook-utils, setup-escaping) are kept intact and unrelated. This resolves the version-parity gap flagged in an earlier review round.

The resolve_tee() logic itself is unchanged from the previously-reviewed revision. I diffed statusline-shim.sh against plugins/context-guard/scripts/statusline-shim.sh directly: the two are logic-identical, differing only in PLUGIN_NAME and header/comment prose (e.g. "contract file" vs "snapshot directory"), which is why scripts/check-cross-plugin-source-drift.sh correctly does not flag them as an unregistered identical cluster — it hashes raw file bytes, and these two files differ byte-for-byte due to the plugin-name literal, so they never hit the IDENTICAL branch that would require a registry entry. That confirms the PR description's claim.

Test additions (case 15: sole-orphan-skipped, case 16: orphan-loses-to-installed-sibling-even-when-newer, at statusline-shim.test.sh#L255-L280) correctly isolate the marker check from mtime: case 16 gives the orphaned directory the newer mtime so only the marker can decide the winner. orphan_tee()'s path-stripping (${tee%/scripts/statusline-tee.sh}/.orphaned_at) matches the shim's own check at statusline-shim.sh#L118 exactly, so the fixture is testing the real code path, not a lookalike.

Verification note: the sandbox in this session declined to run bash plugins/rate-limit-guard/scripts/statusline-shim.test.sh or shellcheck directly, so I did not independently execute the test suite — the 36-passed claim in the PR description is author-claimed and unverified by me. CI (ci workflow) was still in progress at review time; I did not wait on it. Everything above is from static line-by-line reading of the diff and byte-for-byte comparison against context-guard's already-merged analog, not from running the suite.

No blocking issues found. All prior review-round findings (manifest version bump) are resolved; this round's only substantive change from the last-reviewed head is the version renumbering from the main merge, which is correct.
· branch fix/rate-limit-guard-resolve-tee

@claude

claude Bot commented Aug 9, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 1m 7s —— View job


Security review complete

  • Gather context (PR diff, changed files)
  • Review statusline-shim.sh changes for logic/injection/trust-boundary issues
  • Review statusline-shim.test.sh changes
  • Review CHANGELOG.md / plugin.json changes
  • Post findings

Scope: the 4 files changed in this PR (plugins/rate-limit-guard/scripts/statusline-shim.sh, plugins/rate-limit-guard/scripts/statusline-shim.test.sh, plugins/rate-limit-guard/CHANGELOG.md, plugins/rate-limit-guard/.claude-plugin/plugin.json) at head 8e4d64d. No GitHub Actions workflows are touched.

No security issues found.

git diff origin/main...HEAD at this head shows the same substantive change already covered by prior review rounds on this PR — the .orphaned_at skip in resolve_tee() — plus a version bump (0.4.20.4.3, matching the new CHANGELOG entry) picked up from an intervening merge of origin/main. The merge commit (8e4d64d) itself touches no files outside these four.

  • Trust boundary / execution: resolve_tee() still globs the plugin cache and exec bash's whatever it resolves (L127-L128). That trust boundary is pre-existing and unchanged by this PR — the new .orphaned_at check at L116-L118 only narrows the candidate set (skips version dirs marked orphaned), shrinking the window an uninstalled/superseded tee can keep running. Net hardening, not a regression. (Severity: n/a — no issue; Confidence: high)
  • TOCTOU: the .orphaned_at existence check ([[ -e ... ]]) and the later exec are not atomic, but exploiting that gap requires an actor with local write access to the user's own $CLAUDE_CONFIG_DIR/plugins/cache/... — already sufficient to plant a malicious tee directly, pre- and post-PR alike. No new capability introduced. (Confidence: high — not exploitable beyond the existing trust model)
  • Injection surfaces: PLUGIN_NAME (L89) is a hardcoded literal; the glob pattern and marker check take no externally-controlled input. No command/path injection.
  • Marker semantics: correctly treats the marker's absence as "unknown," not "installed" — mtime still arbitrates among unmarked candidates exactly as before, so a staged/pending-update directory can't be wrongly trusted over an installed one.
  • Test file: all new fixtures (orphan_tee, cases 15–16) use fixed, test-authored strings/paths under a mktemp -d sandbox — no injection surface, no interaction with production paths.
  • plugin.json / CHANGELOG.md: version (0.4.3) matches the CHANGELOG heading; documentation/manifest only, no secrets or executable content.

This is the fifth review pass on this PR (four prior rounds already covered 005823d, 278ab38, and 67dd2da — all "no security issues found"); this pass re-confirms the same conclusion at the current merge head, with no new files or logic introduced since.
· branch fix/rate-limit-guard-resolve-tee

…f INFO

Review: the statusline runs the durable copy at
~/.claude/rate-limit-guard/bin/statusline-shim.sh, which a plugin update never
overwrites. An operator who ran `apply` before this release keeps running the
old shim and keeps selecting orphaned tees, and `check` reported that as INFO.

The INFO rule's own premise was "an older revision still resolves the newest
tee" — precisely what this PR falsifies. So the classification now turns on what
the installed revision can do: >= 3 stays INFO (housekeeping), below 3 or
unmarked is FAIL, because the running statusline has a behavior defect rather
than drift.

Names the trap the finding has to carry: uninstalling first takes the setup
skill with the plugin while the stale shim stays behind for its ~14-day orphan
window, leaving no in-product path to the remediation.

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

claude Bot commented Aug 9, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 1m 58s —— View job


Code review complete

  • Gather context (PR diff since last-reviewed head, CLAUDE.md guidelines)
  • Review the new commit (1706edb — FAIL a pre-revision-3 installed shim instead of INFO)
  • Review skills/setup/SKILL.md changes
  • Review statusline-shim.sh / test / CHANGELOG.md for this round
  • Post final review

Summary

This round adds one new commit (1706edb) on top of the previously-reviewed 8e4d64d, touching only plugins/rate-limit-guard/CHANGELOG.md and plugins/rate-limit-guard/skills/setup/SKILL.md — no code changes to statusline-shim.sh or its test suite since the last review pass (those were already reviewed clean across four prior rounds). This commit responds directly to the Codex P1 finding about already-apply'd installs: setup check's "Present but differing" branch now classifies the installed shim by what it can actually do — revision >= 3 stays INFO, revision < 3 (or unmarked) becomes FAIL — since the shim's own header confirms revision 3 is the first to carry the .orphaned_at skip (statusline-shim.sh#L91). That's a sound, narrowly-scoped fix: it correctly turns "the installed copy differs" from a deferrable classification into an actionable one exactly where the behavior actually regresses, and the reasoning in the commit message (the INFO rule's own premise is what the orphan fix falsifies) checks out against the code. The CHANGELOG addition matches the SKILL.md prose change one-for-one.

One cross-plugin consistency finding, posted inline on skills/setup/SKILL.md#L74-L83: context-guard's sibling setup skill (context-guard/skills/setup/SKILL.md#L51-L57) still classifies the identical defect as INFO, not FAIL — even though context-guard's shim already ships the same # shim-revision: 3 orphan fix and has the identical durable-copy problem. Since SKILL.md is in scripts/check-cross-plugin-source-drift.sh's skip_basenames list, nothing in CI will catch this divergence between the two siblings, which this PR's own CHANGELOG describes as a "deliberately unregistered... byte-identical cluster." Not a defect in this PR's own diff (context-guard is untouched here), but worth a fast follow so the two setup skills don't silently diverge on the same finding.

No blocking issues in this PR's own diff. All prior review-round findings (manifest version bump, resolve_tee() logic parity, test coverage) remain resolved; this round's substantive addition is the FAIL/INFO reclassification, which is correct and consistent with the shim's actual behavior.
· branch fix/rate-limit-guard-resolve-tee

@claude

claude Bot commented Aug 9, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 1m 54s —— View job


Security review complete

  • Gather context (gh pr diff, changed files at head 1706edb)
  • Review statusline-shim.sh changes for logic/injection/trust-boundary issues
  • Review statusline-shim.test.sh changes
  • Review skills/setup/SKILL.md changes
  • Review CHANGELOG.md / plugin.json changes
  • Post findings

Scope: the 5 files changed in this PR (plugins/rate-limit-guard/scripts/statusline-shim.sh, plugins/rate-limit-guard/scripts/statusline-shim.test.sh, plugins/rate-limit-guard/CHANGELOG.md, plugins/rate-limit-guard/.claude-plugin/plugin.json, plugins/rate-limit-guard/skills/setup/SKILL.md) at head 1706edb. No GitHub Actions workflows are touched.

No security issues found.

The substantive new content since the last-reviewed head (8e4d64d) is commit 1706edb, which touches only CHANGELOG.md and skills/setup/SKILL.mdgit show --stat 1706edb confirms statusline-shim.sh/statusline-shim.test.sh/plugin.json are untouched by it, so the resolve_tee() logic and its trust boundary (already cleared across five prior review rounds on this PR) are unchanged.

  • SKILL.md is prose, not executable code. It's an instruction file consumed by an LLM agent when a user explicitly runs /rate-limit-guard:setup check; disable-model-invocation: true in its frontmatter means it never fires autonomously. The new content (SKILL.md#L71-L87) reclassifies an installed shim with # shim-revision: < 3 (or unmarked) from INFO to FAIL — it doesn't add any new shell invocation, file write, or data flow. The cmp -s / chmod +x / settings-read operations it describes are pre-existing and unchanged.
  • Marker source is still local-trust-only. The # shim-revision: value being compared is read from the operator's own installed copy under ~/.claude/rate-limit-guard/bin/. An actor able to tamper with that comment (e.g. to force a stale FAIL/INFO misclassification) would already need local write access to the user's own Claude config directory — the same pre-existing trust boundary noted in every earlier round, not a new capability introduced by this classification change.
  • No injection surface added. Nothing in the new SKILL.md text takes attacker-controlled input into a command string; it only branches on a version-comparison outcome (>= 3 vs < 3/absent) that a human/agent reads, not one that gets eval'd or interpolated into a shell command.
  • CHANGELOG.md / plugin.json: version is 0.4.3, matching the new ## [0.4.3] heading; documentation/manifest only, no secrets or executable content.
  • statusline-shim.sh / statusline-shim.test.sh: unchanged from the previously-reviewed revision — resolve_tee()'s orphan-marker skip (statusline-shim.sh#L116-L118) still only narrows the exec bash-ed candidate set (a net hardening, not a regression), PLUGIN_NAME remains a hardcoded literal, and the marker check takes no externally-controlled input.

This is the sixth review pass on this PR; all five prior passes (covering 005823d, 278ab38, 67dd2da, 8e4d64d) already concluded "no security issues found," and this pass confirms the same holds for the SKILL.md-only delta at the current head.
· branch fix/rate-limit-guard-resolve-tee

Comment thread plugins/rate-limit-guard/skills/setup/SKILL.md
…te-limit-guard

Review: context-guard ships the same revision-3 shim with the same orphan-marker
skip and the same durable-copy migration problem, but its setup contract still
classified a differing installed copy as INFO. Fixing only rate-limit-guard left
the byte-identical cluster's two setup contracts disagreeing about the same
defect.

The rule already described the failure correctly — a copy predating revision 3
picks the newest tee by mtime and keeps teeing from an uninstalled plugin — and
then filed it under INFO because the statusline keeps rendering. Rendering is not
the property that matters.

Same shape as #1866: >= 3 stays INFO, below 3 or unmarked is FAIL, and the
finding must state the migration and the uninstall-first trap.

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

claude Bot commented Aug 9, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 1m 51s —— View job


Code review complete

  • Gather context (PR diff, changed files, CLAUDE.md guidelines)
  • Review new commit d279301 (context-guard FAIL-classification port)
  • Diff against rate-limit-guard's 1706edb for consistency
  • Post final review

Scope: this round's only new content is commit d279301, which touches 3 files — plugins/context-guard/.claude-plugin/plugin.json, plugins/context-guard/CHANGELOG.md, plugins/context-guard/skills/setup/SKILL.md. rate-limit-guard's files are unchanged since the last-reviewed head (1706edb), already cleared across six prior review rounds.

Summary

This commit is the fast-follow I flagged in the previous round: context-guard's setup check still classified a pre-revision-3 installed shim as INFO while rate-limit-guard's (this PR's commit 1706edb) had just been reclassified to FAIL for the identical defect. It closes that gap correctly:

  • skills/setup/SKILL.md#L51-L73 reproduces rate-limit-guard's revision-gated split verbatim in structure: installed revision >= 3 stays INFO (housekeeping), < 3 or unmarked is FAIL, plus the same "migration matters more than classification" callout naming the uninstall-first trap (setup skill leaves with the plugin, stale shim stays behind with no in-product remediation path).
  • No code changes were needed in context-guard/scripts/ — I confirmed git diff origin/main d279301 --stat -- plugins/context-guard/scripts/ is empty. context-guard's shim already ships # shim-revision: 3 from the earlier context-guard: an uninstalled plugin's tee keeps executing through the shim, plus a compounding sh -c wrap #1787/fix(context-guard): stop an uninstalled plugin's tee and the compounding sh -c wrap #1844 port, so this commit is purely a setup-contract classification fix, matching the commit message's claim.
  • Version bump is correct and monotonic: plugin.json moves 0.5.20.5.3 (plugin.json#L4), matching the new ## [0.5.3] heading in CHANGELOG.md — I verified 0.5.2 was the immediately-preceding version via git show f5e590c:plugins/context-guard/.claude-plugin/plugin.json, so no version is skipped.
  • SKILL.md is in scripts/check-cross-plugin-source-drift.sh's skip_basenames list (confirmed at scripts/check-cross-plugin-source-drift.sh:34), so the two setup skills are not required to be byte-identical — the minor prose difference between the two (rate-limit-guard's bullet omits "say which of the two behaviors the installed copy has," context-guard's includes it) is cosmetic, not a functional inconsistency.

Verification note: I did not run scripts/check-changelog-parity.sh --check-bump origin/main or scripts/check-cross-plugin-source-drift.sh --check in this session — compound bash invocations required an approval that wasn't available in this sandbox. The version/heading alignment above was instead confirmed by direct file inspection (plugin.json vs. CHANGELOG.md heading, prior version via git show), not by executing those gate scripts. Treat the "would pass CI" framing in the PR description as author-claimed and unverified by me for this specific commit.

No blocking issues. This closes out the last open finding from the previous review round (cross-plugin classification asymmetry) and keeps the two setup contracts in lockstep, consistent with how the resolve_tee() fix itself was ported in the other direction earlier in this PR.
· branch fix/rate-limit-guard-resolve-tee

@claude

claude Bot commented Aug 9, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 1m 18s —— View job


Security review complete

  • Gather context (gh pr diff, changed files at head d279301)
  • Review new commit d279301 (context-guard FAIL reclassification port)
  • Review full diff for logic/injection/trust-boundary issues across all 8 changed files
  • Post findings

Scope: all 8 files changed in this PR at head d279301: plugins/context-guard/.claude-plugin/plugin.json, plugins/context-guard/CHANGELOG.md, plugins/context-guard/skills/setup/SKILL.md, plugins/rate-limit-guard/.claude-plugin/plugin.json, plugins/rate-limit-guard/CHANGELOG.md, plugins/rate-limit-guard/scripts/statusline-shim.sh, plugins/rate-limit-guard/scripts/statusline-shim.test.sh, plugins/rate-limit-guard/skills/setup/SKILL.md. No GitHub Actions workflows are touched.

No security issues found.

The new content since the last-reviewed head (1706edb) is commit d279301, which only touches plugins/context-guard/.claude-plugin/plugin.json, plugins/context-guard/CHANGELOG.md, and plugins/context-guard/skills/setup/SKILL.md — porting the same INFO→FAIL reclassification for a pre-revision-3 installed shim into context-guard's setup skill, mirroring what 1706edb already did for rate-limit-guard. plugins/rate-limit-guard/scripts/statusline-shim.sh and its test file are unchanged from the revision cleared across five prior review rounds on this PR.

  • SKILL.md changes are prose only, no new execution surface. Both context-guard/skills/setup/SKILL.md#L53-L75 and the earlier rate-limit-guard equivalent are instruction text for an LLM agent, consumed only when an operator explicitly runs /context-guard:setup check — the frontmatter carries disable-model-invocation: true (SKILL.md#L5), so it never fires autonomously. The diff only changes which classification label (INFO vs FAIL) a comparison outcome maps to; it introduces no new shell invocation, file write, or command construction, and takes no externally-controlled input.
  • resolve_tee() trust boundary is unchanged and pre-existing. statusline-shim.sh#L98-L123 still globs the plugin cache and exec bash's whatever it resolves; the orphan-marker skip at L116-L118 only narrows the candidate set (net hardening). This was already cleared across five prior rounds and is untouched by d279301.
  • TOCTOU on .orphaned_at: the existence check and later exec are not atomic, but exploiting that requires local write access to the operator's own $CLAUDE_CONFIG_DIR/plugins/cache/... — already sufficient to plant a malicious tee directly, pre- and post-PR alike. No new capability introduced.
  • Injection surfaces: PLUGIN_NAME remains a hardcoded literal; the glob pattern and marker check take no externally-controlled input. No command/path injection anywhere in this diff.
  • plugin.json / CHANGELOG.md (both plugins): version bumps (context-guard 0.5.20.5.3, rate-limit-guard 0.4.20.4.3) matching their respective new CHANGELOG headings; documentation/manifest only, no secrets or executable content.

This is the seventh review pass on this PR; all six prior passes concluded "no security issues found" for the resolve_tee() logic and its supporting files, and this pass confirms the same holds for the context-guard SKILL.md/CHANGELOG/plugin.json delta added in d279301.
· branch fix/rate-limit-guard-resolve-tee

@kyle-sexton
kyle-sexton merged commit 5fe38fb into main Aug 9, 2026
33 checks passed
@kyle-sexton
kyle-sexton deleted the fix/rate-limit-guard-resolve-tee branch August 9, 2026 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rate-limit-guard: statusline-shim resolve_tee() keeps executing an uninstalled plugin's tee (byte-identical clone of the #1787 context-guard defect)

1 participant