feat(hooks): SessionStart instinct primer and research-dispatch hook - #180
feat(hooks): SessionStart instinct primer and research-dispatch hook#180vraspar wants to merge 4 commits into
Conversation
Register two more Claude Code hooks so the retrieval reflex fires before the research does. A SessionStart hook (matcher startup|clear|compact) prints one paragraph on when to search first, with no network call and nothing else riding along; hooks.sessionPrimer off silences it at run time. A PreToolUse hook on Agent|Task|WebFetch asks the marketplace what a subagent was dispatched to find out, sending the description plus at most 400 characters of the prompt, and mentions at most two tested answers in the WebSearch hook's format. WebFetch is logged and never injected into. Both demand arms record into searches.json under new sources, dispatch-hook and webfetch-hook, which the Stop hook skips unnagged: its strong arm stays cli-only and its weak arm websearch-hook-only. The WebSearch hook's store, request and response boundary are now shared source rather than duplicated a third time. 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 primer is clean, the dispatch hook needs its own consent before it ships
Reviewed against main at 0c92f5b, verified at df94055. Read at the head checkout rather than off the diff. The three Majors are all one question asked three ways: this PR moves the hooks from "ask the marketplace what you were already asking the web" to "tell the marketplace what you are about to do internally", and the consent, the defaults, and the local ledger were all sized for the first one.
What's solid:
- The primer is inert by construction:
PRIMER_TEXTis a compile-time constant serialized into the script throughJSON.stringify, and the SessionStart script reads nothing but stdin, which it drains and discards, and the one config key. No stored entry and no marketplace response can reach it, which is the property that matters most for a hook that speaks first in every session. - Writing it to fd 1 directly rather than through
emit()so the update signal cannot join it, with a test that pins exactly that. nextHooks[spec.event]replacinghooks[spec.event]in the wiring loop: with two PreToolUse specs the old read would have had the second spec overwrite the first, and thenote()dedupe keeps the reported events honest.latestSearchnarrowed toundefined || 'cli'rather than excluding one source by name, so neither new source can re-targetoutcome --last.- Uninstall now claims and removes all four scripts from one list, in both directions.
Major
-
[security] the dispatch hook sends internal prompt text off-machine under a key the user granted for something else:
dispatchQuestionbuildsdescription + ': ' + prompt.slice(0, 400)and posts it to tenjin.blog (hook-scripts.ts:775-784), gated only byhooks.searchMode, which defaults toauto. The WebSearch hook's whole justification is that the query was leaving the machine anyway; aTaskorAgentprompt was not. Subagent prompts routinely carry repo and branch names, file paths, SHAs, ticket text, and pasted code, and 400 characters is most of a brief rather than a fragment of one. The consent moment understates the surface twice over: theautochoice hint says "before a WebSearch or a subagent dispatch" and never mentions WebFetch at all (install.ts:1326), and a non-interactive re-install takesstored ?? DEFAULT_HOOK_MODEwithout prompting, so an existingautouser is upgraded into the wider egress with no question asked, which is the path an agent-driven upgrade takes. Fix: give the dispatch arm its own key,hooks.dispatchMode, defaulting toofforremind, sohooks.searchModekeeps meaning what it meant when it was answered. If it must ride one key, treat an existinghooks.searchModein the config file as consent for the old surface only and re-ask once on upgrade. Either way the choice hint should name every tool the key turns on. -
[security] the WebFetch arm is a network call with no return to the person running it:
recordSearchruns before theisFetchmute (hook-scripts.ts:816-828), so every WebFetch posts its prompt and host to the marketplace and writes a store entry, and the comment is explicit that nothing is ever injected back because a hint there measured as noise. The user pays the egress, the latency, and a store slot; the marketplace gets demand data. That may well be a trade worth offering, but it is not onehooks.searchMode: autowas ever asked about, and WebFetch is the highest-frequency of the three triggers. Fix: make the fetch arm opt-in on its own, or hold it until it returns something to the person whose machine it runs on. If it ships as is, it belongs in the consent choice rather than only in the post-install summary line. -
[data-integrity] three writers, one drain, fifty slots:
STORE_MAX_ENTRIESis 50 (hook-scripts.ts:91) and the Stop hook's new three-way deliberately never raisesdispatch-hookorwebfetch-hook(hook-scripts.ts:1053-1062), so those entries are never nagged, never closed, and never leave except by eviction. They still take slots from the two things the store exists for:buy <resourceId>resolves a candidate's payable read URL out of it, andlatestSearchandoutcome --lastneed theclientries. One research turn with a ten-way fan-out and twenty fetches evicts every deliberate search the user might still have wanted to buy from or report on. Fix: budget the demand-only sources separately inside the 50, or keep them in their own ledger, since they are telemetry rather than searches anyone can act on.
Minor
-
[security] marketplace-authored text now lands at the moment a subagent is briefed: the mitigations carry over unchanged and they are the right ones (control bytes stripped,
"folded to', framed as a listing, and the disclaimer line), but the injection point moved. OnAgent/Taskthe hint sits directly beside a prompt about to be handed to another agent, and the marketplace is permissionless, so the title corpus is attacker-authored by design. Worth stating in the code whether the disclaimer travels into the subagent's own context or stays in the parent's, since a crafted title that reaches the child without its framing is the case this defense is for. -
[performance] up to two seconds in front of every dispatch and every fetch:
SEARCH_TIMEOUT_MSis 2000 with a 5s harness kill, andalreadyAskedskips only exact fingerprint repeats within a session, so a fan-out of ten distinct prompts can add up to twenty seconds spread across the dispatches, plus that many calls against the agent-search rate budget. The WebSearch hook paid this once per web search a human-scale flow produced; a fan-out produces them in bursts. Worth either a shorter budget on the dispatch arm or a per-session call ceiling. -
[hygiene]
SESSION_START_MATCHERomitsresume: it is'startup|clear|compact'(harness-hooks.ts:58) and the comment calls that "a new session, and the two ways a running one loses its context". A resumed session is a fresh process with an empty context too, so either it belongs in the set or the comment should say why it does not.
Nits (2), none blocking
- [hygiene] one event can now land in two result lists: with the WebSearch entry already present and the dispatch entry new,
PreToolUseis pushed onto bothaddedandalreadyPresent. The reported line is right becausewrote > 0wins, but the result object says two contradictory things about one event. - [hygiene] the primer states a price: "a hit costs cents" ships in every session's context and pins a number the marketplace controls. "A hit is priced per piece" costs nothing and cannot go stale.
Verified, not issues
- Nothing stored or remote can steer the primer: static constant, no store read, no network, and the
sessionPrimerkey is the only input.writeFileSyncis imported in the shared prelude, so the fd-1 path really writes rather than throwing intomain().catch(quiet)and silently printing nothing. - No hook emits
permissionDecision, so none of the three PreToolUse arms can block or alter a tool call, and a test pins it for the new one. - The question built from tool input passes through
clean()and is sent as a JSON body only, with no shell and no interpolation into the script. HOOK_SCRIPT_VERSIONmoving 17 to 18 collides textually with #177 but not functionally:writeScriptscompares the full script text (onDisk === spec.script), so a stale script is rewritten whatever the version line says.- The
Agent|Taskalternation as one matcher with one entry per script is consistent with the ownership-by-filename rule the module documents.
Heads up on merge order, not a review finding: #113 overlaps on seven of these files including harness-hooks.ts, hook-scripts.ts, install.ts, uninstall.ts and config.ts, and it also adds hook specs and script files to the same two lists, so a careless merge there can drop one side's script from specs() or from the uninstall set and leave orphans behind. Your own #177 and #179 both touch hook-scripts.ts, and #177 rewrites the same Stop hook source branch this PR turns three-way.
Verdict: comments-only, three Majors, all in the consent and accounting around the new arms rather than in the mechanism. The hook code itself is the same careful shape as the existing two, the primer is the cleanest part of the PR, and the test coverage on behavior is genuinely thorough. What is missing is a separate gate for a materially different kind of egress, and a home for demand entries that is not the ledger a purchase depends on. Ran locally: head checkout, read of the generated script bodies and the wiring, and a pass over the prelude to confirm the primer's write path resolves.
Review round on #180. The fetch arm is gone: nothing fires on a WebFetch, the matcher is Agent|Task, and the webfetch-hook source (which shipped only in this PR) is out of the enum. The install disclosure and the search-hooks choice hint now name the subagent prompt and its 400-character bound rather than only "the query text". Two bounds on a fan-out. A session gets at most 10 dispatch lookups, counted from the store before the request, so a ten-way research turn cannot put the fetch budget in front of every subagent. Demand entries hold at most 15 of the store's 50 slots and the oldest go first, so a flood cannot evict a cli entry that buy or outcome --last still depends on. Also: one event now reports its strongest outcome only, instead of landing in both added and alreadyPresent; the injection site states that the hint reaches the parent's context and never the subagent's; and SESSION_START_MATCHER says why resume is omitted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Pushed 8f2b887 addressing the review. Disposition per finding, with the author's decisions marked as such: Major 1 (dispatch egress consent): resolved by author decision. The dispatch arm stays under Major 2 (WebFetch arm): dropped entirely, by author decision. Matcher is now Major 3 (store accounting): fixed. Demand entries are budgeted to 15 of the 50 slots ( Minor 1 (injection point): comment added at the injection site: the hint lands only in the parent's context, Minor 2 (burst cost): fixed with a per-session ceiling of 10 dispatch lookups, counted before the API call. Counted from the bounded store, so it rate-limits bursts rather than capping a session for life; stated in the code. Minor 3 (resume): declined. A resumed session restores its transcript, so the primer from the original SessionStart is already in context; the matcher comment now says exactly that. Nit 1: fixed; result lists are exclusive per event (added > updated > alreadyPresent). Nit 2 ("costs cents"): declined by author decision; pricing is staying in that range and the cheapness claim is doing work in the primer. Thanks for the merge-order heads-up on #113/#177/#179; noted for whoever lands second. |
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 round 2: the two code Majors are closed, the consent one is not yours to close
Delta only, df94055..8f2b887. Verified at 8f2b887, CI green at post time.
Resolved since round 1
-
[closed] Major 2, the WebFetch arm: gone rather than gated, which is the cleaner answer.
DISPATCH_MATCHERis now'Agent|Task'(harness-hooks.ts:54),fetchQuestionand theisFetchbranching are deleted,webfetch-hookis out ofSearchSourceSchema, and the in-script defense in depth is back to two tool names. Checked for leftovers acrosssrc/anddocs/: every remaining mention of WebFetch is a negative statement, and a settings test asserts the serialized wiring contains the string nowhere at all. -
[closed] Major 3, the store drain:
budgeted()sits on the shared save path inmarketplaceSource(hook-scripts.ts:537-547), so both generated scripts enforce it, and iterating newest-first means the entries it drops are the oldest demand ones. Three tests pin the shape that matters: a flood cannot evict aclientry, the newest demand entries survive, and awebsearch-hookentry is deliberately not budgeted because that source is nagged and closable. Fifteen of fifty is a defensible line for something nothing ever closes. -
[closed] Minor 1, the injection site: the comment answers the question I actually asked, and the answer is right:
tool_inputis already formed when PreToolUse fires and the hook emits nopermissionDecisionand no modified input, so the hint reaches the parent only and the titles can never arrive somewhere their disclaimer did not (hook-scripts.ts:854-855). -
[closed] Minor 3,
resumedeclined: accepted on merits. A resumed session restores its transcript, so the primer the original SessionStart printed is still in context, andcompactis in the set precisely because compaction is the case where it is not. The rationale now sits at the matcher where the next reader will find it. -
[closed] round-1 nit, one event in two lists: replaced with a rank map that reports each event once by its strongest outcome, in
HOOK_EVENTSorder.
Still open
- [security] Major 1, the dispatch-egress consent: the disclosure wording now matches what you described. The
autochoice hint names the dispatch arm and the 400-character bound (install.ts:1326), and the install summary says the same in its own words (install.ts:683). The remaining question is whether the arm ships default-on underhooks.searchModewith no re-ask on upgrade, and that is the operator's product-consent call rather than an author decision or a review finding. It stays open awaiting the operator, and I am not re-arguing the merits here.
New
-
[performance] the burst ceiling does not count the calls that cost the most:
spentThisSessioncounts recordeddispatch-hookentries (hook-scripts.ts:817-822), and a record is only written afteraskTenjinreturns non-null, so a lookup that times out costs its full budget and does not count toward the ten. During an outage, which is exactly when the bound should bite, a sixty-way fan-out pays the fetch budget sixty times. The second gap isif (sessionId === null) return 0: on a harness that names no session the ceiling never binds at all, andalreadyAskedonly dedupes identical questions, so distinct prompts stay unbounded there. Fix: count attempts rather than records, or stop the arm for the session after a couple of consecutive failures, the way an unhealthy server already stops a batch elsewhere in this codebase. -
[hygiene] the budget belongs to the store, but only one of its two writers knows about it:
lib/search-store.ts'srecordSearchstill trims with a plain.slice(0, MAX_ENTRIES)(search-store.ts:148-151), so atenjin searchwrite drops the oldest entry whatever its source while fifteen demand entries sit untouched. The bounded-share invariant survives, since any hook write re-applies it, but the property the budget was added for, that a demand entry never costs a deliberate one its slot, holds only on the hook path. Mirroringbudgeted()in the TS writer makes it a property of the store rather than of one writer.
Nits (2), none blocking
- [hygiene] an unconsumed changeset on main now contradicts this one:
.changeset/adoption-loop.mddescribes "Two harness hooks" and "APreToolUsehook matched toWebSearch". It is unreleased, so it will be concatenated into the same CHANGELOG entry as this PR's changeset, which describes four hooks. This PR is what makes it false, so a one-line edit here keeps the released notes internally consistent. - [hygiene] the primer still states a price: "a hit costs cents" is unchanged. Noting it stands rather than re-raising it; your call.
Verified, not issues
- The demand budget counts only
dispatch-hook, leavingwebsearch-hookunbudgeted on purpose, which is right: that source is nagged, closable, and drained. latestSearchand the Stop hook's three-way still exclude the demand source, and a test round-trips it through both.- The ceiling at ten sits below the store budget of fifteen, so within a session the ceiling binds first and the budget is the backstop rather than the mechanism.
Verdict: comments-only. Both code Majors are closed and closed well, the WebFetch removal in particular. Two new Minors, both narrow, both in the bound that was added this round rather than in what it replaced. Major 1 is not a defect and not yours to settle; it sits with the operator.
… both writers Review round 2 on #180. The burst ceiling counted recorded lookups, so the case it should bound hardest was the one it could not see: a lookup that times out records nothing, and during an outage a wide fan-out paid the full fetch budget every time. The dispatch arm now keeps a consecutive-failure count in hook-health.json and goes quiet for ten minutes after two, self-healing when the window expires. An answer clears the count, and a MISS is an answer. The ceiling also binds on a harness that names no session: the unstamped '' bucket is a real session here, exactly as it is for the dedupe. The demand budget moves into the store's other writer. lib/search-store.ts's recordSearch trimmed with a plain slice, so a `tenjin search` write dropped the oldest entry whatever its source while demand entries sat untouched; both writers now apply the same rule, and both bounds are exported from one definition and baked into the generated scripts. Also: .changeset/adoption-loop.md said two hooks and two scripts, which this PR makes false, and both changesets concatenate into one CHANGELOG entry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Pushed bc7ce9e for round 2. Major 1 (consent): closed by the operator, not by an author on their behalf. The auto-enable-on-re-install call, the single-key model, and the final disclosure wording were each decided directly by the operator during this PR's rounds; the wording you verified at install.ts:683 and install.ts:1326 is their approved text verbatim. There is no party left to await, so treating it as open would be re-litigating a settled operator decision. New Minor 1 (ceiling gaps): fixed, with one correction to the review. Two consecutive failed lookups (a throw and an error-null count the same; a clean MISS is an answer) now stop the dispatch arm for 10 minutes, time-bounded so an outage self-heals without intervention, tracked in a new New Minor 2 (store writer): fixed. Nit 1 (changeset): fixed minimally to state current truth. One pre-existing staleness in the same file was left alone deliberately (its closing "uninstalling the hooks is still manual" sentence, made false by an earlier PR, not this one). Nit 2: stands, operator's call, as you noted. Every new gate was verified by reverting it (failure-stop threshold, null-session count, store budget) and confirming the tests fail, then restoring. Full suite 2192 passed, pack-smoke green, and a live check against a dead port ends at failures: 2 with the third dispatch exiting before any attempt. |
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 round 3: everything closes, and one correction is mine
Delta only, 8f2b887..bc7ce9e. Verified at bc7ce9e, checks green. Final round from me.
Correction I owe you first. My round-2 fix line said to stop the arm "the way an unhealthy server already stops a batch elsewhere in this codebase". You are right that no such thing exists here. I checked origin/main: there is no HALTING_FAILURES and no batch-stop anywhere in src/, only a 429 mapped to RATE_LIMITED, which stops nothing. I was describing an unmerged PR of yours as if it were the codebase, which is exactly the kind of unverified claim this loop is supposed to catch rather than produce. The suggestion happened to be right on its own merits, but the precedent I cited for it was not there.
Resolved since round 2
-
[closed] Major 1, the dispatch-egress consent: settled by the operator, who accepts the default-on arm under
hooks.searchModewith the disclosure verified in round 2. Not re-argued, and it does not come back. -
[closed] Minor 1, the ceiling's two gaps: both closed, and the harder one closed properly. A throw and a null are now the same outcome to the counter, an answer of any kind clears the run, and a MISS counts as an answer because it is one (hook-scripts.ts:123-131). Two details worth naming because they are easy to get wrong and are right here: the run restarts rather than increments once the window has passed, so a failure from months ago cannot combine with one today to stop the arm, and
stopped()is a time window rather than a latch, so a recovered server needs nothing done to it. The null-session gap closed the way I would have asked, by making the unstamped bucket a real session on the same terms asalreadyAsked(hook-scripts.ts:829-838), since a harness that scopes nothing is the last place a bound should go missing.hook-health.jsonis its own file on the documentedhook-nags.jsonterms, unreadable reads as healthy, andreadJsonFileis in the prelude so the read path resolves rather than throwing intoquiet(). Seven tests, including a real timeout and the unreadable-file case. -
[closed] Minor 2, one writer knowing the budget: the bounds are exported from the module that owns them and baked into the generated body (search-store.ts:15-23), and
budgetednow runs in the TS writer too, so the bound belongs to the store rather than to whichever process wrote last. Baking the imported constants is the better half of this: the two copies can no longer disagree about the numbers, only about the algorithm. -
[closed] nit, the contradicting changeset: fixed by dropping the count rather than by rewriting the paragraph, which is the right size of edit for a changeset that is not this PR's subject.
Nits (3), none blocking, none needing a reply
- [hygiene] the failure counter undercounts a parallel burst:
hook-health.jsonis unlocked by design, so subagents dispatched in one message all read the count before any of them writes, and an outage-time burst lands at one failure rather than N. It self-corrects on the next dispatch and the trade matches the Stop hook's documented one forhook-nags.json, so this is a note for the next reader rather than a change to make. - [hygiene] the mirrored
budgetedis now pinned by value but not by behavior: the constants cannot drift, the algorithm still can. A test that runs both copies over one fixture would close it cheaply, in the spirit of the existing test that pins the script's lock protocol against the module's. - [hygiene] the deliberately-left staleness: leaving "Uninstalling the hooks is still manual" out of scope is a defensible call, and it still ships as a false sentence in a release note whoever fixes it. Worth an issue so it has an owner rather than a comment thread.
Verdict: comments-only, and nothing is open on my side. Three rounds, eight findings, all closed in the code except the one that was never a defect. The two bounds added under review, the failure stop and the demand budget, are both better than what I asked for: a self-healing window instead of a session latch, and one definition compiled into both writers instead of a second copy to keep in step. Ran locally: head checkout, a read of both generated script bodies, and a check of origin/main for the batch-stop I wrongly cited.
… current uninstall 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 last two nits, closed
Delta only, bc7ce9e..e7d9474. Verified at e7d9474, CI green. Nothing here touches the settled surfaces: the consent wiring, the demand budget itself, and the health file are all untouched, so the standing verdict holds.
-
[closed] the mirrored
budgetedis now pinned by behavior, not just by value: one fixture through both writers, compared searchId for searchId after a sixty-entry flood. That is the guard I described and it is the right shape for it, since the constants were already compiled from one definition and only the algorithm could still drift. -
[closed] the stale uninstall sentence: replaced with what
tenjin uninstallactually does, which matches the code I read in round 1: it unwires all four entries by filename and deletes the scripts.
Verdict: unchanged, comments-only, nothing open on my side. Three rounds plus this one, all findings closed in the code except the consent question, which the operator settled.
Widens the hook trigger surface so the marketplace check no longer depends on the agent using
WebSearch, and puts a one-paragraph Tenjin instinct primer at the top of every session.Closes #173. Implements items 1 and 3 of #174 (WebFetch arm, dispatch hook); the shell-research decision and check-visibility items stay tracked there.
What
tenjin-sessionstart.mjs, matcherstartup|clear|compact): injects one fixed paragraph teaching what a Tenjin-shaped question is and to runtenjin searchbefore spending on research, including when enumerating sources in a subagent prompt. No network, no ledger state, no update line: the primer stays pure and brief. Runtime togglehooks.sessionPrimer(defaulton).tenjin-dispatch.mjs, matcherAgent|Task|WebFetch): the dispatch of a research subagent is the one moment the full question exists as text, before the tokens are spent. The hook forwards the task description plus the first 400 characters of the prompt to/api/agent/search, injects at most 2 candidate lines on a hit (same format and attribution guard as the WebSearch hook), and records HIT/MISS undersource: 'dispatch-hook'. Skips prompts under 80 chars and dedupes per session by normalized question.source: 'webfetch-hook', never injected into (prior dogfooding measured 45% duplicate noise on fetch-time hints). Demand data only.outcome --lastkeeps targeting deliberate searches only.HOOK_SCRIPT_VERSION17 → 18; install disclosure and command reference updated.Privacy note (decision, flagged deliberately)
This is the first hook that sends non-search text off-box: up to 400 characters of a subagent prompt head, plus up to 100 of the task description. Bounded by constants, disclosed in the install prompt and docs, and gated by the existing
hooks.searchModetoggle (offsilences it). If we want a separate toggle or a smaller slice, say so on this PR.Verification
scripts/pack-smoke.shrun explicitly outside vitest after build: PASS.install --harness claudeinto a temp home verified the wiring shape: two PreToolUse entries (WebSearch→ websearch script,Agent|Task|WebFetch→ dispatch script),SessionStart→ primer,Stopunchanged; running the installed primer emits exactly the specified paragraph.🤖 Generated with Claude Code