Skip to content

feat(outcome): batch close, and a WebSearch hook that names itself - #177

Open
vraspar wants to merge 4 commits into
mainfrom
vraspar/outcome-batch-and-hook-origin
Open

feat(outcome): batch close, and a WebSearch hook that names itself#177
vraspar wants to merge 4 commits into
mainfrom
vraspar/outcome-batch-and-hook-origin

Conversation

@vraspar

@vraspar vraspar commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Scope

Partial progress on #167, which asks for multiple --search-id values on both publish and outcome. This PR does the outcome half only; the publish multi-id form (extra ids closing as published-back) and server-side array support on the endpoints stay out of scope.

Three loop-noise fixes: a dogfooding session closed seventeen open hook loops one tenjin outcome call at a time, and every one of those hook queries reached the marketplace labelled tenjin-cli, so unvetted web-search text was indistinguishable from a question an agent deliberately looked up.

  • The WebSearch hook names itself. src/lib/hook-scripts.ts now leads the hook's User-Agent with tenjin-websearch-hook/<version> instead of the CLI product; deliberate tenjin search still identifies as tenjin-cli. Hook script version 17 to 18, so tenjin install rewrites the scripts.
  • tenjin outcome closes a batch. --search-id repeats (one status for all ids), and --all-open closes every open websearch-hook loop in searches.json, in any session. Per-id results in the JSON envelope; a partial failure names what closed and what did not (src/commands/outcome.ts).
  • One nag command, not one per id. The Stop hook's weak-arm line now names tenjin outcome --all-open --status regenerated once for the whole batch. The strong (deliberate-search) arm and the publish path stay per id.

The client-name contract

The name is tenjin-websearch-hook, exactly. It travels as the first User-Agent product, which is the only mechanism that reaches lookups.client_name: tenjin's resolveClientProduct() reads the first product token and falls back to X-Tenjin-Client only when the User-Agent is absent, invalid, or a generic node/undici runtime, so a header alongside a real User-Agent would be ignored. lib/search/telemetry.ts writes that value to both lookups.client_name and search_queries.client_name.

The sibling tenjin branch vraspar/questions-hook-origin-filter (tenjin#703, draft) filters that string out of the /trending questions tier, matching case-insensitively. It is inert until this ships.

Residual risk, stated rather than solved: the string lives in two independent copies with nothing mechanical tying them across repos. This side pins the literal deliberately, in client-meta.test.ts and in the wire-identity test that reads the header off a real socket; tenjin holds its own WEBSEARCH_HOOK_CLIENT_NAME. Each repo's tests only cover its own half, so a rename on either side fails silently — nothing errors, the questions tier just quietly leaks again. Both constants carry a comment naming the contract.

tenjin-websearch-hook is deliberately not added to KNOWN_CLIENT_PRODUCTS, so canonicalClientProduct() folds it to other in tenjin's permanent client columns. That list extends from names operators observe in the swept space, which cannot happen until this ships; it is a one-line follow-up there, not part of either PR. The swept raw client_name the questions filter reads is unaffected.

Why --all-open is regenerated-only

The other four statuses claim what a specific search did for the agent. A blanket used over queries nobody read piece by piece is attribution the marketplace would be right to trust and wrong to believe, so any other status is a USAGE refusal before anything is sent. "Nothing here answered it, I wrote it myself" is the one report that is honest about an unexamined ridealong query.

--all-open never touches a deliberate tenjin search: those are left open, counted, and named in the output ("N deliberate search(es) left open"), so a blanket close cannot look more complete than it is. It is also mutually exclusive with --search-id and --last.

The skills mirror commit is environmental, not feature scope

One commit here regenerates skills/tenjin/SKILL.md, the vendored byte-for-byte mirror of tenjin.blog/skills.md. The canonical page was rewritten upstream when the embeddings search change reached it (agent search now matches a piece's own body, title and excerpt rather than the answer card), so the committed mirror went stale on its own. It is pnpm sync:skill output with no manual edits, and it reproduces identically on a clean origin/main checkout, which is how I confirmed this PR did not cause it. It rides here because the drift check runs on any PR touching skills/**, and the workflow's own remedy for a red PR check is to run the sync locally and commit. Read it as housekeeping; the feature diff is everything else.

Testing

  • pnpm typecheck, pnpm lint, pnpm format:check, pnpm build: green.
  • pnpm test: 2162 passed, 10 skipped, 2 failed. The two failures are doctor.test.ts > names the rule on auto / on full-auto, which fail identically on a clean origin/main worktree on this machine (they read the developer's own ~/.claude/skills state); nothing else fails.
  • New coverage: the hook's wire identity is asserted against a real socket (the name pinned as a literal, and the generated composer still matched against the real composeUserAgent), the batch and --all-open paths including every refusal, the partial-failure report, and the flag wiring at the dispatcher.
  • pack-smoke deliberately not run locally: it rewrites the real ~/.claude/skills on this machine. CI covers it.

🤖 Generated with Claude Code

Closing seventeen open hook loops took seventeen `tenjin outcome` calls in
one session, and every hook query reached the marketplace as `tenjin-cli`,
indistinguishable from a question an agent chose to look up.

- The WebSearch hook leads its User-Agent with `tenjin-websearch-hook/<version>`
  instead of the CLI product. The server attributes on the first product, so
  that position is the mechanism that reaches `lookups.client_name`; tenjin's
  /trending questions tier filters on the name. Hook script v18.
- `tenjin outcome --search-id` repeats, and `--all-open` closes every open
  websearch-hook loop in any session. Per-id results in the JSON envelope, and
  the whole batch is refused before any request if one status is incoherent.
- `--all-open` reports `regenerated` only; deliberate searches are left open
  and counted in the output.
- The Stop hook's weak-arm nag names one batch close command, not one per id.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@vraspar
vraspar marked this pull request as ready for review August 17, 2026 20:57
@vraspar vraspar added the priority: high High priority label Aug 17, 2026

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

@A1igator A1igator 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: two blockers on what --all-open sweeps, the rest is close

Reviewed against main at 0c92f5b, verified at 5adcabb. Findings below were run against a checkout of the head, not read off the diff: probes are named where they exist.

What's solid:

  • The cross-repo claim in the body checks out on the tenjin side, verified rather than taken on the word: resolveClientProduct returns the first User-Agent product and falls back to X-Tenjin-Client only when that is absent, invalid, or a generic runtime (lib/client-product.ts:53-63), and tenjin-websearch-hook is absent from KNOWN_CLIENT_PRODUCTS so it folds to other exactly as described.
  • Negative control on the rename: no tenjin route gates behavior on the literal tenjin-cli. A grep across lib/ and app/ finds it only in client-product.ts's vocabulary, a schema comment, and agent docs, so the new product cannot change server behavior, only attribution.
  • The wire-identity test asserting the field off a real socket rather than off the generated string, and pinning the name as a literal on the side the contract can drift from.
  • --all-open refusing the four non-regenerated statuses before anything is sent, with all four pinned.

Major

  1. [data-integrity] --all-open closes hook searches that Tenjin answered, reporting regenerated for them: the sweep filters on source and unresolved only, with no test on decision (outcome.ts:249-255), but the WebSearch hook records CANDIDATES entries under that same source, before the emit (hook-scripts.ts:610 and :652-656). Probed on the head: seed one hook MISS and one hook CANDIDATES entry, run --all-open --status regenerated, and two POSTs go out, one of them for the CANDIDATES id. That entry is the one where the agent was shown Tenjin lists a paid answer titled "..." and may have bought and read it, so regenerated there is the same unexamined claim the PR refuses to let used be, and it overwrites the only positive-attribution signal the loop exists to collect. The Stop hook's own weak batch is MISS-only (hook-scripts.ts:872), so the line saying "N web search(es) this session had no Tenjin answer" now names a command that reaches past what it describes. Fix: add stored.decision === 'MISS' to the resolveAllOpen filter so the two agree, and count the rest the way deliberateLeftOpen is counted, so the sweep still cannot look more complete than it is. The batch tests seed MISS throughout (outcome.test.ts:525-540), so a case seeding a CANDIDATES hook entry is what would have caught this.

  2. [agent-usability] the sweep is machine-global while the nag that recommends it is session-scoped: ownedByThisSession exists so a session is never nagged about a sibling's loops, and it skips a foreign entry unnagged precisely so the owning session still gets its reminder (hook-scripts.ts:811-822); sessionId was added to a machine-global ledger for exactly that (search-store.ts:66-84). resolveAllOpen applies no session filter, so whichever session stops first sends regenerated for a sibling session's still-live loops and silently removes its reminder and its publish prompt. Probed: two entries stamped session-A and session-B, one --all-open call, two POSTs. The weak line reads "N web search(es) this session had no Tenjin answer" and then names the machine-wide command, and skills/tenjin-search/SKILL.md says only "every open loop the WebSearch hook recorded", so nothing in the agent-facing copy says the blast radius is the machine. Fix: scope --all-open to the current session by default, using the resolver search.ts:233 already has (TENJIN_SESSION_ID, then CLAUDE_CODE_SESSION_ID), keeping unstamped entries in scope the way the Stop hook does, and put an explicit flag on the wider sweep. If the wide sweep is the intended default instead, say "on this machine, in any session" in the skill and the Stop line, and treat it as a decision below.

Minor

  1. [hygiene] the pre-flight covers the status but not the ids, so a typo sends half the batch: outcome.ts:81-85 says "Every target before any request", but the id shape is only checked inside postOutcomes (agent-api.ts:455-459), which runs per id inside the send loop. Probed: --search-id <valid> --search-id not-a-uuid sends one POST, then fails the whole call as API_UNREACHABLE (exit 1) with "Retry the failed ids", where the real class is USAGE (exit 2) and no retry will ever help. Fix: validate every id against the same regex in resolveTargets, next to the coherence loop, and derive the thrown code from the failures so an all-USAGE batch still exits 2.

  2. [agent-usability] a partial batch names no ids in the human rendering: the per-id results ride details (outcome.ts:134-143), and the renderer emits only details.findings, deliberately: "Every other details shape stays machine-only" (output.ts:245-253). Without --json the caller gets "Reported regenerated for 2 of 3 searches; 1 failed" plus "Retry the failed ids with --search-id " and no ids to retry, and the humanLines built just above, including the deliberateLeftOpen line, are discarded by the throw. Fix: put the failed ids in the message or in fix.

  3. [security] the sweep has no bound and no early stop, against a 60/min budget: the store holds up to 50 entries (MAX_ENTRIES, search-store.ts:15), the loop is sequential with the default --timeout at 10000ms (cli.ts:22), and nothing stops it early. Tenjin's OUTCOME_RATE_LIMIT is 60/min keyed on client IP, and its comment says the looser window exists so outcome reporting "must never spend the lookup budget and self-429 an agent mid-task"; one sweep can spend 50 of those 60, and two sessions sweeping in the same minute on one IP 429 each other. During an outage this is the command the Stop hook now injects into every session's context, and it will sit for up to ~8 minutes at turn end before reporting. Server-side batching across searchIds is correctly out of scope here (the route is per search id), so the cheap fix is to stop the sweep on the first RATE_LIMITED or transport failure and report the remainder as untouched: an unclosed loop is the safe state, and the Stop hook will raise it again.

Nits (3), none blocking
  1. [hygiene] --resource reaches a search the caller never named: outcome.ts:74-80 refuses a resource only when targets.length > 1, so --all-open --resource <uuid> with exactly one open hook loop attaches the resource to whatever the sweep happened to find, and with zero open loops it is ignored in silence. Refusing --resource under --all-open outright is the simpler rule.

  2. [hygiene] the "status name first" comment is now false for --all-open: outcome.ts:67-68 claims an unknown status still fails as an unknown status, but resolveTargets now runs first and resolveAllOpen reads args.status before the vocabulary is validated (outcome.ts:237), so --all-open --status bogus reports the --all-open restriction. Harmless, since the message quotes the value back, but the comment says otherwise.

  3. [hygiene] patch understates the changeset: this adds a CLI flag, a repeatable flag, a new MCP input field, and changes the wire identity of the highest-volume request path. The comparable User-Agent change (#126) shipped as a Minor, per CHANGELOG.md under 0.1.0-alpha.14.

Verified, not issues
  • The permanent-column effect of the rename is exactly what the body claims and nothing more: canonicalClientProduct folds an unlisted name to other, the swept raw client_name keeps tenjin-websearch-hook, and no request-path branch reads the product. Worth an issue rather than a PR-body paragraph, since it puts a step change in a permanent metric nobody is watching for.
  • composeUserAgent's product seam is not a new injection surface: the caller handoff is still totally rejected unless every token is a bare product, and OWN_PRODUCT_NAMES now drops a handed-back hook identity in either direction, pinned in both suites.
  • idsOf dedupes before the send loop, so a repeated --search-id is one request, and the flattened single-result fields keep the old data.searchId envelope.
  • The single-target failure path still throws the original error with its original code, so nothing that reads outcome's error code today changes.
  • HOOK_SCRIPT_VERSION is bumped to 18, so an installed script is rewritten rather than left sending the old identity.
  • skill-drift is green on this head, so the skills/tenjin-search/SKILL.md edit is mirrored.

Decisions for the owner: whether --all-open is a per-session or a per-machine sweep (Major 2). Everything else follows from that answer: if per-machine is intended, the copy in the Stop line, SKILL.md, and command-reference.md needs to say so, since all three currently read as session-local.

Heads up on a rebase, not a review finding: #158 touches five of this PR's files (README.md, docs/command-reference.md, skills/tenjin-search/SKILL.md, src/cli.ts, src/mcp/server.ts), #150 touches src/lib/client-meta.ts and src/lib/hook-scripts.ts directly, and #113 touches src/lib/hook-scripts.ts and its test. Whichever lands second eats the conflict.

Verdict: comments-only, two Majors. Both are about what --all-open reaches rather than about the mechanism, and both are a filter away. Ran locally against the head checkout: pnpm install --frozen-lockfile, plus three throwaway probes on runOutcome (hook CANDIDATES sweep, cross-session sweep, malformed id in a batch) with a negative control that a resolved entry is not swept. CI is green on this head, and the tenjin-side claims were checked against that repo rather than taken from the body.

@A1igator

Copy link
Copy Markdown
Contributor

Two operator dispositions on the review's open ends:

  • --all-open scope (Major 2): the operator defers to your preference, per-session or per-machine. Whichever you pick, state the intent at the sweep and guard it with a test, so the cross-session close behavior is a decision rather than an accident.
  • KNOWN_CLIENT_PRODUCTS: the follow-up should not live only in this PR's body. Recommendation: either file the tenjin-side issue yourself, or better, just make the small tenjin change adding tenjin-websearch-hook to KNOWN_CLIENT_PRODUCTS so it lands before or with the CLI release; without it the permanent client column silently steps down the day this ships.

…front

Review fixes from PR 177.

- `--all-open` sweeps MISS entries only. The hook records CANDIDATES under the
  same source, and those are the searches where a priced answer was shown and
  may have been bought, so `regenerated` there would overwrite the one positive
  attribution the loop collects. Answered entries are counted and named, like
  deliberate ones, and the sweep now matches the Stop hook's MISS-only batch.
- Every --search-id is validated before the first request, so a typo no longer
  sends the ids ahead of it, and a batch whose failures agree keeps their code:
  an all-USAGE batch exits 2 rather than reporting an outage.
- The partial-failure fix line names the ids to retry; without --json the
  envelope's per-id results are not rendered.
- A batch stops at the first rate limit or dead network and reports the rest
  untouched. A non-2xx is not a halt: it covers a 400 about one id.
- `--resource` is refused under --all-open outright.
- Changeset patch -> minor: new flags, a new MCP input field, and a changed
  wire identity on the highest-volume request path.

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.

`pnpm sync:skill` output, nothing hand-written. The canonical page changed
upstream (agent search now matches a piece's own body/title/excerpt rather than
the answer card, and the card is what makes a piece a candidate at all), so the
committed mirror went stale on its own.

Not caused by this PR: a clean origin/main checkout drifts identically, and
this branch never touched skills/tenjin/SKILL.md. It rides here because the
drift check runs on any PR touching skills/**, and the workflow's own remedy for
a red PR check is `pnpm sync:skill` locally, then commit.

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.

@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 round 2: both Majors addressed, one new item outside the code

Delta only, 5adcabb7..6aeed4b3 read as one round. Verified at 6aeed4b.

Resolved since round 1

  1. [closed] Major 1, --all-open closing answered hook searches: the sweep now filters decision === 'MISS' and counts the rest as answeredLeftOpen (outcome.ts:325-331), with a human line, a JSON field, and a test that pins one POST, the answered id absent from the wire, and its store entry left unresolved. The flag help, the MCP description, the reference table, and the skill all say MISS now, so the Stop hook's line no longer names a command reaching past what it describes.
  2. [closed] Minor 1, ids validated mid-batch: assertReportableId runs in the pre-flight loop (outcome.ts:249-254) against UUID_RE, which is exactly what postOutcomes enforces (SEARCH_ID_RE = UUID_RE, agent-api.ts:448), so nothing can pass here and fail inside the loop. failureCode restores exit 2 for an all-USAGE batch, and the test asserts zero requests.
  3. [closed] Minor 2, a partial batch naming no ids to a human: the ids ride fix, which is the one detail shape the human renderer prints (output.ts:118-119), bounded at five with a pointer to the envelope past that.
  4. [closed] Minor 3, an unbounded sweep: it halts on RATE_LIMITED or NETWORK_ERROR and marks the remainder untouched without resolving them locally (outcome.ts:64-69). Both codes are reachable on this route, which is what makes the set real rather than decorative: httpRequest returns ok: true for any status so postOutcomes' 429 branch is live, and network and timeout both map to NETWORK_ERROR (http.ts:388-391), which is the turn-end stall case. Excluding API_UNREACHABLE is the right call and the 400 test pins it.
  5. [closed] all three nits: --resource refused whenever allOpen is set, with a test covering the one-loop case that used to slip; the ordering comment rewritten to describe the order that now runs; the changeset bumped to minor.

Still open

  1. [agent-usability] the machine-wide scope is stated in three surfaces, absent from the two an agent reads, and guarded by none (round 1, Major 2): keeping the sweep machine-wide is yours to choose and I am not re-arguing it. What is missing is the half that makes it a decision rather than an accident. "In any session" is stated in --help, in the MCP allOpen description, and in command-reference.md. It is absent from skills/tenjin-search/SKILL.md:119 ("closes every unanswered loop the WebSearch hook recorded"), and the Stop hook line moved the wrong way: close every open hook loop in one call became close **them** in one call (hook-scripts.ts:806), where "them" is the at-most-three MISSes the same line introduces as "this session". That line is the only place an agent reads this at turn end. The docstring at the sweep also lost the qualifier it had at 5adcabb: outcome.ts:297 now reads "Every open loop the WebSearch hook recorded and Tenjin could not answer", where it used to end "in any session". And no test seeds a sessionId anywhere in outcome.test.ts, so a future filter narrowing this to the current session breaks nothing. Fix: put "in any session, on this machine" back at outcome.ts:297 and in the skill, and add one test seeding two entries under different sessionIds that asserts both are swept. ownedByThisSession (hook-scripts.ts:811-822) is the invariant on the other side, so the test is what keeps the two from silently converging.

New

  1. [hygiene] the cross-repo contract the rename is justified by was closed unmerged, and the code still asserts it: tenjin#703 was closed on 2026-08-17 by owner decision, "hiding the entire websearch-hook origin is broader than wanted", with the replacement moving to ingest classification against tenjin#704 and tenjin-agent#179. The PR body still describes it as an open draft that "filters that string out of the /trending questions tier", and client-meta.ts:14-20 still carries it as a load-bearing invariant: "CROSS-REPO CONTRACT, tenjin's /trending questions tier drops this exact name, so renaming it here silently puts unvetted web-search queries back in front of readers". No such filter exists or is planned in that shape, so that comment now promises a safety property the marketplace does not have, and the literal pinned in client-meta.test.ts and the wire-identity test is pinned against a counterpart nobody will write. The rename itself still earns its place, since separating ridealong from deliberate traffic in client_name is what any classifier needs, ingest-time ones included, so this is a claims fix and not a rollback: restate the comment as attribution separation consumed by whatever classifies hook demand, cite tenjin#704 instead of #703, and correct the body. Round 1 verified those claims against tenjin and they were true then; they stopped being true the same day.

Not re-raised

The HALTING_FAILURES comment attributes API_UNREACHABLE to apiFailure, where a 400 or 500 actually gets it from the status !== 202 branch below it, since apiFailure only fires on transport. The conclusion the comment draws is right and the behavior is right, so this is a one-word correction, not a finding.

Decisions for the owner: whether to ship the rename with tenjin#703 dead. It is defensible on its own and the ingest work needs the same separation, but the operator's KNOWN_CLIENT_PRODUCTS item matters more now rather than less: with no filter consuming the name, the permanent client column still steps down to other the day this ships, and nothing in this delta or a linked issue covers that yet.

Verdict: comments-only. Both Majors from round 1 are addressed and the three Minors and three nits are closed, verified against the head rather than the diff: SEARCH_ID_RE is literally UUID_RE, fix is the shape the human renderer prints, and both halting codes are reachable on this route. One round-1 item stays open in narrowed form (state the chosen scope where an agent reads it, and guard it with a test), and one new item is about claims rather than code.

Owner decision on the review's Major 2. The hit/miss loop is per session by
design: a session's open loops are its own, one that ends leaves its unpublished
debt to decay, and no session closes another's. There is no machine-wide sweep,
behind a flag or otherwise.

The session resolver moves to lib/session.ts and is shared rather than copied,
so the Stop hook's reminder, the stamp `search` writes, and the sweep agree on
what "this session" means: TENJIN_SESSION_ID, then CLAUDE_CODE_SESSION_ID, with
an unstamped entry in scope everywhere rather than nowhere, exactly as
ownedByThisSession treats it.

Copy follows the behavior: the skill, the command reference, the flag help, the
MCP tool description and the Stop hook's batch line all say this session's.

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 18, 2026

Copy link
Copy Markdown
Contributor Author

Round closed. Everything below is on the branch; head is 2b28a56, CI and skill-drift green.

Major 1 — --all-open closed answered hook searches. Fixed in bb7b6ae. resolveAllOpen now filters decision === 'MISS', so a CANDIDATES entry the hook recorded is never swept; it is counted and named as answeredLeftOpen, the way deliberate searches already were. That also puts the sweep and the Stop hook's weak batch on the same set. Test seeds one MISS and one CANDIDATES hook entry and asserts one POST, the untouched entry still unresolved.

Major 2 — machine-global sweep vs session-scoped nag. Owner decision, implemented in 2b28a56: scoped to the current session, with no machine-wide flag. The rationale is that the whole hit/miss loop is per session — a session's open loops are its own, a session that ends leaves its unpublished debt to decay, and no session closes another's. The resolver moved to lib/session.ts and is now shared by search, the sweep, and the Stop hook's rule rather than copied: TENJIN_SESSION_ID, then CLAUDE_CODE_SESSION_ID, unstamped entries in scope everywhere rather than nowhere. Skill, command reference, flag help, MCP description and the Stop line all say this session's.

Minor 1 — id shape checked mid-send. Fixed in bb7b6ae; every id is validated before the first request. Deviation: UUID_RE, not SEARCH_ID_WIRE_RE. The outcomes path parameter is declared format: uuid with no pattern in openapi.fixture.json, while the wire regex is the POST /api/posts body contract, so validating here against it would refuse ids this endpoint accepts (0197aaaa-bbbb-cccc-dddd-eeeeeeeeeeee passes one and fails the other). Failure codes are now derived: an all-USAGE batch exits 2.

Minor 2 — partial batch named no ids. Fixed; the failed ids ride the fix line, bounded to five with a count for the rest, since the human renderer prints fix and drops other details shapes.

Minor 3 — unbounded sweep against a 60/min budget. Fixed; the batch stops at the first failure and reports the remainder untouched. Deviation: the halt set is RATE_LIMITED and NETWORK_ERROR only. API_UNREACHABLE is excluded because apiFailure assigns it to every non-2xx, so it also covers a 400 about one id; including it made a batch abandon after one rejected id, which a test caught. Both behaviors are pinned.

Nits. All three: --resource refused under --all-open outright; the stale "status name first" comment corrected; changeset patchminor.

One commit is not feature scope. 6aeed4b regenerates skills/tenjin/SKILL.md, the vendored mirror of tenjin.blog/skills.md. The canonical page was rewritten upstream when the embeddings search change reached it, so the mirror went stale on its own — a clean origin/main checkout drifts identically. It is pnpm sync:skill output with no hand edits, committed here because the drift check runs on any PR touching skills/**.

@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 round 3: the scope reopen is closed, both deviations hold up

Delta only, 6aeed4b3..2b28a56. Verified at 2b28a56. Final round from me on the code.

Resolved since round 2

  1. [closed] the sweep's scope is now decided, stated, and guarded: resolveAllOpen filters on ownedByThisSession(s.sessionId, readSessionId(deps.env)) (outcome.ts:336-341), and the resolver plus the ownership rule live in one module (session.ts:14, :30) that search now imports rather than copies. The intent is stated at the sweep itself ("THIS SESSION ONLY, and there is no flag for the wider sweep", outcome.ts:306) and in all five agent-facing surfaces, including the Stop line, which now says "close this session's in one call" against a batch it introduces as this session's. Three tests cover the shape that matters: a sibling's stamped entry is never swept while an unstamped one is, the CLAUDE_CODE_SESSION_ID fallback resolves, and a harness that names no session sweeps everything. The unstamped-entry direction matches the Stop hook's, so no loop can become unreachable in every session at once, and precedence between the two env vars stays covered at the resolver's original call site in search.test.ts.

  2. [closed] deviation 1, UUID_RE over SEARCH_ID_WIRE_RE for the preflight: correct, and for a second reason worth recording. The fixture agrees with the argument, /api/searches/{id}/outcomes declares its path parameter {"type":"string","format":"uuid"} with no pattern, while SEARCH_ID_WIRE_RE pins RFC version and variant nibbles for the POST /api/posts body (posts-api.ts:65-73); ran both against the placeholder id and it passes UUID_RE and fails the wire regex, exactly as stated. The second reason: postOutcomes already enforced SEARCH_ID_RE, which is literally UUID_RE (agent-api.ts:448), so hoisting that same regex moves a check earlier without changing which ids the command accepts. Reaching for the stricter one here would have been a behavior change smuggled inside a fix.

  3. [closed] deviation 2, the halt set: sound and pinned from both sides. A 400 about one id must not abandon the batch, and a rate limit or a dead network must, which is what the two tests assert. Only the attribution in the comment is off, and it does not change the conclusion: httpRequest returns ok: true for any status, so apiFailure fires on transport only, and a 400's API_UNREACHABLE comes from the status !== 202 branch below it rather than from apiFailure. Excluding the code is right either way.

New

  1. [agent-usability] the MCP description promises session scoping the MCP process may not be able to resolve: deps.env defaults to process.env and the tool description now reads "Close this session's open WebSearch-hook MISSes" (server.ts:135). The docstring is careful that CLAUDE_CODE_SESSION_ID is "what Claude Code exports to Bash subprocesses", and an MCP server is a different process class; I did not verify whether the harness exports it there, so this is a question rather than a defect. If it does not, allOpen over MCP resolves no session and falls back to the global sweep, which is the safe direction by design but is not what the description says. Cheap fix either way: have the description name the fallback, "this session's, or every open hook MISS when the harness names no session", so the tool text is true on both paths. Worth a one-line check against a live MCP session before release.
Nits (2), none blocking
  1. [hygiene] lib/session.ts has no colocated test: the repo convention is foo.test.ts beside foo.ts, and the behavior is covered at both call sites (search.test.ts for precedence, outcome.test.ts for the three scope cases), so this is coverage-by-caller for a module that now has two callers plus a mirrored copy in the hook.
  2. [hygiene] "the sweep covers exactly the set the nag names" (outcome.ts:311-313): within the session the sweep is a superset, since the nag additionally bounds by the open-loop recency window, by three entries per batch, and by once per session. The superset is the right direction and nothing misbehaves; "the same set the nag is drawn from" would be the accurate phrasing.
Verified, not issues
  • The two ownedByThisSession implementations differ on an empty-string stamp, and it is unreachable: the hook's sessionIdOf rejects a zero-length session_id and readSessionId trims to undefined, so no writer can produce one.
  • Counting is computed from the session-scoped set, so deliberateLeftOpen and answeredLeftOpen describe this session rather than the machine, and the empty-sweep line says "in this session".
  • The sweep still has no age filter, which is deliberate here: your own stale open loop is yours to close even after the nag stopped raising it.
Closed as decisions

Two round-2 items are unchanged in this delta and are with the operator rather than with you: the tenjin#703 claims still standing in client-meta.ts and the PR body, and the KNOWN_CLIENT_PRODUCTS follow-up. Not re-argued here.

Verdict: comments-only, and every code finding from rounds 1 through 3 is closed. The scope re-cut is the version I would have asked for: one resolver, one ownership rule, shared with the hook, stated where it is enforced, and tested in the three directions that could drift. What remains is one description-accuracy question on the MCP path and two items that were never yours to close.

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

Labels

priority: high High priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants