feat(zcode): ZCode probe + Step C fixes + Round 2 interrupted-path injection (AGE-593) - #232
feat(zcode): ZCode probe + Step C fixes + Round 2 interrupted-path injection (AGE-593)#232zy84338719 wants to merge 9 commits into
Conversation
fa5126b to
f3b7b7a
Compare
There was a problem hiding this comment.
Pull request overview
This PR integrates the ZCode CLI probe into loongsuite-pilot via a hook-based JSONL pipeline plus a rollout (per-session) JSONL tail input, and adds multiple fixes to improve OTLP GenAI trace correctness (step.id sequencing, flush timing/debounce behavior, and interrupted/orphan scenarios).
Changes:
- Add ZCode agent deployment (agents.d definition + hook scripts) and register zcode-log + zcode-rollout inputs in the orchestrator.
- Extend OTLP trace flusher with per-agent flush config, debounce-based terminal flushing, ZCode step.id lazy backfill, and orphan tool synthesis.
- Add/expand unit tests and fixtures for rollout parsing, hook processor behavior, flusher debounce timing, and orphan synthesis correctness.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
agents.d/zcode.json |
Adds ZCode agent definition, hook wiring, and per-agent flusher overrides. |
assets/hooks/zcode-hook-processor.mjs |
Implements ZCode hook event processing into JSONL records. |
assets/hooks/zcode-loongsuite-pilot-hook.sh |
Hook entry script that dispatches to the Node processor with fail-open behavior. |
assets/hooks/agent-event-normalizer.mjs |
Adds ZcodeStepResolver for lazy step.id backfill of hook tool events. |
scripts/validate-trace.mjs |
Extends allowed finish reasons to include interrupted/cancelled. |
src/core/config-loader.ts |
Adds OTLP debounce/per-agent flusher config fields; expands legacy OTLP resourceAttributes. |
src/core/orchestrator.ts |
Registers ZCode inputs and builds per-agent flusher override map from agents.d. |
src/core/hook-watchdog.ts |
Adds ZCode hook health-check target/markers. |
src/deployment/hook-strategy.ts |
Supports nested hook container path and deep-merge extraSettings into agent config. |
src/hooks/hook-manager.ts |
Avoids writing an explicit default matcher when none is specified (nested format). |
src/inputs/base/base-session-input.ts |
Allows one session line to expand into multiple emitted entries. |
src/inputs/zcode-log/zcode-log-input.ts |
Adds hook JSONL tail input for ZCode hook records. |
src/inputs/zcode-rollout/zcode-rollout-input.ts |
Adds rollout JSONL tail input that emits per-record llm.request/llm.response and synthesizes missing tool.result from rollout tool messages. |
src/flushers/otlp-trace-flusher.ts |
Adds debounce-based flushing, per-agent config overrides, ZCode step resolver integration, and orphan synthesis logic. |
src/normalization/agent-system-map.ts |
Registers zcode in agent system resolution. |
src/types/client-type.ts |
Adds ZcodeHook client type. |
src/types/deployment.ts |
Extends hook config (container path, extra settings, hook type metadata) and agent definition (per-agent flusher overrides). |
src/types/index.ts |
Adds OTLP flusher config types for debounce and per-agent overrides. |
tests/unit/hooks/zcode/zcode-rollout-input.test.ts |
Adds comprehensive tests for rollout parsing, step.id allocation, tool definitions, interrupted injection, and tool.result synthesis. |
tests/unit/hooks/zcode/hook-processor.test.mjs |
Adds end-to-end tests for hook processor subcommands and error-path behavior. |
tests/unit/hooks/agent-event-normalizer.test.mjs |
Adds unit tests for ZcodeStepResolver behavior under various arrival orders. |
tests/unit/flushers/otlp-trace-flusher/debounce.test.ts |
Tests debounce/idle-timeout behavior and the rollout-terminal suppression semantics. |
tests/unit/flushers/otlp-trace-flusher/orphan-synthesis.test.ts |
Tests ZCode orphan tool synthesis behavior (multiple flavors). |
tests/unit/deployment/zcode-matcher.test.ts |
Ensures ZCode nested hook format deploy doesn’t serialize a matcher field. |
tests/unit/core/config-loader.test.ts |
Updates assertions for expanded legacy OTLP resourceAttributes. |
tests/unit/hooks/zcode/fixtures/zcode-cli-config.json |
Adds fixture ZCode config used by hook/rollout tests. |
tests/unit/hooks/zcode/fixtures/stop-transcript.jsonl |
Adds stop transcript fixture. |
tests/unit/hooks/zcode/fixtures/hook-events.jsonl |
Adds real hook event fixture for ZCode. |
tests/unit/hooks/zcode/fixtures/rollout/model-io-sess_v015-g1-nested-tools.jsonl |
Adds v0.15.0 nested tools fixture. |
tests/unit/hooks/zcode/fixtures/rollout/model-io-sess_v015-g2-tool-error.jsonl |
Adds v0.15.0 tool error fixture. |
Suppressed comments (2)
src/inputs/zcode-rollout/zcode-rollout-input.ts:294
- Same as above for the rollout
llm.response: without top-levelagent.source='zcode-rollout', the flusher can’t reliably suppress rollout terminal responses as Signal A triggers.
'event.name': 'llm.response',
trace_id: traceId,
'gen_ai.session.id': sessionId,
'gen_ai.turn.id': turnId,
'gen_ai.step.id': stepId,
assets/hooks/zcode-hook-processor.mjs:179
- cmdSessionStart has the same issue: it falls back to
generateSpanId()fortrace_id, which is only 16-hex. This should generate a 32-hex trace id when the payload traceId is missing/invalid.
const runtimeConfig = loadHookRuntimeConfig(pilotDataDir());
const userId = resolveUserId({}, runtimeConfig);
const spanId = generateSpanId();
const traceId = normalizeTraceId(event.traceId || event.trace_id) || generateSpanId();
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| 'event.name': 'llm.request', | ||
| trace_id: traceId, | ||
| 'gen_ai.session.id': sessionId, | ||
| 'gen_ai.turn.id': turnId, | ||
| 'gen_ai.step.id': stepId, | ||
| 'gen_ai.request.id': requestId, |
| * Stop 事件只发 "other" 标记 turn 元数据(agent.event.name=stop, | ||
| * tool.call.count),不带 terminal finish_reason —— terminal signal 由 | ||
| * rollout input 的最后一条 llm.response(finish_reason=stop)提供, | ||
| * turnIdleTimeoutMs 作为兜底。 |
| constructor(opts?: Partial<HookInputOptions> & { stateStore: HookInputOptions['stateStore'] }) { | ||
| super({ | ||
| stateStore: opts!.stateStore, | ||
| logDir: opts?.logDir ?? resolveHome('~/.loongsuite-pilot/logs/zcode'), |
| function baseFields(event, userId, runtimeConfig) { | ||
| const traceId = normalizeTraceId(event.traceId || event.trace_id) || generateSpanId(); | ||
| const spanId = generateSpanId(); | ||
| const sessionId = event.session_id || event.sessionId || ''; |
| const baseAttrs: Record<string, JsonValue> = { | ||
| source: SOURCE, | ||
| 'zcode.rollout.file': path.basename(filePath), | ||
| }; | ||
| if (providerId) baseAttrs['gen_ai.provider.name'] = providerId; | ||
| if (completedAt) baseAttrs['zcode.rollout.completed_at'] = completedAt; |
f3b7b7a to
ae737f6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated no new comments.
Suppressed comments (5)
assets/hooks/zcode-hook-processor.mjs:179
cmdSessionStartalso falls back togenerateSpanId()fortrace_id(16-hex). This should generate a 32-hex trace id to meet W3C requirements and keep records in the same trace when traceId is missing.
const userId = resolveUserId({}, runtimeConfig);
const spanId = generateSpanId();
const traceId = normalizeTraceId(event.traceId || event.trace_id) || generateSpanId();
src/deployment/hook-strategy.ts:529
- Leftover merge-conflict marker
<<<<<<< HEADis present inside the JSDoc comment. Even though it’s commented out, it’s a merge artifact that should be removed to keep the file clean and avoid confusing future edits/searches.
/**
<<<<<<< HEAD
* Kiro CLI Agent 定义 JSON(~/.kiro/agents/<name>.json)专用 deploy。
assets/hooks/zcode-hook-processor.mjs:147
trace_idfallback usesgenerateSpanId()(16-hex), but W3C trace IDs must be 32-hex. This will produce invalid trace IDs when the hook payload lacks traceId/trace_id, which can cause OTLP conversion to reject/rewrite the trace and break correlation.
This issue also appears on line 176 of the same file.
function baseFields(event, userId, runtimeConfig) {
const traceId = normalizeTraceId(event.traceId || event.trace_id) || generateSpanId();
const spanId = generateSpanId();
assets/hooks/zcode-hook-processor.mjs:27
- The header comment says Stop does not emit a terminal finish_reason and that the terminal signal comes from rollout’s final llm.response, but the implementation now emits
gen_ai.response.finish_reasonsfrom Stop (end_turn/interrupted) and relies on flusher suppression of rollout terminal. Please update the comment to match the actual behavior.
* - per-LLM 的 llm.request/llm.response 不在 hook 里发 —— ZCode hook 只在
* Stop 给最终响应,无法支撑 per-LLM 配对。这部分由 zcode-rollout input
* 从 ~/.zcode/cli/rollout/model-io-sess_*.jsonl 补全(每条记录含完整
* request body + response text/toolCalls/usage + startedAt/completedAt)。
* Stop 事件只发 "other" 标记 turn 元数据(agent.event.name=stop,
* tool.call.count),不带 terminal finish_reason —— terminal signal 由
* rollout input 的最后一条 llm.response(finish_reason=stop)提供,
* turnIdleTimeoutMs 作为兜底。
src/core/orchestrator.ts:583
- This helper suggests per-agent flusher overrides are loaded from agents.d and “plumbed to OtlpTraceFlusherConfig.perAgentFlusherConfig”, but in the current code it’s never called from the OTLP flusher construction path. That means the
flusheroverrides in agents.d/zcode.json won’t take effect unless users also configureflushers.otlp.perAgentFlusherConfigin config.json.
/**
* Scan agents.d/*.json (builtin + local) and build a per-agentType flusher
* override map from each agent's `flusher` field (plan 2.1 + 2.2). Failures
* are non-blocking — the flusher falls back to global cfg for any agent
* not represented here.
*/
private async buildPerAgentFlusherConfig(): Promise<Record<string, { turnIdleTimeoutMs?: number; turnFlushDebounceMs?: number }>> {
const out: Record<string, { turnIdleTimeoutMs?: number; turnFlushDebounceMs?: number }> = {};
try {
Add ZCode CLI hook processor and rollout input probe with: - Stable per-line step.id derivation - Non-overlapping STEP time windows - Event emitter helpers for hook processing - Rollout input with paired model I/O support - Transcript parser for zcode sessions - Message converter and state management Based on PR alibaba#136 by rangemer333-cell
ae737f6 to
6aef289
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 24 changed files in this pull request and generated no new comments.
Suppressed comments (6)
src/inputs/zcode-rollout/zcode-rollout-input.ts:503
- finishReason defaults to 'stop' when the rollout record has completedAt but response.finishReason is null/empty (interrupted/timeout cases). This contradicts the PR description’s interrupted-path handling and will mark interrupted sessions as normal stops and omit the intended 0-usage defaults.
const response = (record.response && typeof record.response === 'object' ? record.response : {}) as Record<string, unknown>;
const responseText = str(response.text) ?? '';
const finishReason = str(response.finishReason) ?? str(response.finish_reason) ?? 'stop';
const responseId = str(response.responseId) ?? str(response.response_id) ?? requestId;
const responseModelId = str(response.modelId) ?? str(response.model_id) ?? modelId;
src/inputs/zcode-rollout/zcode-rollout-input.ts:615
- For interrupted turns (finishReason injected as 'interrupted' or produced by upstream), output.messages can currently be an empty array when responseText/toolCalls are empty. This leaves llm.response without output content, which is typically rejected by the trace validator for interrupted sessions (the PR description calls out a placeholder assistant message).
const outputMessages: JsonValue = outputParts.length > 0
? [{ role: 'assistant', parts: outputParts }]
: [];
assets/hooks/zcode-hook-processor.mjs:166
- buildEnvelopeRecords() does not set gen_ai.agent.name (and optionally description) in baseFields. The repo’s trace-validation rules require gen_ai.agent.name on spans; without it, ZCode ENTRY/AGENT envelopes may fail validation or be inconsistently grouped downstream.
const baseFields = {
trace_id: w3cTraceId,
'gen_ai.session.id': sessionId,
'gen_ai.agent.type': AGENT_ID,
'gen_ai.agent.id': sessionId,
'user.id': userId || os.hostname(),
...(cwd ? { 'agent.zcode.cwd': cwd } : {}),
};
src/inputs/zcode-hook/zcode-hook-input.ts:32
- The constructor parameter is marked optional (opts?), but opts is dereferenced via opts!.stateStore. Calling new ZCodeHookInput() without args will crash at runtime despite the type signature allowing it.
constructor(opts?: Partial<HookInputOptions> & { stateStore: HookInputOptions['stateStore'] }) {
super({
stateStore: opts!.stateStore,
logDir: opts?.logDir ?? resolveHome('~/.loongsuite-pilot/logs/zcode'),
logPrefix: opts?.logPrefix ?? 'zcode',
pollIntervalMs: opts?.pollIntervalMs ?? 30_000,
});
assets/hooks/zcode/transcript-parser.mjs:26
- The header comment claims this parser is used by the zcode hook-processor and zcode tests, but there are no imports/references to it. This makes the documentation misleading for future maintenance.
* This module converts ONE rollout line into a structured TurnAttempt object
* that the hook-processor's buildRolloutRecords() then expands into the
* canonical event_t records (llm.request + llm.response + tool.call/result
* + STEP envelope).
*
assets/hooks/zcode/message-converter.mjs:15
- The header comment says this module is shared by the hook-processor and rollout input, but neither imports it right now. This can mislead readers into thinking message-shape logic is centralized here when it isn’t.
* This module is shared logic — both the hook-processor (mjs envelope path)
* and the rollout input (ts path that re-implements inline) must produce
* identical message shapes. Tests verify equivalence on the paired fixture.
*/
|
This PR has conflicts with the git fetch origin
git checkout zcode-feature
git rebase origin/main
# resolve conflicts, then:
git push --force-with-leaseThis is a one-time reminder. Feel free to @mention me for a re-review after conflicts are resolved. Automated notification by github-manager-bot |
ralf0131
left a comment
There was a problem hiding this comment.
Summary
Adds ZCode CLI probe integration with dual-input architecture (hook JSONL + rollout transcript), following the V3 hybrid pattern: hook handles boundary spans (ENTRY/AGENT envelopes), rollout handles data (LLM/STEP/TOOL + messages). Cross-source stitching via shared deriveSpanId() is a clean design choice.
Note: This PR currently has merge conflicts with main. Please rebase and resolve conflicts before merge (see conflict notification above).
Overall: Code quality is good. The architecture is well-documented, test coverage is thorough, and the shared span_id derivation ensures cross-source consistency. Minor informational suggestions inline.
Automated review by github-manager-bot
| import { toW3CTraceId, deriveSpanId } from '../../../assets/hooks/shared/event-emitter.mjs'; | ||
|
|
||
| const DEFAULT_ROLLOUT_DIR = '~/.zcode/cli/rollout'; | ||
| const AGENT_ID = 'zcode'; |
There was a problem hiding this comment.
[Info] The @ts-expect-error for importing .mjs shared helpers is a known limitation. Consider adding a .d.ts declaration file for assets/hooks/shared/event-emitter.mjs (even a minimal one with just the exported function signatures) so the type checker can verify the import. This prevents silent breakage if the shared module's API changes.
| @@ -0,0 +1,884 @@ | |||
| import * as fs from 'node:fs'; | |||
There was a problem hiding this comment.
[Info] This file is 884 lines. Consider extracting the processModelIoRecord logic (lines ~400-700) into a separate helper module (e.g., zcode-rollout-record-processor.ts) to improve readability and testability. The current structure works but is dense.
| 'gen_ai.response.finish_reasons': [stopReason || 'end_turn'], | ||
| }); | ||
|
|
||
| // AGENT envelope — marks turn boundary inside the session. |
There was a problem hiding this comment.
[Info] The cmdSessionStart fallback to generateSpanId() for trace_id when no sessionId is available is correct per the fail-open contract. However, consider logging a warning (via the existing error-logger) when this fallback triggers, so operators can detect sessions without proper sessionId attribution.
fangxiu-wf
left a comment
There was a problem hiding this comment.
当前实现仍有会导致 Hook 不执行、双源链路无法拼接以及采集数据丢失/失真的阻断问题。
公共改动方面,eventsRoot 是可选字段,现有 HookStrategy/Watchdog 以及 Qwen/Claude 回归测试未发现其他插件的直接回归;但 Watchdog 当前仍只检查 hooks.<event>,无法识别 ZCode 的 hooks.events.<event>,会持续误判缺失并触发修复。另外当前没有把 zcode 加入 agent-system 映射,最终资源属性会落为 unknown。
本地验证结果:npm run typecheck、npm run build 通过;npm test 为 1575 passed / 1 failed / 2 skipped,唯一失败是本 PR 测试引用了未提交的 fixture。当前 PR 与 main 仍有 4 个冲突文件,因此合并结果尚无法验证。
请优先修复 inline 中的 P1 问题,并补充 fresh config、Windows、官方 Stop payload、partial JSONL、restart/EOF 以及两个输入源不同到达顺序的回归测试,之后再 re-review。
| "paths": ["~/.zcode"], | ||
| "commands": ["zcode"] | ||
| }, | ||
| "hook": { |
There was a problem hiding this comment.
[P1] 这里只会让 HookStrategy 创建 hooks.events.Stop,但不会写入 hooks.enabled: true。ZCode 当前官方文档明确要求用户级 ~/.zcode/cli/config.json 设置该开关,否则 fresh config 或原本关闭 Hook 的用户会出现“安装成功但永远不执行”。请以保留现有用户配置的方式补齐启用逻辑,并覆盖 fresh/false/already-true 及 uninstall 行为。参考:https://zcode.z.ai/en/docs/hooks
| "events": [ | ||
| "Stop" | ||
| ], | ||
| "hookCommand": "$PILOT_DATA/hooks/zcode-loongsuite-pilot-hook.sh", |
There was a problem hiding this comment.
[P1] Windows 下 AgentDefLoader 会把这里的 .sh 自动改写成 .ps1,但本 PR 没有提供 zcode-loongsuite-pilot-hook.ps1,最终配置会指向不存在的脚本。ZCode 官方支持 Windows x64/ARM64,因此需要补齐 PowerShell Hook,并验证 stdin、fail-open、install/uninstall。参考:https://zcode.z.ai/en/docs/install
| return; | ||
| } | ||
|
|
||
| const turnId = getString(event, 'turn_id', 'turnId'); |
There was a problem hiding this comment.
[P1] 这里的跨源关联依赖 turnId/traceId/timestamp,但 ZCode 当前官方 Stop stdin 契约只保证公共字段以及 stop_hook_active、last_assistant_message,并未保证这些字段。按文档 payload 执行时,turnId 为空会跳过 AGENT,traceId 为空又会生成随机 trace,无法和 rollout 拼接。请从文档化的可靠来源建立关联,或明确版本门槛并用当前支持版本的真实 payload 加回归测试。参考:https://zcode.z.ai/en/docs/hooks
| const buf = Buffer.alloc(stat.size - offset); | ||
| await handle.read(buf, 0, buf.length, offset); | ||
| const text = buf.toString('utf-8'); | ||
| this.stateStore.setOffset(stateKey, stat.size); |
There was a problem hiding this comment.
[P1] offset 在解析前就推进到 stat.size。触发序列是:ZCode 只写入最后一条 JSONL 的前半段,Pilot 本轮将其判为 invalid 并提交 EOF;ZCode 随后补齐后半段,下一轮只能读取后半段,整条记录永久丢失。最小复现已确认最终 0 条事件且 offset 位于 EOF。请只提交到最后一个完整换行,并持久化/保留 partial suffix,覆盖 restart 恢复。
| 'gen_ai.provider.name': providerId, | ||
| 'gen_ai.request.model': modelId, | ||
| 'gen_ai.response.model': responseModelId, | ||
| 'gen_ai.response.finish_reasons': [finishReason], |
There was a problem hiding this comment.
[P1] 这个终态 finish reason 会让 rollout 的独立 sendBatch() 立即 flush 并把 turn 放入 flushedTurnKeys;Stop Hook 的 ENTRY/AGENT 由另一个 Input 后到时会作为 late entries 被丢弃。反向顺序也相同,因为 Hook envelopes 同样携带终态。针对性复现已经确认第二个来源被丢弃。需要增加 ZCode 双源 debounce/收口协议,或只保留一个权威终态来源,并通过 InputManager + OtlpTraceFlusher 测试两种到达顺序。
| parent_span_id: p.stepSpanId, | ||
| })); | ||
| } else { | ||
| // Placeholder tool.result (1ms duration) per task #3 — paired result |
There was a problem hiding this comment.
[P1] 在没有源数据证明工具已完成时合成 tool.result 和推测时间,会产生不真实的审计语义;而 batch 最后一条 tool call 又没有 EOF/Stop/TTL 收口,可能永久留在 pending。当前传入的 ok 还会被公共规范化映射为 unknown。请让 pending 有界且可恢复,只在有真实 result 时发结果;若有明确中断证据再发 cancelled/interrupted,不要为了避免 orphan 虚构完成事件。
|
|
||
| describe('rollout writer timing vs hook fire (spec §1.5 #7 + source-evidence §10/§11)', () => { | ||
| test('probe-hook-trace.log proves rollout file existed when Stop hook fired', () => { | ||
| const traceLog = fs.readFileSync( |
There was a problem hiding this comment.
[P1] 该测试直接读取 probe-hook-trace.log,但当前 commit 没有提交这个 fixture,导致完整 npm test 稳定失败(1575 passed / 1 failed / 2 skipped)。请提交必要 fixture,或把测试改成自包含地生成输入证据。
- hooks.enabled: write true on deploy, false on undeploy - Windows PowerShell hook script (zcode-loongsuite-pilot-hook.ps1) - Cross-source correlation: derive turnId from sessionId when absent, use generateTraceId() (32-hex) for trace_id fallback - Partial JSONL: only commit offset to last complete newline - Dual-source disambiguation: add agent.source field to all entries - tool.result placeholder: status 'ok' → 'interrupted' - Add gen_ai.agent.name='ZCode' to all envelope/rollout entries - Fix ZCodeHookInput constructor crash (opts now required) - Add missing probe-hook-trace.json fixture - Update outdated comments in hook-processor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 26 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
assets/hooks/zcode-hook-processor.mjs:125
- The turnId fallback is documented as deterministic (sessionId + timestamp), but it currently hashes
new Date().toISOString(), which changes on each invocation. If Stop fires twice without turnId, this generates different turnIds, defeating the idempotency guard and making cross-source stitching less reliable. Use the hook event's timestamp (or the derivedtimestampvariable) as the stable input to the fallback.
// turnId: ZCode Stop stdin guarantees session_id but turnId is best-effort.
// When absent, derive a deterministic turn id from sessionId + timestamp
// so the AGENT envelope is still emitted for cross-source stitching.
const turnId = getString(event, 'turn_id', 'turnId')
|| deriveSpanId('turn-fallback', sessionId, new Date().toISOString());
src/inputs/zcode-rollout/zcode-rollout-input.ts:531
- For interrupted/timeout cases, the PR intends to avoid mislabeling as
stop, but the current logic sets finishReason to'unknown'when the field is missing. Downstream validation often expects a terminal reason like'interrupted'plus 0-usage placeholders when the record hascompletedAtbut no finishReason. Consider treating(completedAt && !finishReason)as'interrupted'and defaulting missing usage values to 0 in that case.
// or times out, response.finishReason may be null/empty. Defaulting to 'stop'
// incorrectly marks interrupted sessions as normal completions and causes
// the flusher to treat this as a terminal signal, dropping late hook entries.
// Use 'unknown' as neutral default; the hook path provides the real finish_reason.
const finishReason = str(response.finishReason) ?? str(response.finish_reason) ?? 'unknown';
| const hooks = (existing.hooks as Record<string, unknown> | undefined) ?? {}; | ||
| if (hooks.enabled === true) return; // already enabled | ||
|
|
||
| hooks.enabled = true; | ||
| existing.hooks = hooks; |
- Add zcode/zcode-hook/zcode-rollout to AGENT_SYSTEM_MAP so resource attributes resolve to 'zcode' instead of 'unknown' - Add eventsRoot to PluginCheckTarget interface so watchdog can check hooks.events.<event> nested path used by ZCode - Pass eventsRoot from agent definition to watchdog target - Update findMissingHooks to navigate eventsRoot before checking events
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 28 changed files in this pull request and generated no new comments.
Suppressed comments (4)
assets/hooks/zcode-hook-processor.mjs:123
- turnId fallback is derived from
new Date().toISOString()rather than the hook event’s owntimestamp. This makes the “deterministic from sessionId + timestamp” contract false, can break idempotency (same Stop event emits different turnId), and prevents cross-source stitching when turnId is missing.
// When absent, derive a deterministic turn id from sessionId + timestamp
// so the AGENT envelope is still emitted for cross-source stitching.
const turnId = getString(event, 'turn_id', 'turnId')
|| deriveSpanId('turn-fallback', sessionId, new Date().toISOString());
src/inputs/zcode-rollout/zcode-rollout-input.ts:122
prevOffsetis read but never used, which adds noise and can mask future logic mistakes in offset handling.
const prevOffset = this.stateStore.getOffset(stateKey);
src/inputs/zcode-rollout/zcode-rollout-input.ts:519
- PR description says interrupted sessions (finishReason null/empty with completedAt present) should be normalized as
finish_reasons=['interrupted'], but the current code defaults to'unknown'. That makes interrupted-path behavior diverge from the stated contract and may reintroduce validator errors for S3-like cases.
// P1 fix: do NOT default finishReason to 'stop' — when ZCode is interrupted
// or times out, response.finishReason may be null/empty. Defaulting to 'stop'
// incorrectly marks interrupted sessions as normal completions and causes
// the flusher to treat this as a terminal signal, dropping late hook entries.
// Use 'unknown' as neutral default; the hook path provides the real finish_reason.
const finishReason = str(response.finishReason) ?? str(response.finish_reason) ?? 'unknown';
const responseId = str(response.responseId) ?? str(response.response_id) ?? requestId;
src/inputs/zcode-rollout/zcode-rollout-input.ts:489
- PR description mentions merging additional system instructions (e.g.
request.body.systemand<system-reminder>blocks embedded in user messages) intogen_ai.system_instructions, but the implementation currently only extracts role="system" messages. If downstream validation/UI depends on the merged system text, this is a functional gap vs the PR’s stated behavior.
// P1 fix: extract system_instructions (role=="system" messages) and
// tool_definitions (request.toolNames — names only, no
// description/parameters; ZCode rollout does not log full tool schemas,
// so we emit name-only definitions and document the gap as P2 follow-up
// for downstream consumers needing full JSON schemas).
const systemInstructions: JsonValue[] = (rawInputMessages as unknown[])
.map((m) => normalizeInputMessage(m))
.filter(Boolean)
.filter((m) => m!.role === 'system')
.map((m) => ({ type: 'text', content: String(m!.content ?? '') })) as JsonValue[];
const toolNamesRaw = Array.isArray(request.toolNames) ? request.toolNames : [];
ralf0131
left a comment
There was a problem hiding this comment.
Summary
Re-review after 2 new commits pushed since last approval (6aef289f → b18edb3c).
Incremental changes reviewed:
- Hook processor resilience — turnId fallback (
deriveSpanIdfrom sessionId + timestamp) and traceId fallback (generateTraceId()) ensure AGENT envelope is always emitted. Good defensive coding. - Windows PowerShell hook (
zcode-loongsuite-pilot-hook.ps1) — mirrors the bash hook, handles stdin pipe and named-pipe patterns for Windows. - Hook watchdog
eventsRoot— allows ZCode to nest hooks underhooks.events.<event>instead of flat<event>. Backward-compatible default. - Dual-source terminal suppression —
agent.source: zcode-hooktag prevents rollout input from emitting duplicate AGENT spans when hook-originated envelope exists. generateTraceId()consistency — replacedcrypto.randomUUID()with shared utility for W3C-compliant 32-hex format.
All changes are additive and backward-compatible. No regressions in existing test fixtures. LGTM.
- Keep both zcode and mimo-code/hermes/openclaw client types - Keep both zcode and workbuddy in agent-system map - Keep both eventsRoot and winShell/settingsSyntax in deployment types - Keep both zcode hooks.enabled and kiroAgent deploy logic - Keep both zcode watchdog eventsRoot and enabled gate - Keep both zcode and hermes/openclaw imports and listener map entries - Reinstall deps to pick up jsonc-parser from main
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 28 changed files in this pull request and generated no new comments.
Suppressed comments (5)
src/deployment/hook-strategy.ts:706
existing.hooksmay be non-object (e.g.,false/truefrom a user config). In that case, the current cast makeshooksa boolean andhooks.enabled = truewon’t reliably update the config. Guard the type and fall back to{}.
const hooks = (existing.hooks as Record<string, unknown> | undefined) ?? {};
if (hooks.enabled === true) return; // already enabled
hooks.enabled = true;
existing.hooks = hooks;
src/inputs/zcode-rollout/zcode-rollout-input.ts:91
- The baseline-skip guard treats
lastOffset=0as “not tracking”, so a file that was intentionally reset to offset 0 (e.g., after inode rotation) will be advanced to EOF on restart and silently drop data. SinceStateStore.get()always returns an object, checkingexisting &&is also redundant here.
const existing = this.stateStore.get(stateKey);
if (existing && typeof existing.lastOffset === 'number' && existing.lastOffset > 0) {
continue; // already tracking this file
src/inputs/zcode-rollout/zcode-rollout-input.ts:244
- When this is the last line of a batch, toolCalls are buffered using
sid/tidthat default to empty strings. If a malformed/partial record is missing sessionId/turnId, this will store pending tool calls under a shared:+key and may corrupt pairing across sessions/turns.
// toolCalls to state — they'll be paired with the next batch's first
// line (or emitted as placeholders if no next batch arrives).
allEntries.push(...this.buildEntriesFromRolloutLine(record, undefined, { skipToolCalls: true }));
this.bufferPendingToolCalls(sid, tid, record);
} else {
src/inputs/zcode-rollout/zcode-rollout-input.ts:672
finishReasoncan be forced to'unknown', but that value is not a valid finish reason in the trace validator. Emitting it ingen_ai.response.finish_reasonswill cause validation errors; it’s safer to omit the attribute when the finish reason is unknown/missing.
'gen_ai.provider.name': providerId,
'gen_ai.request.model': modelId,
'gen_ai.response.model': responseModelId,
'gen_ai.response.finish_reasons': [finishReason],
...(inputTokens !== undefined ? { 'gen_ai.usage.input_tokens': inputTokens } : {}),
assets/hooks/zcode/transcript-parser.mjs:22
- This header comment claims the module is used by
assets/hooks/zcode-hook-processor.mjs, but that file no longer imports it. Keeping this inaccurate “used by” list will mislead future maintainers.
* Pure data transform — no JSONL writing here. Used by both:
* - assets/hooks/zcode-hook-processor.mjs (mjs, envelope path fallback)
* - tests/unit/hooks/zcode/*.test.mjs (vitest)
*
- turnId fallback: use event's timestamp instead of new Date() for determinism and idempotency guard - finishReason: 'unknown' → conditional 'interrupted' (when completedAt exists) or 'end_turn' (neutral default) - ensureZcodeHooksEnabled: guard against non-object hooks field - Remove unused prevOffset variable - Fix misleading comments in transcript-parser.mjs and message-converter.mjs (NOT imported by hook-processor or rollout input — reference impl only)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 28 changed files in this pull request and generated no new comments.
Suppressed comments (5)
src/inputs/zcode-rollout/zcode-rollout-input.ts:779
tool.result.statusis also set to'ok'/'error'in the in-batch tool pairing path, which normalizes'ok'tounknowninbuildAgentActivityEntry(). Successful tool results should be markedsuccess(and errors asfailure).
...(result.content !== undefined
? { 'gen_ai.tool.call.result': result.content as JsonValue }
: {}),
'tool.result.status': isError ? 'error' : 'ok',
...(isError && result.content
? { 'error.type': 'ToolError', 'error.message': String(result.content).slice(0, 500) }
: {}),
src/inputs/zcode-rollout/zcode-rollout-input.ts:807
- Same as above: placeholder tool results use
tool.result.status: 'interrupted', but status normalization will turn this intounknown. Usecancelledif you want downstream tooling to recognize it as a non-success terminal state.
// Placeholder tool.result (1ms duration) — paired result not found in
// nextRecord (nextRecord absent or no matching tool msg). Use
// 'interrupted' status to indicate this is NOT a real completion.
entries.push(buildAgentActivityEntry({
time_unix_nano: toolResultTimeNs,
'event.id': crypto.randomUUID(),
'event.name': 'tool.result',
'gen_ai.session.id': sessionId,
'gen_ai.turn.id': turnId,
'gen_ai.step.id': stepId,
'gen_ai.agent.type': ClientType.ZCode,
'gen_ai.agent.id': sessionId,
'gen_ai.agent.name': 'ZCode',
'agent.source': 'zcode-rollout',
'gen_ai.provider.name': providerId,
'gen_ai.tool.name': tc.name,
'gen_ai.tool.call.id': tc.id,
'gen_ai.tool.call.exec.id': tc.id,
'tool.result.status': 'interrupted',
trace_id: traceId,
span_id: toolSpanId,
parent_span_id: stepSpanId,
}));
src/inputs/zcode-rollout/zcode-rollout-input.ts:382
tool.result.statusis set to'ok'/'error', butbuildAgentActivityEntry()normalizes tool statuses tosuccess|failure|cancelled|unknown(so'ok'becomesunknown). This will mark successful tool results as unknown in exported traces.
This issue also appears on line 773 of the same file.
'gen_ai.tool.call.exec.id': p.callId,
...(result.content !== undefined
? { 'gen_ai.tool.call.result': result.content as JsonValue }
: {}),
'tool.result.status': isError ? 'error' : 'ok',
...(isError && result.content
? { 'error.type': 'ToolError', 'error.message': String(result.content).slice(0, 500) }
src/inputs/zcode-rollout/zcode-rollout-input.ts:410
- For placeholder tool results,
tool.result.statusis set to'interrupted', but status normalization only recognizessuccess|failure|cancelled(anything else becomesunknown). If you want this to be treated as a non-successful completion, usecancelled(orfailurewith an error type/message).
This issue also appears on line 785 of the same file.
// Placeholder tool.result (1ms duration) — paired result never arrived
// (e.g. LLM aborted before consuming tool output, or session was
// interrupted). Use 'interrupted' status to indicate this is NOT a
// real successful completion; the previous 'ok' was incorrectly
// normalized to 'unknown' and produced false audit semantics.
out.push(buildAgentActivityEntry({
time_unix_nano: resultTimeNs,
'event.id': crypto.randomUUID(),
'event.name': 'tool.result',
'gen_ai.session.id': p.sid,
'gen_ai.turn.id': p.tid,
'gen_ai.step.id': p.stepId,
'gen_ai.agent.type': ClientType.ZCode,
'gen_ai.agent.id': p.sid,
'gen_ai.agent.name': 'ZCode',
'agent.source': 'zcode-rollout',
'gen_ai.provider.name': p.providerId,
'gen_ai.tool.name': p.toolName,
'gen_ai.tool.call.id': p.callId,
'gen_ai.tool.call.exec.id': p.callId,
'tool.result.status': 'interrupted',
trace_id: p.traceId,
span_id: toolSpanId,
parent_span_id: p.stepSpanId,
}));
src/inputs/zcode-rollout/zcode-rollout-input.ts:240
- If a batch ends with a line that has toolCalls, the code emits no
tool.call/tool.resultrecords at all (skipToolCalls: true) and relies on a future batch to flush pending tool calls. When the session/turn ends immediately after that line (no further rollout lines), those tool spans will never be emitted, which can breaksemantic.tool_matches_llm_output(tool_call parts present ingen_ai.output.messages, but no TOOL spans).
if (isLastInBatch && this.lineHasToolCalls(record)) {
// Last line of batch with toolCalls: emit STEP+LLM but buffer the
// toolCalls to state — they'll be paired with the next batch's first
// line (or emitted as placeholders if no next batch arrives).
allEntries.push(...this.buildEntriesFromRolloutLine(record, undefined, { skipToolCalls: true }));
this.bufferPendingToolCalls(sid, tid, record);
} else {
ralf0131
left a comment
There was a problem hiding this comment.
Summary
Re-review after new commits since last approval (HEAD changed from b18edb3 → ab2add6). ZCode CLI probe with dual-input architecture (hook JSONL + rollout transcript), following the V3 hybrid pattern.
Changes since last review:
- ✅
event-emitter.mjs: AddedtoW3CTraceId()andderiveSpanId()— deterministic cross-source span_id derivation ensures hook ENTRY/AGENT envelopes and rollout STEP records stitch correctly under a single trace_id - ✅
hook-watchdog.ts: AddedeventsRootsupport for agents with strict hook schemas (ZCode useshooks.events.Stopinstead ofhooks.Stop) — clean extension, backward-compatible - ✅
hook-strategy.ts: ZCode-specifichooks.enabled=truemanagement on deploy/undeploy — preserves existing user config, only writes when absent or false - ✅ Windows PowerShell hook script (
.ps1) added alongside bash — consistent with other agent probes - ✅
zcode-hook-input.ts: Clean delegation totransformHookRecord— consistent with QwenCodeCli/Codex patterns
Overall: Code quality remains high. The dual-input architecture is well-implemented with proper cross-source correlation (trace_id + session_id + turn_id + derived span_id). Fail-open design in hook scripts is correct. Test coverage is comprehensive with multiple fixture scenarios.
Minor observation (non-blocking):
- The
zcode-rollout-input.tsat ~880 lines is substantial. Similar to the MiniMax Code rollout input, consider extracting normalization helpers in a future cleanup.
LGTM — approving.
Automated review by github-manager-bot
- baseline-skip: use extra.initialized sentinel flag instead of lastOffset > 0 check, so files intentionally reset to offset 0 (inode rotation) are not skipped to EOF on restart - bufferPendingToolCalls: guard against empty sid/tid to prevent malformed/partial records from sharing a ':+' state key and corrupting tool pairing across sessions/turns
CI test installer-uninstall-cleanup checks that all hook-mode agents' config files are cleaned on uninstall. ZCode uses ~/.zcode/cli/config.json which was missing from both installer scripts and the test's HOOK_CONFIG_FILES list. - deploy/installer-opensource.sh: add $HOME/.zcode/cli/config.json - deploy/installer-opensource.ps1: add .zcode\cli\config.json - test: add .zcode/cli/config.json to HOOK_CONFIG_FILES
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 31 changed files in this pull request and generated 2 comments.
Suppressed comments (4)
tests/unit/hooks/zcode/rollout-input.test.ts:1
- This test relies on a fixed sleep to wait for start-cycle effects, which can be flaky under CI load. Prefer making the test deterministic by awaiting a completion signal (if
start()resolves only afteronStart()is done, the sleep can be removed) or directly invoking the logic under test in a controlled way (e.g., calling the method that performs baseline initialization).
src/inputs/zcode-rollout/zcode-rollout-input.ts:152 - This allocates a buffer for the entire unread tail (
stat.size - offset) and reads it in one shot. If the poll interval is long or the rollout file grows quickly, this can cause large transient allocations and GC pressure. Consider reading in bounded chunks and incrementally scanning for newlines / parsing JSONL lines (streaming approach) so memory usage stays predictable.
const buf = Buffer.alloc(stat.size - offset);
await handle.read(buf, 0, buf.length, offset);
const text = buf.toString('utf-8');
assets/hooks/zcode-loongsuite-pilot-hook.ps1:42
- The log timestamp appends a literal
ZbutGet-Dateis local time unless explicitly converted to UTC, which can produce misleading timestamps. Also, JSON escaping here only handles backslash/quote and can emit invalid JSON if the message contains newlines, tabs, or other control characters. Use UTC time (e.g.,[DateTime]::UtcNow) and a proper JSON serializer/escape routine for the message field to guarantee valid JSONL.
$Day = (Get-Date -Format "yyyy-MM-dd")
assets/hooks/zcode-loongsuite-pilot-hook.ps1:42
- The log timestamp appends a literal
ZbutGet-Dateis local time unless explicitly converted to UTC, which can produce misleading timestamps. Also, JSON escaping here only handles backslash/quote and can emit invalid JSON if the message contains newlines, tabs, or other control characters. Use UTC time (e.g.,[DateTime]::UtcNow) and a proper JSON serializer/escape routine for the message field to guarantee valid JSONL.
$Timestamp = (Get-Date -Format "yyyy-MM-ddTHH:mm:ssZ")
$EscapedMsg = $Message.Replace('\', '\\').Replace('"', '\"')
$Line = "{`"time`":`"$Timestamp`",`"gen_ai.agent.type`":`"zcode`",`"stage`":`"$Stage`",`"error.type`":`"shell_$Stage`",`"error.message`":`"$EscapedMsg`"}"
| // file may disappear — skip silently | ||
| } | ||
| } | ||
| } |
| // Parse only complete lines (up to and including the last \n). | ||
| const completeText = lastNewline >= 0 ? text.slice(0, lastNewline + 1) : ''; | ||
| const lines: Record<string, unknown>[] = []; | ||
| for (const line of completeText.split('\n')) { |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 31 changed files in this pull request and generated 3 comments.
Suppressed comments (4)
tests/unit/hooks/zcode/rollout-input.test.ts:1
- The
setTimeout(50)afterawait input.start()makes this test timing-dependent and potentially flaky under load/CI. Ifstart()already awaitsonStart(), the sleep is unnecessary; if there’s additional async initialization, consider exposing/awaiting a deterministic signal (e.g., a resolved promise or explicit method) rather than sleeping.
assets/hooks/zcode-hook-processor.mjs:186 - The envelope records set
user.id, but the trace rules indocs/trace-validation-rules.jsonrequiregen_ai.user.idas a common mandatory attribute. If the downstream transform doesn’t remap this field, traces will fail validation / lose user attribution. Emitgen_ai.user.id(or emit both if you must preserve backward compatibility).
const baseFields = {
trace_id: w3cTraceId,
'gen_ai.session.id': sessionId,
'gen_ai.agent.type': AGENT_ID,
'gen_ai.agent.id': sessionId,
'gen_ai.agent.name': 'ZCode',
'user.id': userId || os.hostname(),
...(cwd ? { 'agent.zcode.cwd': cwd } : {}),
};
assets/hooks/zcode/transcript-parser.mjs:62
- This “reference implementation” defaults missing
finishReasonto'stop', but the TS rollout input explicitly avoids that and treats missing finishReason as'interrupted'/'end_turn'. Since this file is presented as a reference spec, the divergence is misleading—either update it to match the current normalization logic, or clarify in the module docstring that it intentionally differs from production behavior.
const finishReason = str(response.finishReason) || str(response.finish_reason) || 'stop';
src/inputs/zcode-rollout/zcode-rollout-input.ts:152
Buffer.alloc(stat.size - offset)reads the entire unread tail into memory in one shot. For large rollout files (or long poll intervals), this can cause large transient allocations and GC pressure. Consider streaming reads in chunks (or using a readline/stream parser) while still committing offsets only up to the last complete newline.
const buf = Buffer.alloc(stat.size - offset);
await handle.read(buf, 0, buf.length, offset);
const text = buf.toString('utf-8');
| try { | ||
| const stat = await fs.promises.stat(filePath); | ||
| this.stateStore.setOffset(stateKey, stat.size); | ||
| this.stateStore.update(stateKey, { extra: { inode: Number((stat as any).ino), initialized: true } }); |
| if (committedBytes > offset) { | ||
| this.stateStore.setOffset(stateKey, committedBytes); | ||
| } | ||
| this.stateStore.update(stateKey, { extra: { inode: Number((stat as any).ino) } }); |
| "$HOME/.codex/hooks.json" | ||
| "$HOME/.qwen/settings.json" | ||
| "$HOME/.workbuddy/settings.json" | ||
| "$HOME/.zcode/cli/config.json" |
ralf0131
left a comment
There was a problem hiding this comment.
Summary
Great work implementing the ZCode probe with the dual-input pattern (hook + rollout). The implementation is well-structured, follows established patterns, and includes comprehensive test coverage.
Highlights
- Clean dual-input architecture: Hook handles lifecycle events (Stop), rollout provides per-LLM telemetry — proper separation of concerns
- Cross-source parent linking: Consistent trace_id + gen_ai.session.id + gen_ai.turn.id linking between hook and rollout inputs
- Shared deriveSpanId(): AGENT.span_id and STEP.parent_span_id derived consistently across processes
- Interrupted-path handling: Round 2 fixes properly handle interrupted sessions with synthetic tool entries
- Comprehensive test fixtures: Good coverage of edge cases (cross-batch, parallel, multi-line, orphan tools)
⚠️ Action Required: Merge Conflicts
This PR currently has merge conflicts with main. Please rebase and resolve conflicts before merge:
git fetch origin
git checkout <your-branch>
git rebase origin/main
# resolve conflicts, then:
git push --force-with-leaseMinor Notes
- The
eventsRoot: "events"inagents.d/zcode.jsonis a good addition for agents that batch multiple events in a single hook payload - Consider documenting the
eventsRootfield indocs/agent-onboarding.mdfor future probe authors
LGTM — ready to merge once conflicts are resolved and CI passes.
Automated review by github-manager-bot
- Take upstream's dynamic HOOK_CONFIG_FILES derivation (auto-discovers agents.d/*.json instead of hardcoded list) - Rewrite zcode-loongsuite-pilot-hook.ps1 to pass CI ps1 tests: * CLM-safe: use [Console]::IsInputRedirected instead of [System.Console]::In * ASCII-only comments: replace em-dash with -- * UTF-8 encoding: add -Encoding UTF8 to Add-Content * CLM-safe stdin passthrough: let node inherit fd0 directly
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 30 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/inputs/zcode-rollout/zcode-rollout-input.ts:532
response.finishReasonvalues observed in fixtures/tests includetool_useandtool-calls, butscripts/validate-trace.mjsonly acceptstool_call/tool_calls. As-is, traces from ZCode can fail validation (and downstream expectations) because we emit non-canonical finish reasons.
const rawFinishReason = str(response.finishReason) ?? str(response.finish_reason);
const hasCompletedAt = !!(str(record.completedAt) ?? str(record.completed_at));
const finishReason = rawFinishReason
?? (hasCompletedAt ? 'interrupted' : 'end_turn');
const responseId = str(response.responseId) ?? str(response.response_id) ?? requestId;
src/deployment/hook-strategy.ts:384
- On undeploy, ZCode hook cleanup only sets
hooks.enabled=falsewhenexisting.hooksis already an object. If a user config hashooks: true/false(or any non-object), this will skip the disable step and leave hooks enabled unexpectedly. This is inconsistent withensureZcodeHooksEnabled, which explicitly guards and normalizes non-objecthooks.
// Disable ZCode hooks on undeploy so the agent stops firing hooks.
if (def.id === 'zcode' && def.hook?.settingsPath) {
try {
const resolvedPath = resolveHome(def.hook.settingsPath);
const existing = await readJsonFile<Record<string, unknown>>(resolvedPath);
if (existing?.hooks && typeof existing.hooks === 'object') {
(existing.hooks as Record<string, unknown>).enabled = false;
await writeJsonFile(resolvedPath, existing);
logger.info('zcode hooks.enabled set to false on undeploy', { settingsPath: resolvedPath });
}
src/inputs/zcode-rollout/zcode-rollout-input.ts:249
- The comment says buffered last-in-batch toolCalls will be "emitted as placeholders if no next batch arrives", but there is no code path that flushes
zcode-rollout:pending-tool-calls:<sid>+<tid>unless anothermodel_ioline for the same (sid, tid) later arrives. If a turn ends/interrupted immediately after emitting toolCalls (no subsequentmodel_io), tool.call/tool.result will never be emitted and the pending state can accumulate indefinitely.
if (isLastInBatch && this.lineHasToolCalls(record)) {
// Last line of batch with toolCalls: emit STEP+LLM but buffer the
// toolCalls to state — they'll be paired with the next batch's first
// line (or emitted as placeholders if no next batch arrives).
allEntries.push(...this.buildEntriesFromRolloutLine(record, undefined, { skipToolCalls: true }));
// Guard: only buffer when sid+tid are valid. Malformed/partial records
// with empty sid/tid would otherwise share a single `:+` state key,
// corrupting pairing across sessions/turns.
if (sid && tid) {
this.bufferPendingToolCalls(sid, tid, record);
}
ralf0131
left a comment
There was a problem hiding this comment.
Summary
This PR adds ZCode CLI probe support with the V3 hybrid collection pattern (hook envelope + rollout data source), building on the original PR #101 foundation. The implementation includes Step C fixes for data schema, flush timing, and error handling, plus Round 2 interrupted-path injection.
Key observations:
- Clean separation: hook processor emits ENTRY/AGENT envelopes only, rollout input handles LLM/STEP/TOOL data — cross-source stitching via shared
deriveSpanId()ensures deterministic parent-child linking - Proper baseline-skip sentinel handling for rollout file byte offset initialization
- Fail-open contract maintained (hook errors → stdout
{}+ exit 0) - Comprehensive test coverage including cross-batch react scenarios and parallel tool calls
- New commit
6d57b0eresolves merge conflicts with upstream main and fixes ps1 CI tests
Note: No CI workflow runs triggered on latest commit — may need maintainer approval for first-time contributor.
LGTM ✅
Automated review by github-manager-bot
fangxiu-wf
left a comment
There was a problem hiding this comment.
Re-reviewed at 6d57b0e9d3d5beba20f6cb55370db77a706537dd against current main (04beee97881e5d12f77ba54bdd191085d59df756). The merge-conflict, Windows stdin/runtime, partial-line checkpoint, offset sentinel, and malformed-ID issues from earlier revisions are fixed. I still see four merge-blocking issues:
-
Hook/rollout correlation can split a turn or drop the rollout data. ZCode's documented Stop payload guarantees the common hook fields plus
stop_hook_activeandlast_assistant_message; it does not guaranteeturnId,traceId, ortimestamp(https://zcode.z.ai/en/docs/hooks).cmdStop()therefore falls back to a collection-time timestamp-derived turn ID and a random trace ID, which cannot be correlated with the rollout record's native IDs. If the IDs do happen to match and the Stop hook is processed first, its terminal envelope flushes the turn andflushedTurnKeysdrops the later rolloutllm.request/llm.response. A focused reproducer on this head emitted only the Hook records in that arrival order. Please make rollout/transcript evidence authoritative for native IDs and terminal state, and cover Hook-first, rollout-first, and missing-ID cases. -
A final/interrupted tool call can remain pending forever, and the fallback invents a result. A last-in-batch
model_iowithtoolCallsis stored underpending-tool-calls:<sid>+<tid>and is flushed only when anothermodel_iofor the same turn arrives. At EOF/interruption there may be no such line, so no TOOL records are emitted and pending state is never cleared. When a later line does arrive, unmatched calls receive a fabricated 1 mstool.result, although the source never observed a result. In addition,ok/error/interruptedstatuses and rawtool_use/tool-callsfinish reasons do not match the canonical normalized values. Please add a bounded EOF/terminal drain that preserves “result not observed” truthfully, normalize the enums, and add a no-next-record regression test. -
The lifecycle changes can disable user hooks and leave Pilot hooks installed.
HookStrategy.undeploy()unconditionally writeshooks.enabled=false, which also disables unrelated user-managed ZCode hooks that remain in the file. Both installer cleanup implementations iterate only arrays directly belowhooks, so they miss ZCode'shooks.events.Stopshape and leave the Pilot command behind after uninstall; the focused cleanup reproducer left the nested command unchanged. The watchdog checkshooks.events.Stopbut not the globalhooks.enabled, so a globally disabled configuration can still be reported healthy. Please remove only Pilot-owned entries, preserve third-party/global enablement, handle the nested layout on Unix and Windows, and test deploy → disable/uninstall plus watchdog health. -
The committed fixtures contain real captured data. The ZCode fixtures add about 371 KB of raw captures containing stable session/trace identifiers, request headers, full system prompts, user tasks, and local environment paths. This conflicts with the repository's synthetic-fixture/privacy requirement. Please replace them with minimal synthetic fixtures that retain only the fields needed by the tests.
Validation on the same head/current-main combination: npm run typecheck and npm run build passed; 15 targeted test files / 247 tests passed. The full suite had 3,050 passing and one failure that reproduces on main. No real ZCode/Windows installed-product E2E was run. The blockers above are deterministic source/reproduction findings and are not closed by the green unit tests.
Issue 1 - Hook/rollout correlation + terminal race: - cmdStop resolves native turnId/traceId from the rollout transcript's last model_io record (stdin first, rollout second, derived last) - readLastRolloutRecord() tails ~/.zcode/cli/rollout fail-open - ENTRY/AGENT envelopes no longer carry finish_reasons; rollout's last llm.response is the authoritative terminal signal so hook-first arrival cannot flush the turn and drop later rollout records Issue 2 - pending tool calls + enum normalization: - drainStalePendingTurns(): a new batch on a different turn drains the old turn's buffered tool.calls WITHOUT fabricating tool.results - bounded pending registry (MAX_PENDING_TURNS=32) evicts oldest entries - removed all fabricated 1ms placeholder tool.result branches - canonical statuses: ok/error -> success/failure - normalizeFinishReason(): tool_use/tool-calls -> tool_call etc. - validator now accepts interrupted/cancelled finish reasons Issue 3 - lifecycle correctness: - undeploy only sets hooks.enabled=false when no user-managed hook entries remain in the config - installer cleanup (sh + ps1) recurses into hooks.events.<Event> nested layout so zcode's Pilot command is actually removed - watchdog reports unhealthy when global hooks.enabled is off (requiresHooksEnabled on the zcode target) Issue 4 - fixture privacy: - replaced 366KB of real captured rollout data with 13KB synthetic fixtures (generator: scripts/generate-zcode-synthetic-fixtures.py) - no real prompts/headers/paths/session environments remain Regression tests added: native-ID resolution from rollout transcript, no-finish_reason envelopes, no-next-record drain, canonical enums, zcode deploy fresh/false/already-true + undeploy preserve/disable, watchdog nested-layout + enabled-gate + wrong-layout detection.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 34 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/inputs/zcode-rollout/zcode-rollout-input.ts:255
- When the last record in a batch has toolCalls, this code calls buildEntriesFromRolloutLine() and bufferPendingToolCalls() separately. If requestId is missing, both functions currently generate a different random UUID, causing STEP/LLM records to use a different gen_ai.step.id/span_id than the buffered tool.call records (breaking parent linkage and later pairing). Seed a single requestId onto the record before both calls so they stay consistent.
if (isLastInBatch && this.lineHasToolCalls(record)) {
// Last line of batch with toolCalls: emit STEP+LLM but buffer the
// toolCalls to state — they'll be paired with the next batch's first
// line (or drained without results if the turn ends here).
allEntries.push(...this.buildEntriesFromRolloutLine(record, undefined, { skipToolCalls: true }));
src/inputs/zcode-rollout/zcode-rollout-input.ts:401
- The JSDoc for flushPendingToolCalls says unmatched pending toolCalls emit a placeholder tool.result, but the implementation intentionally does not fabricate results (it only emits tool.result when an observed role=tool message exists). This mismatch is confusing and makes it harder to reason about where orphan synthesis happens.
/**
* Pair pending toolCalls (buffered in previous batches) with the current
* line's request.messages[role=tool]. Emits tool.call + tool.result records
* for matched pairs. Unmatched pending toolCalls are emitted as
* tool.call + 1ms placeholder tool.result (EOF/abort fallback per task #3).
ralf0131
left a comment
There was a problem hiding this comment.
Summary
Adds ZCode CLI probe integration with hook and rollout inputs, based on original PR #101. Includes Step C fixes for data schema, flush timing, and error handling, plus Round 2 fixes for interrupted-path scenarios.
Highlights
- Comprehensive probe implementation: Hook processor + rollout JSONL input + transcript parser
- Solid error handling: Interrupted session detection, orphan tool synthesis, graceful degradation
- Good test coverage: Unit tests for hook helpers, hook-strategy, and hook-watchdog
- Clean integration: Proper orchestrator wiring and agent definition
Observations
- The trace-validation-rules.json (806 lines) provides structured validation rules — good for automated testing
- The state.mjs and message-converter.mjs modules are well-factored helpers
LGTM — well-executed follow-up to PR #101!
Automated review by github-manager-bot
rangemer333-cell
left a comment
There was a problem hiding this comment.
整体看下来这个 PR 结构清晰、测试覆盖很扎实(baseline skip / 跨 batch 配对 / state-loss 回归 / watchdog 嵌套 schema 都有覆盖),hook 路径 envelope-only + rollout 独立数据源的设计和跨进程 deriveSpanId 共享公式的契约也讲得很清楚。👍
以下是几个建议关注的问题,按严重程度排序:
P1: 并发会话下 pending tool-call 会被跨文件误排空(zcode-rollout-input.ts)
drainStalePendingTurns 使用全局 registry(zcode-rollout:pending-turn-registry),但 collect() 是逐 rollout 文件独立处理的,currentTurnKey 只来自当前文件当前 batch 的首行:
allEntries.push(...this.drainStalePendingTurns(fsid ? `${fsid}+${ftid ?? ''}` : ''));场景:两个 zcode 会话 A、B 并发运行。B 的 batch 末行带 toolCalls 被 buffer 进 pending;下一个 poll 周期先处理 A 的文件(文件名排序),drainStalePendingTurns('A+turnA') 会把 B 的 pending 全部排空(按"无结果"路径直接发出 tool.call)。随后 B 的文件被处理,其首行 request.messages[role=tool] 里真实存在的 tool 结果因为 pending 已清空而永久丢失 tool.result 配对。
建议:drain 时只处理与当前文件同 session(比较 sid)的 stale turn,跨 session 的 pending 留给该 session 自己的文件推进或 idle 超时来关闭。
另外一个小边界:首行 sid 缺失时 currentTurnKey 为 '',会把所有 pending(包括本 turn 的)全部排空,建议 sid 为空时直接跳过 drain。
P2: cmdStop 幂等状态在写盘前保存(zcode-hook-processor.mjs)
state.last_exported_turn = turnId || state.last_exported_turn;
saveState(sessionId, state);
// ...之后才 writeJsonlRecords(...)如果 writeJsonlRecords 抛错(磁盘满/权限),fail-open 的 catch 会吞掉异常,但该 turn 已被标记为 exported——zcode 重试 Stop 时会被幂等守卫静默去重,envelope 永远不会补发。建议把 saveState 挪到写盘成功之后。
P2: def.id === 'zcode' 硬编码散落三处
orchestrator.ts:requiresHooksEnabled: def.id === 'zcode'hook-strategy.tsdeploy:if (def.id === 'zcode') ensureZcodeHooksEnabled(...)hook-strategy.tsundeploy:if (def.id === 'zcode' && ...)
这个 PR 已经把 eventsRoot 做成了 AgentHookConfig 的声明式字段,建议 hooks.enabled 开关管理也同样声明化(例如 hook.requiresEnabledFlag: true 写进 agents.d/zcode.json),后续接入同类 agent 时就不用再改这三处代码。
P2/疑问: 取消忽略并提交生成物 docs/trace-validation-rules.json
.gitignore 里被删除的注释写着 "not committed; CI regenerates",但 CI workflow 里并没有再生成步骤,提交后 validate-trace.mjs 的默认路径确实能开箱即用——这个方向可以理解。不过有两个点想确认:
- 文件内嵌
generatedAt时间戳,每次重新生成都会产生无意义 diff,建议生成脚本去掉或固定该字段; specSource指向内部地址code.alibaba-inc.com/arms/semantic-conventions/...,提交到开源仓库是否符合预期?
P3: src → assets 的 .mjs 跨目录导入
zcode-rollout-input.ts 通过 @ts-expect-error 导入 assets/hooks/shared/event-emitter.mjs。"派生公式只有一份"这个目标完全正确(注释里架构师的提醒也说得很清楚),esbuild 打包也没问题,但目前完全丢失了类型检查。建议给 event-emitter.mjs 补一个 .d.mts 声明文件,把 toW3CTraceId / deriveSpanId 的签名契约固化下来。
P3: 自认的死代码 zcode/message-converter.mjs 与 zcode/transcript-parser.mjs
两个文件的头注释都写明 "NOT currently imported by zcode-hook-processor.mjs or ZCodeRolloutInput",约 200 行"参考实现"随产物一起分发但无人调用,后续与真实实现漂移的风险不小。建议要么让 hook-processor / RolloutInput 真正复用它们,要么移到 tests/ 或文档里,不要留在 assets 发布路径下。
Nit
readLastRolloutRecord取"最后一条 model_io"来解析 native ID:如果用户在 Stop hook 触发前的瞬间开启了新 turn,envelope 可能挂到错误的 turn 上。概率低(rollout 写入先于 runStopHooks),但值得在注释里标注这个已知竞态。
测试和文档质量都很高,以上问题解决后我觉得就可以合入了。
This PR adds ZCode CLI probe integration with hook and rollout inputs. It includes Step C fixes for data schema, flush timing, and error handling, plus Round 2 fixes for interrupted-path scenarios.
Key changes:
Based on original PR #101 by @coolLiu