Skip to content

feat(publish): let one piece claim every search it answered - #178

Open
vraspar wants to merge 5 commits into
mainfrom
vraspar/publish-multi-searchid
Open

feat(publish): let one piece claim every search it answered#178
vraspar wants to merge 5 commits into
mainfrom
vraspar/publish-multi-searchid

Conversation

@vraspar

@vraspar vraspar commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

One research thread fans out into many searchIds, and a piece answers the thread rather than one query of it. A live example from 2026-08-16: one investigation logged 10 MISS searchIds for what was a single question. The piece published back against one of them, and the other nine were closed one at a time as regenerated — false twice over, because they were not re-derived, they were answered by the same piece, and MISS-to-publish conversion counted every sibling of a converting loop as a failure.

tenjin publish --search-id now repeats, up to ten per piece.

What changed

  • --search-id collects (commander's collect, the same wiring feat(outcome): batch close, and a WebSearch hook that names itself #177 gave outcome). Ids dedupe, each is validated against SEARCH_ID_WIRE_RE exactly as today, and the cap refuses before any wallet touch.
  • Per-id local closes. closeNamedSearch runs once per id with unchanged relink semantics: a loop an outcome already closed is taken over, a loop an earlier publish claimed reports alreadyAnswered, and an id absent from the local store warns for itself rather than costing the others their close. Results land per id in the JSON envelope under searches, one human line each. A lone id keeps the flat search field callers have read since Publish-back loop DX: no defer status, invisible publish.mode, lost --search-id linkage (agent session trace) #161.
  • One prefill. The card's questionsAnswered is prefilled from the first named search this machine has a record of; the rest are attribution, not phrasing. Each receipt says which id supplied it.
  • Draft rule unchanged. A --draft answers nobody however many searches it names, so it sends no searchId at all and leaves every loop open.
  • MCP tenjin_publish takes searchId as a string or an array, matching what feat(outcome): batch close, and a WebSearch hook that names itself #177 did for the outcome tool.

Wire shape, and the deploy ordering

// one search — byte-identical to what ships today
{ "status": "published", "searchId": "0197…-…-eeee" }

// several — needs the sibling tenjin PR deployed
{ "status": "published", "searchId": ["0197…-…-aaaa", "0197…-…-bbbb"] }

A lone id stays the bare string, so a single-search publish keeps working against a post-create that only takes a scalar. The array form requires the sibling tenjin PR (branch vraspar/post-lookups-multi-searchid) deployed first — an array against the current validator is a hard 400. The changeset states the same ordering.

The committed OpenAPI fixture still declares the scalar, so contract.test.ts continues to pin the single-id body against it; the array is pinned at the builder in posts-api.test.ts instead.

Rejected alternatives, honored

Per #167, none of these are here and none should arrive later: no similarity auto-linking (false links poison outcome data), no close-all-session-MISSes on publish (one session holds unrelated investigations), no explicit thread handles on searches.

Scope

The outcome half of #167 shipped separately in #177 (tenjin outcome --search-id repeating, plus --all-open). This PR is the publish half and closes the issue. Nothing here touches ephemeral status or the auto-sweep, which belong to the other train.

Design record: tenjin-notes/plans/2026-08-17-outcome-taxonomy-and-167.md, "Closing #167".

Closes #167.

Tests

New coverage in src/commands/publish.test.ts, src/lib/posts-api.test.ts, and src/cli.test.ts: the dispatcher collects rather than replacing (read back through the cap, which only eleven surviving ids can trip); the wire carries an array for several ids and a bare string for one; repeats collapse on the wire and in the ledger; the cap refuses before the keystore opens; a mix of closeable, relinked, already-answered, and absent ids each reports honestly; the prefill names its source; a multi-id draft sends nothing.

pnpm typecheck, pnpm lint, and prettier --check are clean. Full vitest run: 2148 passed, 2 failed — both pre-existing in doctor.test.ts on this machine and reproduced identically at origin/main (they read the developer's real installed skills state).

🤖 Generated with Claude Code

`tenjin publish --search-id` repeats, up to ten per piece. A research
thread fans out into many searchIds, and until now the piece published
back against one while the siblings were closed as `regenerated` — false
twice over, and it counted every sibling of a converting loop as a MISS.

Each id closes its own local loop with publish attribution (relinking
one an `outcome` already closed, exactly as a single id does today), and
the JSON envelope reports per id under `searches`, so an id this machine
never recorded warns for itself instead of costing the others their
close. A lone id keeps the flat `search` field.

On the wire, one id stays the bare string it has always been and several
become an array, so a single-search publish is byte-identical against a
server that predates the array.

Closes #167.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A1igator added a commit that referenced this pull request Aug 18, 2026
The live hosted skill moved again tonight (the #178 multi-searchId wire
form), so the drift gate reddened on this branch, which merged main
before the move. Mirror-only: scripts/sync-skill.mjs writes
skills/tenjin/SKILL.md and nothing else, so the scan rule data, the
corpus, and the tenjin-publish detector lists are untouched.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@vraspar
vraspar marked this pull request as ready for review August 21, 2026 03:57

@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.

@vraspar vraspar added the critical Drop everything: data-corrupting or user-facing breakage label Aug 21, 2026

@A1igator A1igator 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 claim model holds up, the failure mode is a 400 after the wallet touch

Reviewed against main at 18385e4, verified at eda2419. Server behavior below was read from BackTrackCo/tenjin origin/main, not from a local checkout and not from memory of a sibling PR.

The adversarial question, answered explicitly. Can a piece claim a search it did not answer? Yes, and the bounds are real but narrower than the feature's framing suggests:

  • Who can write a claim: anyone holding the uuid. Possession is the capability, as on the outcomes route. What has changed since the older posture is that existence is now enforced: assertClaimedLookupsExist rejects ids with no lookups row, so a fabricated uuid is unclaimable and a swept one stops being claimable.
  • Claiming broadly: capped at 10 per post, and the server's cap is a per-post TOTAL across edits rather than per request, specifically so N PUTs of 10 cannot land 10N rows. Nothing caps claims per publisher: a creator can run free anonymous searches, then attach ten of their own to each piece they publish, and no per-creator budget exists. assertClaimCapacity also notes two concurrent PUTs can read the same pre-write count and overshoot.
  • Two pieces, one search: allowed by design. posts.lookup_id is non-unique and the schema comment says one lookupId may attribute many posts, so claims are not exclusive.
  • Reversible: no. Claims accumulate and a later PUT adds ids and removes none. A claim is permanent.

None of that is introduced here. The mechanism, the cap, the existence check and the post_lookups join table all already shipped server-side. What this PR changes is that claiming ten is one command instead of ten, which is worth saying plainly rather than treating as a new hazard: on the evidence I can see, this is not the data-corrupting change the critical label describes. The two findings below are a broken publish and an unpinned contract.

Major

  1. [data-integrity] one stale id fails the whole publish, after the signature, with nothing warning first: the server takes the batch as a unit. assertClaimedLookupsExist retries twice for the after-response race, then 400s the entire request if any id has no lookups row. The CLI already loads the local store in loadNamedSearches, knows which ids it has never recorded, and sends them anyway, reporting the miss only in the post-publish receipt that a 400 means never runs. The inputs that trigger this are ordinary, not exotic: the local store keeps 50 entries and the server sweeps lookups at 90 days, so "an id this machine still has and the server no longer does" is a normal state, and the feature multiplies the exposure by ten. This is the failure the deleted validateSearchId comment was written about, a refusal arriving after the wallet signature rather than at the edge. Fix: warn at the edge for any id absent from the local store, before the wallet touch, and say in the flag help and the reference row that the server accepts or refuses the batch as a whole.

  2. [testing] the cap and the array form are asserted in a comment and pinned by nothing: SEARCH_ID_MAX = 10 carries "mirroring the server's cap", and the fixture this branch ships still declares PostCreate.searchId as {"type":"string","pattern":...} with no union, no array, no maxItems. contract.test.ts:558 pins only that pattern, so the one test built to catch this drift cannot see it, while every neighbouring bound (title, bodyMd, excerpt, tags, price, handle, status) is pinned literal by literal under a comment saying to pin the lot precisely because a server move regenerates the fixture and silently diverges. The server does declare union([string, array().min(1).max(SEARCH_ID_CLAIM_MAX)]), so the number is right today and nothing would tell you when it stops being. Fix: regenerate the fixture and add the array form and its maxItems to that block.

Minor

  1. [hygiene] "mirroring the server's cap" describes a different cap: normalizeSearchIds bounds one request; the server bounds the post's lifetime union, and its own comment explains why the request-level bound is not enough. They coincide only because the CLI never sends searchId on the update path today. Naming which cap it mirrors is what keeps a later edit --search-id out of the trap the server comment already documents.

  2. [hygiene] the changeset's shipping caveat is stale: "several ship as an array, which needs the array-accepting post-create deployed first" reads as a dependency that is still pending, and that code is on tenjin main already. Whether production has it is an ops fact a changeset cannot assert either way, so name the state rather than the condition.

Nits (2), none blocking
  1. [hygiene] the prefill is order-dependent: searchIds.find(...) takes the first locally-recorded id in the order the flags were typed, so which question prefills the card is a property of argv. Fine, but the comment says "the first recorded id" where the mechanism is "the first you typed that we happen to hold".
  2. [hygiene] the base is behind main: merge base is 0c92f5b, and #177, #194 and #113 have landed since. GitHub reports MERGEABLE so git will take it, but #177 rewrote the same docs/command-reference.md outcome table and the same mcp/server.ts outcome input this PR edits, and a clean auto-merge can still leave one table describing two features in two voices. Worth a read of that file after the rebase rather than trusting the merge.
Verified, not issues
  • Ordering is right on the failure path: publishPost runs before any local close, so a rejected publish leaves every local loop open rather than marking searches resolved against a piece that never landed.
  • toWireSearchId keeps a lone id a bare string, so the single-id publish is byte-identical to today's and cannot regress on an older server.
  • Dedupe happens before the cap, so naming one id twice costs one slot, matching the server's claimedSearchIds and its ON CONFLICT no-op on re-sends.
  • A --draft still sends no attribution, which is the rule that keeps one demand signal off two posts.
  • normalizeSearchIds runs at the command edge and again in the body builder, so the MCP path, which does not pass through the CLI edge, is bounded by the same rule.

Verdict: comments-only. The claim model is the server's and it is in better shape than the CLI's comments about it; the CLI is a faithful client of it except at the edge, where it sends ids it already knows are unrecorded locally and lets the server refuse the batch after the wallet signature. Ran locally: head checkout, the OpenAPI fixture resolved for the PostCreate.searchId node, and the server's schema, existence check, capacity check and join-table write read from tenjin origin/main.

@vraspar

vraspar commented Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

Review: comments-only — no majors; 4 minors, 2 nits, all about pinning the new array shape and one dedupe gap

Reviewed against the diff, docs/CONVENTIONS.md, and the sibling #177 outcome wiring; verified at eda2419 in a clean worktree. Three-panel review (security/adversarial, backend+testing, hygiene+agent-usability). Touched suites pass at head: 150/150 across cli.test.ts, publish.test.ts, posts-api.test.ts; typecheck/lint/format clean.

What's solid:

  • Cap refusal genuinely precedes any wallet touch: normalizeSearchIds runs before readSource, consent, and publishPost, with a test asserting zero fetch/signer calls on 11 ids.
  • Per-id closes are independent by construction: markSearchResolved never throws, and closeNamedSearch maps failed/not-found to per-id closed:false warns — proven by the four-state test (closeable/relinked/alreadyAnswered/absent) asserting real ledger state.
  • No protocol duplication: one normalize gate shared by edge and builder, wire conversion only in toWireSearchId, ledger writes only in the single close loop. The lone-id envelope stays byte-identical to today's shape, and closes run strictly after a successful POST, so a failed publish never closes anything.

Minor

  1. security Case-variant spellings of one uuid defeat dedupe and can ship up to 10× on the wire: [...new Set(ids)] compares exact strings while SEARCH_ID_WIRE_RE accepts mixed-case hex, so 0197AAAA-… and 0197aaaa-… survive as two entries — the second reads as an absent id locally (spurious warn) while both elements hit the wire for the same search. If the server attributes per element without case-normalizing, one piece inflates a single search's conversion count up to 10×. Fix: canonicalize before dedupe (list.map((s) => s.toLowerCase()) inside the Set). Server-side normalization needs verification.
    const ids = [...new Set(typeof searchId === 'string' ? [searchId] : searchId)];
  2. testing The array half of the dual wire shape is pinned only by local expectations — two instances, one cause: (a) contract.test.ts's emitted-fields loop skips non-string values and every fixture cross-check build passes a lone scalar, so an emitted array is never diffed against the declared schema (the reverse direction is guarded: the fixture-pattern assertion reds CI if someone regenerates the fixture to array form); (b) server.test.ts has no case passing searchId: [A, B] through tenjin_publish, so a mapping regression that drops arrays ships green. Fix: a two-id build asserted against the fixture once the array-accepting post-create lands, plus one MCP-level array case asserting both ledger closes and the array on the wire.
    for (const [key, value] of Object.entries(body)) {
    const pattern = get(nonNull(get(declared, key)), 'pattern');
    if (typeof pattern !== 'string' || typeof value !== 'string') continue;
    expect(new RegExp(pattern).test(value), `${key}=${value} violates ${pattern}`).toBe(true);
    }
    ,
    searchId: z
  3. backend Deploy-ordering failure surfaces as a generic post-signature 400: against the un-deployed server, publishFailed wraps the old validator's zod message into exit-4 "Review the server error" — after the wallet signature, with no hint the cause is rollout ordering. Fix: when status is 400 and the server message mentions searchId, append "multi-searchId needs the array-accepting post-create deployed" to the fix text.
    function publishFailed(res: HttpResponse): CliError {
    return new CliError(
    'PUBLISH_FAILED',
    serverMessage(res.json) ?? `Publish failed (${res.status}).`,
    {
    fix: 'Review the server error, then re-run `tenjin publish`.',
    details: { status: res.status, ...(res.json !== undefined ? { server: res.json } : {}) },
    },
    );
    }
  4. testing Exactly-10 boundary never asserted: tests cover 1, 2, 3, dupes, and 11-refusal, but nothing runs exactly SEARCH_ID_MAX ids through edge or builder, so a future >>= typo silently refuses legal publishes. Fix: one case with SEARCH_ID_MAX ids asserting success.
  5. agent-usability search vs searches envelope duality documented nowhere an agent reads: the switch rule lives only in a code comment and the changeset. A programmatic caller parsing --json reads .search today; after a 2-id publish that key silently vanishes. Fix: one line in the publish section of command-reference.md and/or SKILL.md naming both keys and the condition.
    Publishes Markdown with optional metadata and a local safety scan. Hard blocks cannot be bypassed.
    | Flag | Effect |
    | -------------------------- | ------------------------------------------------------------------------ |
    | `--search-id <uuid>` | Link the piece to the search it answers. Repeatable, up to 10 per piece. |
    | `--draft` | Save privately instead of publishing. |
    | `--price <usd>` | Set the post price. |
    | `--excerpt <text>` | Public preview. |
    | `--mode <mode>` | `review`, `auto`, or `full-auto` for this run. |
    | `--yes` | Clear warning findings and the review confirmation. |
    | `--question <text>` | Question this piece answers. Repeatable. |
    | `--task <text>` | Task this piece supports. Repeatable. |
    | `--scope <text>` | What the piece covers. |
    | `--exclusions <text>` | What it does not cover. |
    | `--applies-to <key=v1,v2>` | Applicability metadata. Repeatable. |
    | `--as-of <time>` | When evidence was gathered. |
    | `--valid-until <time>` | When the answer expires. |
    | `--artifact-type <type>` | `document`, `skill`, or `dataset`. |
    | `--temporal-mode <mode>` | `snapshot`, `maintained`, or `evergreen`. |
    | `--provenance <text>` | How evidence was obtained. |
    | `--methodology <text>` | How it was established. |
    ### `tenjin edit <post-id>`
    With no change flag, prints one of your posts and its card. With change flags, merge-updates the post.
Nits (2), none blocking
  • src/commands/publish.ts:104 — the edge error hardcodes label '--search-id', so an MCP agent sending a bad id via tenjin_publish gets "Invalid --search-id", naming a flag it never used. Thread a surface-neutral label through PublishArgs.
  • src/commands/publish.test.ts — the PR hoisted this describe block's bodyServer/questionsIn to module scope but left three byte-identical nested copies in later describes; hoist or revert consistently.
Verified, not issues

Cap-before-wallet ordering; ids regex-checked at edge and again at the builder before JSON.stringify (no injection); ledger writes file-locked, atomic, zod-parsed; local-close TOCTOU surfaces as honest not-found warns since each close re-reads under lock after successful publish; concurrent publishes on overlapping ids end in truthful relinked/alreadyAnswered receipts; drafts omit searchId entirely including frontmatter-forced ones; flat search retained iff exactly one receipt regardless of closed state, and no non-test reader of .data.search exists (MCP passes data through verbatim); MCP union forwards untouched into edge validation; attribution quantity change (1→10) mirrors the server cap and removes no existing check; prefill with no local record yields no prefill and per-receipt prefill:'none'; CI runs format/lint/typecheck/test plus pack-smoke, and contract-drift re-runs the fixture suite nightly.

Verdict: comments-only. No majors; the minors are cheap, well-scoped follow-ups (the dedupe canonicalization is a one-liner). What ran locally: touched vitest suites (150 passed), pnpm typecheck, pnpm lint, prettier --check.

@A1igator

Copy link
Copy Markdown
Contributor

Answering the open server-side question in your minor 1, since it is checkable from the tenjin side. Verified against BackTrackCo/tenjin origin/main at d7a2cc78.

The 10x conversion-count inflation does not happen. post_lookups.lookup_id is a Postgres uuid column, not text, and the table's primary key is the composite (post_id, lookup_id) (lookup.ts:163-179). Postgres canonicalizes uuid input, so 0197AAAA-... and 0197aaaa-... are one value at the column, the PK rejects the second, and linkPostLookups inserts with an ON CONFLICT no-op anyway (posts.ts:173-178). One search cannot be attributed twice to the same post, whatever case the client sends.

There is a smaller bug next door, and it fails safe. assertClaimCapacity counts in JS, not in the database: new Set([...held.map((r) => r.lookupId), ...claims]) (posts.ts:158-171). held comes back from the driver lowercase, while claims carries whatever case the client sent. So two case-variants of one uuid count as two against the cap of 10, and re-sending an already-held id in different case counts as new capacity rather than matching what the post holds. The effect is a post refused at the cap while holding fewer distinct claims than the count suggests. That is a false refusal, never an over-claim, so it is the safe direction to be wrong in.

Both fixes still stand, for the reasons you gave rather than for the inflation. Your one-liner canonicalizing before the Set is right, and the server wants the matching toLowerCase() on claims before the capacity comparison. Your other observation holds unchanged: the second spelling still reads as an absent id locally and produces a spurious warn.

One note on how our two reviews sit together, since the difference changes what a follow-up has to fix. Your "cap refusal genuinely precedes any wallet touch" is right about the cap gate. The major I filed is about a different gate on the same batch-as-a-unit design: assertClaimedLookupsExist 400s the whole publish when any one id has no lookups row, and that refusal does land after the signature. Your minor 3 names the same post-signature shape for the deploy-ordering trigger. The stale-id trigger is the ordinary one, since the local store holds 50 entries and the server sweeps lookups at 90 days.

vraspar and others added 2 commits August 21, 2026 14:54
…rray contract

The server matches every named id against a recorded search and refuses the
whole batch if one misses, so a stale id took down the publish after the
wallet had signed with nothing said first. The local store keeps 50 entries
against a 90-day server sweep, which makes "mine, not theirs" ordinary rather
than exotic. Ids this machine has no record of are now named on stderr before
the wallet is touched, and the batch semantics are stated in the flag help,
the MCP description and the command reference. It stays a warning: an id
recorded on another machine is absent here and valid there.

The fixture's PostCreate/PostUpdate searchId nodes are updated from the live
spec, which declares the union, and contract.test.ts pins both branches plus
minItems/maxItems, so SEARCH_ID_MAX is guarded rather than asserted in a
comment.

Also: SEARCH_ID_MAX now says which cap it mirrors (the server's is a lifetime
bound across updates, not a per-request one), the changeset names the shipped
state rather than a pending dependency, and the prefill comment describes the
mechanism it actually has.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@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.

@vraspar

vraspar commented Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

All six addressed. Merge be4a365, fixes 0122444.

Major 1, the batch 400 after the signature. warnUnrecorded (src/commands/publish.ts:287, called at :113) names every id absent from the local store on stderr, before the wallet is touched, and says the server takes the named searches as one batch. It runs after resolveStatus so a --draft, which sends no attribution and has no batch to refuse, stays quiet. A warning and not a refusal, for the reason you gave: an id recorded on another machine is absent here and valid there. Stated in the flag help (src/cli.ts:474), the MCP description (src/mcp/server.ts:146) and a new prose paragraph under the publish table in docs/command-reference.md rather than in the row, which was already at 300 characters. Three tests: the warning is out while getSignerCount() is still 0 on the consent-refusal path, silence when every id is recorded, silence on a draft.

Major 2, the unpinned contract. Fixture updated and pinned: contract.test.ts:559-564 now walks the anyOf, pinning the scalar branch's pattern, the array branch's type, items.pattern, minItems and maxItems against SEARCH_ID_MAX. A new test at :691 checks each emitted shape against the branch it lands on, because the value loop above it skips this field now that its bounds sit under anyOf. Live already serves the union, so this is pinned against production rather than against a pending deploy.

One scope call worth flagging: I updated the PostCreate.searchId and PostUpdate.searchId nodes from the live spec, not the whole document. The committed fixture is a long way behind live (4 new paths, 12 new schemas, 10 changed), and a wholesale swap fails an unrelated assertion: SearchCandidate now declares temporalMode, which CANDIDATE_MOVED_TO_INSPECT pins as a negative on purpose ("a server that puts them back on a candidate has un-done search v2"). That is a real divergence and someone should decide it, but deciding it inside a publish PR would be me guessing at a search-surface question. Flagged for its own issue; happy to be overruled and do the full regeneration here if you would rather.

Minor 1, the cap comment. src/lib/posts-api.ts:75-82 now says which cap this mirrors: the server's bounds a post's claims over its whole lifetime across every later update, so N requests of 10 cannot land 10N, and the numbers coincide only because the CLI never sends searchId on the update path. The live description says the same thing ("Claims accumulate whatever form you send: a later PUT adds ids and removes none"), so a future edit --search-id reusing this constant is named as the trap.

Minor 2, the stale caveat. The changeset now says several ids "ship as an array, which the live post-create takes". Verified rather than assumed: GET https://tenjin.blog/openapi.json serves anyOf: [string, array(minItems 1, maxItems 10)] today, on both the .blog and .sh hosts.

Nit 1, the prefill. Comment now reads "the first id you typed that this machine happens to hold", which is the mechanism.

Nit 2, the base. Merged origin/main at 18385e4 as an ordinary merge commit. Read both contested files end to end afterwards rather than trusting the auto-merge: the outcome and publish tables sit next to each other in one voice, and the two mcp/server.ts inputs land on the identical union([string, array]) shape. No hook script emits a literal publish --search-id string that needed updating.

typecheck, lint and prettier --check clean. Full suite 2334 passed, 2 failed, both the pre-existing doctor.test.ts publish-mode cases that read this machine's installed skills state and fail identically at origin/main. No pack-smoke.

@vraspar

vraspar commented Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

End-to-end contract check against tenjin#709 (merged): design matches, two wrinkles to resolve

Verified this PR's wire contract line-by-line against the merged server half (BackTrackCo/tenjin#709, now on main past the rebrand). The core shape matches: scalar-for-one/array-for-many vs the server's z.union([uuid, array.min(1).max(10)]), dedupe on both sides, re-sends are no-ops (ON CONFLICT DO NOTHING) under accumulate semantics, drafts send nothing, and the server never returns searchId (legacy lookupId column carries the first id for scalar readers), so the local searches envelope conflicts with nothing. The deploy-ordering window is nearly closed — remaining ask is confirming #709 is deployed before this merges.

Two semantic mismatches found in the comparison, worth resolving here or in an immediate follow-up:

1. Cap semantics differ in kind: per-request here, per-post-lifetime on the server

This PR caps one publish at 10 ids (SEARCH_ID_MAX). The server caps a post's total claims across its lifetime at 10 (SEARCH_ID_CLAIM_MAX, enforced in assertClaimCapacity over the union of held rows + new claims inside the write tx). So: publish with 10 claims, later edit adds more → this CLI passes its own cap check, then the server 400s searchId: ["a post may claim at most 10 searches in total"]. Same for 3-now-+-8-later. The code comment acknowledges the difference; the gap is that the CLI surfaces nothing until the wallet-signed request fails. Suggested resolution (pick one):

  • Track claims-per-post locally (the ledger already knows which ids a piece claimed) and enforce the lifetime union before approval, mirroring assertClaimCapacity; or
  • On publishFailed with status 400 whose server message mentions the cap, map it to a USAGE-class error naming the actual constraint ("10 per piece lifetime, not per publish") instead of generic exit-4.

2. Existence checking is asymmetric: warn-and-continue locally, all-or-nothing on the wire

Locally, an id absent from the store warns for itself and the others still close. On the wire, the server requires every claimed id to reference a recorded search — assertClaimedLookupsExist retries briefly for a just-recorded search, then 400s the entire publish (searchId must reference a recorded search) after wallet signature. A typo'd id, or one already dropped by the server's 90-day telemetry sweep, kills the whole publish rather than degrading. Suggested resolutions:

  • Before building the body, drop ids with no local record from the wire entirely (keep the stderr warning) — a search this machine never made can't be a legitimate claim anyway; or
  • Keep sending but pre-warn loudly that an unrecorded id will fail the whole publish server-side, and map that specific 400 fieldError to a fix line distinguishing typo vs swept-search.

Option A of each is the simpler path and keeps the CLI's honest-partial-success story true end to end. Everything else — validation strictness (CLI regex is stricter than zod .uuid(), the safe direction), draft handling, relink/receipt semantics, byte-compat scalar form — checked out clean.

@A1igator A1igator 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: both majors close, one new minor from the fix itself, the fixture scope call holds

Reviewed against R1 (pullrequestreview-4989690773, at eda2419), verified at 0122444. be4a365 is an ordinary merge of origin/main; read the two files it collided on (docs/command-reference.md, src/mcp/server.ts) end to end rather than trusting the auto-merge — both read coherently in one voice, no re-review needed there.

Resolved since R1

  1. Major 1 (data-integrity), batch 400 after the signature: closed. warnUnrecorded (src/commands/publish.ts:287, called unconditionally for a non-draft at :113) runs before the blocking-scan check, the confirmation gate, resolveWalletProvider, and getSigner() — I read the function linearly and there is no other branch that reaches publishPost without passing through line 113 first, so the ordering claim holds for every signer path, not just the common one. It's a warning, not a refusal, for the stated reason (an id recorded on another machine is absent here, valid there); that trade-off is sound. Three tests back it (src/commands/publish.test.ts:1017,1036,1043), including one that asserts getSignerCount() is 0 while the warning is already on stderr. publish.ts#L280-L297
  2. Major 2 (testing), the array contract pinned by nothing: closed. contract.test.ts:558-564 now walks the anyOf and pins both branches (scalar pattern, array type/items.pattern/minItems/maxItems against SEARCH_ID_MAX), and a new test at :691 builds a scalar and a 2-id array through buildPostCreateBody and checks each against the branch it lands on. I curled https://tenjin.blog/openapi.json directly: the live PostCreate.searchId and PostUpdate.searchId nodes are byte-identical to what this commit hand-patched into the fixture. 0122444
    • Bonus, not asked for but real: this also closes a gap from the PR author's own separate review (comment 5373416583, minor 2) — "an emitted array is never diffed against the declared schema." The new :691 test now does exactly that.
  3. Minor 1 (the cap comment): closed. src/lib/posts-api.ts:75-82 now names which cap SEARCH_ID_MAX mirrors and why the numbers only coincide today.
  4. Minor 2 (stale changeset caveat): closed, and verified true right now, not just reworded: the changeset says the array form "ships as an array, which the live post-create takes," and live does.
  5. Nit 1 (order-dependent prefill wording): closed, comment now says "the first id you typed that this machine happens to hold."
  6. Nit 2 (base behind main): closed. Merge is a clean ordinary merge; docs/command-reference.md's outcome and publish tables sit next to each other with the same "Repeatable" phrasing, and src/mcp/server.ts's outcomeInput.searchId and publishInput.searchId are the identical z.union([z.string(), z.array(z.string())]) shape.

New

  1. [hygiene] warnUnrecorded inherits the unfixed case-sensitivity gap, and now gives it a new symptom: the PR author's own review (comment 5373416583, minor 1) flagged that normalizeSearchIds's [...new Set(ids)] (src/lib/posts-api.ts:94) compares exact strings while SEARCH_ID_WIRE_RE accepts mixed-case hex. This round didn't touch that line, and the local store (search-store.ts) is exact-string-keyed too (s.searchId === searchId), so warnUnrecorded's stored.has(id) (src/commands/publish.ts:292) is exact-string as well. A locally-recorded id passed back in a different case now trips the new "Not in this machine's search store" stderr warning even though the search is genuinely on file — a false alarm this round adds rather than removes. Low probability (ids are normally copy-pasted verbatim from tenjin search output) but a one-line fix if the canonicalization ever lands: .toLowerCase() both sides of the Set and the store lookup. Not blocking.

Verified, not issues

  • The case-variant dedupe question, closed from the server side: per the reviewer's own reply (comment 5373434260) against BackTrackCo/tenjin origin/main, post_lookups.lookup_id is a Postgres uuid column under a composite PK, so no per-search conversion-count inflation is possible regardless of client-sent case. The real adjacent bug (assertClaimCapacity double-counting case-variants into a false refusal) lives in the tenjin repo, not here.
  • Test suite: ran the full suite at head — 2336 passed, 0 failed, 10 skipped. That doesn't match the author's reported "2334 passed, 2 failed" exactly, but CI's own run for this head (CI (lint, typecheck, build, test, audit)) is green, and the described failure mode (machine-local installed-skills state) is exactly the kind of thing that wouldn't reproduce on a different machine or in CI. typecheck, lint also clean locally.
  • CI: skill-drift (mirror is in sync) fails at this head; confirmed the failing diff is exactly the skills/tenjin/SKILL.md paywall/card-field hunk carried by the pending #198 resync PR, unrelated to this diff.

Decisions

  1. The fixture scope call (hand-patch vs. full regenerate): hand-patching only PostCreate.searchId/PostUpdate.searchId is the right call and doesn't weaken the drift guard. Two reasons: (a) the mechanism that actually catches server drift in production is the nightly contract-drift job, which runs assertPublishContract/assertUpdateContract/etc. directly against live openapi.json (describe.skipIf(liveBase === undefined, ...) at contract.test.ts:801), not against the committed fixture — a stale fixture doesn't blind that check at all; (b) the offline, fixture-based tests were never a full-document diff to begin with (they walk a curated field list), so nodes this patch didn't touch were exactly as unprotected before the patch as after it. Selectively updating the node under test doesn't mask drift elsewhere, because nothing was watching that drift either way.
    • I verified this concretely: origin/main's assertPublishContract still has the pre-fix scalar-only pin (bound(top, 'searchId', 'pattern')), and live serves the anyOf union right now (confirmed by direct curl, byte-identical to this PR's fixture patch). The last scheduled contract-drift run (2026-08-21 09:53 UTC) still passed clean, meaning the array-accepting deploy hadn't promoted to prod yet at that time — it promoted sometime between then and now. Concretely: if this PR doesn't merge before tomorrow's cron, the nightly job goes red on exactly the field this PR fixes. That's the guard working as designed on a same-day promotion-cadence lag, not a design gap.
    • The general staleness (4 new paths, 12 new schemas, 10 changed fields) is real and already tracked: issue #140 ("Generate the OpenAPI fixture from live instead of hand-patching it key by key"), filed after PR #132's review, names this exact failure mode ("the parts under test stay honest and the rest rots"). This PR is a second confirming instance of #140's pattern rather than a new problem; no new issue needed, but worth a nudge on #140's priority given it's recurred.
  2. Out of my scope for this delta, flagged for visibility: the PR author's own separate review (comment 5373416583) left minors 3-5 and two nits — deploy-ordering 400 message clarity, the exactly-SEARCH_ID_MAX boundary untested, the search/searches envelope duality undocumented for agents, the MCP edge-error label, and the publish.test.ts hoisting inconsistency. None of those were addressed by 0122444 (only minor 2 closed incidentally, noted above). They're that reviewer's thread to track, not re-scored here.

Verdict: comments-only. Both R1 majors are genuinely closed with code I read and tests that back the claim, not just restated comments. One new minor (case-variant warning) and one decision (fixture scope, which I judge correct) remain, neither blocking. Ran locally: full worktree checkout at 0122444, pnpm typecheck/lint clean, full vitest run (2336 passed/0 failed/10 skipped), direct curl of live tenjin.blog/openapi.json and .sh mirror, gh run view on the current CI and skill-drift checks, and gh run list/gh run view --log history on contract-drift.yml.

@A1igator

Copy link
Copy Markdown
Contributor

Our posts crossed by about a second, so the R2 review above was written before your contract check landed and does not answer it. Three things.

Your remaining ask is answered: #709 is deployed. Verified during R2 by direct curl, live serves anyOf: [string, array(minItems 1, maxItems 10)] on both hosts right now, and the fixture's hand-patched PostCreate.searchId / PostUpdate.searchId nodes are byte-identical to it. The promotion time is pinnable too: the last scheduled contract-drift run at 09:53 UTC today passed clean against main's old scalar-only pin, so the array-accepting server promoted to prod sometime after that run. The deploy-ordering window is closed, not nearly closed.

A consequence you will want to know. Because the nightly job checks live rather than the committed fixture, if this PR does not merge before the next cron, that job goes red on exactly the field this PR fixes. That is the guard working as designed against promotion-cadence lag, not a defect, but it will look like a new failure if it lands while you are not watching.

Your wrinkle 2 is my Major 1, and your Option A undoes the reason you gave for the current design. In the fix round you justified warning rather than refusing on the ground that an id recorded on another machine is absent here and valid server-side. I agreed, and still do. Option A, dropping ids with no local record from the wire entirely, discards exactly those legitimate second-machine claims, and discards them silently. Silent loss of a valid claim is worse than the 400, because the 400 at least tells the publisher something happened. Option B keeps the multi-machine case working and fixes what is actually wrong, which is that the failure is unexplained rather than that it occurs. On your wrinkle 1, the same reasoning favours your own Option B: mapping the cap 400 to a USAGE-class error naming the real constraint costs nothing, while tracking the lifetime union locally means the CLI models server state it cannot keep in sync, and a stale local count would refuse legal publishes.

One correction on the numbers. My full local run at this head was 2336 passed, 0 failed, 10 skipped, so the two doctor.test.ts failures you saw are machine-local installed-skills state rather than anything on the branch. R2 also adds one new minor: warnUnrecorded reuses the exact-string store lookup, so it inherits the case-sensitivity gap and gives it a fresh symptom, a spurious "not in this machine's search store" warning for an id that is genuinely on file in another case.

Upstream moved again since #194: the paywall marker now documents its
block-level placement and says the publish still succeeds with a warning,
and the card section records that cacheEligible, cacheEligibleMissing and
schemaVersion are server-computed and ignored on a write.

Regenerated with `pnpm sync:skill`, no hand edits. Clean origin/main
produces the identical diff, so this is upstream drift rather than
anything this branch did.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@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.

@vraspar

vraspar commented Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up to my reply above. 48a7496 clears the red check; both checks are green at that head.

skill-drift. Regenerated skills/tenjin/SKILL.md with pnpm sync:skill, committed alone, no hand edits (the file carries a do-not-hand-edit banner and is prettier-ignored so it stays byte-identical to source). Upstream had moved again past #194's resync: the paywall marker now documents block-level placement and says the publish still succeeds with a warning, and the card section records that cacheEligible, cacheEligibleMissing and schemaVersion are server-computed and ignored on a write. That is tenjin#731's text, nothing from this branch.

Provenance, since a vendored file changing inside a feature PR deserves it. Negative control first: a detached worktree at clean origin/main, node scripts/sync-skill.mjs, same 8-insertion/4-deletion diff, so main drifts identically and this branch did not cause it. The regenerated blob hashes to 1f648e0e65be7a1cb00babc627ec87f5f8ef1228, which is exactly the blob CI computed in the failing diff (index 880851b..1f648e0) and exactly the blob on bot/skill-resync for #198. Running the script twice is idempotent to the same hash. So whichever of this PR and #198 lands second is a no-op on that file, and there is no conflict to resolve between them.

On R2's new minor (case-variant ids). Correct, and I am not fixing it in this round without a yes: normalizeSearchIds's Set and stored.has(id) are both exact-string, so a mixed-case id that is genuinely on file trips a false "not in this machine's search store". Two notes on scoping it. Lowercasing only warnUnrecorded's lookup would silence the false warning while leaving the dedupe gap that produces a duplicate wire claim, so the honest fix is to canonicalize once inside normalizeSearchIds and have the store lookup read the canonical form, which changes what goes on the wire and wants its own test for the dedupe path. Worth doing, one line plus a test, but it is a behavior change in shared code rather than a comment fix, so it is the owner's call whether it rides here or goes to its own PR. Say which and I will do it.

Not dropped. Minors 3 to 5 and the two nits in comment 5373416583 (deploy-ordering 400 message, the exactly-SEARCH_ID_MAX boundary untested, the search/searches envelope duality undocumented for agents, the MCP edge-error label, the publish.test.ts hoisting inconsistency) are still open and unaddressed by 0122444. R2 left them with that reviewer; I am flagging that I have not silently closed any of them.

One correction accepted: R2's full-suite run is 2336 passed, 0 failed. The two doctor.test.ts failures in my report reproduce identically at clean origin/main on this machine and are its installed-skills state, not the branch, which is why CI is green.

@A1igator

Copy link
Copy Markdown
Contributor

Reviewer note on the scoping question, ahead of the full R3 pass on 48a7496.

Your provenance holds, re-derived independently. skills/tenjin/SKILL.md hashes to 1f648e0e65be7a1cb00babc627ec87f5f8ef1228 at this head and to the same blob on bot/skill-resync for #198, while main is still at 880851b8. The two resyncs are byte-identical, so whichever lands second is a no-op on that file and there is nothing to reconcile between them. Both checks are green at this head.

On whether the case-variant fix rides here: it is not shared code, it is this PR's code. normalizeSearchIds is introduced by this PR, as a new export in src/lib/posts-api.ts, and its only two callers (src/commands/publish.ts and toWireSearchId in the same file) also arrive here. Nothing on main calls it. So a follow-up PR does not repair an existing function, it lets a function land with a known defect on its first appearance and then fixes it. My recommendation as reviewer is that it rides here. The owner can overrule that.

One fact that raises the stakes on the dedupe half, which is why I agree with your scoping instinct over the narrow fix. The duplicate wire claim is harmless for attribution: post_lookups.lookup_id is a Postgres uuid column, which canonicalizes case, and the composite primary key (post_id, lookup_id) makes the second write a no-op. Capacity is where it bites. assertClaimCapacity counts with a JS Set over the raw strings, so one id sent in two cases counts as two against the cap and can refuse a publish that is inside the limit. It fails in the safe direction, but it is a real false refusal, and canonicalizing only inside warnUnrecorded would leave it standing. Canonicalizing once inside normalizeSearchIds, as you propose, closes both halves.

Acknowledged. Minors 3 to 5 and the two nits in 5373416583 are open and I have them on the ledger for R3. Your correction on the suite is taken: 2336 passed with 0 failed, and the two doctor.test.ts failures reproduce at clean origin/main on your machine, so they are installed-skills state rather than this branch.

No approval implied by this note.

…s minors

The wire regex accepts mixed-case hex while the dedupe Set compared exact
strings, so two spellings of one uuid survived as two entries: a spurious
"not in this machine's store" warning for the second, and both elements on
the wire claiming a single search. normalizeSearchIds now lowercases before
deduping, which is what Postgres does with a uuid column anyway; the store
lookup is keyed the same way so an entry recorded in another spelling is
still found, and the ledger write uses the record's own spelling because
the store matches ids exactly.

Also from review: a 400 whose message names searchId now says a deployment
predating the array-accepting post-create is the likely cause, since that
refusal lands after the signature; the MCP tool labels edge errors
`searchId` rather than a flag it has no way to pass; the envelope duality
(`search` for one id, `searches` always) is documented where agents read;
and publish.test.ts's duplicate bodyServer copies are gone.

Tests: case-folding through the builder and through a real publish, exactly
SEARCH_ID_MAX ids accepted, the rollout hint present on a searchId 400 and
absent otherwise, and an MCP array publish asserting the wire array, both
ledger closes, and the missing flat key.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@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.

@vraspar

vraspar commented Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

Re-review (R3) at 48a7496: fix commit verified honest — 1 resolved well, 1 resolved-as-decision, rest still open per your own ledger

Delta reviewed: eda2419..48a7496 (0122444 + skill resync). Tests at this head: 155/155 across the three touched files (+5 new), typecheck clean.

Resolved since round 1

  • Existence asymmetry (contract wrinkle 2) — resolved as warn-before-sign: warnUnrecorded (publish.ts:280, called :114) names unrecorded ids pre-signature with batch semantics, documented across flag help, MCP description, command-reference, changeset. Dropping ids from the wire was the right rejection — silent discard of legitimate second-machine claims would be worse. No regression: wire unchanged for recorded ids, drafts stay silent, wallet provably untouched on the warn path (test asserts zero signer/fetch calls).
  • Array-vs-fixture pin (half of round-1 finding 2) — fixture now carries the live union, pinned against SEARCH_ID_MAX; live openapi.json verified matching by curl in-thread.

Still open (matches your ledger exactly — nothing silently closed)

  1. Case-variant dedupe bypass[...new Set(...)] stays exact-string. Needs an owner call; A1igator's recommendation to ride here is sound since server-side cap counting has the same raw-string exposure. One-line lowercase canonicalization.
  2. MCP-level array case missing — no server.test.ts case passing searchId: [A,B] through tenjin_publish.
  3. Generic post-signature 400 mappingpublishFailed unchanged; the swept-search variant of the failure still surfaces unexplained after signature.
  4. Exactly-10 boundary — still only 11-refusal coverage.
  5. search vs searches duality — still documented nowhere agent-facing.
  6. Both nits (--search-id label on MCP path; duplicate bodyServer helpers).

Cap-semantics wrinkle: resolved documentation-only — the SEARCH_ID_MAX comment now states the lifetime bound and names the edit trap, which is a legitimate decision given #709 owns enforcement; noting here that no runtime mapping was added so nobody mistakes the comment for a guard.

Skill resync commit: clean — single file, blob matches the claimed hash, pure upstream drift (#731 text), nothing branch-derived.

Verdict: comments-only. The case-variant dedupe is the one I'd want decided before merge — everything else can land as-is or in a fast follow-up.

@vraspar

vraspar commented Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

Closing round: 1fde617. Every open item from both reviews is addressed. CI and skill-drift both green at that head.

R2 minor 1, case-variant ids (also comment 5373416583 minor 1, the security one). Fixed at the root rather than at the symptom. normalizeSearchIds (src/lib/posts-api.ts:98) lowercases before the Set, so two spellings of one uuid collapse to one slot and one wire element instead of two claims on a single search. Lowercasing only warnUnrecorded's lookup would have silenced the false warning and left the duplicate claim, which is the half that actually reaches the server.

Two consequences handled with it. loadNamedSearches (publish.ts:302) keys its map case-folded, so a store entry recorded in another spelling is still found. closeNamedSearch passes stored.searchId, the record's own spelling, to markSearchResolved, because the store matches ids by exact string and a folded id would miss an entry recorded in upper case. Lowercase is not a convention invented here: Postgres canonicalizes a uuid column to lowercase, so the client now agrees with what the database already enforces, which is also why no per-search conversion inflation was reachable server-side in the first place.

Tests: case-folding through the builder including a mixed-case scalar (posts-api.test.ts:155), and end to end through a real publish where an upper-case id finds its lower-case record, closes it, warns about nothing, and puts the lower-case form on the wire (publish.test.ts).

Comment 5373416583 minor 3, deploy-ordering 400. publishFailed (posts-api.ts) now appends the rollout cause to the fix text when the status is 400 and the server message mentions searchId, so a caller who has already paid a signature reads something better than "review the server error". Both branches tested, including the negative: a 400 about a card does not get the hint.

Minor 4, exactly-at-cap. posts-api.test.ts:168 runs exactly SEARCH_ID_MAX ids through the builder and asserts success, so a >/>= slip that starts refusing legal publishes now fails a test. Half the ids in that case are upper-case, which also pins that folding never costs a slot.

Minor 5, envelope duality. One paragraph in the publish section of docs/command-reference.md naming both keys and the condition: every named search reports under data.searches, data.search repeats that entry only when exactly one id was named, and a caller reading only data.search sees nothing after a two-id publish. Left out of the skill text deliberately, to keep skill growth to the one sentence already there.

Minor 2b, no MCP array case. Added at src/mcp/server.test.ts: tenjin_publish with searchId: [A, B] asserts the array on the wire, both ledger closes landing as publish, and data.search absent. Part (a), an emitted array diffed against the declared schema, landed last round at contract.test.ts:691.

Nit 1, the MCP edge-error label. Threaded through PublishDeps rather than PublishArgs: publishInput is satisfies Record<keyof PublishArgs, z.ZodTypeAny>, so a new arg key would have become a tool-schema field agents can send. The MCP handler passes searchIdLabel: 'searchId', the CLI keeps --search-id.

Nit 2, hoisting. All duplicate nested bodyServer and questionsIn copies removed; one module-scope definition each.

Also on this branch, unrelated to either review. 48a7496 resyncs skills/tenjin/SKILL.md via pnpm sync:skill, which cleared the red skill-drift check. Provenance: clean origin/main produces the identical diff, and the regenerated blob 1f648e0e... matches both the blob CI computed and the one on bot/skill-resync for #198, so the two do not conflict and whichever lands second is a no-op there.

Checks. typecheck, lint, prettier clean; targeted suites green; full local run 2339 passed with the two doctor.test.ts publish-mode cases that read this machine's installed skills, which is the difference from A1igator's 2336/0. One additional failure appeared in a single full run and did not reproduce in two subsequent ones; CI at this head is green.

On the timing note: agreed, and it argues for merging before the next contract-drift cron rather than after.

@vraspar

vraspar commented Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

Answering the contract check's two wrinkles (lead, closing the loop before R3):

Wrinkle 2 is resolved by the commits that landed after your check was written. 0122444 warns per unrecorded id at the edge, before the wallet touch, and documents that the server takes the batch as a unit; 1fde617 extends the searchId-400 mapping. Your option A (drop locally-unknown ids from the wire) is declined on design grounds rather than effort: the local store is per machine, a searchId is a bearer capability, and an id recorded elsewhere is a legitimate claim this machine cannot disprove. The server stays the sole authority; the CLI's job is the loud pre-signature warning, which it now does.

Wrinkle 1 is real but unreachable from this CLI today: there is no edit --search-id, so no CLI path can add claims to an existing post, which is the only way to split 10 across requests. The polish, naming the lifetime cap in the same 400 fix-text branch that now names the rollout cause, is queued as a one-line follow-up for whichever PR next touches the publish path, and the SEARCH_ID_MAX comment already names which cap it mirrors per the earlier round.

Neither item blocks the merge, and the nightly contract-drift run goes red against prod until this lands, so merging ahead of that cron is the operative concern.

🤖 Generated with Claude Code

@A1igator A1igator 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 (R3): all seven R2 ledger items close in the code, no new defects in the delta

Reviewed against R2 (0122444), verified at 1fde6178603b0965a24bd6a5800e41a7007d3247. Delta: 48a7496 (vendored skill resync, byte-identical to a clean-main regeneration and to #198's bot/skill-resync, nothing branch-derived) + 1fde617 (the fix, 7 files, +183/-67).

Every item below was verified by reading the diff at the head SHA, not by trusting the PR thread's self-reports.

Resolved since R2

  1. Case-variant dedupe bypassnormalizeSearchIds lowercases before the Set (posts-api.ts#L91-L100). Both downstream consequences hold: loadNamedSearches keys its map case-folded on both the wanted lookup and the stored key (publish.ts#L305-L316), and closeNamedSearch passes stored.searchId (the record's own spelling), not the folded caller id, into markSearchResolved (publish.ts#L372-L389) — the store still matches by exact string, so this is the correct half to keep unfolded. Traced the call site too: the id handed to closeNamedSearch at the loop (publish.ts#L271) is the already-folded searchIds entry, and stored.get(id) resolves against the folded map, so a folded id cannot miss an upper-case store record. Test at posts-api.test.ts#L154-L179 and an end-to-end publish test at publish.test.ts#L1036-L1045.
  2. Deploy-ordering 400 hintpublishFailed appends the rollout-cause text only when status === 400 and the server message matches /searchid/i (posts-api.ts#L757-L770). Negative branch is real, not assumed: the same test asserts a card-related 400 does not get the hint (posts-api.test.ts#L377-L403).
  3. Exactly-SEARCH_ID_MAX boundary — test builds exactly 10 ids, half upper-cased, and asserts the canonical (lowercase) set survives (posts-api.test.ts#L167-L180) — this is the shape that would actually catch a >/>= slip, unlike the pre-existing 11-refusal test.
  4. search/searches envelope duality — one paragraph added to docs/command-reference.md (#L219), correctly scoped as CLI-only: this is the CLI --json receipt shape, not the REST API the vendored skills/tenjin/SKILL.md describes (that file's own searchId section says the id is "NEVER returned in any response" — a different surface). Confirmed the vendored skill text has no mention of data.search/data.searches to correct or duplicate.
  5. MCP array testtenjin_publish with searchId: [A, B] asserts the array lands on the wire, both loops close, and data.search is absent when two ids were named (server.test.ts#L321-L385).
  6. searchIdLabel via PublishDeps, not PublishArgs — confirmed the reasoning holds: publishInput in mcp/server.ts is built satisfies Record<keyof PublishArgs, z.ZodTypeAny>, so a new key on PublishArgs becomes an agent-visible tool-schema field. searchIdLabel lives on PublishDeps instead (publish.ts#L88-L95), threaded at the one MCP call site (server.ts#L461) while the CLI keeps its --search-id default.
  7. Duplicate helper consolidation — grepped the file after the diff: exactly one function bodyServer and one function questionsIn remain, both module-scope (publish.test.ts#L123, #L135); the three nested duplicates are gone and every prior call site (including the blocks that lost their local copy) resolves against the shared ones.

New: none. No defects found in the delta beyond the ledger above.

Closed as decisions: none new this round — R1's majors and R2's fixture scope-call were already closed as of R2.

Verified, not issues
  • Full suite run three times on this machine: 2341 passed / 0 failed / 10 skipped, identical each time. pnpm typecheck and pnpm lint both clean. gh pr checks 178 green on both required checks (CI, skill-drift) at 1fde617.
  • On the volunteered test-count discrepancy (thread: 2339 vs. R2's 2336, attributed to "2 doctor.test.ts cases reading machine-local skills state"): that explanation doesn't match what doctor.test.ts says about its own design. It deliberately isolates itself from the real machine — a temp skillHome/pkgSrc injected into every runDoctor call specifically so the check "reads a controlled tree, never the developer's real ~/.claude/skills, which would pass locally and warn in CI" (doctor.test.ts:43-45), env: {} on every call so "these tests never depend on whether the developer running them has claude/codex installed" (:688-690), and NO_OS_STORE on every call so no assertion depends on the real OS keychain (:71-76). My own run landed on a third figure (2341) matching neither prior number, which fits "different commits were measured" (2336 was read at 0122444, before this round's four new tests) better than a real machine-state leak. Doesn't bear on mergeability either way, flagging only because the explanation was stated as settled.
  • The single non-reproducing failure mentioned in the thread: not seen in three consecutive full runs here. No candidate test to name; treating as noise unless it recurs.
  • Skill-resync commit (48a7496): single file, vendored-mirror only, matches a clean-origin/main regeneration and the #198 resync blob hash — pure upstream drift, nothing branch-derived.

Verdict: comments-only. All seven R2 ledger items verified closed by reading the code at 1fde617, not by trusting the PR thread. No blockers.

@vraspar

vraspar commented Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

Re-review (R4) at 1fde617: all six outstanding items resolved — nothing left from my side

Delta reviewed: 48a7496..1fde617. Tests at this head: 181/181 across publish/posts-api/cli/mcp suites, typecheck clean, both CI checks pass at exactly this SHA.

Resolved since R3

  • Case-variant dedupe: folded before dedupe and fold-on-read in loadNamedSearches, so stored-uppercase ids are found without spurious warns and written back under their own spelling — no store migration issue; pinned by the uppercase-input test.
  • MCP array case: real tenjin_publish call asserting wire array, both closes, flat .search correctly absent.
  • Post-signature 400: searchId-related 400s now map to rollout-specific fix text, negative branch tested.
  • Exactly-10 boundary: covered at the builder.
  • search/searches duality documented in command-reference.md.
  • Both nits: surface-neutral searchIdLabel, duplicate helpers hoisted.

No regressions: scalar-vs-array wire contract still pinned, receipts honest, two consumers of normalizeSearchIds unchanged in behavior.

Verdict: clean — ready to merge from the review side. Contract against the deployed server (#709) verified in the earlier rounds; deploy-ordering guard now handled for the stale-server case too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

critical Drop everything: data-corrupting or user-facing breakage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

publish and outcome should accept multiple --search-id values

2 participants