fix(telemetry): require base64-run evidence before labelling a match blob-embedded - #2707
Conversation
…blob-embedded A token shape occurring by chance inside an encoded blob passes the leak table's boundary anchor, because the base64 characters + / = all fall outside the anchor class. Measured 2026-07-25: 35 of 113 GitHub-token shaped matches were chance substrings inside Codex encrypted_content blobs, sitting in the same rows as real credentials. Label them from evidence of a surrounding base64 run, never from the boundary character alone. #2520 tried the latter and was closed: only + and / CAN match mid-blob, but that does not make / MEAN mid-blob, and a JWT in a URL path segment takes / as its boundary too — so the label would have downgraded a genuinely exposed credential. The label never suppresses: the row keeps its shape and count, and any value without positive run evidence falls through to the plain high-signal row. The blob set is partitioned from the SAME single decode pass as the table, so an excluded image payload cannot manufacture a blob label, and the credential-table jq startup count is unchanged. Part of #2522
devantler
left a comment
devantler
left a comment
There was a problem hiding this comment.
🤖 Generated by the Agentic Engineer
Self-review (cross-instance, tick 1005)
Reviewed commit 558299c37b2db7fff74a22d1590fb26a950d521d.
One finding, in the direction the PR explicitly says it must never fail in. Raised as a thread rather than pushed, because a live sibling session currently owns this branch's worktree and the two-writer rule says stand down rather than write through.
Verdict: 1 finding (P1: 1).
= is both a base64 padding character and the assignment operator, so an assignment whose key is a long unbroken alphanumeric run cleared the run threshold on its own. myverylongsecrettoken=ghp_... satisfied the run AND the token shape, and was labelled blob-embedded — downgrading the most common real leak form to encoding noise, the one direction this detector must never fail in. The boundary class is now [+/]. The trade is deliberate and cheap in the safe direction: a token sitting immediately after base64 padding stays a plain high-signal row, costing one extra triage, where the alternative buries a live credential. = stays in the run class, because padding legitimately appears inside a blob, and both legs keep byte-identical boundary classes so the two normalisers cannot diverge.
New head @coderabbitai full review |
|
|
|
Warning Review limit reached
Next review available in: 3 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe credential scanner now detects credential-shaped values inside base64-like runs. It extracts blob and ordinary matches in one pass, normalizes them consistently, and stores blob evidence in a temporary file with cleanup traps. Classification adds Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Tried and evaluated as a user (head
|
Parked at
|
| lane | state at this head | evidence |
|---|---|---|
| CodeRabbit | rate limited | "Next review available in: 51 minutes" — 01:15Z, so free from roughly 02:06Z |
| Codex | usage limit | "You have reached your Codex usage limits for code reviews" — 01:26Z |
| Cursor Bugbot | never ran | check-run neutral with output.title: Error; "Bugbot couldn't run - usage limit reached" — 01:29Z |
CodeRabbit's window reopens shortly and it is the priority lane, so the next run requests it rather than substituting a local round for a lane that is about to serve. If the window has passed and CodeRabbit still will not deliver, the local-review-round conditions are already met and recorded here.
Nothing else is owed on this PR.
Re-requesting CodeRabbit at the same head. The earlier refusal at this head (01:15Z) stated a @coderabbitai full review |
|
Your plan includes PR reviews subject to rate limits. Reviews are available now. |
CodeRabbit did not serve at this head. Its stated 51-minute window (from 01:15Z) had elapsed when I Advancing to Codex (lane 2). @codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Advancing to Cursor Bugbot at this head. CodeRabbit acknowledged at 02:14:47Z ("Reviews are available |
|
@cursor review |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_49704d04-5549-47b4-88c3-48a8a0dcdc15) |
devantler
left a comment
devantler
left a comment
There was a problem hiding this comment.
🤖 Generated by the Agentic Engineer
Self-review (fallback — CodeRabbit, Codex and Cursor Bugbot unavailable)
Reviewed commit: 2da9ceab56593c79ff4e29f184fb96386bedbc37
Per-lane evidence at this exact head, checked directly on this PR:
- CodeRabbit — requested 02:14:24Z; acknowledged 02:14:47Z ("Reviews are available now"), then
produced no review object at all in the ~55 minutes to 03:09Z. This PR has zero
coderabbitai[bot]review objects at any head. - Codex —
chatgpt-codex-connector[bot]reported usage limits exhausted at 02:23:43Z. - Cursor Bugbot — requested 03:09:41Z; check-run started 03:09:49Z and failed 03:09:50Z with
conclusion=neutral,output.title=Error, and the comment "Bugbot couldn't run - usage limit
reached". No retry window is stated, so the lane is genuinely unavailable rather than slow.
Reviewed for correctness and, because the diff is credential-detection logic, for security.
Verdict: 1 finding (P0: 0, P1: 1).
The finding is a false-downgrade in the direction this PR's own comments name as the one it must never
fail in. It does not suppress a row — the shape and count survive the label — so a live credential
is still reported; what changes is that it is annotated as probable noise. Detail inline.
…t blob evidence The blob-embedded label existed to stop a token shape occurring by chance inside a base64 blob from being counted as a real credential. Its evidence is a surrounding base64 run, deliberately not the boundary character, because `/` is also how a URL path segment ends. But `/` was itself in the RUN class, which made the run's LENGTH the only test — and a URL path reaches any length simply by being nested. `test/session` is 12 characters and stayed plain; `test/api/v1/sessions` is 20, so the very same JWT was downgraded to "probably encoding noise" purely for sitting one path segment deeper. That is the one direction this detector must never fail in, and it reached github-tokens too: a token pasted into an API URL is a live credential. Length never distinguished a blob from a path; it only looked like it did, because the single fixture chosen happened to sit under the bar. What separates them is how the run is broken up. Base64 emits `/` about once per 64 characters, so a genuine blob carries long slash-free stretches while a URL path is short segments between slashes. Requiring the run to be unbroken by `/` keeps the blob evidence and drops the path. `/` stays a valid boundary char. Residual error is in the safe direction: a blob whose final slash-free stretch is short stays a plain high-signal row, costing one extra triage, where the opposite error buries a live credential. Measured on the real 7-day corpus (120 sessions): one match moved out of the blob bucket back to plain high-signal, and its shape is a four-segment URL path, not a blob. The five genuine blob matches kept their label. The strip class is changed identically. That half is not observable by example — verified by ablation, because the strip only sees matches the label already accepted — so it is bound by a structural assertion that the two classes stay byte-identical instead, which fires when either drifts.
New head What changed: the blob-embedded run class no longer contains @coderabbitai review |
|
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/scripts/agent-telemetry.sh:
- Around line 250-256: Update the credential-matching flow around CREDMATCH and
CREDBLOB so raw or normalized credential values are never persisted in temporary
files, including after SIGKILL. Keep blob membership and classification in
memory, or replace file contents with an opaque non-reversible identity while
preserving the existing matching behavior and cleanup handling.
- Around line 953-964: The blob classification regex around CRED_BLOB_TABLE_RE
and CRED_BLOB_STRIP_RE must not treat a 16-character slash-free URL path segment
as blob evidence. Update the classification logic to exclude URL context or
require stronger non-URL evidence, while keeping both normalizer regexes
aligned; add a regression covering a URL such as
https://example.test/abcdefghijklmnop/ghp_… and verify the token is not labeled
blob-embedded.
In @.claude/scripts/agent-telemetry.test.sh:
- Around line 892-903: The parity assertion around BLOB_TABLE_CLASS and
BLOB_STRIP_CLASS currently compares only the run classes; extend it to extract
and compare each regex’s trailing boundary class ([+/]) as well. Keep the
existing run-class checks, add deterministic fixture-driven coverage under the
executable script tests, and report mismatched boundary values through the
parity assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4aa37068-94fa-4c1a-9a0d-4f6ec34d499d
📒 Files selected for processing (2)
.claude/scripts/agent-telemetry.sh.claude/scripts/agent-telemetry.test.sh
📜 Review details
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-07-20T18:30:45.905Z
Learnt from: devantler
Repo: devantler-tech/monorepo PR: 2309
File: .claude/scripts/portfolio-surveyor.test.sh:35-38
Timestamp: 2026-07-20T18:30:45.905Z
Learning: For prose “subagent definition” documents under `.claude/agents/**` (e.g., `portfolio-surveyor.md`) that are interpreted by an LLM rather than executed deterministically: in CI, use focused textual contract assertions (e.g., checking required sections, prompts, and structural constraints). Do not attempt behavioral validation by simulating it via non-deterministic model invocations; behavioral validation should require live Surveyor runs.
For executable components under `.claude/scripts/**` (e.g., `release-bot-exemption.sh`): use fixture-driven, deterministic tests that exercise the script logic with controlled inputs/outputs (rather than model calls).
Applied to files:
.claude/scripts/agent-telemetry.sh.claude/scripts/agent-telemetry.test.sh
📚 Learning: 2026-07-29T22:21:01.108Z
Learnt from: devantler
Repo: devantler-tech/monorepo PR: 2566
File: .claude/scripts/merge-confirmation-read.test.sh:0-0
Timestamp: 2026-07-29T22:21:01.108Z
Learning: In shell test harness/ablation scripts, capture the exit status of the command being tested immediately into `rc=$?` on the very next line. Avoid using `$?` as an argument to another command (e.g., `printf ... "$?"` or similar) when the same line also includes `$(...)` command substitution, because shell evaluation/expansion order can replace the tested command’s exit status with the exit status of the command substitution, producing incorrect measurements.
Applied to files:
.claude/scripts/agent-telemetry.sh.claude/scripts/agent-telemetry.test.sh
📚 Learning: 2026-07-29T16:38:50.481Z
Learnt from: devantler
Repo: devantler-tech/monorepo PR: 2554
File: .claude/scripts/worktree-cleanup.test.sh:36-55
Timestamp: 2026-07-29T16:38:50.481Z
Learning: In `.claude/scripts` test fixture scripts, when running Git setup commands (e.g., `git worktree add`, `git submodule add`), do not suppress failures (e.g., by ignoring exit codes or redirecting errors to `/dev/null`) without explicitly checking command status. A setup failure can otherwise cause contract tests to falsely pass (for example, due to empty/incorrect Git porcelain output). Preserve stderr for diagnostics and fail the fixture immediately if setup steps return a non-zero exit code (use `set -e`/`set -o pipefail` and/or explicit `if ! command; then ...; exit 1; fi`).
Applied to files:
.claude/scripts/agent-telemetry.test.sh
🪛 Shellcheck (0.11.0)
.claude/scripts/agent-telemetry.sh
[info] 2993-2993: Expressions don't expand in single quotes, use double quotes for that.
(SC2016)
Addresses CodeRabbit's review of the previous commit. Excluding `/` from the run class stopped multi-segment URL paths from chaining into one long run, but left the run as a single path SEGMENT — and 16 characters is well under what ordinary segments reach. A bare 32-character hex id, the commonest long segment in a REST URL, cleared the bar outright, so `…/0f8e7d6c5b4a392817263544536271a1/ghp_…` was still buried as encoding noise. 16 had only ever been calibrated against multi-segment runs, which no longer exist here. The threshold is now 40: above every identifier shape that actually occurs (32-hex ids, UUID fragments, slugs), while a genuine blob is hundreds of characters with `/` arriving about once per 64, so it still presents a 40-character slash-free stretch roughly half the time. That miss is the safe half — the label exists to suppress noise, so under-labelling costs one triage where over-labelling buries a live credential. The positive fixture was 24 characters, which is not a realistic blob run; it is now a 48-character one, so it exercises the rule rather than sitting under it. Also extends the structural parity assertion to the BOUNDARY class, not just the run class: if the boundaries drift, the strip removes a different span than the label matched and an accepted match keeps part of its run inside the credential's value. 531 assertions green. Both new guards ablated and each fires exactly its own named assertion: reverting the threshold to 16 fails the hex-segment case, and diverging the strip's boundary class fails the boundary-parity case.
All three findings from the
CI green, 531 assertions green, both new guards ablated and each fires exactly its own assertion. @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- PR metadata and changed files ---'
gh pr view 2707 --repo devantler-tech/monorepo --json headRefOid,baseRefOid,files --jq '{headRefOid, baseRefOid, files: [.files[] | {path, additions, deletions}]}'
printf '%s\n' '--- Changed-file outlines ---'
ast-grep outline .claude/scripts/agent-telemetry.sh --items all --view expanded || true
ast-grep outline .claude/scripts/agent-telemetry.test.sh --items all --view expanded || true
printf '%s\n' '--- Pull-request diff ---'
gh pr diff 2707 --repo devantler-tech/monorepo -- .claude/scripts/agent-telemetry.sh .claude/scripts/agent-telemetry.test.sh
printf '%s\n' '--- Regex, evidence, and temp-file references ---'
rg -n -C 5 -e 'blob-embedded|base64|BASE64|boundary|run class|mktemp|trap|tmp' \
.claude/scripts/agent-telemetry.sh .claude/scripts/agent-telemetry.test.shLength of output: 1693 🏁 Script executed: #!/bin/bash
set -euo pipefail
base='d0a7a9e5ccd071f4e8478003ab1ba7870cdbb60e'
head='473bc1889be8e68bbb0699a132e6d6c31f356472'
printf '%s\n' '--- Complete changed-file diff ---'
git diff --no-ext-diff --unified=80 "$base" "$head" -- \
.claude/scripts/agent-telemetry.sh \
.claude/scripts/agent-telemetry.test.sh
printf '%s\n' '--- Source: telemetry implementation, relevant lines ---'
rg -n -C 12 -e 'blob-embedded|base64|BASE64|boundary|mktemp|trap|tmp|decode|extract|normaliz' \
.claude/scripts/agent-telemetry.sh
printf '%s\n' '--- Source: telemetry regression tests, relevant lines ---'
rg -n -C 12 -e 'blob-embedded|base64|BASE64|boundary|URL|path|assignment|parity|exclude' \
.claude/scripts/agent-telemetry.test.sh
printf '%s\n' '--- Full changed test file with line numbers ---'
cat -n .claude/scripts/agent-telemetry.test.shLength of output: 187881 🏁 Script executed: #!/bin/bash
set -euo pipefail
base='d0a7a9e5ccd071f4e8478003ab1ba7870cdbb60e'
head='473bc1889be8e68bbb0699a132e6d6c31f356472'
context_for_matches() {
local file="$1"
local pattern="$2"
local line start end
while IFS=: read -r line _; do
start=$(( line > 35 ? line - 35 : 1 ))
end=$(( line + 55 ))
printf '\n--- %s: lines %s-%s ---\n' "$file" "$start" "$end"
sed -n "${start},${end}p" "$file" | nl -ba -v "$start"
done < <(rg -n -i "$pattern" "$file")
}
printf '%s\n' '--- Changed hunks containing blob-related terms ---'
git diff --no-ext-diff --unified=18 "$base" "$head" -- .claude/scripts/agent-telemetry.sh \
| awk '
/^diff --git/ {show=0}
/^@@/ {hunk=$0; buf=$0 "\n"; show=0; next}
{buf=buf $0 "\n"}
/blob-embedded|base64|BASE64|boundary|BLOB/ {
if (!show) {printf "%s", buf; show=1}
}
show && /^@@/ {show=0}
'
printf '%s\n' '--- Implementation contexts ---'
context_for_matches .claude/scripts/agent-telemetry.sh 'blob-embedded|base64|boundary|leak.table|leak_table'
printf '%s\n' '--- Regression-test contexts ---'
context_for_matches .claude/scripts/agent-telemetry.test.sh 'blob-embedded|base64|boundary|long URL|URL path|assignment'
printf '%s\n' '--- All references to the telemetry target and test invocation ---'
rg -n -C 3 'agent-telemetry(\.test)?\.sh|TARGET=' .claude/scripts/agent-telemetry.test.shLength of output: 7549 🏁 Script executed: #!/bin/bash
set -euo pipefail
head='473bc1889be8e68bbb0699a132e6d6c31f356472'
impl=$(mktemp)
testf=$(mktemp)
trap 'rm -f "$impl" "$testf"' EXIT
git show "$head:.claude/scripts/agent-telemetry.sh" > "$impl"
git show "$head:.claude/scripts/agent-telemetry.test.sh" > "$testf"
printf '%s\n' '--- Implementation: credential regex declarations ---'
sed -n '850,980p' "$impl" | nl -ba -v 850
printf '%s\n' '--- Implementation: complete changed safety block ---'
sed -n '2960,3150p' "$impl" | nl -ba -v 2960
printf '%s\n' '--- Test match locations ---'
rg -n -i 'blob-embedded|base64|boundary|long URL|URL path|assignment' "$testf" || true
printf '%s\n' '--- Bounded test contexts ---'
while IFS=: read -r line rest; do
start=$(( line > 24 ? line - 24 : 1 ))
end=$(( line + 42 ))
printf '\n--- test lines %s-%s (%s) ---\n' "$start" "$end" "$rest"
sed -n "${start},${end}p" "$testf" | nl -ba -v "$start"
done < <(rg -n -i 'blob-embedded|base64|boundary|long URL|URL path|assignment' "$testf" | head -n 40)Length of output: 1412 🏁 Script executed: #!/bin/bash
set -euo pipefail
head='473bc1889be8e68bbb0699a132e6d6c31f356472'
impl=$(mktemp)
testf=$(mktemp)
trap 'rm -f "$impl" "$testf"' EXIT
git show "$head:.claude/scripts/agent-telemetry.sh" > "$impl"
git show "$head:.claude/scripts/agent-telemetry.test.sh" > "$testf"
numbered_range() {
local file="$1" start="$2" end="$3"
sed -n "${start},${end}p" "$file" | awk -v n="$start" '{printf "%6d %s\n", n++, $0}'
}
printf '%s\n' '--- Implementation: credential regex declarations ---'
numbered_range "$impl" 850 980
printf '%s\n' '--- Implementation: complete changed safety block ---'
numbered_range "$impl" 2960 3150
printf '%s\n' '--- Test match locations ---'
rg -n -i 'blob-embedded|base64|boundary|long URL|URL path|assignment' "$testf" || true
printf '%s\n' '--- Bounded test contexts ---'
while IFS=: read -r line rest; do
start=$(( line > 24 ? line - 24 : 1 ))
end=$(( line + 42 ))
printf '\n--- test lines %s-%s (%s) ---\n' "$start" "$end" "$rest"
numbered_range "$testf" "$start" "$end"
done < <(rg -n -i 'blob-embedded|base64|boundary|long URL|URL path|assignment' "$testf" | head -n 32)Length of output: 34897 🏁 Script executed: #!/bin/bash
set -euo pipefail
head='473bc1889be8e68bbb0699a132e6d6c31f356472'
show_range() {
local path="$1" start="$2" end="$3"
git show "$head:$path" | sed -n "${start},${end}p" \
| awk -v n="$start" '{printf "%6d %s\n", n++, $0}'
}
printf '%s\n' '--- Implementation: combined scan and blob regex definitions ---'
show_range .claude/scripts/agent-telemetry.sh 969 1025
printf '%s\n' '--- Regression tests: blob-evidence fixtures and parity assertion ---'
show_range .claude/scripts/agent-telemetry.test.sh 769 955
printf '%s\n' '--- Regression tests: value normalization and duplicate handling ---'
show_range .claude/scripts/agent-telemetry.test.sh 956 1110
printf '%s\n' '--- Exact diff at the reviewed head for the blob-related test range ---'
git diff --no-ext-diff --unified=12 \
d0a7a9e5ccd071f4e8478003ab1ba7870cdbb60e "$head" -- \
.claude/scripts/agent-telemetry.test.sh \
| sed -n '/blob-embedded/,+230p'Length of output: 44574 🏁 Script executed: #!/bin/bash
set -euo pipefail
base='d0a7a9e5ccd071f4e8478003ab1ba7870cdbb60e'
head='473bc1889be8e68bbb0699a132e6d6c31f356472'
show_range() {
local path="$1" start="$2" end="$3"
git show "$head:$path" | sed -n "${start},${end}p" \
| awk -v n="$start" '{printf "%6d %s\n", n++, $0}'
}
printf '%s\n' '--- Diff whitespace check ---'
git diff --check "$base" "$head"
printf '%s\n' '--- Scratch-file creation and cleanup ---'
show_range .claude/scripts/agent-telemetry.sh 220 285
printf '%s\n' '--- Every reference to the new evidence variables ---'
git show "$head:.claude/scripts/agent-telemetry.sh" \
| grep -nE 'CREDMATCH|CREDBLOB|CRED_BLOB_(TABLE|STRIP|ANCHORED|RUN_MIN)' \
|| true
printf '%s\n' '--- All changed implementation hunks ---'
git diff --no-ext-diff --unified=8 "$base" "$head" -- .claude/scripts/agent-telemetry.sh
printf '%s\n' '--- Pull-request checks reported by GitHub ---'
gh pr view 2707 --repo devantler-tech/monorepo --json statusCheckRollup \
--jq '.statusCheckRollup[] | {name, status, conclusion, detailsUrl}'Length of output: 32581
I found no blocking issues. The revised rule uses a 40-character, slash-free base64 run and The regression coverage includes URL paths, long URL segments, assignment keys, regex-class parity, and image-payload exclusion. The new scratch files use
You are interacting with an AI system. |
|
@coderabbitai review |
|
CodeRabbit is rate limited at this head ( @codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Codex is at its usage limit at this head, so the ladder advances to Cursor Bugbot. The bare trigger |
|
@cursor review |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_8ba011e8-c5f1-4f2e-b8ad-488beedf0a35) |
Found reviewing my own diff. Raising CRED_BLOB_RUN_MIN to 40 silently made an existing safety test vacuous. Test (2b) exists to prove that `=` stays OUT of the blob boundary class, because `<long alnum key>=<token>` is the commonest real leak shape and labelling it encoding noise would bury a live credential. Its fixture key `myverylongsecrettoken` is 21 characters. Under the old threshold of 16 that cleared the run and the test genuinely reached the boundary check; under 40 it does not, so the test now passes on the run length alone and can never exercise its own subject. Proven by ablation: adding `=` back to the boundary class — precisely what (2b) guards against — did not fail (2b) at all, only the new structural parity assertion. With the key lengthened to 47 characters, that same ablation fails (2b) as it should. This is the same failure mode this PR was opened to fix, one test over: a fixture whose parameter sits outside the region the rule applies to tests the value, not the invariant. Changing a threshold means re-checking every fixture calibrated against the old one. 531 assertions green.
State at
|
| lane | evidence | time |
|---|---|---|
| CodeRabbit | CodeRabbit status → Review rate limited |
04:5xZ |
| Codex | You have reached your Codex usage limits for code reviews |
04:57:17Z |
| Cursor Bugbot | check-run neutral / output.title: Error + Bugbot couldn't run - usage limit reached |
04:59:14Z |
Bugbot and Codex are account-level usage limits with no stated retry window, so re-triggering
them is waste rather than diligence. CodeRabbit did serve at bae44933 (its review is the reason
this head exists), so it is the lane to retry once its window rolls.
Not claiming the local-review fallback on this evidence, because it was gathered at the
superseded head 473bc188 and the fallback requires per-lane evidence at the current head. The
next run re-checks the three lanes at 338b9e54 and either takes a real review or posts a qualifying
local round then.
Staying a draft. CI green apart from the macOS telemetry job still running at the time of writing;
531 assertions pass locally, and every guard added in this PR has been ablated and fires exactly its
own named assertion.
@coderabbitai review |
|
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
CodeRabbit is rate limited at this head and Codex did not respond, so the ladder advances to Cursor |
|
@cursor review |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_0be0f9b1-6a65-40d8-af91-a1c0c3661716) |
devantler
left a comment
devantler
left a comment
There was a problem hiding this comment.
🤖 Generated by the Agentic Engineer
Cross-instance finding — P1, verified by probe at 338b9e54
Posted as a review rather than a push: this branch is checked out in a worktree owned by a live session, so I stood down from writing to it (two-writer discipline).
A credential that appears BOTH inside a base64 run AND plainly is labelled [blob-embedded: … likely a chance substring]. The plain occurrence is real exposure evidence, and it is buried — the exact direction this change's own comments say the detector must never fail in ("a wrong label buries a live credential", "every ambiguous value falls through to the plain high-signal row").
The value is not ambiguous here: there is positive plain evidence, and it loses to the blob label anyway.
Probe, run against the script at this head
Both controls behave as designed, so the probe discriminates in both directions (a negative control alone could not prove the matcher works):
| Case | Corpus contains | Result |
|---|---|---|
| A (control) | blob occurrence only | 1 github-token … [blob-embedded: …] ✅ as designed |
| B (control) | plain occurrence only | 1 github-token (classic/app) ✅ as designed |
| C | the same token, both ways | 1 github-token … [blob-embedded: …] ❌ plain occurrence buried |
Fixture for C was a single session record containing sig=<48 base64 chars>/<token> and, later in the same text, the identical token plainly. No trailing characters after either occurrence — with a trailing zz the greedy gh[pousr]_[A-Za-z0-9]{16,} absorbs it, the two occurrences become different values, two rows appear, and the probe is vacuous. That vacuous version is what I ran first.
Root cause
CREDBLOB is the set of values that have a blob occurrence, not the set of values whose occurrences are all blob-embedded. sort -u in cred_normalise then collapses the plain and blob occurrences of one value into a single row, and the label is applied on set membership alone.
Suggested fix
Label only when the value has no plain occurrence — subtract the plain set from the blob set, rather than taking blob membership as sufficient. That keeps the stated asymmetry intact: ambiguity, and now also any positive plain evidence, falls through to the plain high-signal row.
A regression test belongs alongside (2b)/(2c): same value both ways, assert the row is not labelled.
Lane state at this head, for the record
All three review lanes refused at 338b9e54, each within ~10s of its trigger: CodeRabbit Review rate limited (05:12:09Z, head status description confirms), Codex usage limit (05:18:00Z), Bugbot usage limit + check-run neutral/Error (05:19:44Z). I did not claim the local-review fallback, because this finding means the PR is not ready regardless.
The blob-embedded label keyed on membership in the set of values with *a* base64-run occurrence, not the set whose occurrences are *all* blob-embedded. cred_normalise ends in `sort -u`, so a credential occurring both inside an encoded blob and plainly collapses to ONE row — and that row was labelled "likely a chance substring", burying the plain occurrence. The plain occurrence is the genuine exposure evidence, and a leaked token routinely appears both in prose and inside an encoded payload of the same transcript, so this downgraded exactly the shape a real leak takes. The label's own contract already states that ambiguity falls through to the plain high-signal row. Subtracting the plain set enforces that rather than assuming it. Subtraction uses awk set membership on a file rather than `comm`: both sides are `sort -u` output, but `comm` re-compares them, so the two would have to agree on collation as well as order. This also matches the file-reading idiom the label pass already uses, whose getline-on-empty-file behaviour is safe where the NR==FNR idiom would eat the first data line. Proven in both directions by ablation: dropping the subtraction fires "a token ALSO occurring plainly is NOT labelled blob-embedded"; emptying the blob set fires the existing "a token inside a base64 run is labelled blob-embedded". Each ablation fires exactly one named assertion.
@coderabbitai review |
|
|
Validation record for
|
| Ablation | Assertion that fires |
|---|---|
Drop the plain-set subtraction (!($0 in plain) → 1) |
a token ALSO occurring plainly is NOT labelled blob-embedded |
Empty the blob set (!($0 in plain) → 0) |
the pre-existing a token inside a base64 run is labelled blob-embedded |
The second direction is the one that matters for scope: it proves the subtraction did not quietly disable the label, which a one-sided ablation would have missed.
Tried as a user, and the result is honestly negative. I ran the real script over the live 2-day corpus, then ran the pre-fix version over the same corpus and diffed the credential tables: identical. So on current data this changes nothing an operator sees — no regression, and no live occurrence of the defect in that window either. The defect is a latent fail-open proven by fixture, not one currently firing; I am not claiming a measured improvement it did not produce.
Both runs exit 0. Shellcheck reports no new findings — the four warnings on this file (lines 888, 1362, 1384, 1487) all pre-date this change and lie outside the edited region.
CodeRabbit rate-limited at this head (09:22:42Z, no window stated). Advancing to Codex. @codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Codex reports its usage limit reached. Advancing to Bugbot. |
|
@cursor review |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_4dfbc5b4-5795-43d0-ae44-2fe37801ac7a) |
devantler
left a comment
devantler
left a comment
There was a problem hiding this comment.
🤖 Generated by the Agentic Engineer
Self-review (fallback — CodeRabbit, Codex and Cursor Bugbot unavailable)
Reviewed commit: af19ace5334409623af4afba6af5c0a26a2f1f25
Per-lane state at this exact head — all three tried, none will deliver:
| Lane | Outcome | When |
|---|---|---|
| CodeRabbit | Review rate limited, no retry window stated |
2026-08-07T09:22:42Z |
| Codex | You have reached your Codex usage limits for code reviews — no window; needs account credits |
2026-08-07T09:35:28Z |
| Cursor Bugbot | check-run neutral / Error, Bugbot couldn't run - usage limit reached — admin must raise the limit |
2026-08-07T09:36:15Z |
Two are billing limits and one is a rolling rate limit; none is a judgement about this change, and none states a window that would clear inside this run.
What I checked
The subtraction is correct at the boundaries, not just the happy path.
- Empty plain set —
getlineon an empty file yields nothing,plainstays empty, every blob value keeps its label. This is why the plain set is read as a file rather than joined withNR==FNR, which would eat the first data line when the joined file is empty. - Ordering —
$CREDPLAINis written before theawkthat reads it, so the subtraction can never run against a stale or absent file. - Comparability — the fix relies on the blob and plain legs normalising to the same string for the same credential. That property already underpinned the original
$0 in blobtest; it is not newly assumed, and the new fixture exercises it directly.
Shell-level safety. The script runs set -uo pipefail (no -e), so the added cred_plain_matches | cred_normalise pipeline exiting non-zero on an empty corpus cannot abort the run. Verified by running it against an empty corpus: exit 0, table renders as clean.
Value hygiene. $CREDPLAIN holds normalised credential values, so it is created with mktemp (mode 600) and added to both the EXIT and the HUP/INT/TERM traps, matching every other credential scratch file. It is never printed.
Decode-pass budget preserved. The change adds no second decode pass — the batching contract test, which asserts exactly one credential-table jq invocation, is among the 533 passing.
Observation (pre-existing, deliberately not fixed here)
If mktemp fails partway through the temp-file block, the files created before it leak, because the traps are installed after the whole block. My new line participates in that pattern but does not introduce it — every scratch file in that block has the same shape. Fixing it belongs in its own change, not smuggled into a correctness fix.
Verdict
Verdict: no P0/P1 findings.
I held this to the bar I would hold a bot finding to rather than waving it through as my own work; the diff is small, single-concern, and its central claim is proven in both directions by ablation. I also did not overclaim the user evaluation — the pre/post differential on the live corpus is identical, so this closes a latent fail-open rather than changing anything an operator currently sees, and the PR says exactly that.
1 participant
Why
The leak table is how we notice a credential reaching a transcript. Today it can't separate a real one from noise: a token-shaped string that occurs by chance inside an encoded blob lands in the same rows as a genuinely exposed credential. Measured last month, 35 of 113 GitHub-token-shaped matches were chance substrings of encoded data — so roughly a third of the table is noise nobody can tell apart from the thing it exists to catch.
An earlier attempt at this was closed rather than shipped, because it would have made the detector worse in the one direction that matters: it could have marked a genuinely exposed credential as "probably just encoding noise", quietly lowering triage priority on a real leak.
What
Separates the two, but only on positive evidence that the match really sits inside encoded data — never on a guess from a single character, which is what sank the earlier attempt. Anything ambiguous stays in the plain high-signal row.
Nothing is suppressed or filtered out: a labelled row keeps its shape and its count, so the numbers are unchanged and only the triage order improves. The scan cost is unchanged too.
Part of #2522 — this delivers the classification half. The issue's remaining item (having the locator and concentration lines derive from the same filtered input as the table, so the three surfaces cannot contradict each other) is untouched and keeps the issue open.
Blocked — GitHub Actions outage
GitHub reported Actions:
major_outage(verified live at 2026-08-06T22:03Z), so CI cannot run at this head. Validated locally instead: the full contract suite is 520 passed / 0 failed (baseline 515 + 5 new), with the new behaviour proven RED before GREEN and two ablations each firing their own named assertion.