Skip to content

feat(publish): answer the marketplace scan gate instead of failing on it - #185

Merged
A1igator merged 10 commits into
mainfrom
A1igator/scan-ack-flow
Aug 26, 2026
Merged

feat(publish): answer the marketplace scan gate instead of failing on it#185
A1igator merged 10 commits into
mainfrom
A1igator/scan-ack-flow

Conversation

@A1igator

@A1igator A1igator commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

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.ts parses the two 422 codes and merges server findings with the local scan's, deduped by detector and offset. src/lib/consent.ts maps them into the exit-3 flow publish and edit already share: scan_blocked is terminal with no acknowledgement path, scan_needs_ack renders once and re-runs the identical content with the ack token on an explicit yes, exactly once. Risk is the wire body: scanAck only 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.ts acksServerWarnings), so a --yes clears 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 scan and 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 check exits 0.
  • New: 19 unit tests in 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 in publish.test.ts and 6 in edit.test.ts against a stubbed gate, 3 decision tests in consent.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 in output.test.ts for the source marker and the tier.
  • Every gate test runs against a stubbed response. No live server.

Definition of Done

  • RELEASING.md gains a server-coupled-releases section: the tenjin repo's SCAN_WARN_MODE=enforce flip waits for this release to propagate, which is the whole reason the flag exists.
  • docs/safety-model.md states the client/server split: the local scan runs first so a block-tier secret never leaves the machine (now true of --excerpt too); the server is authoritative for the marketplace.
  • RELEASING.md and docs/safety-model.md both 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.
  • Changeset added (minor).

Notes

  • Server sibling: BackTrackCo/tenjin#723 (A1igator/ingest-scan-gate), built against head 5fbed15f. This lands after it merges.
  • The openapi.fixture.json contract pin is deliberately untouched: scanAck is not in the deployed contract yet. Regenerate it with the tenjin deploy, not here.
  • The skill-drift (mirror is in sync) check does not fire here (no skills/ 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.

  • Parses blocked, acknowledgement-required, and advisory server scan reports.
  • Merges local and server findings while conservatively tracking newly surfaced warnings.
  • Retries identical write content once with a server-issued acknowledgement token when policy permits.
  • Adds a global acknowledgement policy, receipt rendering, documentation, and comprehensive gate tests.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
src/lib/scan-gate.ts Parses scan envelopes and merges findings with one-to-one cross-side matching so additional server sites remain visible.
src/lib/consent.ts Integrates server holds into shared publish/edit consent and performs at most one policy-authorized token retry.
src/commands/publish.ts Routes publish writes through the scan gate and reports normalized advisory findings.
src/commands/edit.ts Applies the same scan-gate protocol and receipt normalization to edits.
src/lib/settings.ts Resolves the global-only server-warning acknowledgement policy while ignoring project-level attempts to loosen it.

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
  end
Loading

Reviews (7): Last reviewed commit: "fix(consent): derive the held fix string..." | Re-trigger Greptile

Context used:

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Your trial has ended. Reactivate Greptile to resume code reviews.

@A1igator A1igator added the priority: critical Blocks other PRs; merge-priority label Aug 18, 2026

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Your trial has ended. Reactivate Greptile to resume code reviews.

@A1igator A1igator added priority: high High priority and removed priority: critical Blocks other PRs; merge-priority labels Aug 21, 2026
@A1igator
A1igator force-pushed the A1igator/scan-ack-flow branch from b498ef0 to b7edb15 Compare August 22, 2026 03:53

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Your trial has ended. Reactivate Greptile to resume code reviews.

@A1igator

Copy link
Copy Markdown
Contributor Author

Rebased onto main at b7edb15. The branch was CONFLICTING after #150, #181 and #199 landed. It is MERGEABLE now.

Two files conflicted, and both were semantic rather than textual, so recording how each resolved:

src/lib/posts-api.tsmain widened searchId to string | string[] (the multi-search-id change) and derives a local through toWireSearchId(normalizeSearchIds(...)). This branch adds scanAck. The two do not interact: I took main's widened type and its normalized local on all three hunks, and kept scanAck beside them.

src/commands/publish.tsmain replaced the single searchInfo object with a searches array plus a closeLine helper. That supersedes this branch's version of the same code outright, so I dropped my side of the hunk rather than merging the two shapes. What this branch actually contributes there is two lines, both kept: ...scanNoteLines(result.scan) in the human output and ...(result.scan !== undefined ? { scan: scanReceipt(result.scan) } : {}) in the data payload. The deps interface took both new optional fields, searchIdLabel from main and ackServerWarnings from here.

No searchInfo reference survives in the file, so nothing is left pointing at the shape main removed.

What ran at this head: pnpm install --frozen-lockfile, pnpm typecheck, and pnpm run check (lint, format, full suite) which exits 0 with 76 test files passed and 2414 tests passed, 10 skipped. The notify-registry-pin: FAIL and skill-resync push-error lines in that output are fixture text from tests that assert loud failure, not real failures.

Still REVIEW_REQUIRED.

A1igator and others added 3 commits August 24, 2026 22:26
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
@A1igator
A1igator force-pushed the A1igator/scan-ack-flow branch from b7edb15 to 6a1ac17 Compare August 25, 2026 02:29
Comment thread src/lib/scan-gate.ts
…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
Comment thread src/lib/scan-gate.ts Outdated
@A1igator A1igator added priority: medium Medium priority and removed priority: high High priority labels Aug 25, 2026

@vraspar vraspar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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_ack at 422 with details.scan.{findings, checks.semantic, ackToken}, finding fields check/severity/line/span/excerpt/field, scanAck accepted by both strictObject post schemas (max 2048), success scan top-level and visible through the passthrough ownPostSchema.
  • The retry is sound: the body is rebuilt from the same input with only scanAck added, which is exactly the title/excerpt/body set ack.ts hashes; auth.headersFor runs per attempt so the fresh-nonce requirement holds; a second needs_ack cannot loop.
  • scan_blocked is terminal in every mode and parseScanRejection strips 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

  1. correctness: --yes acknowledges server warn findings the operator was never shown: needsConfirmation('review', n) is true for every n, so a review run reaches the network only with args.yes === true, and acksServerWarnings(mode, yes) returns yes || full-auto, so the first server contact re-sends with the token. The same holds for auto whenever the local scan had a warn. The --yes was 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 is auto with zero local warns and no --yes. The changeset and the acksServerWarnings docstring 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 honour yes when 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.
    /**
    * Whether this run may acknowledge the SERVER gate's warn findings on the
    * operator's behalf, mirroring {@link needsConfirmation} exactly: `review` and
    * `auto` stop on a server warn the same way they stop on a local one, and only
    * `full-auto` — or an explicit `--yes`, which is the operator's own yes against
    * the findings the previous run rendered — re-runs carrying the token.
    *
    * `override` exists for a caller whose answer is never yes whatever the mode:
    * the session observer (PR 5) publishes unattended, so a server warn must drop
    * its candidate to draft rather than be acked by a config value. Keeping that a
    * parameter rather than a second mode is what stops the observer's rule from
    * being re-derived from `publish.mode`, which it is deliberately orthogonal to.
    */
    export function acksServerWarnings(mode: PublishMode, yes: boolean, override?: boolean): boolean {
    if (override !== undefined) return override;
    return yes || mode === 'full-auto';
    }
    and
    if (needsConfirmation(settings.mode, warns.length) && args.yes !== true) {
    throw new CliError('NEEDS_CONFIRMATION', confirmMessage(warns.length, price.usd), {
    fix: 'Review the findings, then re-run with --yes (or resolve the source and re-run).',
    details: {
    mode: settings.mode,
    price: { atomic: price.atomic, usd: price.usd },
    findings: warns.map(publicFinding),
    card: eligibility,
    target: { status, titlePreview: sanitizeForTerminal(title ?? '(untitled draft)') },
    },
    });
    }
    // Approved (or nothing to confirm): from here a wallet is required. The write
    // base URL is resolved through the shared settings seam and used for BOTH the
    // SIWX/session header domain and the POST host, so the two never diverge. In
    // team mode that is the team shelf and nowhere else — a publish never reaches
    // `publicShelfUrl`, which is consume-only.
    const provider = resolveWalletProvider(
    ctx,
    deps.provider !== undefined ? { provider: deps.provider } : {},
    );
    await describeWallet(provider); // surfaces WALLET_MISSING with its own fix
    const signer = await provider.getSigner();
    const auth = resolveWriteAuth({
    signer,
    baseUrl: runtime.baseUrl,
    dataDir: ctx.dataDir,
    // A publish always writes.
    scope: 'read+write',
    ...(deps.useSession !== undefined ? { useSession: deps.useSession } : {}),
    env,
    });
    const input: PublishInput = {
    ...(title !== undefined ? { title } : {}),
    bodyMd: body,
    ...(excerpt !== undefined ? { excerpt } : {}),
    ...(tags !== undefined ? { tags } : {}),
    priceAtomic,
    ...(handle !== undefined ? { handle } : {}),
    status,
    ...(card !== undefined ? { resource: card } : {}),
    // The attribution half of `--search-id`, and it follows the SAME rule the
    // local ledger already follows: a draft answers nobody, so it claims nobody's
    // demand either. Sending it on a draft put one demand signal on two posts —
    // no command promotes a draft, so reaching a public piece means a second
    // publish carrying the same id — with one of them possibly never shipping.
    ...(claimableIds.length > 0 && status !== 'draft' ? { searchId: claimableIds } : {}),
    };
    const client = {
    baseUrl: runtime.baseUrl,
    timeoutMs: ctx.flags.timeout,
    ...(runtime.bypass !== undefined ? { bypass: runtime.bypass } : {}),
    ...(deps.fetchImpl !== undefined ? { fetchImpl: deps.fetchImpl } : {}),
    };
    // The server ingest gate runs the same rule corpus in the marketplace's write
    // path, so its warn tier joins this command's exit-3 flow rather than arriving
    // as an opaque write failure. Its block tier has no acknowledgement path.
    const result = await throughScanGate({
    send: (scanAck) =>
    publishPost(scanAck === undefined ? input : { ...input, scanAck }, auth, client),
    localWarns: warns,
    mode: settings.mode,
    yes: args.yes === true,
    ...(deps.ackServerWarnings !== undefined ? { ackOverride: deps.ackServerWarnings } : {}),
    detail: { mode: settings.mode, price: { atomic: price.atomic, usd: price.usd } },
    noun: 'Publish',
    heldSuffix: `, price $${price.usd}`,

Minor

  1. 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 is byOffset.get(oKey) ?? byValue.get(vKey), and byOffset holds local entries only, so a distinct server finding with coincident coordinates collapses into the local one relabelled both and 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.
    * and value alone is not enough either, since a redaction can differ across
    * corpus versions. Erring toward collapsing is safe (the finding is still
    * rendered, once); erring toward splitting shows the operator the same secret
    * twice and teaches them to skim.
    *
    * The offset key is a LOCAL-side key only, and that is the whole reason server
    * findings are keyed by value alone. Offsets are per-field: the server reports
    * `line`/`span` relative to the submitted field it matched in, so two findings
    * from DIFFERENT fields sharing a detector, a line and a span are a coordinate
    * coincidence rather than one finding, and two different secrets in a one-line
    * `title` and a one-line `excerpt` collide constantly. Collapsing those would
    * drop material the operator then acks without ever seeing, which is the one
    * direction this merge must not err in.
    */
    export function mergeScanFindings(
    local: ScanFinding[],
    server: ServerScanFinding[],
    ): RenderedFinding[] {
    const out: RenderedFinding[] = [];
    const byOffset = new Map<string, RenderedFinding>();
    const byValue = new Map<string, RenderedFinding>();
    for (const f of local) {
    const oKey = offsetKey(f.check, f.line, f.span);
    const vKey = valueKey(f.check, f.excerpt);
    if (byOffset.has(oKey) || byValue.has(vKey)) continue;
    const rendered: RenderedFinding = {
    check: f.check,
    severity: f.severity,
    line: f.line,
    excerpt: f.excerpt,
    source: 'local',
    };
    byOffset.set(oKey, rendered);
    byValue.set(vKey, rendered);
    out.push(rendered);
    }
    for (const f of server) {
    const oKey = offsetKey(f.check, f.line, f.span);
    const vKey = valueKey(f.check, f.excerpt);
    const seen = byOffset.get(oKey) ?? byValue.get(vKey);
    if (seen !== undefined) {
    if (seen.source === 'local') seen.source = 'both';
    continue;
    }
    const rendered: RenderedFinding = {
    check: f.check,
    severity: f.severity,
    line: f.line,
    excerpt: f.excerpt,
    source: 'server',
    ...(f.field !== undefined ? { field: f.field } : {}),
    };
    byValue.set(vKey, rendered);
  2. correctness: the blocked branch renders local warns as block reasons and shows no tier: describeRendered runs 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 only blocking. findingLines prints 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.
    throw new CliError(
    'PUBLISH_BLOCKED',
    `${flow.noun} blocked by the marketplace scan: ${describeRendered(findings)}.`,
    {
    fix: 'Remove the flagged material from the content (the block tier has no acknowledgement path), then re-run.',
    details,
    },
    );
  3. correctness: a second ScanGateError escapes raw: flow.send(token) sits in the catch with no wrapper, so a second needs_ack or a block on the retry surfaces ScanGateError's own details (unmerged server findings, no source, no fix), and output.ts renders them with no [server] marker, as if local. The non-loop test asserts only the code.
    ? 'Resolve the findings in the content, then re-run.'
    : 'Review the findings, then re-run with --yes to proceed anyway (or resolve them in the content).',
    details,
    },
    );
    }
    return await flow.send(token);
    }
  4. security: the new "a block-tier secret never leaves the machine" claim is false for --excerpt: scanDraft scans raw plus cardScanText(card), and neither covers args.excerpt (CLI --excerpt, MCP tenjin_publish({excerpt})), which resolveExcerpt ships; frontmatter excerpts are inside raw and are covered, and edit.ts's shippedTypedText already scans args.excerpt. The gap predates this PR; the doc sentence does not. One line in scanDraft makes the sentence true.
    async function scanDraft(
    args: PublishArgs,
    cwd: string,
    raw: string,
    card: ResourceCardInput | undefined,
    ): Promise<ScanFinding[]> {
    const markerRoot = args.file !== undefined ? dirname(resolve(cwd, args.file)) : cwd;
    const scanContext: ScanContext = { projectMarkers: await deriveProjectMarkers(markerRoot) };
    return dedupeFindings([...scan(raw, scanContext), ...scan(cardScanText(card), scanContext)]);
    and
    The marketplace runs its own scan at ingest, and it is authoritative. The local scan still runs first, for one reason: it fails before a signature is made, so a block-tier secret never leaves the machine. Server-side, the block tier has no acknowledgement path, and the warn tier is held with a token that `--yes` acknowledges, exactly like a local warn. Findings the server contributed are marked as such where they are rendered, including detectors this release has never heard of.
  5. agent-docs: the MCP tool descriptions no longer describe what can refuse a call: tenjin_publish promises 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_edit promises only "the before/after summary" and never mentions findings. One clause each: the marketplace also scans server-side and may hold with findings.
    'Publish a Markdown file as a paid or free piece with an optional ' +
    'answer card. Gated by a deterministic local scan and your publish.mode consent: in review ' +
    'mode, or on a soft finding, it returns NEEDS_CONFIRMATION with the exact payload (mode, ' +
    'price, findings, card, target) for you to show the user before re-calling with yes:true. A ' +
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.
    **On any exit 3, render THAT payload's findings as one yes/no, then re-run with
    `--yes` on an explicit yes.** Never ask a generic "shall I publish?" before
    running: the findings are the question, and a `--yes` re-run after a bare yes
    silently clears the three warn checks that survive here (`secret-assignment`,
    `hex32-value`, `embedded-instruction`) — which on this shelf are the only findings
    there are, and each one is either a live credential or text that would steer the
    next agent to read it. A hard block refuses in every mode and no `--yes` clears it.
    <!-- tenjin:else -->
    **On any exit 3, render THAT payload's findings and price as one yes/no, then
  • 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#723 is fine. The D38 line is pre-existing.
    * `override` exists for a caller whose answer is never yes whatever the mode:
    * the session observer (PR 5) publishes unattended, so a server warn must drop
    * its candidate to draft rather than be acked by a config value. Keeping that a
  • PR body says 16 scan-gate unit tests; the file has 18.

Decisions

  1. Team mode and a shelf at SCAN_WARN_MODE=enforce. throughScanGate runs 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 findings survivesTeamDrop removes, and under the dogfood's full-auto they are all silently acked and shipped, while under auto the capture loop holds on the first clean-local note. The runbook sets no SCAN_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.
  2. ackServerWarnings has no production caller. Only the test suite sets it (PublishDeps/EditDeps), PUBLISH_CONFIG_KEYS lacks it, and no flag exists, yet the changeset tells operators to pass it. With publish.mode full-auto on 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: false overrides the mode on both commands, and publishPost/updatePost have exactly two callers, both wrapped.
  • Advisory findings on a 200 reach the receipt scan field 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.
  • buildPostUpdateBody adds scanAck after the emptiness check, so a token-only PUT is still "no changes".
  • RELEASING.md's server-coupled section is accurate: SCAN_WARN_MODE defaults advisory and requires SCAN_ACK_SECRET under 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 --yes does 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_ack envelope 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
@A1igator

Copy link
Copy Markdown
Contributor Author

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, --yes acks server warn findings the operator was never shown

Took the code fix, not the docs retreat. The principle now in acksServerWarnings (src/lib/consent.ts) is that a confirmation covers the findings it post-dates. A --yes answered a payload rendered before any server call, so it covers the local warns and nothing the server added afterwards.

Concretely, serverAddedUnseen is findings.some(f => f.source === 'server') on the merged set, and the decision is:

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:

  • off never acks whatever the mode says. This is the switch a full-auto dogfood machine gets without changing its mode.
  • on is a standing yes for server findings, and it still requires a --yes on the run. It restores the pre-gate reading of --yes and 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

  1. Fixed. const seen = byValue.get(vKey) completes the rename, with the reason on the line. Regression test keeps 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.
  2. 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". findingLines prints the tier on every path, since a scan_blocked envelope ships block and warn mixed; open string, sanitized, absent tier still renders.
  3. Fixed. flow.send(token) is wrapped, so a second needs_ack or a retry block arrives as NEEDS_CONFIRMATION/PUBLISH_BLOCKED with scan.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.
  4. Fixed. scanDraft scans args.excerpt, so the doc sentence is true. Test: a secret in --excerpt blocks in every mode with zero calls and zero getSigner. safety-model.md now names the covered set explicitly.
  5. Fixed. Both tool descriptions gain the clause, including that a yes:true given 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.

Comment thread src/lib/scan-gate.ts Outdated
A1igator and others added 2 commits August 26, 2026 02:31
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
Comment thread src/lib/scan-gate.ts
A1igator and others added 2 commits August 26, 2026 02:57
…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 vraspar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

  1. [Major] --yes acks unseen server findings: acksServerWarnings now derives from override / setting / mode / yes / serverAddedUnseen, where serverAddedUnseen is computed from the merged set's source === 'server'. Verified the 36-input table matches the author's; every true branch was true under the old yes || full-auto, so nothing acks more than before. Test matrix present in publish, edit and consent suites.
  2. [Minor] offset collision: byValue.get only; server set keyed by check + line + span + field + excerpt, so distinct server findings survive each other; regression test added.
  3. [Minor] blocked branch: renders mergeScanFindings([], server), fix says block-tier material, tier printed on every path.
  4. [Minor] second ScanGateError: wrapped for both kinds with scan.source: server and a fix; exactly two sends, the token never rides a different body.
  5. [Minor] --excerpt: scanDraft scans args.excerpt, with a zero-fetch zero-signer test in all three modes.
  6. [Minor] MCP descriptions: both tools name the server gate.
  7. 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.ackServerWarnings ships now, global-only, three values): accepted; the seam is validated on load and at the edge, unreachable from project config, env, or MCP, and config set stays NEVER_ALLOWLISTED.

New

Major

  1. correctness: heldFix decides on flow.yes alone while the ack decides on ackSetting and override too, so the held payload advises a --yes that provably cannot ack: under publish.ackServerWarnings off, the switch this PR ships for full-auto dogfood machines, a hold whose findings all dedup into local ones has serverAddedUnseen === false, so heldFix takes 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 --yes advice. The same branch fires on the primary hold path (auto, clean local, no --yes: every finding is server, so the advised --yes cannot clear them and the operator burns a signed round trip to be told the real answer), and under off + full-auto the third branch advises ackServerWarnings on, undoing the off the operator set. One-line fix: branch on acksServerWarnings({...flow, yes: true, serverAddedUnseen}) instead of flow.yes; no test asserts the fix string on the off path.
    * What the operator does next about a held write. The three cases are different
    * instructions, and the third is the one that matters: their `--yes` answered an
    * earlier render that could not have contained these findings, so pointing them
    * back at `--yes` would be pointing them at the same wall.
    */
    function heldFix<T>(flow: ScanGateFlow<T>, hasToken: boolean, serverAddedUnseen: boolean): string {
    if (!hasToken) return 'Resolve the findings in the content, then re-run.';
    if (!(flow.yes && serverAddedUnseen)) {
    return 'Review the findings, then re-run with --yes to proceed anyway (or resolve them in the content).';
    }
    return (
    'The marketplace found these beyond what your --yes answered, so that yes does not clear them. ' +
    'Resolve them in the content, or acknowledge marketplace findings standingly with ' +
    '`tenjin config set publish.ackServerWarnings on` and re-run with --yes.'
    );
    }

Minor

  1. security: lossy-excerpt detectors let two different values collapse into one local finding, so serverAddedUnseen under-counts: the cross-side merge keys on check:excerpt, and secret-assignment, customer-identifier (both warn on both sides) render NAME=[redacted N chars], so a body API_KEY="aaaaaaaaaa" plus --excerpt 'API_KEY="bbbbbbbbbb"' renders one local line (dedupeFindings is keyed the same way), the server's two findings both hit that one byValue entry (the map is read, never consumed), both become both, and the token is sent. The operator did acknowledge a secret-assignment finding, 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 using email, whose excerpt is verbatim.
    byValue.set(vKey, rendered);
    out.push(rendered);
    }
    const sites = new Set<string>();
    for (const f of server) {
    const site = serverSiteKey(f);
    if (sites.has(site)) continue;
    sites.add(site);
    // The cross-side match, and the ONLY read of a local-side key here. A hit
    // means the operator can see this value in their own file, which is what
    // makes collapsing it safe; `byValue` is deliberately not written back, so
    // one server entry never absorbs another.
    const agreed = byValue.get(valueKey(f.check, f.excerpt));
    if (agreed !== undefined) {
    agreed.source = 'both';
    continue;
    }
    out.push({
    check: f.check,
    severity: f.severity,
  2. 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 with ackServerWarnings off does stop on a server warn, and this line is in the model's context every turn end. Read the setting there too.
    */
    function modeLine(mode) {
    if (mode === 'auto') return 'publish.mode=auto: a clean publish proceeds without asking.';
    if (mode === 'full-auto') {
    return 'publish.mode=full-auto: publish without asking; hedge warnings, stop only on hard blocks.';
    }
    return 'publish.mode=review: publishing asks first.';
    }
  3. agent-usability: a project .tenjin.json naming the key is ignored silently: ProjectConfigSchema.publish is .passthrough(), so {"publish":{"ackServerWarnings":"on"}} parses and resolveAckServerWarnings never reads it. Failing closed is right; publish.mode's own project gate warns on stderr when it demotes, this one should too.
    * which stores atomic because `config set` converts at the command edge. The
    * decimal string is converted to atomic at load via the same money util the
    * config command uses. Partial + passthrough: forward-compatible with subkeys a
    * newer CLI adds, same posture as the global config.
    */
    const ProjectConfigSchema = z
    .object({
    publish: z
    .object({
    mode: PublishModeSchema.optional(),
    defaultPrice: z.string().optional(),
  4. test-coverage: the new config surface has no test: parseAckServerWarnings and the setPublishKey branch have no persist test and no bogus-value USAGE test, and the held fix instructs operators to run exactly that command.
    const entry: RenderedSetting =
    key === 'publish.mode'
    ? { value: parsePublishMode(value), source: 'file' }
    : key === 'publish.ackServerWarnings'
    ? { value: parseAckServerWarnings(value, key), source: 'file' }
    : { value: toMoney(parseUsdToAtomic(value)), source: 'file' };
    const subkey =
    key === 'publish.mode'
    ? 'mode'
    : key === 'publish.ackServerWarnings'
    ? 'ackServerWarnings'
    : 'defaultPrice';
    const stored =
    key === 'publish.defaultPrice' ? (entry.value as Money).atomic : (entry.value as string);
    await persist(ctx.dataDir, (existing) => ({
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 resolveAckServerWarnings inline; two global-only readings that must stay identical, and the comment does not name the sibling.
  • permissions.ts NEVER_ALLOWLISTED reason for config set lists what it can loosen and does not name publish.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 render server and hold.
  • off beats full-auto and is honoured on the Stop-hook path, which hands the agent a plain tenjin 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 because skills/tenjin/SKILL.md on this branch predates the live tenjin.blog/skills.md text (the /api/answer MISS wording); not a file this PR touches. Merging origin/main after 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
@A1igator

Copy link
Copy Markdown
Contributor Author

Thanks. Everything here is taken; nothing declined. Pushed at b467919, on top of a merge with origin/main (#183 and #187 landed while this was in review).

Major 1, heldFix decides on flow.yes while the ack decides on more

Right on all three counts, including the one I would have argued was the safe branch: the primary hold path. Under mode, auto, clean local content and no --yes, every finding is server-only, so the advised --yes cannot clear it either. That is a loop on the most common hold, not just under off.

The approve-once condition is met literally: the fix string now asks the ack's own function about the run the operator is being sent to make.

const yesWouldAck = acksServerWarnings({
  mode: flow.mode,
  yes: true,
  setting: flow.ackSetting,
  ...(flow.ackOverride !== undefined ? { override: flow.ackOverride } : {}),
  serverAddedUnseen,
});
if (yesWouldAck) return 'Review the findings, then re-run with --yes ...';

Below it the three ways a --yes cannot help are three different remedies, because they are: ackOverride === false (nothing the operator configures will ack, resolve in content), ackSetting === 'off' (says the setting is off and points at mode, never at on, so it cannot advise undoing the operator's own switch), and otherwise the marketplace-added case. That last string no longer says "beyond what your --yes answered", since it now also fires where no --yes was given.

Tests on the fix string: never advises a --yes that the setting has already ruled out (the off path, asserting it does not say re-run with --yes to proceed anyway and does not say ackServerWarnings on), does not advise a --yes on the no-yes hold it cannot clear, and the positive case folded into the on-without---yes hold, which is the one reachable state where a hold and a working --yes coexist.

Minor 1, lossy-excerpt multiplicity

Already fixed, in e88a24d, from Greptile's parallel report of the same thing at 0410efe, so you were both reading the same stale tree. I took the stricter of your two options: each local entry absorbs at most ONE server site rather than its pre-dedupe count, since the local render shows one line whatever the pre-dedupe count was. The rest render server, which is also what keeps serverAddedUnseen true and holds the run. You are right that my test pinned the unbounded absorption with email; it now uses a secret-assignment excerpt that collides by construction, and publish.test.ts has holds when two server sites coincide with one local warn pinning the consent consequence end to end.

Minor 2, the primer's full-auto line

Taken. readConfig reads publish.ackServerWarnings (global file only, no env, no project layer, matching resolveAckServerWarnings), and modeLine appends "Marketplace scan findings still stop it (publish.ackServerWarnings=off)." under off. Two tests, one per branch, as separate cases rather than a loop since the hook nags once per session.

Minor 3, project file ignored silently

Taken, with publish.mode's downgrade warning as the precedent. loadProjectConfig records ignoredAckServerWarnings when the passthrough block carries the key, and resolvePublishSettings pushes a stderr warning naming the file and the command that does work. Tests for both the warned and the silent global path.

Minor 4, no test on the config surface

Taken: round-trip through the file for all three values, USAGE on five bogus ones, sibling-subkey merge, and one asserting it does not sync the harness allowlist the way publish.mode deliberately does.

Nits

  1. The invariant is now total over override rather than exempting it, compared against override ?? (yes || full-auto), which is what the old rule actually did. 108 inputs.
  2. resolveAckServerWarnings is exported from config.ts and settings.ts calls it; the docstring names the sibling caller.
  3. Taken, and the same entry's team-mode survivor list was stale after feat(scan): harden the publish scan with 20 detectors, a rule corpus, and a labeled fixture gate #183, so it now names all five.
  4. Fair, the table row was shorthand for a precedence I described in prose above it. Noted rather than restated.

Skill-drift

Cleared by the merge: #213 resynced the vendored mirror on main, which is the repo's designed path for an upstream edit. Not a file this PR touches.

Gates on the merged tree, each as its own command: typecheck 0, lint 0, format:check 0, build 0. pnpm test 2828 passed / 11 skipped / 82 files. The one red is push-scripts.test.ts, a pre-existing timing flake untouched by this PR: it passes in isolation and I reproduced it on a clean worktree at eb39bad, 2 failures in one of six runs. Worth its own issue.

@vraspar vraspar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

  1. [Major] heldFix ignored the setting: it now calls acksServerWarnings with yes: true for the run the operator is being sent to make, and only advises --yes when that returns true; the three no-help cases (ackOverride === false, standing off, marketplace-added) each get their own remedy, and the off string points at mode, never at on. Tests pin the off path's fix string and the no-yes hold.
  2. [Minor] lossy-excerpt multiplicity: one server finding may collapse into a given local entry, tracked by a claimed set; a second on the same entry renders as server and holds the run. Test now uses a colliding secret-assignment excerpt, plus an end-to-end publish hold.
  3. [Minor] primer line: modeLine appends the off caveat when set.
  4. [Minor] project .tenjin.json key: recorded as ignoredAckServerWarnings and warned on stderr, matching publish.mode's precedent.
  5. [Minor] config surface tests: round-trip for all three values, USAGE on bogus ones, no allowlist sync.
  6. Nits: invariant enumerates override (108 inputs), resolveAckServerWarnings shared, 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.

@A1igator
A1igator merged commit 7da12bf into main Aug 26, 2026
3 of 4 checks passed
@A1igator
A1igator deleted the A1igator/scan-ack-flow branch August 26, 2026 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority: medium Medium priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants