feat(publish): let one piece claim every search it answered - #178
feat(publish): let one piece claim every search it answered#178vraspar wants to merge 5 commits into
Conversation
`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>
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>
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
A1igator
left a comment
There was a problem hiding this comment.
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:
assertClaimedLookupsExistrejects ids with nolookupsrow, 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.
assertClaimCapacityalso notes two concurrent PUTs can read the same pre-write count and overshoot. - Two pieces, one search: allowed by design.
posts.lookup_idis 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
-
[data-integrity] one stale id fails the whole publish, after the signature, with nothing warning first: the server takes the batch as a unit.
assertClaimedLookupsExistretries twice for the after-response race, then 400s the entire request if any id has nolookupsrow. The CLI already loads the local store inloadNamedSearches, 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 deletedvalidateSearchIdcomment 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. -
[testing] the cap and the array form are asserted in a comment and pinned by nothing:
SEARCH_ID_MAX = 10carries "mirroring the server's cap", and the fixture this branch ships still declaresPostCreate.searchIdas{"type":"string","pattern":...}with no union, no array, nomaxItems.contract.test.ts:558pins 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 declareunion([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 itsmaxItemsto that block.
Minor
-
[hygiene] "mirroring the server's cap" describes a different cap:
normalizeSearchIdsbounds 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 sendssearchIdon the update path today. Naming which cap it mirrors is what keeps a lateredit --search-idout of the trap the server comment already documents. -
[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
mainalready. 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
- [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". - [hygiene] the base is behind
main: merge base is0c92f5b, and #177, #194 and #113 have landed since. GitHub reports MERGEABLE so git will take it, but #177 rewrote the samedocs/command-reference.mdoutcome table and the samemcp/server.tsoutcome 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:
publishPostruns before any local close, so a rejected publish leaves every local loop open rather than marking searches resolved against a piece that never landed. toWireSearchIdkeeps 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
claimedSearchIdsand itsON CONFLICTno-op on re-sends. - A
--draftstill sends no attribution, which is the rule that keeps one demand signal off two posts. normalizeSearchIdsruns 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.
Review: comments-only — no majors; 4 minors, 2 nits, all about pinning the new array shape and one dedupe gapReviewed against the diff, What's solid:
Minor
Nits (2), none blocking
Verified, not issuesCap-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 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), |
|
Answering the open server-side question in your minor 1, since it is checkable from the tenjin side. Verified against The 10x conversion-count inflation does not happen. There is a smaller bug next door, and it fails safe. Both fixes still stand, for the reasons you gave rather than for the inflation. Your one-liner canonicalizing before the 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: |
…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>
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
All six addressed. Merge Major 1, the batch 400 after the signature. Major 2, the unpinned contract. Fixture updated and pinned: One scope call worth flagging: I updated the Minor 1, the cap comment. 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: 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
|
End-to-end contract check against tenjin#709 (merged): design matches, two wrinkles to resolveVerified this PR's wire contract line-by-line against the merged server half ( 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 serverThis PR caps one publish at 10 ids (
2. Existence checking is asymmetric: warn-and-continue locally, all-or-nothing on the wireLocally, 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 —
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 |
A1igator
left a comment
There was a problem hiding this comment.
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
- 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, andgetSigner()— I read the function linearly and there is no other branch that reachespublishPostwithout 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 assertsgetSignerCount()is 0 while the warning is already on stderr.publish.ts#L280-L297 - Major 2 (testing), the array contract pinned by nothing: closed.
contract.test.ts:558-564now walks theanyOfand pins both branches (scalar pattern, arraytype/items.pattern/minItems/maxItemsagainstSEARCH_ID_MAX), and a new test at:691builds a scalar and a 2-id array throughbuildPostCreateBodyand checks each against the branch it lands on. I curledhttps://tenjin.blog/openapi.jsondirectly: the livePostCreate.searchIdandPostUpdate.searchIdnodes 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:691test now does exactly that.
- Bonus, not asked for but real: this also closes a gap from the PR author's own separate review (comment
- Minor 1 (the cap comment): closed.
src/lib/posts-api.ts:75-82now names which capSEARCH_ID_MAXmirrors and why the numbers only coincide today. - 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.
- Nit 1 (order-dependent prefill wording): closed, comment now says "the first id you typed that this machine happens to hold."
- 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, andsrc/mcp/server.ts'soutcomeInput.searchIdandpublishInput.searchIdare the identicalz.union([z.string(), z.array(z.string())])shape.
New
- [hygiene]
warnUnrecordedinherits the unfixed case-sensitivity gap, and now gives it a new symptom: the PR author's own review (comment5373416583, minor 1) flagged thatnormalizeSearchIds's[...new Set(ids)](src/lib/posts-api.ts:94) compares exact strings whileSEARCH_ID_WIRE_REaccepts 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), sowarnUnrecorded'sstored.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 fromtenjin searchoutput) but a one-line fix if the canonicalization ever lands:.toLowerCase()both sides of theSetand 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) againstBackTrackCo/tenjinorigin/main,post_lookups.lookup_idis a Postgresuuidcolumn under a composite PK, so no per-search conversion-count inflation is possible regardless of client-sent case. The real adjacent bug (assertClaimCapacitydouble-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,lintalso clean locally. - CI:
skill-drift (mirror is in sync)fails at this head; confirmed the failing diff is exactly theskills/tenjin/SKILL.mdpaywall/card-field hunk carried by the pending #198 resync PR, unrelated to this diff.
Decisions
- The fixture scope call (hand-patch vs. full regenerate): hand-patching only
PostCreate.searchId/PostUpdate.searchIdis the right call and doesn't weaken the drift guard. Two reasons: (a) the mechanism that actually catches server drift in production is the nightlycontract-driftjob, which runsassertPublishContract/assertUpdateContract/etc. directly against liveopenapi.json(describe.skipIf(liveBase === undefined, ...)atcontract.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'sassertPublishContractstill 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 scheduledcontract-driftrun (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.
- I verified this concretely:
- 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_MAXboundary untested, thesearch/searchesenvelope duality undocumented for agents, the MCP edge-error label, and thepublish.test.tshoisting inconsistency. None of those were addressed by0122444(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.
|
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 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 |
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>
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Follow-up to my reply above. skill-drift. Regenerated Provenance, since a vendored file changing inside a feature PR deserves it. Negative control first: a detached worktree at clean On R2's new minor (case-variant ids). Correct, and I am not fixing it in this round without a yes: Not dropped. Minors 3 to 5 and the two nits in comment One correction accepted: R2's full-suite run is 2336 passed, 0 failed. The two |
|
Reviewer note on the scoping question, ahead of the full R3 pass on Your provenance holds, re-derived independently. On whether the case-variant fix rides here: it is not shared code, it is this PR's code. 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: Acknowledged. Minors 3 to 5 and the two nits in 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>
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Re-review (R3) at
|
|
Closing round: R2 minor 1, case-variant ids (also comment Two consequences handled with it. Tests: case-folding through the builder including a mixed-case scalar ( Comment Minor 4, exactly-at-cap. Minor 5, envelope duality. One paragraph in the publish section of Minor 2b, no MCP array case. Added at Nit 1, the MCP edge-error label. Threaded through Nit 2, hoisting. All duplicate nested Also on this branch, unrelated to either review. Checks. typecheck, lint, prettier clean; targeted suites green; full local run 2339 passed with the two On the timing note: agreed, and it argues for merging before the next |
|
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. Wrinkle 1 is real but unreachable from this CLI today: there is no 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
left a comment
There was a problem hiding this comment.
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
- Case-variant dedupe bypass —
normalizeSearchIdslowercases before theSet(posts-api.ts#L91-L100). Both downstream consequences hold:loadNamedSearcheskeys its map case-folded on both thewantedlookup and the stored key (publish.ts#L305-L316), andcloseNamedSearchpassesstored.searchId(the record's own spelling), not the folded caller id, intomarkSearchResolved(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: theidhanded tocloseNamedSearchat the loop (publish.ts#L271) is the already-foldedsearchIdsentry, andstored.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. - Deploy-ordering 400 hint —
publishFailedappends the rollout-cause text only whenstatus === 400and 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). - Exactly-
SEARCH_ID_MAXboundary — 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. search/searchesenvelope duality — one paragraph added todocs/command-reference.md(#L219), correctly scoped as CLI-only: this is the CLI--jsonreceipt shape, not the REST API the vendoredskills/tenjin/SKILL.mddescribes (that file's ownsearchIdsection says the id is "NEVER returned in any response" — a different surface). Confirmed the vendored skill text has no mention ofdata.search/data.searchesto correct or duplicate.- MCP array test —
tenjin_publishwithsearchId: [A, B]asserts the array lands on the wire, both loops close, anddata.searchis absent when two ids were named (server.test.ts#L321-L385). searchIdLabelviaPublishDeps, notPublishArgs— confirmed the reasoning holds:publishInputinmcp/server.tsis builtsatisfies Record<keyof PublishArgs, z.ZodTypeAny>, so a new key onPublishArgsbecomes an agent-visible tool-schema field.searchIdLabellives onPublishDepsinstead (publish.ts#L88-L95), threaded at the one MCP call site (server.ts#L461) while the CLI keeps its--search-iddefault.- Duplicate helper consolidation — grepped the file after the diff: exactly one
function bodyServerand onefunction questionsInremain, 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 typecheckandpnpm lintboth clean.gh pr checks 178green on both required checks (CI,skill-drift) at1fde617. - 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.tssays about its own design. It deliberately isolates itself from the real machine — a tempskillHome/pkgSrcinjected into everyrunDoctorcall 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), andNO_OS_STOREon 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 at0122444, 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/mainregeneration and the#198resync 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.
Re-review (R4) at
|
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-idnow repeats, up to ten per piece.What changed
--search-idcollects (commander'scollect, the same wiring feat(outcome): batch close, and a WebSearch hook that names itself #177 gaveoutcome). Ids dedupe, each is validated againstSEARCH_ID_WIRE_REexactly as today, and the cap refuses before any wallet touch.closeNamedSearchruns once per id with unchanged relink semantics: a loop anoutcomealready closed is taken over, a loop an earlier publish claimed reportsalreadyAnswered, 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 undersearches, one human line each. A lone id keeps the flatsearchfield callers have read since Publish-back loop DX: no defer status, invisible publish.mode, lost --search-id linkage (agent session trace) #161.questionsAnsweredis 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.--draftanswers nobody however many searches it names, so it sends nosearchIdat all and leaves every loop open.tenjin_publishtakessearchIdas 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
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.tscontinues to pin the single-id body against it; the array is pinned at the builder inposts-api.test.tsinstead.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-idrepeating, 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, andsrc/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, andprettier --checkare clean. Full vitest run: 2148 passed, 2 failed — both pre-existing indoctor.test.tson this machine and reproduced identically atorigin/main(they read the developer's real installed skills state).🤖 Generated with Claude Code