feat(publish): answer the marketplace scan gate instead of failing on it - #185
Conversation
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
b498ef0 to
b7edb15
Compare
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Rebased onto Two files conflicted, and both were semantic rather than textual, so recording how each resolved:
No What ran at this head: Still |
The tenjin backend now runs the publish scan server-side in its shared write path (tenjin#723), so a publisher not running this CLI is gated too. Its refusals reached `publish` and `edit` as opaque post-consent write failures, which is the wrong shape for a decision the operator can act on. Both writing commands map the gate's two codes into the consent flow they already have: `scan_blocked` is a hard exit-3 failure with the redacted findings and no acknowledgement path, and `scan_needs_ack` merges the server's findings with the local scan's, deduped by detector and offset, renders them once, and re-runs the identical content carrying the ack token on an explicit yes. Mode semantics are the local ones: review and auto stop, full-auto acks, and `ackServerWarnings: false` never acks whatever the mode says. Findings travel as data, so a detector this release predates renders faithfully and the server stays authoritative under version skew. Advisory findings on a success response reach the receipt and the human summary as information. Refs #182, tenjin#723. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A raw NUL byte in the source made git treat scan-gate.ts as binary, so its diff never rendered for review. Same key, written as an escape. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The same-machine publish dedup landed on main keyed on the body's content hash, so the auto half of the server-warn test republished the exact bytes the full-auto half had just published into the same dataDir and returned alreadyPublished before reaching the gate. Vary the second body. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013TE4zSrBc7Qw1qPodTGCT9
b7edb15 to
6a1ac17
Compare
…r collapse The server reports line and span relative to the submitted field it matched in, so the offset key does not identify a finding across fields: two different secrets in a one-line title and a one-line excerpt land on the same line and often the same span. Collapsing them dropped the second from the render while the ack still covered it. Server findings now dedupe on detector plus excerpt alone, which still collapses one secret repeated across fields and still collapses against a local finding by offset. 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.
Review: the wire and the once-only retry are right; --yes acks server findings the operator never saw, which inverts the mode ladder the changeset promises
Reviewed against docs/CONVENTIONS.md and the consent model, at eb39bad, with the merged server side (tenjin#723, lib/ingest-scan/gate.ts, ack.ts, lib/posts.ts) read alongside. Code read only; CI and Greptile green at head; clean merge-tree against current main.
What's solid:
- Wire shape matches end to end:
scan_blocked/scan_needs_ackat 422 withdetails.scan.{findings, checks.semantic, ackToken}, finding fieldscheck/severity/line/span/excerpt/field,scanAckaccepted by both strictObject post schemas (max 2048), successscantop-level and visible through the passthroughownPostSchema. - The retry is sound: the body is rebuilt from the same
inputwith onlyscanAckadded, which is exactly the title/excerpt/body setack.tshashes;auth.headersForruns per attempt so the fresh-nonce requirement holds; a secondneeds_ackcannot loop. scan_blockedis terminal in every mode andparseScanRejectionstrips a token off a blocked envelope, so no client path can ack a block. The local block gate still runs before any network call, team mode included.- No secret echo: server block-tier excerpts are redaction-pinned server-side, semantic excerpts are fixed strings, and both CLI renderers sanitize for the terminal. The ack token never reaches a receipt, details payload, dedup store, or log.
Major
- correctness:
--yesacknowledges server warn findings the operator was never shown:needsConfirmation('review', n)is true for every n, so a review run reaches the network only withargs.yes === true, andacksServerWarnings(mode, yes)returnsyes || full-auto, so the first server contact re-sends with the token. The same holds forautowhenever the local scan had a warn. The--yeswas the operator's answer to a payload of local findings rendered before any server call; the server's set is strictly larger (semantic-private-context, semantic-rights-encumbered, semantic-pii from the judge cannot come from the local corpus), and the operator meets it as an "Acknowledged marketplace scan findings" line after the piece is public. The only configuration where the server hold fires isautowith zero local warns and no--yes. The changeset and theacksServerWarningsdocstring say "review and auto stop on a server warn the same way they stop on a local one"; docs/safety-model.md:41 describes what the code does; the test comment at publish.test.ts:1976 calls the reuse deliberate. Pick one: either make the yes have to post-date a rendered server finding (an ack flag, or only honouryeswhen the local pass rendered nothing to answer), or drop the mode claim from the changeset and docstring. No test covers review/auto + local warn +--yes+needs_ack.andtenjin-agent/src/lib/consent.ts
Lines 69 to 85 in eb39bad
tenjin-agent/src/commands/publish.ts
Lines 283 to 352 in eb39bad
Minor
- correctness: a server finding sharing detector, line and span with a local one is swallowed: the server loop only ever writes
byValue, but the read at line 227 isbyOffset.get(oKey) ?? byValue.get(vKey), andbyOffsetholds local entries only, so a distinct server finding with coincident coordinates collapses into the local one relabelledbothand the operator acks an incomplete set, the one direction lines 190-198 say the merge must not err in. Reachable when the draft has no frontmatter (body and raw line numbers agree).const seen = byValue.get(vKey)completes the commit that renamed the key. Greptile's open comment is right on this half.tenjin-agent/src/lib/scan-gate.ts
Lines 186 to 240 in eb39bad
- correctness: the blocked branch renders local warns as block reasons and shows no tier:
describeRenderedruns over the merged set, so the message reads "blocked by the marketplace scan: 3 finding(s) (local-path, email, aws-access-key)" and the fix tells the operator to remove non-blocking material, while the local block path correctly names onlyblocking.findingLinesprints no severity, so on the blocked envelope (which ships block and warn mixed) nothing distinguishes them. Message from the server's findings only, and print the tier on that path.tenjin-agent/src/lib/consent.ts
Lines 133 to 140 in eb39bad
- correctness: a second
ScanGateErrorescapes raw:flow.send(token)sits in the catch with no wrapper, so a secondneeds_ackor a block on the retry surfacesScanGateError's own details (unmerged server findings, nosource, nofix), and output.ts renders them with no[server]marker, as if local. The non-loop test asserts only the code.tenjin-agent/src/lib/consent.ts
Lines 150 to 157 in eb39bad
- security: the new "a block-tier secret never leaves the machine" claim is false for
--excerpt:scanDraftscansrawpluscardScanText(card), and neither coversargs.excerpt(CLI--excerpt, MCPtenjin_publish({excerpt})), whichresolveExcerptships; frontmatter excerpts are insiderawand are covered, andedit.ts'sshippedTypedTextalready scansargs.excerpt. The gap predates this PR; the doc sentence does not. One line inscanDraftmakes the sentence true.andtenjin-agent/src/commands/publish.ts
Lines 564 to 572 in eb39bad
tenjin-agent/docs/safety-model.md
Line 41 in eb39bad
- agent-docs: the MCP tool descriptions no longer describe what can refuse a call:
tenjin_publishpromises NEEDS_CONFIRMATION "with the exact payload (mode, price, findings, card, target)" gated by "a deterministic local scan and your publish.mode consent"; the scan-gate refusal carries{mode, price, findings, scan}and comes from the server.tenjin_editpromises only "the before/after summary" and never mentions findings. One clause each: the marketplace also scans server-side and may hold with findings.tenjin-agent/src/mcp/server.ts
Lines 428 to 431 in eb39bad
Nits (3), none blocking
- skills/tenjin-publish/SKILL.md:320-321: the team branch says the local survivors are "the only findings there are"; true while the team shelf stays advisory, false if it flips to enforce. The rest of the skill is payload-driven and handles a server refusal correctly without edits.
tenjin-agent/skills/tenjin-publish/SKILL.md
Lines 316 to 324 in eb39bad
- src/lib/consent.ts, src/lib/scan-gate.ts: four new comments cite plan-round labels ("PR 5" x3, "PR 2b") rather than a file or symbol;
tenjin#723is fine. The D38 line is pre-existing.tenjin-agent/src/lib/consent.ts
Lines 76 to 78 in eb39bad
- PR body says 16 scan-gate unit tests; the file has 18.
Decisions
- Team mode and a shelf at
SCAN_WARN_MODE=enforce.throughScanGateruns unconditionally (publish.ts:344, edit.ts:272) with no team branch, and the sidecar README keeps the server's block on the shelf on purpose, so this is consistent with the plan, not a conflict. What the plan does not say: under enforce the shelf's warn tier re-imposes exactly the findingssurvivesTeamDropremoves, and under the dogfood'sfull-autothey are all silently acked and shipped, while underautothe capture loop holds on the first clean-local note. The runbook sets noSCAN_WARN_MODE, so today the shelf is advisory and findings only ride the team receipt. Either write "the team shelf stays advisory" into the runbook and docs/safety-model.md's team paragraph, or add the team branch. No team-mode gate test exists in either command's suite. ackServerWarningshas no production caller. Only the test suite sets it (PublishDeps/EditDeps),PUBLISH_CONFIG_KEYSlacks it, and no flag exists, yet the changeset tells operators to pass it. Withpublish.mode full-autoon the dogfood machines and the Stop hook publishing unattended, every server warn including a semantic PII verdict is auto-acked with no off switch short of changing the mode. Ship the seam here or move the sentence to the PR that adds the caller.
Verified, not issues
ackServerWarnings: falseoverrides the mode on both commands, andpublishPost/updatePosthave exactly two callers, both wrapped.- Advisory findings on a 200 reach the receipt
scanfield and the human summary in both commands; edit drops the raw passthrough when the normalized report is null. - No double publish: the server persists nothing on a 422, so the ack retry cannot duplicate; the lost-response-after-success window is the pre-existing single-POST behaviour.
buildPostUpdateBodyaddsscanAckafter the emptiness check, so a token-only PUT is still "no changes".- RELEASING.md's server-coupled section is accurate:
SCAN_WARN_MODEdefaults advisory and requiresSCAN_ACK_SECRETunder enforce.
Refuted, not posted
- "Sidecar plan conflict: team mode must skip the server gate": the same README bullet keeps the server's block on the shelf on purpose; residual is Decision 1.
- "Skill must document the source marker and advisory lines" as a major: re-running with
--yesdoes send the token with no extra step, and the skill's "render THAT payload's findings" already covers a server refusal; residual is the team-sentence nit. - "Server notice and warn look identical on the hold": the server tier set is block|warn and a
needs_ackenvelope is warn-only by construction; the residual (no tier on the blocked render) is folded into Minor 2. - Greptile's "two server fields collapse": fixed at head, server findings are value-keyed; only the local/server half survives (Minor 1).
Verdict: comments-only. One major (the mode ladder the PR advertises is not what ships: --yes given against local findings acks server-only findings unseen), five minors, three nits, two decisions for the owner. The wire contract and the once-only retry are correct and well tested; the consent semantics need either a code change or an honest changeset before this ships.
A confirmation covers the findings it post-dates. A `--yes` answered a payload built before any server call, so it cannot have covered the marketplace's own findings; `review` and `auto` now hold on a server-only warn instead of re-sending the token, and say in the fix that the yes did not reach it. `full-auto` still acks unasked, which is that mode's contract. Ships the `publish.ackServerWarnings` seam behind it: `off` never acks (the switch a full-auto machine needs without changing its mode), `on` is a standing yes that still requires a yes on the run, `mode` derives as above. Global config only, since it can only loosen what a yes covers. Also from the review: the merge no longer collapses a server finding into a local one on a coordinate coincidence; a block renders the server's findings alone so its fix stays true; the retry's own rejection is wrapped rather than escaping raw without a source marker; `--excerpt` is scanned; the two MCP tool descriptions name the server scan; finding lines print the tier. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013TE4zSrBc7Qw1qPodTGCT9
|
Thanks, this was a good catch on the major: the mode ladder the changeset advertised was not what shipped. All of it addressed at 3059f3a; nothing refuted. Two decisions were made conservatively by the orchestrator on the operator's behalf, and the operator's veto is still pending on both. They are Decision 2 (ship the seam here) and Decision 1 (write the reality into the docs rather than add a team branch). Flagging that explicitly so neither reads as settled. Major 1,
|
| override | config | mode | --yes |
server added unseen | acks |
|---|---|---|---|---|---|
| set | any | any | any | any | the override |
| any | off |
any | any | any | no |
| unset | mode/on |
full-auto |
any | any | yes |
| unset | mode/on |
review/auto | no | any | no |
| unset | mode |
review/auto | yes | yes | no (was yes) |
| unset | mode |
review/auto | yes | no | yes |
| unset | on |
review/auto | yes | any | yes |
full-auto keeps the auto-ack: clearing soft findings unasked is that mode's contract.
On the re-prompt: there is no interactive prompt on this path and I did not add one. Exit 3 with the payload IS this CLI's confirmation channel, and it is what the MCP NEEDS_CONFIRMATION surface and the skill both drive, so a terminal-only prompt would be a second consent channel only one of the two surfaces could answer. That leaves the post-dating question you raised: nothing persists "these server findings were rendered", so a re-run's --yes is indistinguishable from the first one and pointing the operator back at --yes would point them at the same wall. So the honest landing is the config seam, and the held fix says so in that exact case rather than repeating the --yes advice.
Test matrix added at publish.test.ts in a --yes covers the findings it post-dates and no others: review/auto x local warn (WARN carries a wallet-address the local scan flags) x --yes x needs_ack, in three shapes each (server adds nothing new, server adds one, standing yes clears it), plus the two negatives (on does not stand in for a missing --yes; off refuses under full-auto). edit.test.ts gets the matching hold. consent.test.ts carries the decision table itself plus a test asserting no reachable input acks more than the old yes || full-auto rule did, config values included.
Decision 2, ackServerWarnings has no production caller
Shipped the seam. publish.ackServerWarnings in PUBLISH_CONFIG_KEYS, wired through both commands, three values:
offnever acks whatever the mode says. This is the switch afull-autodogfood machine gets without changing its mode.onis a standing yes for server findings, and it still requires a--yeson the run. It restores the pre-gate reading of--yesand never manufactures one, which is what keeps the invariant above true.mode(default) derives as in the table.
Global config only, never the project .tenjin.json layer: the key can only loosen what a yes covers, so a file a cloned repo carries must not be able to set it. That is resolvePublishMode's full-auto project gate applied a step earlier. Changeset rewritten to describe what actually ships. deps.ackServerWarnings stays, above the config, for in-process callers whose answer is not the operator's to configure.
Decision 1, team mode and SCAN_WARN_MODE=enforce
No team branch. The reality is now written down in both places you named: RELEASING.md's coupling bullet ("Team shelves stay advisory", and why flipping it is a change of what a team note may carry rather than a rollout step) and docs/safety-model.md's team paragraph.
Minors
- Fixed.
const seen = byValue.get(vKey)completes the rename, with the reason on the line. Regression testkeeps a server finding that only shares coordinates with a local one; the neighbouring test's name no longer claims offset collapse, since offsets play no part on the server side now. - Fixed. A block renders
mergeScanFindings([], server)for both the message and the details, so the count, the checks, and the fix are all about the block. The fix now says "block-tier material".findingLinesprints the tier on every path, since ascan_blockedenvelope ships block and warn mixed; open string, sanitized, absent tier still renders. - Fixed.
flow.send(token)is wrapped, so a secondneeds_ackor a retry block arrives asNEEDS_CONFIRMATION/PUBLISH_BLOCKEDwithscan.source: server, the[server]markers, and a fix. The no-loop test now asserts the wrapper rather than only the code, and there is a new test for a block arriving on the retry. - Fixed.
scanDraftscansargs.excerpt, so the doc sentence is true. Test: a secret in--excerptblocks in every mode with zero calls and zerogetSigner.safety-model.mdnow names the covered set explicitly. - Fixed. Both tool descriptions gain the clause, including that a
yes:truegiven before server findings does not clear them.
Nits
All three. Team sentence reworded (the shelf scans at ingest, advisory today, so the survivors are what a payload carries rather than "the only findings there are"). Plan-round labels replaced with file/symbol cites. PR body test counts corrected, and while there re-stated to match what is actually in the tree.
One extra, unasked: the skill's public arm now says a --yes re-run can hit a second exit 3 with [server] findings and to follow that payload's own fix rather than re-running with --yes again, because otherwise an agent loops on it. The pinned public digest moved with it, re-pinned with the reasoning.
Gates: pnpm check exits 0. lint, typecheck, format:check clean, 2720 tests pass across 80 files.
A redacted excerpt is lossy, and a semantic one is a fixed string, so two distinct server findings can arrive byte-identical apart from where they matched. Keyed by detector + excerpt, the second vanished from the render while the ack token still covered it, which is the one direction this merge must not err in. Server entries now collapse only on a full match site (detector, coordinates, field, excerpt), i.e. a finding the server sent twice. The cross-side match is unchanged and stays by value, since a local excerpt is one the operator can see in their own file. `byValue` is no longer written by the server loop, so one server entry can never absorb another. The cost is that one secret genuinely occurring in two fields renders twice, on two different lines. That is the safe direction: a duplicate line rather than material acked unseen. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013TE4zSrBc7Qw1qPodTGCT9
…inding A hoisted object literal gets no contextual type, so `span` widened to number[] and the tuple the schema declares rejected it. Inline literals were fine, which is why this only bit the two fixtures held in a const. Annotates both against the real type instead of reshaping the values, and drops the `Record<string, unknown>` spread from the other fixture, which type-checked only because the index signature swallowed `span` as unknown. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013TE4zSrBc7Qw1qPodTGCT9
…veral A local excerpt is masked too: `secret-assignment` renders `KEY=[redacted N chars]` and `wallet-address` first-6-last-4, so two distinct values of equal length collide on the value key exactly as they do server-side. The cross-side leg absorbed every coincident server finding into the one local entry, which cleared `serverAddedUnseen` and let an eligible --yes ack a token covering findings no render had shown. One match is the ordinary case and stays collapsed. A second server finding on the same local entry is the server reporting more than the local render stands for, so it renders on its own line and the run holds. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013TE4zSrBc7Qw1qPodTGCT9
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
vraspar
left a comment
There was a problem hiding this comment.
Re-review: the consent fix is real and strictly tighter; the held fix string does not know about the new setting and loops under off
Reviewed at 0410efe, delta over eb39bad (3059f3a, 6f8f426, 0410efe). Code read; CI and Greptile green, skill-drift red for an unrelated mirror drift (below).
Resolved since round 1
- [Major]
--yesacks unseen server findings:acksServerWarningsnow derives from override / setting / mode / yes /serverAddedUnseen, whereserverAddedUnseenis computed from the merged set'ssource === 'server'. Verified the 36-input table matches the author's; every true branch was true under the oldyes || full-auto, so nothing acks more than before. Test matrix present in publish, edit and consent suites. - [Minor] offset collision:
byValue.getonly; server set keyed by check + line + span + field + excerpt, so distinct server findings survive each other; regression test added. - [Minor] blocked branch: renders
mergeScanFindings([], server), fix says block-tier material, tier printed on every path. - [Minor] second
ScanGateError: wrapped for both kinds withscan.source: serverand a fix; exactly two sends, the token never rides a different body. - [Minor]
--excerpt:scanDraftscansargs.excerpt, with a zero-fetch zero-signer test in all three modes. - [Minor] MCP descriptions: both tools name the server gate.
- Nits: team sentence reworded, plan-round labels replaced, test counts corrected.
Closed as decisions
- Decision 1 (team shelf stays advisory, written into RELEASING.md and safety-model.md rather than a team branch): accepted.
- Decision 2 (
publish.ackServerWarningsships now, global-only, three values): accepted; the seam is validated on load and at the edge, unreachable from project config, env, or MCP, andconfig setstays NEVER_ALLOWLISTED.
New
Major
- correctness:
heldFixdecides onflow.yesalone while the ack decides onackSettingandoverridetoo, so the held payload advises a--yesthat provably cannot ack: underpublish.ackServerWarnings off, the switch this PR ships for full-auto dogfood machines, a hold whose findings all dedup into local ones hasserverAddedUnseen === false, soheldFixtakes the "re-run with--yes" branch; re-running reproduces the identical state forever, and the skill's new loop guard ("follow that payload's own fix") is defeated because the payload's own fix is the--yesadvice. The same branch fires on the primary hold path (auto, clean local, no--yes: every finding isserver, so the advised--yescannot clear them and the operator burns a signed round trip to be told the real answer), and underoff+full-autothe third branch advisesackServerWarnings on, undoing theoffthe operator set. One-line fix: branch onacksServerWarnings({...flow, yes: true, serverAddedUnseen})instead offlow.yes; no test asserts the fix string on theoffpath.tenjin-agent/src/lib/consent.ts
Lines 258 to 273 in 0410efe
Minor
- security: lossy-excerpt detectors let two different values collapse into one local finding, so
serverAddedUnseenunder-counts: the cross-side merge keys oncheck:excerpt, andsecret-assignment,customer-identifier(both warn on both sides) renderNAME=[redacted N chars], so a bodyAPI_KEY="aaaaaaaaaa"plus--excerpt 'API_KEY="bbbbbbbbbb"'renders one local line (dedupeFindingsis keyed the same way), the server's two findings both hit that onebyValueentry (the map is read, never consumed), both becomeboth, and the token is sent. The operator did acknowledge asecret-assignmentfinding, which is why this is minor rather than the round-1 major returning; but the invariant "a yes covers the findings it post-dates" is bent by multiplicity. Fix options: let each local entry absorb at most its pre-dedupe count, or restrict the cross-side collapse to excerpt kinds that identify the value. The delta's own test pins the unbounded absorption usingemail, whose excerpt is verbatim.tenjin-agent/src/lib/scan-gate.ts
Lines 240 to 260 in 0410efe
- agent-docs: the session primer's full-auto line is now false under
off:publishModeLine(hook-scripts.ts) still injects "publish without asking; hedge warnings, stop only on hard blocks", but a full-auto machine withackServerWarnings offdoes stop on a server warn, and this line is in the model's context every turn end. Read the setting there too.tenjin-agent/src/lib/hook-scripts.ts
Lines 1770 to 1778 in 0410efe
- agent-usability: a project
.tenjin.jsonnaming the key is ignored silently:ProjectConfigSchema.publishis.passthrough(), so{"publish":{"ackServerWarnings":"on"}}parses andresolveAckServerWarningsnever reads it. Failing closed is right;publish.mode's own project gate warns on stderr when it demotes, this one should too.tenjin-agent/src/lib/settings.ts
Lines 380 to 390 in 0410efe
- test-coverage: the new config surface has no test:
parseAckServerWarningsand thesetPublishKeybranch have no persist test and no bogus-value USAGE test, and the held fix instructs operators to run exactly that command.tenjin-agent/src/commands/config.ts
Lines 361 to 375 in 0410efe
Nits (4), none blocking
- consent.test.ts: the "never acks more than the old rule" invariant enumerates 36 inputs but omits
override; either enumerate it and exempt it explicitly, or say in the comment it is out of scope. - settings.ts re-implements
resolveAckServerWarningsinline; two global-only readings that must stay identical, and the comment does not name the sibling. - permissions.ts NEVER_ALLOWLISTED reason for
config setlists what it can loosen and does not namepublish.ackServerWarnings on, now the most direct consent-loosening write on the verb. - The reply's table row 2 (
override: any/off→ no) is not literal: override is checked first. Comment only.
Verified, not issues
- Semantic verdicts can never key onto a local warn (no local detector is named
semantic-*), so they always renderserverand hold. offbeatsfull-autoand is honoured on the Stop-hook path, which hands the agent a plaintenjin publish.- Retry: the second envelope's token is dropped from details, receipt and logs; server binds the token to content and finding-set hashes anyway.
- Nothing else in the 22-file delta widens unattended publishing; the tier now printing on local-only payloads is benign.
skill-drift (mirror is in sync)is red becauseskills/tenjin/SKILL.mdon this branch predates the live tenjin.blog/skills.md text (the/api/answerMISS wording); not a file this PR touches. Mergingorigin/mainafter the next resync clears it.
Verdict: comments-only. The round-1 major and all five minors are fixed in code. One new major, a one-line fix in heldFix plus a test on the off path's fix string; four minors. Approve once the fix string is derived from the same decision the ack is.
`heldFix` branched on `flow.yes` while the acknowledgement branches on the setting and the override too, so a held payload advised a `--yes` that provably could not clear it. Under `publish.ackServerWarnings off` a hold whose findings dedup into local ones took the "re-run with --yes" branch and the re-run reproduced the same state forever, which the skill's "follow that payload's own fix" rule cannot escape when the fix is the loop. The same branch fired on the primary hold (auto, clean local, no --yes), spending a signed round trip to say so, and under off plus full-auto the third branch advised undoing the operator's own off. It now asks `acksServerWarnings` itself, with `yes: true`, about the run the operator is being sent to make, then splits the three ways a --yes cannot help: a caller that never acks, a standing off, and a finding the marketplace added. Alongside, from the same review: the Stop hook's mode line says when off still stops a full-auto publish, a project `.tenjin.json` naming the key is ignored with a stderr line rather than silently, settings.ts shares config.ts's resolver instead of re-reading the key, the config surface gets round-trip and USAGE tests, the ack invariant enumerates `override`, and the never-allowlisted reason for `config set` names this key. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013TE4zSrBc7Qw1qPodTGCT9
|
Thanks. Everything here is taken; nothing declined. Pushed at b467919, on top of a merge with Major 1,
|
vraspar
left a comment
There was a problem hiding this comment.
Re-review: the held fix now asks the ack's own decision; approve
Reviewed at b467919, delta over 0410efe (e88a24d, b467919, plus a merge of main that brought #183 and #187). Code read; CI and Greptile green, clean merge-tree.
Resolved since round 2
- [Major]
heldFixignored the setting: it now callsacksServerWarningswithyes: truefor the run the operator is being sent to make, and only advises--yeswhen that returns true; the three no-help cases (ackOverride === false, standingoff, marketplace-added) each get their own remedy, and theoffstring points atmode, never aton. Tests pin theoffpath's fix string and the no-yes hold. - [Minor] lossy-excerpt multiplicity: one server finding may collapse into a given local entry, tracked by a
claimedset; a second on the same entry renders asserverand holds the run. Test now uses a collidingsecret-assignmentexcerpt, plus an end-to-end publish hold. - [Minor] primer line:
modeLineappends theoffcaveat when set. - [Minor] project
.tenjin.jsonkey: recorded asignoredAckServerWarningsand warned on stderr, matchingpublish.mode's precedent. - [Minor] config surface tests: round-trip for all three values, USAGE on bogus ones, no allowlist sync.
- Nits: invariant enumerates
override(108 inputs),resolveAckServerWarningsshared, NEVER_ALLOWLISTED reason names the key.
Verdict: approve. The skill-drift (mirror is in sync) red is global, not this branch: the vendored skills/tenjin/SKILL.md here is byte-identical to main's, and the live tenjin.blog/skills.md moved again after #213 (the bot/skill-resync branch is the fix). The push-scripts.test.ts timing flake the author saw locally predates this PR and is worth its own issue.
Scope
The CLI now answers the server-side ingest scan gate, so a warn-tier refusal becomes a decision the operator can act on instead of an opaque exit-4 write failure.
src/lib/scan-gate.tsparses the two 422 codes and merges server findings with the local scan's, deduped by detector and offset.src/lib/consent.tsmaps them into the exit-3 flowpublishandeditalready share:scan_blockedis terminal with no acknowledgement path,scan_needs_ackrenders once and re-runs the identical content with the ack token on an explicit yes, exactly once. Risk is the wire body:scanAckonly ships on a retry the server itself asked for, so a deployment predating the gate never sees the key on a body its strictObject schema would reject.A confirmation covers the findings it post-dates (
src/lib/consent.tsacksServerWarnings), so a--yesclears the server's hold only when the merge added nothing the local pass had not already rendered: review and auto stop on a server-only warn, full-auto still acks unasked.publish.ackServerWarnings(off/on/mode) sets a standing answer without changing the mode, from the global config only since it can only loosen what a yes covers.Advisory findings on a success response reach the receipt as
scanand the human summary as informational lines; findings travel as data, so a detector this release predates renders faithfully and the server stays authoritative under version skew.Testing
pnpm lint,pnpm typecheck,pnpm format:check,pnpm build: clean.pnpm test: 2720 passed, 10 skipped, 80 files.pnpm checkexits 0.src/lib/scan-gate.test.ts(envelope parsing, token-never-on-block, unknown detector and tier, merge/dedupe by value, coordinate-coincidence non-collapse, malformed-finding drop), 21 gate tests inpublish.test.tsand 6 inedit.test.tsagainst a stubbed gate, 3 decision tests inconsent.test.ts(the full mode x yes x config x unseen table, the override, and a proof no input acks more than the pre-gate rule did), and 4 renderer tests inoutput.test.tsfor the source marker and the tier.Definition of Done
RELEASING.mdgains a server-coupled-releases section: thetenjinrepo'sSCAN_WARN_MODE=enforceflip waits for this release to propagate, which is the whole reason the flag exists.docs/safety-model.mdstates the client/server split: the local scan runs first so a block-tier secret never leaves the machine (now true of--excerpttoo); the server is authoritative for the marketplace.RELEASING.mdanddocs/safety-model.mdboth record that a team shelf's warn tier stays advisory, so the gate and the team drop do not re-ask each other's question.minor).Notes
A1igator/ingest-scan-gate), built against head5fbed15f. This lands after it merges.openapi.fixture.jsoncontract pin is deliberately untouched:scanAckis not in the deployed contract yet. Regenerate it with the tenjin deploy, not here.skill-drift (mirror is in sync)check does not fire here (noskills/change). Where it does run on a fresh branch it is red for a shared cause owned by feat(cli): tenjin pay and tenjin discover, with the bazaarPay toggle #151, and is not chased.Refs #182, tenjin#723.
Greptile Summary
The PR integrates marketplace ingest-scan responses into the existing publish and edit consent lifecycle.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Sequence Diagram
sequenceDiagram participant O as Operator participant C as CLI participant S as Local scan participant M as Marketplace O->>C: publish/edit C->>S: scan submitted content alt local hard block S-->>C: block finding C-->>O: refuse before write else locally permitted C->>M: signed write without scanAck alt server block M-->>C: scan_blocked C-->>O: terminal refusal with findings else server acknowledgement required M-->>C: scan_needs_ack + token C->>C: merge local and server findings alt acknowledgement policy permits C->>M: retry identical content with scanAck M-->>C: success receipt else acknowledgement not covered C-->>O: confirmation required end else advisory or clean success M-->>C: success with optional scan report C-->>O: normalized receipt end endReviews (7): Last reviewed commit: "fix(consent): derive the held fix string..." | Re-trigger Greptile
Context used: