feat(claude-code): capture system_instructions and TTFT via BUN_OPTIONS preload#67
Conversation
…NS preload Anthropic's Claude Code CLI is a Bun-compiled single binary, so its system prompt content and per-LLM-call time-to-first-token never reach the transcript that pilot's existing hook processor parses. This change adds an in-process fetch interceptor that captures both fields and merges them into the standard llm.request / llm.response events. Mechanism - New `assets/hooks/claude-code-fetch-intercept.mjs`: a Bun --preload script that monkey-patches globalThis.fetch, filters /v1/messages requests, extracts system_instructions from the request body (filtering the Claude Code billing-header block and mapping Anthropic's `text` to the spec's `content` field), and measures TTFT off the first SSE content_block_delta event. Each LLM call is persisted atomically to ~/.loongsuite-pilot/intercept/claude-code/<session_id>/<response_id>.json. - New `AgentHookConfig.env` field + applyEnvToSettings in HookStrategy: deploy-time injection of BUN_OPTIONS=--preload=... into ~/.claude/settings.json, with idempotent coexistence semantics so a user's own preload entries are preserved. - claude-code-hook-processor.mjs: at Stop, load per-session intercept files, merge by response_id (= Anthropic message_id) into the emitted llm.request (gen_ai.system_instructions) and llm.response (gen_ai.response.time_to_first_token, integer nanoseconds) records, then unlink consumed files and reap stragglers older than 1h. Cleanup strategy is per-Stop only (no separate sweeper): merged files deleted immediately, stale ones swept opportunistically. Abandoned sessions leave at most a few small files until their next Stop. Requires @loongsuite/otel-util-genai >= 0.1.0-beta.7, which reads gen_ai.response.time_to_first_token from the response record and forwards it through invocation.attributes onto the chat span. Verification - Unit: 8 new fetch-intercept cases, 6 new hook-processor merge cases, 8 new HookStrategy env-injection cases (idempotency, coexistence, failure-tolerance). Full suite 1321/1323 (2 pre-existing skips). - E2E: local-reinstall + real `claude --print` confirms intercept files land, hook merges, JSONL carries both new fields, and the OTLP chat span attribute set includes both gen_ai.system_instructions (27399 chars over 2 blocks) and gen_ai.response.time_to_first_token (3845797875 ns). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ralf0131
left a comment
There was a problem hiding this comment.
Summary
Implements BUN_OPTIONS preload-based fetch interception for Claude Code to capture gen_ai.system_instructions and gen_ai.response.time_to_first_token — two fields not available via the existing hook + transcript pipeline. The intercept → merge → reap lifecycle is well-designed with strong error isolation.
Review by Dimensions
- Correctness: SSE parsing via
\n\nsplit is correct;response_idjoin key (Anthropicmessage_id) ensures 1:1 matching; billing-header filtering andtext→contentfield mapping match the spec. All intercept operations wrapped in try/catch — intercept failures never break Claude Code fetch flow. ✓ - Performance: Pass-through TransformStream (enqueue first, parse second) avoids blocking the response stream. Parsing stops once both
response_idandttft_nsare captured, bounding memory.reapStaleInterceptcleans 1h-old orphan files. ✓ - Tests: 22 new test cases across 3 files (fetch-intercept 8, hook-processor 6, hook-strategy 8) with good edge-case coverage (malformed JSON, missing headers, flush without delta, stale reaping, env idempotency). ✓
- Compatibility:
AgentHookConfig.envis an additive interface change. BUN_OPTIONS injection is idempotent and coexists with user preload scripts. Dependency bumpotel-util-genaibeta.5→beta.7 is minimal. ✓
Minor Observations (non-blocking)
readBodyAsTexthandlesstring/Uint8Array/ArrayBufferbut notBloborReadableStreambody types. Acceptable for Claude Code current string-body fetch calls, but system_instructions capture would silently no-op if the body type changes in the future.applyEnvToSettingshardcodesresolveHome('~/.loongsuite-pilot')for$PILOT_DATAexpansion while the preload script respectsLOONGSUITE_PILOT_DATA_DIR. Consistent today, but worth noting if the data dir becomes configurable.- The
package-lock.jsonversion correction (1.1.5→1.0.0to matchpackage.json) is a welcome fix — the main branch lockfile had a stale version.
LGTM. Well-structured feature with thorough test coverage.
Automated review by github-manager-bot
…json settings.json.env values are read by the agent's host process AFTER the Bun runtime has already loaded preloads — so the BUN_OPTIONS we injected in the previous commit only affected Claude Code's child processes, never the main Bun process itself. On a fresh terminal (where the shell env's BUN_OPTIONS comes from launchd's qodercli setenv), Claude Code's main process loaded the qodercli preload instead of ours, and the new TTFT / system_instructions fields silently disappeared from JSONL and OTLP. The previous PR's e2e checks coincidentally passed only because the test bash session was spawned by Claude Code's Bash tool — which DOES use the settings.json env merge — so the parent shell already had our BUN_OPTIONS set. Fresh-terminal scenarios reveal the gap. Fix: install a shell-rc function `claude()` that prepends our preload to BUN_OPTIONS before invoking the real binary, modelled exactly on the existing `inject_qodercli_token_intercept` helper. The wrapper preserves any pre-existing BUN_OPTIONS (e.g. qodercli's launchd setenv) by appending it after our preload, so both interceptors coexist. Changes: - agents.d/claude-code.json: drop env.BUN_OPTIONS block (was ineffective) - deploy/installer-opensource.sh: add inject/remove_claude_code_fetch_intercept (~zsh/.bashrc wrapper function), wire into install/uninstall flows - src/types/deployment.ts: document the BUN_OPTIONS caveat on AgentHookConfig.env - package.json: bump @loongsuite/otel-util-genai to ^0.1.0-beta.8 (beta.8 fixes the parent_span_id consistency check that was triggered by every multi-step hook agent — see PR #67 CI failure) The AgentHookConfig.env mechanism itself is kept (8 unit tests + impl in hook-strategy.ts retained) because the general settings.json env merge is still useful for any future agent-host-readable env values; only BUN_OPTIONS is excluded from this path. Verification - Full vitest suite still 1321 passed / 2 skipped / 0 failed - qwen-code-cli e2e-self-check (PR #67 CI failure case): now passes - Real e2e in clean shell (env -i + zsh -l -i, simulating GUI Terminal with launchd BUN_OPTIONS=qodercli): claude --print with multi-step Bash tool calls → both LLM events carry ttft_ns and gen_ai.system_instructions in JSONL and OTLP chat-span attributes, pilot service log has no Inconsistent parent_span_id warnings Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The test harness had `require('undici').Response` as a fallback for
environments where globalThis.Response is missing. But undici is not a
pilot dependency, so on Node 18 (where globalThis.Response IS available
natively since 18.17+) the require() throws, the spawned node child
exits non-zero, and all 8 fetch-intercept cases fail with
"expected 1 to be 0" — even though the production preload script is
fine.
Node 18.17+ / 20+ / 22+ all expose globalThis.Response natively; older
patches aren't supported by pilot anyway (engines.node = ">=18"). Drop
the dead fallback.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
require() of a .mjs module is rejected with ERR_REQUIRE_ESM on Node 18.
Node 20.17+ permits it as experimental; Node 22+ ships it stable — which
is why local + CI on Node 20/22 passed but Node 18 had all 8 spawned
child processes exit non-zero before reaching any assertion.
Switch the test bootstrap to `await import('file://' + PRELOAD)`, which
works uniformly across Node 18 / 20 / 22.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
linrunqi08
left a comment
There was a problem hiding this comment.
Code Review Summary
Reviewed via 7 independent angles (line-by-line, removed-behavior, cross-file, reuse, simplification, efficiency, altitude) with adversarial verification.
1 bug (hardcoded path breaks --data-dir installs), 1 latent issue (dead code with wrong fallback), 3 cleanup suggestions. Overall the PR is well-structured with good fail-open semantics and solid test coverage.
Findings (ranked by severity)
| # | Severity | File | Summary |
|---|---|---|---|
| 1 | 🐛 Bug | installer-opensource.sh:939 | Shell wrapper hardcodes $HOME/.loongsuite-pilot path, ignoring --data-dir |
| 2 | hook-strategy.ts:308 | $PILOT_DATA re-expansion is dead code with divergent fallback path |
|
| 3 | 💡 Robustness | hook-strategy.ts:325 | BUN_OPTIONS idempotency uses includes() substring match |
| 4 | 🧹 Cleanup | hook-processor.mjs | reapInterceptFiles + reapStaleIntercept could be a single pass |
| 5 | 🧹 Cleanup | fetch-intercept.test.mjs:121 | Last test inlines near-duplicate of runScenario bootstrap |
See inline comments for details.
| cat >> "$file" << 'INTERCEPTBLOCK' | ||
|
|
||
| # loongsuite-pilot BEGIN claude-code-intercept | ||
| claude() { BUN_OPTIONS="--preload=$HOME/.loongsuite-pilot/hooks/claude-code-fetch-intercept.mjs ${BUN_OPTIONS}" command claude "$@"; } |
There was a problem hiding this comment.
Bug: hardcoded path breaks --data-dir override
The guard at line 922 correctly uses $DATA_DIR:
local intercept_script="$DATA_DIR/hooks/claude-code-fetch-intercept.mjs"But this heredoc (single-quoted 'INTERCEPTBLOCK' → no expansion at write time) hardcodes $HOME/.loongsuite-pilot.
When someone installs with --data-dir=/opt/pilot, the guard passes (file exists at /opt/pilot/hooks/...), but the shell wrapper written to .zshrc/.bashrc references $HOME/.loongsuite-pilot/hooks/... where the file doesn't exist. Bun's --preload of a missing file causes a fatal error or silent no-op, breaking either Claude Code or the interception.
Suggested fix: Switch to a double-quoted heredoc and interpolate $DATA_DIR at install time:
cat >> "$file" << INTERCEPTBLOCK
# loongsuite-pilot BEGIN claude-code-intercept
claude() { BUN_OPTIONS="--preload=$DATA_DIR/hooks/claude-code-fetch-intercept.mjs \${BUN_OPTIONS}" command claude "\$@"; }
# loongsuite-pilot END claude-code-intercept
INTERCEPTBLOCKOr keep the single-quoted heredoc but replace $HOME/.loongsuite-pilot with a variable that was pre-resolved during install.
There was a problem hiding this comment.
Fixed in d6068a8. Switched both inject_qodercli_token_intercept and inject_claude_code_fetch_intercept to double-quoted heredocs so $DATA_DIR expands at install time; ${BUN_OPTIONS} and $@ are escaped to defer expansion until the wrapper runs.
The same bug existed in the qodercli wrapper (which this one was modeled on) — kept them in lockstep to avoid future drift.
Thanks for catching this — --data-dir overrides would have been silently broken without your call-out.
| settingsPath: string, | ||
| env: Record<string, string>, | ||
| ): Promise<void> { | ||
| const pilotData = resolveHome('~/.loongsuite-pilot'); |
There was a problem hiding this comment.
Dead code with divergent fallback path
AgentDefLoader.resolveVariables() recursively resolves all $PILOT_DATA tokens (including nested hook.env values) before the config reaches HookStrategy.deploy(). By the time applyEnvToSettings runs, there are no $PILOT_DATA tokens left to replace — making rawValue.replace(/\$PILOT_DATA/g, pilotData) on line 316 dead code.
Worse, the fallback here (resolveHome('~/.loongsuite-pilot')) differs from AgentDefLoader's resolution which respects LOONGSUITE_PILOT_DATA_DIR. If a future code path reaches applyEnvToSettings without prior AgentDefLoader resolution, it would silently use the wrong path for custom data dirs.
Suggestion: Either (a) remove the $PILOT_DATA replacement here since it's already done upstream, or (b) accept pilotData as a parameter from the caller (who has the correctly resolved dataDir) instead of hardcoding the default.
There was a problem hiding this comment.
Fixed in d6068a8. Removed the local pilotData variable and the replace(/\$PILOT_DATA/g, ...) call. You're right that AgentDefLoader.resolveVariables() handles the expansion upstream — the local fallback was both dead and (worse) divergent from the canonical resolution that honors LOONGSUITE_PILOT_DATA_DIR.
AgentHookConfig.env doc updated to reflect this. Tests updated to pass already-resolved paths (mirroring real input shape after AgentDefLoader runs).
| // can probe substring presence rather than exact match. | ||
| const eqIdx = expanded.indexOf('='); | ||
| const ourToken = eqIdx >= 0 ? expanded.slice(eqIdx + 1) : expanded; | ||
| if (current.includes(ourToken)) { |
There was a problem hiding this comment.
Nit: includes() is a substring match, not a token-boundary match
current.includes(ourToken) would false-positive if the existing BUN_OPTIONS contained a superstring of our path (e.g., ...intercept.mjs-debug). In practice this is near-impossible because ourToken is a long, project-specific path, but a stricter check would split on whitespace boundaries:
const tokens = current.split(/\s+/);
if (tokens.some(t => t.includes(ourToken))) {
continue;
}Or even match the full --preload=<path> token rather than just the path portion.
There was a problem hiding this comment.
Fixed in d6068a8. Switched to whitespace-split token-boundary match:
const ourTokens = value.split(/\\s+/).filter(Boolean);
const currentTokens = current.split(/\\s+/).filter(Boolean);
if (ourTokens.every((t) => currentTokens.includes(t))) {
continue; // already injected (exact tokens present)
}Added a regression test (BUN_OPTIONS token-boundary match: superstring is NOT treated as already injected) that asserts ...intercept.mjs-debug does not false-positive against ...intercept.mjs.
| const dir = interceptSessionDir(sessionId); | ||
| let entries; | ||
| try { entries = fs.readdirSync(dir); } catch { return; } | ||
| const now = Date.now(); |
There was a problem hiding this comment.
Minor: reapInterceptFiles + reapStaleIntercept could be a single pass
Both functions operate on the same session directory. reapInterceptFiles iterates the Map, then reapStaleIntercept re-reads the directory with readdirSync + statSync per file. Merging into one function that deletes a file if it was merged OR if it's stale (by mtime) in a single readdirSync + statSync loop would halve the syscall count and reduce ~15 lines.
Not critical since this runs once per session end, but it's a clean simplification.
There was a problem hiding this comment.
Considered, deciding to keep the two-function form.
Three reasons:
- Operationally, each session dir holds 0–N small files where N is the number of LLM calls in the just-finished Stop —
reapStaleInterceptafterreapInterceptFilestypically iterates an already-empty (or near-empty) directory. Real syscall savings are <5 per Stop. - The two functions encode distinct semantics: "delete what I just consumed" vs. "opportunistically GC orphans from earlier turns." Merging would entangle them into one function with two branching purposes.
- The 6 existing merge tests target each function's contract independently; merging would force a rewrite of those for no observable behavioral change.
Open to revisiting if profiling ever shows this dir-scan in the hot path.
| data: { type: 'message_start', message: { id: opts.msgId ?? MSG_ID, model: 'claude-test', role: 'assistant' } }, | ||
| }]; | ||
| if (opts.includeContentDelta !== false) { | ||
| events.push({ event: 'content_block_start', data: { type: 'content_block_start', index: 0 } }); |
There was a problem hiding this comment.
Nit: last test inlines a near-duplicate bootstrap script
The "malformed JSON body" test (line ~233) inlines a ~30-line copy of the bootstrap script instead of reusing runScenario. If the bootstrap shape changes (import path, stream draining, TransformStream polyfill), this copy must be updated separately.
Consider adding a rawBody option to runScenario to pass a string body directly, avoiding the JSON.stringify(body) step while reusing the rest of the harness.
There was a problem hiding this comment.
Fixed in d6068a8. Added a rawBody option to runScenario that bypasses JSON.stringify:
const bodyJson = rawBody !== undefined ? rawBody : JSON.stringify(body);The "malformed JSON body" test now reuses the harness in 5 lines instead of inlining ~30. Future bootstrap changes (import path, stream draining, etc.) only need to be made once.
ralf0131
left a comment
There was a problem hiding this comment.
Summary
Re-review following 3 new commits (a6a9025, 3ff0846, ed17610). The BUN_OPTIONS injection switched from settings.json env to a shell-rc wrapper — this is the correct approach. Bun reads BUN_OPTIONS at process startup before any JS executes, so settings.json env values (applied after the main process starts) could only affect child processes, not the Bun preload of the main process itself.
Findings
- [Info] deploy/installer-opensource.sh:inject_claude_code_fetch_intercept — The shell function claude() wrapper is well-implemented: idempotent (greps for existing block before injecting), preserves any existing BUN_OPTIONS, uses
command claudeto avoid recursion, properly wired into both cmd_install() and cmd_uninstall(), and handles bash/zsh. One minor note: the wrapper only applies to interactive shells (.bashrc/.zshrc), so non-interactive claude invocations (CI, scripts) won't have the intercept — acceptable for the local-dev use case. - [Info] src/types/deployment.ts — Good documentation update explaining why settings.json env cannot influence BUN_OPTIONS and directing to the shell-rc wrapper instead.
- [Info] tests/unit/hooks/claude-code/fetch-intercept.test.mjs — Correct fixes: require() to dynamic import() for .mjs (Node 18 ERR_REQUIRE_ESM), dropped undici Response fallback (native in Node 18.17+).
Previous APPROVE stands. New changes look good — no blocking issues.
Automated review by github-manager-bot
Addresses 4 of 5 reviewer comments on PR #67: #1 BUG (installer-opensource.sh): The single-quoted heredoc that writes the claude wrapper to ~/.zshrc / ~/.bashrc hardcoded `$HOME/.loongsuite-pilot`, ignoring `--data-dir` overrides. A user installing with `--data-dir=/opt/pilot` would have the intercept script at /opt/pilot/hooks/... while the wrapper pointed at $HOME/.loongsuite-pilot/hooks/... — Bun's --preload of a missing file silently no-ops, breaking interception. Fix: double-quoted heredoc so $DATA_DIR expands at install time; escape ${BUN_OPTIONS} and $@ so they expand at wrapper-runtime. The same defect existed in inject_qodercli_token_intercept (which this PR's wrapper was modeled on); both are fixed in lockstep. #2 DEAD CODE (hook-strategy.ts): `applyEnvToSettings` did a `rawValue.replace(/\$PILOT_DATA/g, ...)` step, but AgentDefLoader.resolveVariables() already recursively resolves `$PILOT_DATA` (honoring LOONGSUITE_PILOT_DATA_DIR) before the config reaches HookStrategy. The local fallback also diverged from the canonical resolution (`resolveHome('~/.loongsuite-pilot')` vs AgentDefLoader's `this.dataDir`). Remove the local replace + pilotData var; tests updated to pass already-resolved paths to reflect real input shape. AgentHookConfig.env doc updated too. #3 NIT (hook-strategy.ts): BUN_OPTIONS idempotency check used `current.includes(ourToken)` — a substring match that would false-positive on a superstring (e.g., `...intercept.mjs-debug`). Replace with whitespace-split token-boundary match. Added a regression test guarding this case. #5 NIT (fetch-intercept.test.mjs): the "malformed JSON body" test inlined a near-duplicate ~30-line bootstrap copy of runScenario. Add a `rawBody` option to runScenario that bypasses JSON.stringify; the test now reuses the harness. Comment #4 (merging reapInterceptFiles + reapStaleIntercept) is deliberately not addressed — the two functions have distinct single-responsibility semantics, syscall savings are <5 per Stop (target dir is near-empty), and merging would force a rewrite of 6 existing merge tests for no operational benefit. Verification: typecheck passes; full vitest suite 1322 passed / 2 skipped / 0 failed (added one new regression test for #3); CI matrix on Node 18/20/22 expected green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
BUN_OPTIONS=--preload在 Claude Code 的 Bun 二进制进程内拦截 LLM fetch,把现有 hook + transcript 方案拿不到的gen_ai.system_instructions(完整 system prompt)和gen_ai.response.time_to_first_token(纳秒,per-LLM-call)采集出来。AgentHookConfig.env通用字段 +HookStrategy.applyEnvToSettings,部署期把BUN_OPTIONS写入~/.claude/settings.json,带幂等 / 多 preload 共存语义。~/.loongsuite-pilot/intercept/claude-code/<sid>/<rid>.json),Stop hook 按response_id(= Anthropicmessage_id) 1:1 merge 到现有llm.request/llm.response事件后立即删除,孤儿文件 1h opportunistic 清理。关键设计决策
response_id: 1:1 强匹配,实测三方对账(preload / transcript / pilot JSONL)完全一致。content),符合specs/gen-ai-system_instructions.json。preload 自动过滤首个x-anthropic-billing-header:元数据 block。@loongsuite/otel-util-genai >= 0.1.0-beta.7(新增了从 record 读 ttft 字段的能力)。文件改动
assets/hooks/claude-code-fetch-intercept.mjs(preload 脚本,~285 行)agents.d/claude-code.json(env 块)src/types/deployment.ts(AgentHookConfig.env)src/deployment/hook-strategy.ts(applyEnvToSettings)assets/hooks/claude-code-hook-processor.mjs(intercept load + merge + reap)package.json/package-lock.json(util-genai 0.1.0-beta.7)tests/unit/hooks/claude-code/fetch-intercept.test.mjs(8 case)tests/unit/hooks/claude-code/hook-processor.test.mjs(+6 merge case)tests/unit/deployment/hook-strategy.test.ts(+8 env injection case)Test plan
npm run typecheck通过npx vitest run全量:1321 passed / 2 skipped / 0 failed(125 files)local-reinstall.sh打包安装 →~/.claude/settings.json.env.BUN_OPTIONS自动注入 → 触发claude --print→ intercept 文件落盘并被 Stop hook 清理 → pilot JSONL 含两新字段 → OTLP chat span attributes 含gen_ai.response.time_to_first_token: 3845797875和gen_ai.system_instructions(27399 chars / 2 blocks)验证过的鲁棒性
writeFileSync同步落盘)\n\n事件块切分,实测稳健(早期版本用 sliding-window 正则会切坏长 preamble,已避免)🤖 Generated with Claude Code