Skip to content

fix(rate-limit-guard): scope the tee's account forward-pass claim to what the writer does - #1734

Merged
kyle-sexton merged 2 commits into
mainfrom
fix/1685-reader-contract-forward-pass-claim
Jul 29, 2026
Merged

fix(rate-limit-guard): scope the tee's account forward-pass claim to what the writer does#1734
kyle-sexton merged 2 commits into
mainfrom
fix/1685-reader-contract-forward-pass-claim

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

🤖 Agent-authored (autonomous worker lane).

Closes #1685.

Summary

The plugin told readers that the release adding an account identifier upgrades the tee file for free. The writer cannot guarantee that.

scripts/statusline-tee.sh selects on the top-level key name onlyto_entries over the root object — matching account as a case-insensitive substring, and an unmatched key is dropped with no diagnostic. So the promise holds only when the new field's own top-level key name contains account. A field called user, identity, org, or seat, or an account_uuid buried inside a non-matching object, vanishes silently — in a contract that fail-closes on every other unresolvable input.

The doc now follows the code. statusline-tee.sh's behavior is unchanged: the jq filter is byte-identical to main and only its header comment moved. Widening the filter is a design question owned by TODO(#1218), which the issue and triage both scoped out of this correction.

What changed

Four surfaces carried the same overstatement, not the one the issue quotes. The issue quotes only the reader contract's Invariants bullet; fixing that alone would have shipped a document contradicting its own Tee-file-shape section, and would have left the claim standing on the plugin's front door.

Surface Was Now
reference/reader-contract.md "Tee file shape" "...so the release that adds an account identifier upgrades this file without a plugin change" States the writer's actual reach; owns the rule
reference/reader-contract.md "Invariants" (the quoted one) "...so an identity field costs no plugin change the release one appears" Scoped to that shape; points at the rule rather than restating it
README.md known-gap bullet "The wrapper automatically adopts any future account-identifying field the schema grows" — fully unqualified, and broader than the sentence the issue quotes Scoped, with a pointer to the contract
scripts/statusline-tee.sh header comment qualifiers sat in the parenthetical; the so ... inference did not carry them forward Inference now carries the qualifiers

One correction the issue did not anticipate. An earlier draft of this PR implied that nesting defeats the forward-pass. It does not: a selected top-level key crosses with its whole value, nested objects included (account_info: {uuid, display_name} arrives complete). The drop is caused by no top-level key matching, not by depth. Because a passed key may therefore carry an object of arbitrary user/AI-influenced strings, the untrusted-value discipline is restated to cover the subtree rather than a scalar — the parse-with-a-JSON-parser, never-interpolate rule applies to all of it.

Terminology unified on "writer change"; the contract had split between "writer change" and "filter change" for the same fact.

Two phrasings from the issue body were rejected as not surviving contact with the code: the filter also selects rate_limits, so it is not "the whole of" the forward-pass; and map(select(...)) has no branch to lack an else — the accurate statement is that unmatched keys are dropped with no diagnostic.

Verification

This is a doc-only prose correction with no executable surface. No test can reach the claim, and I did not invent coverage for it. Verification is the read of the filter against the new prose, plus the gates below. The one executable file touched changed only a comment; git diff origin/main -- scripts/statusline-tee.sh shows two comment hunks and nothing else.

Gate Result
check-changelog-parity.sh --check pass
check-changelog-parity.sh --check-bump origin/main pass
generate-catalog.mjs --check catalog in sync
validate-plugins.sh all manifests + catalog validated
validate-plugin-contracts.mjs 43 setup skills, 2123 plugin files
check-cross-plugin-source-drift.sh --check no drift
check-changed-skills.sh origin/main no changed skills (correctly a no-op)
check-shell-portability.sh origin/main no unexcused GNU-only constructs, 1 file
shellcheck + bash -n on statusline-tee.sh clean
markdownlint-cli2 on all 3 changed .md 0 errors

The operable floor (the block the three lane skills inline byte-identically per loop-lane convention §6) is untouched, so no lane-skill fan-out is needed and §6 needs nothing.

CHANGELOG.md:146 deliberately still carries the old wording. It is a historical [0.2.0] release note, not a live surface — changelog entries are immutable history. Not a missed site.

Reviewer note — version collision with #1692

This PR takes rate-limit-guard 0.3.4 → 0.3.5. PR #1692 also claims 0.3.5 for this plugin. Whichever merges second must rebase and re-bump, and check-changelog-parity.sh --check-bump will fail for the loser until it does. Not pre-emptively jumping to 0.3.6, since that guesses at merge order and would leave a version gap.

The bump is warranted by docs/MIGRATION-PLAYBOOK.md:373-375 — a version bump is the only delivery vehicle, so an unbumped plugin never delivers a corrected reference to consumers. Note --check-bump only fires when a version changes; it does not itself compel one.

Related

🤖 Generated with Claude Code

kyle-sexton and others added 2 commits July 29, 2026 03:15
…ass claim to what the filter does

The reader contract described the tee writer's forward-pass filter
accurately and then drew a conclusion broader than the filter supports.
Two bullets promised that the release adding an account identifier
"upgrades this file without a plugin change" / "costs no plugin change".

The filter is `to_entries` over the root object only, with
`test("account"; "i")` matching a substring of the key, and it has no
else branch. The promise holds only for a field that is both top-level
and `account`-named; a nested field (`user.account_uuid`) or one named
`user`, `identity`, `org`, or `seat` is dropped, and dropped silently —
in a contract that fail-closes on every other unresolvable input.

Both bullets now state the filter's actual reach, name the silent drop,
and scope the no-change claim to that one shape. The tee-shape bullet
owns the statement; the invariant bullet points at it instead of
restating it. `statusline-tee.sh` is unchanged — widening the filter is
a design question owned by TODO(#1218).

Closes #1685.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…the value-scope of a passed key

Review found the first pass fixed the least-read of four surfaces and
introduced one error of its own.

Two surfaces still carried the unqualified promise: README.md's
known-gap bullet ("the wrapper automatically adopts any future
account-identifying field the schema grows") and statusline-tee.sh's
header comment, whose "so ..." inference did not carry forward the
qualifiers its own parenthetical stated. The README is the plugin's
front door, and the harm the item names — a planner reading "the writer
is already ready" and skipping the filter — routes through it at least
as readily as through the reference doc.

The first pass also implied that nesting defeats the forward-pass. It
does not. Selection is on the top-level key name alone, and a selected
key crosses with its whole value, nested objects included; the drop is
caused by no top-level key matching, not by depth. Since a passed key
may therefore carry an object of arbitrary user/AI-influenced strings,
the untrusted-value discipline now covers the subtree rather than a
scalar.

Also unified on "writer change" (the contract had split between "writer
change" and "filter change"), and dropped two phrasings inherited from
the item body that do not survive contact with the code: the filter also
selects rate_limits, so it is not "the whole of" the forward-pass, and
`map(select(...))` has no branch to lack an else.

statusline-tee.sh's behavior is unchanged — the jq filter is
byte-identical to main and only its header comment moved.

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

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

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


PR Review

  • Gather context and understand the request
  • Read changed files
  • Review documentation accuracy against the script
  • Check CHANGELOG parity and version bump
  • Final review summary

This is a doc-only correctness fix. I verified each prose claim against the jq filter byte-for-byte. Here's the full read.


jq filter verification

The filter at statusline-tee.sh:80–86 is byte-identical to origin/main — only the header comment changed. Every doc claim checks out against it:

Claim Filter evidence
"top-level key name only" to_entries over the root object, not recursive
"contains account (case-insensitive)" .key | test("account"; "i") — regex substring, account has no metacharacters so this equals a literal contain
"selected key carries its whole value" map(select(...)) | from_entries reconstructs the full .key/.value pair; .value is never narrowed
"dropped with no diagnostic" map(select(...)) excludes non-matching entries silently; 2>/dev/null || return 0 on line 86 suppresses jq stderr

Reader contract ("Tee file shape")

reader-contract.md:50–61 — the revised paragraph is accurate and well-structured. Three things it gets right that the old text didn't:

  1. account_uuid buried in a non-matching object — the explanation that user: {account_uuid: ...} vanishes because the top-level key user doesn't match is correct and the most likely reader confusion point.
  2. Nested-object value passthrough — the account_info: {uuid, display_name} example is exactly right; from_entries brings the full .value through regardless of depth.
  3. Untrusted-value scope expanded to "object of arbitrary strings" — previously only called out for session_name (a scalar), now covers a potential object value. Accurate and the security note is load-bearing.

Reader contract ("Invariants")

reader-contract.md:113–115 — the new wording scopes the claim correctly and uses a cross-reference to "Tee file shape" rather than restating the rule. Good DRY choice; the invariant now says "under the exact rule 'Tee file shape' states" which is both accurate and traceable.


README

README.md:38–40 — accurately scopes the claim and adds a pointer to the contract section. Correct.


CHANGELOG

The [0.3.5] entry is accurate and traces the fix well. The historical [0.2.0] entry at (pre-PR) line 146 — which reads "an identity field costs no plugin change the release one appears" — is correctly left untouched as immutable history.


Version bump

0.3.4 → 0.3.5 is appropriate. The playbook requires a version bump as the only delivery vehicle for a corrected contract reference to consumers; an unbumped plugin never delivers the fix. Doc-only doesn't exempt a bump here.

Version collision note (flagged in PR body): PR #1692 also claims 0.3.5 for this plugin. Whichever merges second must rebase and re-bump before check-changelog-parity.sh --check-bump will pass. No action needed on this PR — the PR body already surfaces it clearly.


Observations (non-blocking)

  • The phrase "only when it arrives under a top-level key of that shape" in the updated statusline-tee.sh comment (lines 26–27) uses "that shape" as a back-reference to "whose name contains 'account'" from the line before. It's unambiguous in context but slightly more implicit than the reader-contract prose, which spells the condition out in full. Acceptable for a header comment.

  • The two rejected phrasings from the issue body (filter as "the whole of" the forward-pass; map(select(...)) as having "no else branch") are correctly excluded — the explanations in the PR body are sound and consistent with the filter.


Verdict

The documentation now faithfully represents the filter's behavior. No executable surface changed. Scope is tight, all four overstating surfaces are corrected, no site is missed, and the CHANGELOG is accurate. This is ready to merge on its own merits — the only external dependency is the version collision with #1692 being resolved at merge time.

@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

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


Security Review

  • Fetch PR diff and read changed files
  • Review statusline-tee.sh for injection/data-flow issues
  • Review markdown documentation changes for security claims
  • Review plugin.json and CHANGELOG.md
  • Synthesize findings

No security issues introduced by this PR. Findings below.

Scope of executable changes

The only executable file touched is scripts/statusline-tee.sh. The jq filter (lines 80–86) is byte-identical to origin/main; the two diff hunks are in the header comment only. All remaining changed files (plugin.json, CHANGELOG.md, README.md, reference/reader-contract.md) are documentation.

Data-flow review of the tee script (pre-existing, confirmed clean)

The script reads raw stdin into $INPUT, processes it through jq -c --arg ts "$ts" (not string-interpolated into the filter — passed as a typed jq argument), and writes the resulting JSON with printf '%s\n' "$payload" >"$tmp" under umask 077. No field value from the input is ever interpolated into a shell command, another filter, or a filename. The temp filename uses $$.$RANDOM, adequate for collision-avoidance in the user's own ~/.claude/ directory. All variables are double-quoted throughout; no word-splitting or globbing surfaces. The filter selects by key name only (test("account"; "i") applied to .key, not to the value), so the regex is a literal constant — not attacker-controlled — and there is no regex injection risk.

Security-positive changes in the documentation

The contract corrections in reference/reader-contract.md (lines 50–61) are a net security improvement:

  1. The previous text warned consumers to treat a forwarded account field as untrusted on the assumption it was a scalar. The new text correctly states the forwarded value may be an object of arbitrary strings — nested objects cross the filter intact — and explicitly extends the "parse with a JSON parser, never string-interpolate" rule to the whole subtree. This closes a documentation gap that could have led consumers to apply the injection guard only to a top-level string while silently trusting nested fields.

  2. The README.md and invariants section now correctly bound the no-writer-change promise to keys matching account at the top level, removing an overstatement that could have caused a consumer to skip re-validation when a new account field arrived under a non-matching key name.

Neither the injection surface nor the trust model changed; the documentation now matches the code's actual behavior instead of promising broader coverage.

Pre-existing observations (not introduced by this PR, noted for completeness)

  • Symlink attack on directory creation (lines 70–76): mkdir -p "$dir" followed by chmod 700 "$dir" does not defend against a pre-placed symlink at ~/.claude/rate-limit-guard pointing to a directory the attacker controls. The script acknowledges this: "Best-effort (no-op on filesystems without POSIX modes…)." In practice, $HOME is the user's own home directory and an attacker needing write access there has already escalated; practical risk on a single-user workstation is low. The existing note in the code is accurate.
  • $RANDOM in temp filename (line 89): 15 bits of entropy paired with the PID is collision-resistant but not cryptographically unpredictable. Combined with the umask 077 write and the chmod 700 directory, this is adequate for the operational threat model. Worth noting for any future hardening pass.

Both observations predate this PR and are outside its scope.

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: reader-contract overstates the tee's forward-pass upgrade path

1 participant