feat(scan): harden the publish scan with 20 detectors, a rule corpus, and a labeled fixture gate - #183
Conversation
… and a labeled fixture gate Move the detector set out of code into src/lib/scan-rules.json as data (id, tier, pattern, description, attribution) so the tenjin server-side ingest gate can vendor the same corpus. scan.ts compiles that data and implements the handlers and algorithmic detectors it names, refusing to start on an unknown handler rather than silently dropping a block-tier rule. Twenty new detectors. Block: BIP-39 seed phrases, otpauth:// TOTP URIs, framing-free OpenSSH private keys, and Supabase/Twilio/SendGrid/Hugging Face/ Vercel/Notion/Linear/Figma/GitLab/Docker/Cloudflare/Databricks token shapes. Warn: RFC1918 and loopback endpoints, collaboration workspace links, cloud resource ids, pasted .env blocks, and a Shannon-entropy catch-all. Placeholder suppression drops docs-shaped matches before they reach the findings list. A labeled fixture corpus holds per-detector precision and recall at 1.0 in CI, enforces the redaction invariant, and pins a ReDoS budget against transcript-scale input, which caught three quadratic patterns (email, internal-hostname, db-connection-uri). Closes #45. Refs #182. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
The failing skill-drift (mirror is in sync) check is the known shared cause owned by #151: the mirror resync fix (97f8650) has not reached main yet, so every fresh branch inherits the drift. Not this PR's diff; no action here. It clears when #151 (or a split-out sync:skill fix) lands on main. All local gates on this PR are green (lint, typecheck, format, 2179 tests, build, pack-smoke). |
|
Merged |
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
vraspar
left a comment
There was a problem hiding this comment.
Review: strong corpus and ReDoS work; the placeholder suppression bypasses the block tier on realistic input
Reviewed against docs/CONVENTIONS.md and the adversarial money/secrets model, verified at 8347a08, run at transcript scale.
What's solid:
- The ReDoS budget test is real and meaningful: it exercises the exact quadratic shapes (uri separators, email locals, nested paths, base64 blobs) on a 200KB single line with a 1000ms budget. I reproduced the guarded email pattern at ~38ms on a 140K-token line, while an unbounded variant already hits ~102ms on a 4K-token line (quadratic, seconds at scale), so the budget reddens on the old pattern with a wide margin.
- The corpus negatives are genuine hard lookalikes, not softballs: the 11-word BIP-39 boundary,
getUserProfileByAccountIdentifier2vs entropy, an all-lowercase git SHA,AKIALOOKSLIKE,sk-user-profile-...,SKU12345. Precision/recall 1.0 over 121 samples is earned on this set. - Licensing is honest: gitleaks (15 shapes), secretlint preset-recommend (exactly the 8 non-gitleaks shapes, each with a matching
sourcefield), BIP-39 wordlist credited to scure-bip39, and TruffleHog (AGPL) explicitly disclaimed. NOTICE matches the rule data. - The compile step throws on an unknown tier / algorithm / skip / excerpt handler rather than silently dropping a block rule (scan.ts compileLineDetectors / compileExcerpt).
- The skills-text guard now reads
scan-rules.jsondirectly instead of scraping source and inferring tier by proximity — a strictly better coupling.
Major
-
security: the docs-placeholder suppression bypasses the block tier on realistic secrets:
isDocsPlaceholderrunsDOCS_PLACEHOLDER.test(m[0])— a SUBSTRING test over the whole match — ahead of every detector including block-tier ones (,Line 221 in 8347a08
). Because the pattern isLines 250 to 255 in 8347a08
x{6,}|<[^<>]*>|\{\{[^{}]*\}\}|...unanchored, any placeholder-shaped SUBSTRING inside a match suppresses the finding. I ran these against the real scan and every one produced ZERO block findings while the clean control blocked correctly:postgres://appuser:Str0ng<Pw>Value@prod-db.acme.com:5432/main— password with angle brackets, no block.mysql://root:Pa{{ss}}w0rd99@10.2.3.4:3306/app— password with braces, no block.postgres://appuser:realpwxxxxxx99@prod-db.acme.com/main— password with a 6-xrun, no block.Authorization: Bearer ghp_realtokenxxxxxx0123456789abcdef— bearer token with an embedded 6-xrun, no block.
The db-connection-uri case is the sharp one: passwords legitimately contain
<,>,{,}, and a connection string is one of the most common ways a live secret lands in a transcript — exactly the input the session-observer gate must hold on. This defeats the file's own stated guarantee that block findings "refuse a publish in every mode and are never--yes-clearable". Root cause: this is the same substring-vs-whole-value class the PR already fixed forisPlaceholder/PLACEHOLDER_WORD(whole-value anchored, scan.ts#L380-L395, pinned at scan.test.ts#L227-L238) but did NOT fix forisDocsPlaceholder. Fix: don't let the docs-placeholder test suppress a block-tier finding, or apply it to the captured secret value anchored (as the per-detector skip handlers already do) rather than as a substring of the whole match. The existing coverage misses this because the placeholder-suppression tests only assert an all-xor all-Zbody, never a real block token/URI carrying an embedded placeholder substring () — add the embedded-substring case as a regression pin with the fix.tenjin-agent/src/lib/scan.test.ts
Lines 663 to 685 in 8347a08
Minor
-
backend/agent-usability: safety-model and changeset overstate the block guarantee: docs/safety-model.md now says the scan blocks "connection URIs with an embedded password ... in every mode" (
), and the changeset frames block-tier placeholder suppression as a clean feature. Both are downstream of the Major: until the substring bypass is closed, the doc claims a non-bypassable block the code does not deliver for bracket/brace/tenjin-agent/docs/safety-model.md
Line 30 in 8347a08
x-run passwords. Fixing the Major makes the docs true; no separate doc change needed if it lands. -
hygiene: source comments cite ephemeral review rounds: scan.ts#L299 and scan.ts#L343 carry
(review r5)provenance in code comments (the invariant they explain is load-bearing; the round citation is not). Per the anti-slop provenance rule, keep the "why" and drop the round tag. (Test-name(review rN)tags are fine — they document which regression each test pins.)
Nits (1), none blocking
- scan.ts comment density is ~28% (198/696 non-blank), above the 12–15% target. Most of it is genuinely load-bearing "why" for security regexes (ReDoS bounds, charset exclusions, span-alignment fixes), so this is a light note, not a demand — a pass could thin the narration around the straightforward handlers.
Verified, not issues
- Token-charset detectors (aws, github, stripe, jwt, npm, etc.) are NOT bypassable via
<...>/{{...}}because their charsets exclude those characters; the only vector into them is a 6-xrun, which a real working key cannot be made to contain on demand and hits by chance at negligible probability. The db-connection-uri and bearer detectors are the real exposure (broad value charsets), which is why the Major centers on them. - ReDoS: guarded email/db-uri/internal-hostname patterns all bounded; budget test runs at 200KB single-line scale and passes fast; reproduced the guarded-vs-unbounded gap directly.
- Redaction invariant: block excerpts masked (pem armor the one header-only exception), corpus test asserts no excerpt carries the labeled secret; the email-inside-db-uri suppression prevents a password leaking through the email excerpt.
- BIP-39: whitespace-only 12-word run; 11-word and punctuation-broken prose are negatives. Residual false-positive risk on transcript-scale prose or a crypto tutorial that lists 12+ wordlist words is an owner-accepted, documented tradeoff (scan.ts comment) and fail-safe (blocks publish, never leaks) — not a defect.
- Pin: a13e98e is an ancestor of head; scan-rules.json byte-identical since the pin, so tenjin#723's vendored copy is current.
- Local run: scan.test.ts + scan.corpus.test.ts + skills-text.test.ts green (168 tests); CI green at head including pack-smoke and skill-drift.
Verdict: comments-only. One Major — the placeholder suppression is a substring test that silently converts a block-tier finding into nothing for connection URIs whose password contains <, >, {, }, or a 6-x run, defeating the non-bypassable-block guarantee on exactly the transcript-scale input this corpus is being built to gate; the same bug class the PR fixed elsewhere. Two Minors (a doc that overstates until the Major lands, review-round provenance in two source comments) and one density nit. Corpus, ReDoS, and licensing work are solid.
The docs-placeholder rule is a substring test over the whole match and ran
ahead of every detector, so any secret merely CONTAINING a placeholder shape
was suppressed instead of blocked. Real passwords carry `<`, `>`, `{`, `}` and
letter runs, and a connection string is one of the commonest ways a live
credential reaches a transcript, so this defeated the non-bypassable-block
guarantee on exactly the input the observer gate exists to hold.
Suppression may now downgrade a warn, never a block. Block-tier suppression
stays per-detector and anchored to the captured secret value, which is the
whole-value form the rest of the file already uses. Adversarial fixtures pin
all four reported bypasses plus a `your…`-label variant, in scan.test.ts and in
the corpus; they are red without the one-line tier guard.
An all-x body in a block-tier shape now blocks rather than suppressing, which
is fail-safe: the corpus sample moves from negative to positive.
Also drops two review-round tags from source comments, corrects the changeset's
claim that suppression covered the block tier, and tightens the module header.
Refs #183 review 4965955596.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Thanks — the Major is correct and it was the sharp one. Reproduced your four inputs verbatim against the scan before touching anything: all four returned 0 block findings, the clean control returned 1. Fixed in
Major — placeholder suppression bypassed the block tier. Fixed as you framed it: the substring test may now only ever suppress a
Minor 1 — safety-model / changeset overstated the guarantee. Agreed, and as you said the doc becomes true once the Major lands, so Minor 2 — review-round tags in source comments. Fixed. Dropped Nit — comment density. Partially taken. I tightened the module header, which was the one place genuinely duplicating what NOTICE.md and the data file's Gates on |
…log carries An advisory replay of the deterministic tier over 389 published posts (tenjin#723) found the block tier's only hits were three false positives, all public hex constants: a committee hash with a word between label and value, an EIP `source_id =` identifier, and the ERC-20 Transfer event topic0 in a code sample. Roughly 0.8% of the real catalog would have been refused at publish. The demotion to the warn hex32-value now recognizes an id-class label set (salt, id, topic, root, digest, commitment alongside hash/tx/blockhash), a label sitting up to two short tokens before the value, and a data list of universal public constants. Every constant is computed, not transcribed: the topic0s are keccak256 of the event signature and the EIP-1967 slots are keccak256(label) - 1. The floor holds. Demotion is to warn, never silence, so a real key mislabeled `hash` still surfaces. An unlabeled bare 64-hex, a label four tokens out, and a word merely ending in a label all still block, each pinned. Bounding the lookback to 256 characters also closes a fourth quadratic: the label test re-sliced the whole line per match, so a line dense with 64-hex values took 18.6s. It now takes 63ms, with a ReDoS fixture pinning it. Refs #183, tenjin#723 comment 5335652890. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Implemented the detector fix proposed in the pre-flip replay (tenjin#723, comment 5335652890 — 389 published posts, 3 block-tier hits, all false positives). Commit
What changed, against your three triaged FPs:
The floor holds, exactly as you scoped it. Demotion is to the warn Constants are computed, not transcribed. Generated with viem: topic0s are Coverage: the three catalog shapes plus a CREATE2 Bonus, and it would have bitten you at scale: adding a dense-64-hex ReDoS fixture exposed a fourth quadratic. The label test re-sliced the whole line per match, so a line carrying 3,000 hex values took 18.6s. Bounding the lookback to 256 chars makes it 63ms (295x), with the fixture pinning it. This predates the widening — the narrower regex just hid it — so it was latent in what you replayed. Gates on On the operator call in your comment: with this landed there is no ~0.8% FP left to accept, so holding for the widening should no longer cost anything. |
The live hosted skill moved again tonight (the #178 multi-searchId wire form), so the drift gate reddened on this branch, which merged main before the move. Mirror-only: scripts/sync-skill.mjs writes skills/tenjin/SKILL.md and nothing else, so the scan rule data, the corpus, and the tenjin-publish detector lists are untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Resynced the vendored hosted-skill mirror in |
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Post-b09a57f gap from the tenjin#723 catalog replay: the widened hex32 demotion still misses one real published post whose shape is |
… and value The tenjin#723 replay re-run at b09a57f cut block-tier hits from 3 to 1. The survivor is a real published post writing its hash as inline code — hash was `0x…` — where a backtick sits between the label window and the value, and neither separator class in HASH_LABEL_RE carried backticks or quotes. The b09a57f fixture for that shape had dropped the formatting the post carries, so the corpus agreed with the bug; it now keeps the backticks. Only the two SEPARATOR classes gain ` ' " and the curly quotes. The label set and the two-token bound are untouched, which is what keeps the floor: no amount of punctuation tolerance can invent a label that is not in the set, so PRIVATE_KEY=`0x…` in backticks still blocks, as does `the wallet key is "0x…"`. Both are pinned, alongside the existing out-of-window and word-ending-in-a-label cases. Refs #183, tenjin#723 comment 5335789613. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Closed the last survivor from the replay re-run (comment 5335789613) in The fix is separators only. Both classes in You were also right that the fixture was complicit: Floor cases added, per your note that they should be unaffected — pinned rather than assumed:
Red without the fix, verified by reverting just the two character classes: 3 tests fail (the new backtick/quote test plus both corpus precision assertions), 103 pass with it. The Gates on |
vraspar
left a comment
There was a problem hiding this comment.
Re-review: the block-tier bypass is fixed and pinned on the embedded-substring cases; the catalog-driven demotion widening holds its floor, with one label-breadth decision worth a look
Reviewed at 68e6d2e, delta over 8347a08. Three fix commits (placeholder-tier confinement, then two 64-hex-demotion widenings from the tenjin#723 pre-flip replay) plus a mirror resync. Re-verified by reproducing the round-1 bypass inputs and a fresh adversarial set against the built scan.
Resolved since round 1
- [Major] Placeholder suppression confined to the warn tier.
isDocsPlaceholderis gone;isSuppressedAsDocs(detector, m)gates the substring test ondetector.severity === 'warn', so the block tier keeps only its per-detector skips, which are whole-value-anchored on the captured secret. Re-ran the four round-1 inputs plus the all-x body against the fixed scan: the angle-bracket password, brace password, 6-x-run password, embedded-xbearer token, andghp_+ 36xall now BLOCK (5/5), and each block excerpt is redacted (no leak). Warn-tier suppression survives — the corpus precision/recall stays 1.0, so the FP-reduction purpose is intact — and the all-x-in-a-block-shape sample was relabeledneg-placeholder→pos-placeholder-body(fail-safe: refuses a publish, does not leak). The regression pinscan — the block tier is non-bypassablecovers the embedded-substring cases explicitly, not just all-x, and 3 tests go red when the one-line guard is reverted. - [Minor] docs and changeset now state the truth. safety-model.md and the changeset now say "Warn tier only: the block tier stays non-bypassable, and its own suppressions are anchored to the captured value," and spell out that a password containing
</>/{/}/anxrun, an unlabeled bare 64-hex, and a secret-named assignment all still block. - [Minor] Review-round provenance removed. Both
(review r5)code comments are gone (zero remain in scan.ts).
New — the two catalog-replay commits (1f066b5, 68e6d2e)
Both are false-positive fixes derived from real published content: the tenjin#723 pre-flip replay over 389 published posts found 3 block-tier hits, all genuine FPs on public hex constants (an ERC-20 Transfer topic0, an EIP source_id, a hash was 0x… mention), then 1 survivor (a backticked `0x…`), then 0. The fixes widen the 64-hex→warn hex32-value demotion: the label set gains salt/id/topic\d*/root/digest/commitment, a two-short-token window between label and value, a publicHexConstants data list (12 keccak values, computed via viem not transcribed), and markdown/quote punctuation as separators. Verified the floor holds against the real scan: bare 0x…, PRIVATE_KEY=0x… (+ backtick form), "privateKey": "0x…", and the wallet key is "0x…" all still BLOCK, while legitimate public values (a tx hash, the ERC-20 topic0 constant) demote to warn. Demotion is to warn, never silence.
Decision (non-blocking, owner's call)
- The demotion label set now includes broad, innocuous words —
idmost of all — so a 64-hex mislabeled with one rides down to warn. Confirmed against the real scan:id = 0x<64-hex privkey>,salt: 0x<privkey>,digest 0x<privkey>,my backup id is 0x<privkey>, andkey_id: 0x<privkey>all demote fromraw-private-key(block) tohex32-value(warn). This is the same ambiguity-class tradeoff accepted in round 1 — a private key and a 32-byte public value are byte-identical on Base, so the scan can only guess from context — now with a wider label set. It is not a silent-leak bug: demotion surfaces the finding and requires an ack under enforce, the common accidental shapes still block, and the widening is evidence-driven from real FPs. The one thing worth a deliberate look is thatid(andkey_id, which the_-boundary catches) is common and innocuous enough that an accidental key-under-an-id-field leak —{"id": "0x…"},key_id: 0x…— demotes to warn, and under the DEFAULT advisory mode a warn publishes with only a logged finding. Options to tighten: dropidspecifically, or withhold the demotion when the value also sits in a secret-named /*_keycontext. Not blocking on it — the ambiguity is fundamental and round 1 accepted the principle — but it is the residue of the widening, so here is the concrete behavior.
Corpus / pin lockstep with tenjin#723
scan-rules.json changed in this delta (the new publicHexConstants array), and scan.ts changed (the Major fix plus the demotion widening). Note for #723: its drift check pins only the JSON data files, so advancing its sha256 pin to 68e6d2e syncs the data, but #723's hand-ported scan.ts must independently receive BOTH the isSuppressedAsDocs tier fix and the HASH_LABEL_RE/publicHexConstants demotion logic — exactly the "code outside the drift check diverges silently" gap from round 1. Verification of #723's copy happens in that PR's re-review.
Verdict
Approve. The Major is fixed at the tier boundary and pinned red-without-fix on the embedded-substring cases; both Minors are closed; the demotion widening is evidence-driven with the floor preserved and demotion never silencing. The one decision (label breadth, esp. id) is a considered tradeoff the owner may want to revisit, not a defect. Ran scan + corpus + skills-text suites (176 passing); CI green at head.
Drops the per-detector genre examples from the first triage list, which repeated "in a <kind> piece" six times to say one thing, and folds the second list's trailing sentence into its lead. Every detector name the skills-text coverage guard reads is unchanged. Also corrects the intro while it is shorter: it claimed rights and employer-internal content "have no detector at all", which was already untrue of paid-content-marker and confidential-marker, and it now names seed phrases in the blocking tier. Refs #183 inline comment 3809528654. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Resolves two conflicts against the alpha release, the hooks rename, and the skill mirror resync on main. - src/lib/scan.ts: keep both additions. This branch's BIP-39 and entropy helpers land beside main's survivesTeamDrop, whose three named warns (secret-assignment, hex32-value, embedded-instruction) all still exist in the rebuilt detector set. - skills/tenjin-publish/SKILL.md: keep main's teamMode arm and its closing marker, with this branch's rewritten warn triage in the else arm. - src/skills-text.test.ts: re-pin the public tenjin-publish render, which moves on purpose now that the triage names the new detectors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013TE4zSrBc7Qw1qPodTGCT9
vraspar
left a comment
There was a problem hiding this comment.
Re-review: the merge is clean, but the two new credential catch-alls never joined survivesTeamDrop, so team mode drops them on the unattended path
Reviewed at 9a1e157, delta over 68e6d2e (round 2, approved): commit 4231f5a (publish-skill condensation) and the merge of origin/main, which brought #201's team mode into the file this PR rewrites. Code read only; CI, Greptile and skill-drift green at head; clean merge-tree against current main. Nothing from rounds 1 and 2 is re-raised.
Resolved since round 2
- The publish skill's scan triage is condensed as asked (4231f5a): the per-detector genre examples are gone, the block tier now names seed phrases, and the false "rights and employer-internal content have no detector at all" sentence is corrected (
paid-content-markerandconfidential-markerexist as warn rules).
Still open
- The demotion-label breadth decision (
idmost of all) from round 2 stays the owner's call. One new input: the Stop hook now generates notes from transcripts and JSON tool output, where a 64-hex under a literalid:key is far likelier than in a hand-written post, and the dogfood runsfull-auto, which clears the survivinghex32-valuewarn unseen. Ifidwas ever going to leave the label set, this is the argument.
New
Major
- security: add
high-entropy-stringandenv-dump-blocktosurvivesTeamDrop: the predicate came in from main (#201) as a hardcoded list written against main's detector set (block +secret-assignment+hex32-value+embedded-instruction), and its docstring says the question that survives a team drop is "is this a live credential", justifyingsecret-assignmentas "a live key whose shape no block detector matches". This PR adds exactly two warn detectors that answer that question and did not exist on main:high-entropy-string(the PR's own "catch-all behind the named shapes", which fires only when no named detector did) andenv-dump-block(three or moreKEY=VALUElines with substantial values).secret-assignmentdoes not backstop them, since it requires a secret-named key (API_KEY|SECRET|ACCESS_KEY|PRIVATE_KEY|PASSWORD|TOKEN|CREDENTIALS|AUTH_TOKEN), soSEGMENT_WRITE_KEY=<base62>,SENTRY_DSN=..., orAuthorization: Basic <base64>in a team note yields only the two new warns, both filtered at publish.ts:247 and edit.ts:232 beforewarnsis computed, so no prompt inrevieworautoeither. The Stop-hook capture path is exactly the transcript-and-config-paste input these detectors were built for. Not a regression (main had no such detector), but a merge-time semantic gap in the file this PR rewrote, and nothing pins it: no test namessurvivesTeamDropagainst the rule set, and the skills-text corpus guard rendersteamMode=false. Fix options: add the two ids, or move the decision into the data as ateamSurvivesfield onscan-rules.jsonso the next credential warn joins by construction; either way docs/safety-model.md:43, docs/command-reference.md:380 and the skill's team arm ("exactly four survive") need the count updated.Lines 724 to 767 in 9a1e157
Minor
- integrity: pin the survivor names to real check ids: all three names resolve today (
secret-assignmentandembedded-instructionare rules;hex32-valueis emitted only by the demotion literal at scan.ts:491 and has no rule entry, hand-added to the corpus id set at scan.corpus.test.ts:49). It is now named by string in five places; rename the demotion literal and team mode silently stops surfacing 64-hex key material with every test green, the same silent-shrinking-set failure this PR's skills-text guard rewrite was built to prevent. One assertion that every name insurvivesTeamDropis a corpus rule id or a known emitted check closes it.Lines 760 to 767 in 9a1e157
- agent-docs: the team arm's block-tier enumeration is now the stale one: 4231f5a corrected the public arm (109-110) to add seed phrases and safety-model.md:38 lists five families, but the team arm at 86-87 still reads "provider token formats, private keys, connection URIs with an embedded password". Three surfaces, three enumerations of one tier.
tenjin-agent/skills/tenjin-publish/SKILL.md
Lines 84 to 100 in 9a1e157
Nits (2), none blocking
- skills/tenjin-publish/SKILL.md:117-119: 4231f5a dropped the qualifiers ("
high-entropy-stringwhere the piece quotes an opaque handle", "private-network-endpointin a local-dev walkthrough") from the "usually fine" bucket, so the bucket now reads as unconditional, five lines under "a secret with no recognizable shape is a prompt to look". The bucket membership itself predates this delta.tenjin-agent/skills/tenjin-publish/SKILL.md
Lines 112 to 119 in 9a1e157
- src/lib/bip39-wordlist.json:4 is a single 2046-word wordlist line, so the repo ships a file that hard-blocks its own publish scan: a team note that pastes that line or a diff of it is permanently unpublishable to the shelf. Fail-safe and narrow; noted because the capture loop makes "write up what you just worked on" the common case and this PR is what it just worked on.
Verified, not issues
- Merge fidelity: main's 45-line
survivesTeamDropblock is present verbatim;git diff 4231f5a..9a1e157 -- src/lib/scan.tsis that block and nothing else, so no PR-side detector, skip handler, or round-2 tier fix was lost. scan.ts exports and signatures are unchanged main to head, and no post-68e6d2e main commit (sidecar, hooks, push-scripts) calls into scan. - SKILL.md:
tenjin:when teamMode/elsearms intact; the team survivor list matches the predicate exactly; the public arm's two triage lists cover all 17 warn ids with no extras. - BIP-39 prose false positives, measured: of ~50 common English function words only "a" is in the wordlist, capitals and attached punctuation break a run, and a token walk over ~93k lines of real prose-about-code in this workspace produced no run longer than 7 outside intentional test fixtures. The owner-accepted tradeoff from round 1 holds.
- Block-tier FP survey on team-shaped content:
openssh-private-keymatches the base64 body magic, not the armor header;db-connection-uriskips the default passwords;bearer-token's placeholder skip is whole-value anchored. Onlytotp-urihas no skip and would block a note quoting the canonical Google Authenticator docs example; very narrow. embedded-instructionpatterns are byte-identical to main, and the warn-tier placeholder suppression cannot reach them, so team mode surfaces neither more nor less injection text than before.- A refused capture is loud, not silent: the Stop hook hands the agent the
tenjin publishcommand, andPUBLISH_BLOCKEDplus its fix lands in the agent's transcript; the block tier stays fail-safe. The dropped-warn path in the Major is the silent one. - The tenjin server's
lib/ingest-scan/rules.jsonon main is byte-identical to this head'sscan-rules.jsonand its drift check is pinned to this PR's head commit, so merging unblocks repointing that pin tomain.
Verdict: comments-only for this round. The delta itself (skill text and the merge) is clean; the Major is what the merge exposed: the team-drop predicate was written before these detectors existed and now filters the two that matter most on the unattended path. One-line fix plus a pin, or the data-field form. Approve once the survivor set is updated and pinned.
|
Correction to the Major in the review above: "no prompt in |
survivesTeamDrop arrived from #201 as a hardcoded list written against main's detector set, so this branch's two credential catch-alls never joined it: a team note pasting SEGMENT_WRITE_KEY=<base62> or a .env block surfaced nothing under `auto` and published unattended under `full-auto`. The survivor set is now a `teamSurvives` flag on the rule in scan-rules.json, so the next credential detector joins by marking itself. hex32-value is named explicitly in scan.ts: it is emitted by the hex64 demotion, not by a rule. Pins: publish tests that a team-mode `auto` scan of an env dump and of an unrecognized high-entropy token both stop; corpus tests that every survivor name resolves to a rule id or a known emitted check, and that the emitted one is still emitted. Docs: safety-model.md, command-reference.md and the publish skill's team arm carry the new count, and the block tier is now enumerated the same five families on all three surfaces. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013TE4zSrBc7Qw1qPodTGCT9
|
All four items addressed in 884df83. Every claim in the review verified against the tree before changing anything. Major,
Your correction is the framing I used in the code and the tests: Pins ( Minor 1, survivor names. Minor 2, block-tier enumeration. Unified across all three surfaces at five families, including TOTP provisioning URIs, which the public arm was missing too: Docs. Nits. Took the first: the "usually fine" bucket has its qualifiers back plus an explicit statement that "genuinely about them" is the whole condition ( Security invariant, checked: the public path cannot move, since Gates: One heads-up for whoever merges second: PR #185 also edits |
|
Correction to the line references in my previous comment. The content stands, the numbers were off. Accurate ones against 884df83:
Everything else in that comment, including the file paths and the invariant and gate results, is as written. |
vraspar
left a comment
There was a problem hiding this comment.
Re-review: survivor set is data now, pinned both ways; approve
Reviewed at 884df83, delta over 9a1e157 (one commit). Code read; CI, Greptile and skill-drift green.
Resolved since round 3
- [Major] survivor set:
teamSurvives: trueonsecret-assignment, bothembedded-instructionentries,env-dump-block,high-entropy-string;survivesTeamDropisblock || TEAM_SURVIVOR_CHECKS.has(check);compileLineDetectorsthrows on the flag outside the warn tier. Strict superset of the old set,survivesTeamDroponly called underteamMode, no detector or tier changed, so the public path cannot move. Two team-modeautopins on aSEGMENT_WRITE_KEYenv dump and the same token loose in prose, each producing exactly the warn under test. - [Minor] survivor names pinned: corpus test resolves every name to a rule id or a known emitted check, asserts
hex32-valueis still emitted by a real scan, and floors the five credential/injection checks as a subset check. - [Minor] block-tier enumeration: unified at five families across team arm, public arm and safety-model.md; public digest re-pinned with the reason.
- [Nit] "usually fine" qualifiers: restored.
Closed as decisions
- The wordlist line self-blocking: left as fail-safe rather than a path exemption. Agreed.
- Demotion label breadth (
id): stays the owner's call, unchanged across rounds.
Verdict: approve. Note for the merge order: tenjin main's lib/ingest-scan/rules.json is pinned to this PR's head, and scan-rules.json changed in this commit (the flag), so the server pin should be advanced to main once this lands. #185 also edits skills/tenjin-publish/SKILL.md and docs/safety-model.md; whichever lands second needs a small doc reconcile.
Second lander after #183 (scan hardening) and #187 (builder code). Three conflicts, all resolved by uniting the two intents rather than picking a side. docs/safety-model.md: keeps #183's masked-excerpt paragraph and its widened team arm (five warn survivors, `teamSurvives` as data), keeps this branch's local-first/`--excerpt` coverage sentence, its consent paragraph, and its team-shelf-stays-advisory paragraph. skills/tenjin-publish/SKILL.md: takes #183's five-check survivor list and keeps this branch's correction to the same sentence, that the survivors are not the only findings there are once the shelf scans at ingest. skills-text.test.ts: both re-pin notes kept, digest re-pinned for the merged render. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013TE4zSrBc7Qw1qPodTGCT9
Scope
Hardens the deterministic publish scan with 20 new detectors and a labeled fixture corpus that measures per-detector precision and recall in CI, so a detector edit shows its false-positive cost before it merges.
src/lib/scan-rules.json: the detector set moves out of code into data (id, tier, pattern, description, per-rule attribution).src/lib/scan.tscompiles it and throws on an unknown handler rather than silently dropping a block-tier rule. The tenjin server-side ingest gate vendors this same file.src/lib/scan.ts: block tier gains BIP-39 seed phrases,otpauth://TOTP URIs, framing-free OpenSSH keys, and 12 provider token shapes; warn tier gains RFC1918/loopback endpoints, collaboration links, cloud resource ids,.envdump blocks, and a Shannon-entropy catch-all. Placeholder suppression drops docs-shaped matches (sk-xxxx,<YOUR_KEY>,user@example.com) including for block-tier detectors, which is the one bypass risk here; it is narrowly scoped toxruns, angle/brace templates,your…labels, and the RFC 2606 reserved domains, and pinned by tests.src/lib/scan-corpus.json+src/lib/scan.corpus.test.ts: 121 labeled samples (positives and benign lookalikes for every detector, plus an adversarial transcript-shaped sample) hold precision and recall at 1.0, enforce the redaction invariant, and pin a ReDoS budget against transcript-scale input.Notes
The ReDoS audit found and fixed three quadratic patterns:
email,internal-hostname, anddb-connection-urieach had an unbounded run that backtracked against a following obligation. On a 200KB single line (the shape a JSONL transcript record takes) the worst detector now costs 39ms; before, two of them took 25 and 31 seconds.The rule port was diffed against secretlint's
preset-recommend. Eight shapes present there and absent from the gitleaks subset are now included and credited in NOTICE.md, along with the BIP-39 wordlist. Nothing derives from TruffleHog (AGPL).Credential literals in the corpus are stored split into thirds and rejoined at test time, so the committed bytes carry no contiguous token shape. GitHub push protection rejected the first version of this branch, which is the correct behaviour and the reason for the split.
src/skills-text.test.tshad a guard tying the publish skill's warn-triage lists to the detector set. It fired on this change; the skill's two lists now name the five new warn detectors, and the guard readsscan-rules.jsondirectly instead of scraping source and inferring each tier from proximity.Testing
pnpm lint,pnpm typecheck,pnpm format:check: clean.pnpm test: 2179 passed, 10 skipped, 69 files.pnpm buildandbash scripts/pack-smoke.sh: pass (the corpus JSON is bundled intodist, not shipped loose).Closes #45. Refs #182.
Greptile Summary
The PR moves publish-scan definitions into a shared data corpus and substantially expands secret, credential, private-context, and entropy detection while adding labeled precision, recall, redaction, and performance fixtures.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Publish or edit content] --> B[Load embedded scan rule corpus] B --> C[Run regex and algorithmic detectors] C --> D[Apply placeholder suppressions and deduplicate] D --> E{Team shelf?} E -->|Yes| F[Keep blocks and teamSurvives warnings] E -->|No| G[Keep all findings] F --> H{Blocking finding?} G --> H H -->|Yes| I[Refuse publication or edit] H -->|No| J{Consent required for warnings?} J -->|Yes| K[Return findings for confirmation] J -->|No| L[Proceed to signer and remote write]Reviews (2): Last reviewed commit: "fix(scan): move the team-shelf survivor ..." | Re-trigger Greptile
Context used (3)