diff --git a/.opencode/plugins/sce-agent-trace.ts b/.opencode/plugins/sce-agent-trace.ts index 980ceb06c..c57c3e603 100644 --- a/.opencode/plugins/sce-agent-trace.ts +++ b/.opencode/plugins/sce-agent-trace.ts @@ -5,6 +5,7 @@ type OpenCodeEvent = Parameters>[0]["event"]; const SCE_INSTALL_URL = "https://sce.crocoder.dev/docs/getting-started#install-cli"; +const TOOL_NAME = "opencode" as const; const REQUIRED_EVENTS: Set = new Set([ "message.updated", @@ -48,6 +49,7 @@ type ConversationTraceItem = | ConversationTraceMessagePartUpdatedItem; type ConversationTracePayload = { + tool_name: typeof TOOL_NAME; payloads: ConversationTraceItem[]; }; @@ -166,6 +168,7 @@ function buildConversationTracePayload( const eventInfo = event.properties.info; return { + tool_name: TOOL_NAME, payloads: [ { type: "message", @@ -182,6 +185,7 @@ export function buildMessagePartConversationTracePayload( eventPart: EventAllowedPart, ): ConversationTracePayload { return { + tool_name: TOOL_NAME, payloads: [ { type: "message.part", @@ -205,6 +209,7 @@ function buildQuestionToolConversationTracePayload( } return { + tool_name: TOOL_NAME, payloads: [ { type: "message.part", @@ -314,7 +319,7 @@ async function buildTrace( await runDiffTraceHook(repoRoot, { ...diffTracePayload, - tool_name: "opencode", + tool_name: TOOL_NAME, tool_version: clientVersion, }); } diff --git a/.pi/extensions/sce/index.ts b/.pi/extensions/sce/index.ts index 25770ff88..cbd2441ac 100644 --- a/.pi/extensions/sce/index.ts +++ b/.pi/extensions/sce/index.ts @@ -26,6 +26,7 @@ interface JsonPolicyResult { const SCE_INSTALL_URL = "https://sce.crocoder.dev/docs/getting-started#install-cli"; +const TOOL_NAME = "pi" as const; type ConversationTraceMessageItem = { type: "message"; @@ -49,6 +50,7 @@ type ConversationTraceItem = | ConversationTraceMessagePartItem; type ConversationTracePayload = { + tool_name: typeof TOOL_NAME; payloads: ConversationTraceItem[]; }; @@ -57,7 +59,7 @@ type DiffTracePayload = { diff: string; time: number; model_id: string | null; - tool_name: "pi"; + tool_name: typeof TOOL_NAME; tool_version: string | null; }; @@ -198,7 +200,7 @@ function buildMessageEndConversationTracePayload( }); } - return { payloads }; + return { tool_name: TOOL_NAME, payloads }; } /** @@ -409,6 +411,7 @@ export default function sceExtension(pi: ExtensionAPI): void { const patchMessageId = `${event.toolCallId}-patch`; void runConversationTraceHook(ctx.cwd, { + tool_name: TOOL_NAME, payloads: [ { type: "message", @@ -433,7 +436,7 @@ export default function sceExtension(pi: ExtensionAPI): void { diff, time: generatedAtUnixMs, model_id: ctx.model ? `${ctx.model.provider}/${ctx.model.id}` : null, - tool_name: "pi", + tool_name: TOOL_NAME, tool_version: await piToolVersionPromise, }); }); diff --git a/cli/src/services/hooks/mod.rs b/cli/src/services/hooks/mod.rs index eb2914e0e..3f916bbd3 100644 --- a/cli/src/services/hooks/mod.rs +++ b/cli/src/services/hooks/mod.rs @@ -44,9 +44,18 @@ pub(crate) const DIFF_TRACE_PI_SESSION_ID_PREFIX: &str = "pi_"; const OPENCODE_TOOL_NAME: &str = "opencode"; const CLAUDE_TOOL_NAME: &str = "claude"; const PI_TOOL_NAME: &str = "pi"; +const NORMALIZED_CONVERSATION_TRACE_TOOL_NAMES: &[&str] = &[OPENCODE_TOOL_NAME, PI_TOOL_NAME]; type PayloadValidationError = fn(&str) -> String; pub(crate) fn prefixed_diff_trace_session_id(tool_name: &str, raw_session_id: &str) -> String { + prefixed_session_id(tool_name, raw_session_id) +} + +fn prefixed_conversation_trace_session_id(tool_name: &str, raw_session_id: &str) -> String { + prefixed_session_id(tool_name, raw_session_id) +} + +fn prefixed_session_id(tool_name: &str, raw_session_id: &str) -> String { let prefix = match tool_name { OPENCODE_TOOL_NAME => DIFF_TRACE_OPENCODE_SESSION_ID_PREFIX, CLAUDE_TOOL_NAME => DIFF_TRACE_CLAUDE_SESSION_ID_PREFIX, @@ -129,12 +138,14 @@ pub struct ConversationTracePayload { pub struct ConversationTraceMessageBatch { pub inserts: Vec, pub skipped: Vec, + diagnostic_session_id: Option, } #[derive(Clone, Debug, Eq, PartialEq)] pub struct ConversationTracePartBatch { pub inserts: Vec, pub skipped: Vec, + diagnostic_session_id: Option, } #[derive(Clone, Debug, Eq, PartialEq)] @@ -365,10 +376,7 @@ where log_skipped_conversation_trace_payloads(logger, EVENT_TYPE, &batch.skipped); let valid_count = batch.inserts.len(); - let session_id = batch - .inserts - .first() - .map(|insert| insert.session_id.clone()); + let session_id = batch.diagnostic_session_id; let persisted = if valid_count == 0 { 0 } else { @@ -408,10 +416,7 @@ where log_skipped_conversation_trace_payloads(logger, EVENT_TYPE, &batch.skipped); let valid_count = batch.inserts.len(); - let session_id = batch - .inserts - .first() - .map(|insert| insert.session_id.clone()); + let session_id = batch.diagnostic_session_id; let persisted = if valid_count == 0 { 0 } else { @@ -502,12 +507,19 @@ pub fn parse_conversation_trace_payload(stdin_payload: &str) -> Result) -> Result<&Vec> { @@ -520,12 +532,17 @@ fn required_payloads_array(payload: &serde_json::Map) -> Result<& }) } -fn parse_conversation_trace_payloads(payloads: &[Value]) -> ConversationTracePayload { +fn parse_conversation_trace_payloads( + payloads: &[Value], + tool_name: &str, +) -> ConversationTracePayload { let mut message_inserts = Vec::new(); let mut message_skipped = Vec::new(); let mut part_inserts = Vec::new(); let mut part_skipped = Vec::new(); let mut skipped = Vec::new(); + let mut message_diagnostic_session_id = None; + let mut part_diagnostic_session_id = None; for (index, item) in payloads.iter().enumerate() { let session_id = non_empty_string(item.get("session_id")).map(str::to_owned); @@ -548,7 +565,14 @@ fn parse_conversation_trace_payloads(payloads: &[Value]) -> ConversationTracePay match event_type.as_str() { CONVERSATION_TRACE_MESSAGE_UPDATED => match parse_message_updated_item(item) { - Ok(input) => message_inserts.push(input), + Ok(mut input) => { + if message_diagnostic_session_id.is_none() { + message_diagnostic_session_id.clone_from(&session_id); + } + input.session_id = + prefixed_conversation_trace_session_id(tool_name, &input.session_id); + message_inserts.push(input); + } Err(error) => message_skipped.push(SkippedConversationTracePayload { index, reason: error.to_string(), @@ -557,7 +581,14 @@ fn parse_conversation_trace_payloads(payloads: &[Value]) -> ConversationTracePay }, CONVERSATION_TRACE_MESSAGE_PART_UPDATED => { match parse_message_part_updated_item(item) { - Ok(input) => part_inserts.push(input), + Ok(mut input) => { + if part_diagnostic_session_id.is_none() { + part_diagnostic_session_id.clone_from(&session_id); + } + input.session_id = + prefixed_conversation_trace_session_id(tool_name, &input.session_id); + part_inserts.push(input); + } Err(error) => part_skipped.push(SkippedConversationTracePayload { index, reason: error.to_string(), @@ -580,10 +611,12 @@ fn parse_conversation_trace_payloads(payloads: &[Value]) -> ConversationTracePay message_updated: ConversationTraceMessageBatch { inserts: message_inserts, skipped: message_skipped, + diagnostic_session_id: message_diagnostic_session_id, }, message_part_updated: ConversationTracePartBatch { inserts: part_inserts, skipped: part_skipped, + diagnostic_session_id: part_diagnostic_session_id, }, skipped, } @@ -2206,6 +2239,7 @@ mod tests { ]) .to_string(); let payload = serde_json::json!({ + "tool_name": "opencode", "payloads": [ { "type": "message", @@ -2251,21 +2285,21 @@ mod tests { assert_eq!(parsed.message_updated.inserts.len(), 1); let message = &parsed.message_updated.inserts[0]; - assert_eq!(message.session_id, "session-1"); + assert_eq!(message.session_id, "oc_session-1"); assert_eq!(message.message_id, "message-1"); assert_eq!(message.role, MessageRole::Assistant); assert_eq!(message.generated_at_unix_ms, 1_800_000_000_000_i64); assert_eq!(parsed.message_part_updated.inserts.len(), 3); let reasoning_part = &parsed.message_part_updated.inserts[0]; - assert_eq!(reasoning_part.session_id, "session-1"); + assert_eq!(reasoning_part.session_id, "oc_session-1"); assert_eq!(reasoning_part.message_id, "message-1"); assert_eq!(reasoning_part.part_type, PartType::Reasoning); assert_eq!(reasoning_part.text, "thinking through validation"); assert_eq!(reasoning_part.generated_at_unix_ms, 1_800_000_000_001_i64); let patch_part = &parsed.message_part_updated.inserts[1]; - assert_eq!(patch_part.session_id, "session-1"); + assert_eq!(patch_part.session_id, "oc_session-1"); assert_eq!(patch_part.message_id, "message-1"); assert_eq!(patch_part.part_type, PartType::Patch); assert_eq!( @@ -2276,7 +2310,7 @@ mod tests { assert_eq!(patch_part.generated_at_unix_ms, 1_800_000_000_002_i64); let question_part = &parsed.message_part_updated.inserts[2]; - assert_eq!(question_part.session_id, "session-1"); + assert_eq!(question_part.session_id, "oc_session-1"); assert_eq!(question_part.message_id, "message-1"); assert_eq!(question_part.part_type, PartType::Question); assert_eq!(question_part.text, question_text); @@ -2291,6 +2325,7 @@ mod tests { }) .to_string(); let payload = serde_json::json!({ + "tool_name": "opencode", "payloads": [ { "type": "message", @@ -2378,6 +2413,107 @@ mod tests { .contains("field 'type' must be a string")); } + fn normalized_conversation_trace_message_payload(tool_name: &str, session_id: &str) -> String { + serde_json::json!({ + "tool_name": tool_name, + "payloads": [ + { + "type": "message", + "session_id": session_id, + "message_id": "message-1", + "role": "assistant", + "generated_at_unix_ms": 1_800_000_000_000_i64 + } + ] + }) + .to_string() + } + + #[test] + fn conversation_trace_normalized_payload_accepts_pi_tool_name_with_prefixed_session_id() { + let stdin_payload = normalized_conversation_trace_message_payload("pi", "session-1"); + + let parsed = parse_conversation_trace_payload(&stdin_payload) + .expect("Pi normalized conversation-trace payload should parse"); + + assert_eq!(parsed.message_updated.inserts.len(), 1); + assert_eq!(parsed.message_updated.inserts[0].session_id, "pi_session-1"); + } + + #[test] + fn conversation_trace_normalized_payload_rejects_unsupported_tool_name() { + let stdin_payload = normalized_conversation_trace_message_payload("cursor", "session-1"); + + let error = parse_conversation_trace_payload(&stdin_payload) + .expect_err("unsupported tool_name should be rejected"); + + assert!(error.to_string().contains("unsupported tool_name 'cursor'")); + assert!(error.to_string().contains("'opencode'")); + assert!(error.to_string().contains("'pi'")); + } + + #[test] + fn conversation_trace_normalized_payload_rejects_empty_tool_name() { + let stdin_payload = normalized_conversation_trace_message_payload("", "session-1"); + + let error = parse_conversation_trace_payload(&stdin_payload) + .expect_err("empty tool_name should be rejected"); + + assert!(error + .to_string() + .contains("field 'tool_name' must be a non-empty string")); + } + + #[test] + fn conversation_trace_normalized_payload_rejects_missing_tool_name() { + let stdin_payload = serde_json::json!({ + "payloads": [ + { + "type": "message", + "session_id": "session-1", + "message_id": "message-1", + "role": "assistant", + "generated_at_unix_ms": 1_800_000_000_000_i64 + } + ] + }) + .to_string(); + + let error = parse_conversation_trace_payload(&stdin_payload) + .expect_err("missing tool_name should be rejected"); + + assert!(error + .to_string() + .contains("missing required field 'tool_name'")); + } + + #[test] + fn conversation_trace_normalized_payload_keeps_already_prefixed_session_id() { + let stdin_payload = + normalized_conversation_trace_message_payload("opencode", "oc_session-1"); + + let parsed = parse_conversation_trace_payload(&stdin_payload) + .expect("already-prefixed OpenCode session ID should parse"); + + assert_eq!(parsed.message_updated.inserts[0].session_id, "oc_session-1"); + } + + #[test] + fn conversation_trace_raw_claude_event_uses_claude_identity_with_cc_prefixed_session_id() { + let stdin_payload = serde_json::json!({ + "hook_event_name": "UserPromptSubmit", + "session_id": "session-1", + "prompt": "hello" + }) + .to_string(); + + let parsed = parse_conversation_trace_payload(&stdin_payload) + .expect("raw Claude UserPromptSubmit event should parse"); + + assert_eq!(parsed.message_updated.inserts.len(), 1); + assert_eq!(parsed.message_updated.inserts[0].session_id, "cc_session-1"); + } + fn diff_trace_payload(model_id: Option<&str>, tool_version: Option<&str>) -> DiffTracePayload { diff_trace_payload_with( "claude", diff --git a/config/lib/agent-trace-plugin/opencode-sce-agent-trace-plugin.ts b/config/lib/agent-trace-plugin/opencode-sce-agent-trace-plugin.ts index 980ceb06c..c57c3e603 100644 --- a/config/lib/agent-trace-plugin/opencode-sce-agent-trace-plugin.ts +++ b/config/lib/agent-trace-plugin/opencode-sce-agent-trace-plugin.ts @@ -5,6 +5,7 @@ type OpenCodeEvent = Parameters>[0]["event"]; const SCE_INSTALL_URL = "https://sce.crocoder.dev/docs/getting-started#install-cli"; +const TOOL_NAME = "opencode" as const; const REQUIRED_EVENTS: Set = new Set([ "message.updated", @@ -48,6 +49,7 @@ type ConversationTraceItem = | ConversationTraceMessagePartUpdatedItem; type ConversationTracePayload = { + tool_name: typeof TOOL_NAME; payloads: ConversationTraceItem[]; }; @@ -166,6 +168,7 @@ function buildConversationTracePayload( const eventInfo = event.properties.info; return { + tool_name: TOOL_NAME, payloads: [ { type: "message", @@ -182,6 +185,7 @@ export function buildMessagePartConversationTracePayload( eventPart: EventAllowedPart, ): ConversationTracePayload { return { + tool_name: TOOL_NAME, payloads: [ { type: "message.part", @@ -205,6 +209,7 @@ function buildQuestionToolConversationTracePayload( } return { + tool_name: TOOL_NAME, payloads: [ { type: "message.part", @@ -314,7 +319,7 @@ async function buildTrace( await runDiffTraceHook(repoRoot, { ...diffTracePayload, - tool_name: "opencode", + tool_name: TOOL_NAME, tool_version: clientVersion, }); } diff --git a/config/lib/pi-plugin/sce-pi-extension.ts b/config/lib/pi-plugin/sce-pi-extension.ts index 25770ff88..cbd2441ac 100644 --- a/config/lib/pi-plugin/sce-pi-extension.ts +++ b/config/lib/pi-plugin/sce-pi-extension.ts @@ -26,6 +26,7 @@ interface JsonPolicyResult { const SCE_INSTALL_URL = "https://sce.crocoder.dev/docs/getting-started#install-cli"; +const TOOL_NAME = "pi" as const; type ConversationTraceMessageItem = { type: "message"; @@ -49,6 +50,7 @@ type ConversationTraceItem = | ConversationTraceMessagePartItem; type ConversationTracePayload = { + tool_name: typeof TOOL_NAME; payloads: ConversationTraceItem[]; }; @@ -57,7 +59,7 @@ type DiffTracePayload = { diff: string; time: number; model_id: string | null; - tool_name: "pi"; + tool_name: typeof TOOL_NAME; tool_version: string | null; }; @@ -198,7 +200,7 @@ function buildMessageEndConversationTracePayload( }); } - return { payloads }; + return { tool_name: TOOL_NAME, payloads }; } /** @@ -409,6 +411,7 @@ export default function sceExtension(pi: ExtensionAPI): void { const patchMessageId = `${event.toolCallId}-patch`; void runConversationTraceHook(ctx.cwd, { + tool_name: TOOL_NAME, payloads: [ { type: "message", @@ -433,7 +436,7 @@ export default function sceExtension(pi: ExtensionAPI): void { diff, time: generatedAtUnixMs, model_id: ctx.model ? `${ctx.model.provider}/${ctx.model.id}` : null, - tool_name: "pi", + tool_name: TOOL_NAME, tool_version: await piToolVersionPromise, }); }); diff --git a/context/plans/restrict-conversation-trace-tool-name.md b/context/plans/restrict-conversation-trace-tool-name.md new file mode 100644 index 000000000..73ed23acb --- /dev/null +++ b/context/plans/restrict-conversation-trace-tool-name.md @@ -0,0 +1,104 @@ +# Plan: restrict-conversation-trace-tool-name + +## Change summary + +`sce hooks conversation-trace` currently accepts any non-empty `tool_name` for +normalized (non-Claude-raw) envelopes. The shared `prefixed_session_id()` +helper in `cli/src/services/hooks/mod.rs` silently falls through to the raw, +unprefixed session ID for any `tool_name` it does not recognize, so an unknown +producer (for example `tool_name: "cursor"`) is persisted with an unprefixed +session ID instead of being rejected. This extends existing behavior: it +tightens validation for the normalized conversation-trace entry point +(`parse_conversation_trace_payload`, around `cli/src/services/hooks/mod.rs:512`) +to require `tool_name` to be one of the currently supported normalized +producers (`opencode`, `pi`), erroring with the supported values named when it +is not. Raw Claude hook events (routed via `hook_event_name`) are untouched: +they keep deriving `claude` identity internally and are not gated by the new +allow-list, since that identity never comes from untrusted normalized input. +`diff-trace` intake, which independently accepts an unrestricted `tool_name` +by design (documented in `context/sce/agent-trace-hooks-command-routing.md`), +is out of scope and is not modified. + +## Acceptance criteria + +- [x] AC1: A normalized conversation-trace envelope with `tool_name: "opencode"` persists message/part rows with `oc_`-prefixed session IDs. + - Validate: `cargo test --manifest-path cli/Cargo.toml services::hooks -- conversation_trace` (run via `nix flake check`, `checks.cli-tests`, passing) +- [x] AC2: A normalized conversation-trace envelope with `tool_name: "pi"` persists message/part rows with `pi_`-prefixed session IDs. + - Validate: `cargo test --manifest-path cli/Cargo.toml services::hooks -- conversation_trace` (run via `nix flake check`, `checks.cli-tests`, passing) +- [x] AC3: A normalized conversation-trace envelope with an unsupported `tool_name` (for example `"cursor"`) is rejected with a conversation-trace validation error naming the supported producer set, and no row is persisted with an unprefixed session ID. + - Validate: `cargo test --manifest-path cli/Cargo.toml services::hooks -- conversation_trace` (run via `nix flake check`, `checks.cli-tests`, passing) +- [x] AC4: A normalized conversation-trace envelope with an empty or missing `tool_name` is still rejected (no backward-compatibility fallback). + - Validate: `cargo test --manifest-path cli/Cargo.toml services::hooks -- conversation_trace` (run via `nix flake check`, `checks.cli-tests`, passing) +- [x] AC5: A raw Claude conversation event (`hook_event_name` present) still derives `claude` identity internally, unaffected by the new allow-list, and persists with `cc_`-prefixed session IDs. + - Validate: `cargo test --manifest-path cli/Cargo.toml services::hooks -- conversation_trace` (run via `nix flake check`, `checks.cli-tests`, passing) +- [x] AC6: An already-prefixed session ID (`oc_`, `pi_`, or `cc_`) for a valid producer is left unchanged (idempotent), not double-prefixed. + - Validate: `cargo test --manifest-path cli/Cargo.toml services::hooks -- conversation_trace` (run via `nix flake check`, `checks.cli-tests`, passing) + +### Full validation + +- `nix flake check` +- `nix run .#pkl-check-generated` + +### Context sync + +- `context/sce/agent-trace-hooks-command-routing.md`: replace the current "Normalized envelopes require a non-empty `tool_name`" statement with the restricted supported-producer-set contract for conversation-trace, while leaving the diff-trace `tool_name` description (which stays unrestricted) unchanged. + +## Constraints and non-goals + +- **In scope:** `cli/src/services/hooks/mod.rs` normalized conversation-trace entry validation (`parse_conversation_trace_payload` and its normalized branch), its unit tests, and `context/sce/agent-trace-hooks-command-routing.md`. +- **Out of scope:** `diff-trace` intake and its `tool_name` handling; the shared `prefixed_session_id()` helper's fallback arm as used by `diff-trace` (must keep working for arbitrary producer names there); adding `claude` as an accepted normalized producer value; any generic/pluggable producer registry. +- **Constraints:** Preserve the existing idempotent `oc_`/`pi_`/`cc_` prefixing behavior for valid producers; keep the raw-Claude-event path (`hook_event_name`-driven) deriving `claude` identity internally without going through the new allow-list. +- **Non-goal:** Introducing a generic/extensible producer namespace, or refactoring `diff-trace`'s independent `tool_name` contract. + +## Assumptions + +- The allow-list check is added at the normalized conversation-trace entry point (before `parse_conversation_trace_payloads` is called with an externally supplied `tool_name`), rather than inside the shared `prefixed_session_id()` helper, because that helper's permissive fallback is still required by `diff-trace`'s separate, intentionally unrestricted `tool_name` contract, and the change request scopes the fix to "normalized conversation-trace payloads" only. +- The internal raw-Claude call site (`parse_conversation_trace_payloads(&items, CLAUDE_TOOL_NAME)`) is not routed through the new allow-list check, since `CLAUDE_TOOL_NAME` is an internal constant, not attacker/producer-controlled input, matching the request's instruction not to add `claude` as a normalized producer. + +## Task stack + +- [x] T01: `Reject unsupported normalized conversation-trace tool_name values` (status:done) + - Task ID: T01 + - Goal: `sce hooks conversation-trace` rejects normalized envelopes whose `tool_name` is not `opencode` or `pi`, with a clear error naming the supported values, while `opencode`/`pi`/raw-Claude paths keep working exactly as before. + - Boundaries (in/out of scope): In — the normalized-branch validation in `parse_conversation_trace_payload` (`cli/src/services/hooks/mod.rs`), its unit tests, and the `context/sce/agent-trace-hooks-command-routing.md` doc update. Out — `diff-trace` code/tests/docs, the shared `prefixed_session_id()` fallback arm, any new producer-registry abstraction. + - Dependencies: none + - Done when: the normalized conversation-trace entry validates `tool_name` against `{"opencode", "pi"}` and returns a `conversation_trace_validation_error` naming the supported values for any other non-empty value; empty/missing `tool_name` remains rejected by the existing `required_non_empty_string_field` check; tests for AC1-AC6 pass; `context/sce/agent-trace-hooks-command-routing.md` reflects the restricted contract. + - Verification notes (commands or checks): `cargo test --manifest-path cli/Cargo.toml services::hooks`; `cargo clippy --manifest-path cli/Cargo.toml -- -D warnings`. + - Implementation evidence: Added `NORMALIZED_CONVERSATION_TRACE_TOOL_NAMES = ["opencode", "pi"]` and an allow-list check in `parse_conversation_trace_payload` (`cli/src/services/hooks/mod.rs`) right after the existing `tool_name` non-empty check, bailing with `conversation_trace_validation_error` naming the supported producers for any other value. The raw-Claude branch (`hook_event_name` present) is unchanged and still calls `parse_conversation_trace_payloads(&items, CLAUDE_TOOL_NAME)` directly, bypassing the new allow-list. Added six unit tests covering AC2-AC6 (`conversation_trace_normalized_payload_accepts_pi_tool_name_with_prefixed_session_id`, `conversation_trace_normalized_payload_rejects_unsupported_tool_name`, `conversation_trace_normalized_payload_rejects_empty_tool_name`, `conversation_trace_normalized_payload_rejects_missing_tool_name`, `conversation_trace_normalized_payload_keeps_already_prefixed_session_id`, `conversation_trace_raw_claude_event_uses_claude_identity_with_cc_prefixed_session_id`); AC1 is already covered by the existing `conversation_trace_mixed_payload_maps_to_message_and_part_insert_inputs` test. Updated `context/sce/agent-trace-hooks-command-routing.md:78` to describe the restricted `{opencode, pi}` producer set for conversation-trace while leaving the `diff-trace` `tool_name` description unchanged. + - Verification outcome: `nix flake check` — all checks passed (includes `checks.cli-tests` running the full cargo test suite, and `checks.cli-clippy` running `cargo clippy -- -D warnings`). Direct `cargo test`/`cargo clippy` invocations are blocked by this repository's bash-tool policy (`use-nix-flake-check-over-cargo-test`), so `nix flake check` was run in place of the plan's literal `cargo test --manifest-path ...` / `cargo clippy --manifest-path ...` commands; it exercises the same test and clippy targets. + - Deviations/assumptions: None beyond the plan's stated assumptions. + +## Open questions + +None. The change request's six named test cases plus its explicit non-goals (no `claude` normalized producer, no generic producer namespace, `diff-trace` untouched) fully pin down where the new validation belongs and what it must and must not affect. + +## Validation Report + +**Status:** validated +**Date:** 2026-08-15 + +### Commands run + +- `nix flake check` -> exit 0 (all checks passed, including `checks.cli-tests`: 347 passed, 0 failed) +- `nix run .#pkl-check-generated` -> exit 0 (Ephemeral Pkl generation passed: 101 files) + +### Scaffolding removed + +- None. + +### Success-criteria verification + +- [x] AC1: Normalized `tool_name: "opencode"` persists `oc_`-prefixed session IDs -> `conversation_trace_mixed_payload_maps_to_message_and_part_insert_inputs` passed. +- [x] AC2: Normalized `tool_name: "pi"` persists `pi_`-prefixed session IDs -> `conversation_trace_normalized_payload_accepts_pi_tool_name_with_prefixed_session_id` passed. +- [x] AC3: Unsupported `tool_name` (e.g. `"cursor"`) is rejected naming the supported producer set, no unprefixed row persisted -> `conversation_trace_normalized_payload_rejects_unsupported_tool_name` passed; diff confirms the allow-list check runs before any payload parsing. +- [x] AC4: Empty or missing `tool_name` is rejected -> `conversation_trace_normalized_payload_rejects_empty_tool_name` and `conversation_trace_normalized_payload_rejects_missing_tool_name` passed. +- [x] AC5: Raw Claude event derives `claude` identity internally, unaffected by the allow-list, persists `cc_`-prefixed session ID -> `conversation_trace_raw_claude_event_uses_claude_identity_with_cc_prefixed_session_id` passed; diff confirms the raw-Claude branch calls `parse_conversation_trace_payloads(&items, CLAUDE_TOOL_NAME)` directly, bypassing the new allow-list. +- [x] AC6: Already-prefixed session ID for a valid producer is left unchanged -> `conversation_trace_normalized_payload_keeps_already_prefixed_session_id` passed. + +### Failed checks and follow-ups + +- None. + +### Residual risks + +- None identified. diff --git a/context/sce/agent-trace-db.md b/context/sce/agent-trace-db.md index 5b03e939f..fd9eaec9b 100644 --- a/context/sce/agent-trace-db.md +++ b/context/sce/agent-trace-db.md @@ -191,14 +191,14 @@ Post-commit intersection rows are written by the active `post-commit` hook flow `sce hooks conversation-trace` is the current runtime writer for `messages` and `parts`. -- The hook accepts normalized snake_case mixed-batch STDIN envelopes: top-level `payloads` is an array and each persisted item owns its own `type`; any top-level `type` is ignored and does not classify same-kind old-envelope items. -- `message` items validate and map payloads without message-level `text`, `agent`, or `summary_diffs` to `InsertMessageInsert`; valid rows are inserted through at most one multi-row `RepositoryAgentTraceDb::insert_messages(...)` call per invocation so repeated `(session_id, message_id)` events are ignored without failing. +- The hook accepts normalized snake_case mixed-batch STDIN envelopes: top-level `tool_name` is a required non-empty producer identity, `payloads` is an array, and each persisted item owns its own `type`; any top-level `type` is ignored and does not classify same-kind old-envelope items. +- `message` items validate and map payloads without message-level `text`, `agent`, or `summary_diffs` to `InsertMessageInsert`; valid rows are inserted through at most one multi-row `RepositoryAgentTraceDb::insert_messages(...)` call per invocation so repeated `(session_id, message_id)` events are ignored without failing. Rust prefixes valid stored session IDs by normalized producer (`oc_` for OpenCode, `pi_` for Pi) or by raw Claude event classification (`cc_`), without double-prefixing an already-prefixed ID. - `message.part` items validate and map payloads with required part `text` to `InsertPartInsert`; valid rows are inserted through at most one multi-row `RepositoryAgentTraceDb::insert_parts(...)` call per invocation so parts remain append-only and do not require a pre-existing message row. - Unsupported item types, missing/non-string item types, non-object items, and event-specific parser validation failures are retained as skipped-item diagnostics, logged, and counted as skipped while valid sibling items remain eligible for persistence. -- The hook opens one repository-scoped `RepositoryAgentTraceDb` per invocation through lazy repository storage resolution before insertion; all clones/worktrees of the same logical repository share the same repository-level message/part rows. DB open/initialization failures are logged at error level through `sce.hooks.conversation_trace.agent_trace_db_open_failed` and returned as hook success because conversation-trace intake is fail-open to producers. The event uses the existing best-effort producer-native session route and does not also emit `sce.hooks.conversation_trace.error` for the same open failure. +- The hook opens one repository-scoped `RepositoryAgentTraceDb` per invocation through lazy repository storage resolution before insertion; all clones/worktrees of the same logical repository share the same repository-level message/part rows. DB open/initialization failures are logged at error level through `sce.hooks.conversation_trace.agent_trace_db_open_failed` and returned as hook success because conversation-trace intake is fail-open to producers. Skipped-item, batch-failure, and DB-open diagnostics use producer-native unprefixed session IDs; the event does not also emit `sce.hooks.conversation_trace.error` for the same open failure. - Multi-row insert failures are logged once and count the whole valid-item batch as skipped without failing the command; the hook does not fall back to row-by-row insertion after a batch failure. Successful inserts contribute to deterministic success output counts (`attempted`, `persisted_messages`, `persisted_parts`, `skipped`). Duplicate parent message inserts preserve the existing `ON CONFLICT DO NOTHING` affected-row semantics. - No `context/tmp` artifact is written for conversation traces. -- The generated OpenCode agent-trace plugin sends mixed-batch envelopes for conversation traces: regular `message` and `message.part` events each carry one per-item `type`, while diff-backed `message` events send one envelope containing the synthetic parent message item plus patch part items. +- The generated OpenCode agent-trace plugin sends mixed-batch envelopes for conversation traces with `tool_name: "opencode"`: regular `message` and `message.part` events each carry one per-item `type`, while diff-backed `message` events send one envelope containing the synthetic parent message item plus patch part items. Pi sends `tool_name: "pi"` in its message-end and synthetic patch envelopes. `sce hooks session-model` is no longer a supported command route, generated Claude settings no longer produce `SessionStart` model-attribution events, and the Agent Trace DB adapter no longer exposes a `session_models` API or fresh-schema table. See [agent-trace-hooks-command-routing.md](agent-trace-hooks-command-routing.md). diff --git a/context/sce/agent-trace-hooks-command-routing.md b/context/sce/agent-trace-hooks-command-routing.md index 1f9086512..4ef6d33f9 100644 --- a/context/sce/agent-trace-hooks-command-routing.md +++ b/context/sce/agent-trace-hooks-command-routing.md @@ -75,17 +75,17 @@ - Neither TypeScript runtime writes `context/tmp/*-diff-trace.json` artifacts or AgentTraceDb rows directly. - `diff-trace` command success reports AgentTraceDb persistence only. AgentTraceDb open failures are logged at error level through `sce.hooks.diff_trace.agent_trace_db_open_failed`; later conversion/insert failures retain the warn-level `sce.hooks.diff_trace.agent_trace_db_write_failed` event. Both paths keep the deterministic failed-persistence success text and create no parsed-payload artifact fallback. Diagnostics route through the logger's optional session argument with the original producer-provided session ID, never the AgentTraceDb-only tool-prefixed value. A DB-open failure emits only the open-specific event, not the broader write-failure event. - `diff-trace` producer-facing intake failures are logged through `sce.hooks.diff_trace.error` and returned as hook success. Fail-open routing checks only the expected non-empty top-level session field: `session_id` when `hook_event_name` identifies a Claude raw event, otherwise `sessionID`; malformed JSON, wrong-shaped input, or a missing/empty expected field remains sessionless. The valid-payload path is DB-only and does not write parsed-payload artifacts. -- `conversation-trace` is a recognized hook subcommand routed through `HookSubcommand::ConversationTrace`. Rust intake classifies incoming STDIN JSON by the presence of a top-level `hook_event_name` field: raw Claude hook events are routed through `transform_claude_user_prompt_submit`, `transform_claude_stop`, or `transform_claude_post_tool_use` depending on the event name, while payloads without `hook_event_name` follow the existing mixed-batch `{ payloads: [...] }` path. +- `conversation-trace` is a recognized hook subcommand routed through `HookSubcommand::ConversationTrace`. Rust intake classifies incoming STDIN JSON by the presence of a top-level `hook_event_name` field: raw Claude hook events are routed through `transform_claude_user_prompt_submit`, `transform_claude_stop`, or `transform_claude_post_tool_use` depending on the event name, while payloads without `hook_event_name` follow the normalized mixed-batch `{ tool_name, payloads: [...] }` path. Normalized envelopes require `tool_name` to be one of the supported conversation-trace producers, `opencode` or `pi`; any other non-empty value (including unrecognized producer names) is rejected with a conversation-trace validation error naming the supported set, and an empty or missing `tool_name` remains rejected as before. OpenCode and Pi producers send hardcoded `opencode` and `pi` values. This allow-list is specific to conversation-trace intake and does not apply to `diff-trace`, whose `tool_name` remains unrestricted. - **Raw Claude `UserPromptSubmit` events** (detected by `hook_event_name = "UserPromptSubmit"`): the raw event payload is validated and transformed by `transform_claude_user_prompt_submit` before being forwarded to `parse_conversation_trace_payloads`. - Validates that `hook_event_name` is exactly `"UserPromptSubmit"` and the required `session_id` and `prompt` fields are present and non-empty. - Generates a UUIDv7 `message_id` and a parse-time `generated_at_unix_ms` timestamp. - - Produces exactly two normalized items sharing the same `session_id` and generated `message_id`: + - Produces exactly two normalized items sharing the same `session_id` and generated `message_id`; conversation-trace persistence stores both with the idempotent `cc_` session prefix. - A `message` item with `role: "user"` and `generated_at_unix_ms`. - A `message.part` item with `part_type: "text"`, `text` set to the raw event `prompt` value, and `generated_at_unix_ms`. - **Raw Claude `Stop` events** (detected by `hook_event_name = "Stop"`): the raw event payload is validated and transformed by `transform_claude_stop` before being forwarded to `parse_conversation_trace_payloads`. - Validates that `hook_event_name` is exactly `"Stop"` and the required `session_id` and `last_assistant_message` fields are present and non-empty. - Generates a UUIDv7 `message_id` and a parse-time `generated_at_unix_ms` timestamp. - - Produces exactly two normalized items sharing the same `session_id` and generated `message_id`: + - Produces exactly two normalized items sharing the same `session_id` and generated `message_id`; conversation-trace persistence stores both with the idempotent `cc_` session prefix. - A `message` item with `role: "assistant"` and `generated_at_unix_ms`. - A `message.part` item with `part_type: "text"`, `text` set to the raw event `last_assistant_message` value, and `generated_at_unix_ms`. - **Raw Claude `PostToolUse` events** (detected by `hook_event_name = "PostToolUse"`): the raw event payload is validated and transformed by `transform_claude_post_tool_use` before being forwarded to `parse_conversation_trace_payloads`. @@ -93,11 +93,11 @@ - Silently produces zero items when `tool_name` is not `Write` or `Edit` (e.g. `Read`, `Think` events pass through without producing conversation-trace rows). - For `Write` and `Edit` tools, delegates to `build_claude_post_tool_use_patch(payload)` (from `structured_patch.rs`) instead of reading `tool_response.structuredPatch` directly. - Generates a UUIDv7 `message_id` and a parse-time `generated_at_unix_ms` timestamp. - - Produces one `message` item (with `role: "assistant"` and `generated_at_unix_ms`) plus one `message.part` item sharing the same `session_id` and generated `message_id`: + - Produces one `message` item (with `role: "assistant"` and `generated_at_unix_ms`) plus one `message.part` item sharing the same `session_id` and generated `message_id`; conversation-trace persistence stores both with the idempotent `cc_` session prefix: - On `PatchBuildResult::Built(parsed_patch)`: produces one `message.part` with `part_type: "patch"` and `text` set to JSON-serialized `ParsedPatch`. - On `PatchBuildResult::Skipped(_)`: silently returns zero items (no-op, e.g. for unsupported tools or malformed payloads that would previously have been validation errors). - Unsupported `hook_event_name` values (not `"UserPromptSubmit"`, `"Stop"`, or `"PostToolUse"`) produce an `Invalid conversation-trace payload from STDIN: unsupported Claude hook event '...': supported events are 'UserPromptSubmit', 'Stop' and 'PostToolUse'` error internally, then fail open through `sce.hooks.conversation_trace.error` with hook success text. - - **Mixed-batch path** (no `hook_event_name`): Rust intake expects a top-level `payloads` array and per-item `type` discriminators. A top-level `type` field is ignored by the parser; old homogeneous `{ type, payloads }` envelopes are not a compatibility path because same-kind items without their own `type` are skipped rather than classified from the envelope. + - **Mixed-batch path** (no `hook_event_name`): Rust intake expects a non-empty top-level `tool_name`, a `payloads` array, and per-item `type` discriminators. A top-level `type` field is ignored by the parser; old homogeneous `{ type, payloads }` envelopes are not a compatibility path because same-kind items without their own `type` are skipped rather than classified from the envelope. - `payloads[].type: "message"` parses that item into `InsertMessageInsert` with required non-empty `session_id`, `message_id`, valid `role` (`user|assistant`), and non-negative signed-64-bit `generated_at_unix_ms`; message-level `text`, `agent`, and `summary_diffs` are not required or mapped because body text belongs to `message.part` / `parts.text`. - `payloads[].type: "message.part"` parses that item into `InsertPartInsert` with required non-empty `session_id`, `message_id`, valid `part_type` (`text|reasoning|patch|question`), string `text`, and non-negative signed-64-bit `generated_at_unix_ms`. - `part_type: "text"` and `part_type: "reasoning"` store the raw `text` string unchanged. @@ -105,12 +105,12 @@ - `part_type: "question"` requires `text` to be a JSON string whose parsed value is an array of objects with string `question` and `answer` fields; valid question text is stored unchanged, while invalid question text is skipped through the same per-item validation path as malformed patch items. - Unsupported item `type` values, missing/non-string item `type`, non-object items, and event-specific item validation failures are recorded as skipped-item diagnostics (`index`, `reason`, optional producer session) while valid sibling items remain eligible for persistence; skipped validation items are logged through `sce.hooks.conversation_trace.payload_skipped` using that item's usable `session_id` for logger file routing only. Top-level JSON/object/`payloads` shape failures produce `Invalid conversation-trace payload from STDIN: ...` diagnostics internally, then fail open through `sce.hooks.conversation_trace.error` with hook success text. - Shared persistence (both classification paths converge before DB writes): - - Current persistence opens the repository-scoped `RepositoryAgentTraceDb` for the current repository through lazy storage resolution, then inserts the non-empty valid `message` batch through at most one multi-row `RepositoryAgentTraceDb::insert_messages(...)` call and the non-empty valid `message.part` batch through at most one multi-row `RepositoryAgentTraceDb::insert_parts(...)` call. + - Current persistence opens the repository-scoped `RepositoryAgentTraceDb` for the current repository through lazy storage resolution, then inserts the non-empty valid `message` batch through at most one multi-row `RepositoryAgentTraceDb::insert_messages(...)` call and the non-empty valid `message.part` batch through at most one multi-row `RepositoryAgentTraceDb::insert_parts(...)` call. Before insert construction, valid message and part session IDs receive the producer prefix (`oc_`, `pi_`, or `cc_`) idempotently; skipped-item and batch-failure diagnostics continue routing with the original producer-native session ID. - DB open/initialization failures log at error level through `sce.hooks.conversation_trace.agent_trace_db_open_failed` and return the existing hook success text without also emitting the broader `sce.hooks.conversation_trace.error` event. Session routing checks only top-level `session_id` for a raw Claude event identified by `hook_event_name`; otherwise it checks only `payloads[0].session_id`. Malformed JSON, wrong-shaped input, or a missing/empty expected field remains sessionless; later mixed-batch items are not inspected for this fail-open route. - - Valid-item multi-row insert failures are logged once through `sce.hooks.conversation_trace.agent_trace_db_batch_failed`, count the whole valid-item batch as skipped, and do not fail the command. The batch warning routes with the first valid insert's `session_id`; an empty batch remains sessionless. The diagnostic is emitted once and the hook does not fall back to row-by-row insertion after a multi-row insert failure. + - Valid-item multi-row insert failures are logged once through `sce.hooks.conversation_trace.agent_trace_db_batch_failed`, count the whole valid-item batch as skipped, and do not fail the command. The batch warning routes with the first valid insert's original producer-native `session_id`; an empty batch remains sessionless. The diagnostic is emitted once and the hook does not fall back to row-by-row insertion after a multi-row insert failure. - Current valid-payload success output reports deterministic mixed-batch accounting: `conversation-trace hook persisted mixed payload batch to AgentTraceDb: attempted=, persisted_messages=, persisted_parts=, skipped=.` The hook does not persist `context/tmp` artifacts. - Fail-open output for conversation-trace intake failures is `conversation-trace hook intake failed open; error logged.` so hook callers do not receive app-level classified errors or non-zero exits for intake failures. - - The generated OpenCode agent-trace plugin emits this mixed-batch shape for conversation-trace handoff: ordinary message/part events produce one-item mixed envelopes, completed question-tool parts produce `message.part` items with `part_type: "question"`, and diff-backed message events produce one envelope containing the synthetic parent `message` item plus patch `message.part` items. + - The generated OpenCode agent-trace plugin emits this mixed-batch shape for conversation-trace handoff with `tool_name: "opencode"`: ordinary message/part events produce one-item mixed envelopes, completed question-tool parts produce `message.part` items with `part_type: "question"`, and diff-backed message events produce one envelope containing the synthetic parent `message` item plus patch `message.part` items. The Pi extension uses the same shape with `tool_name: "pi"`. - `session-model` is no longer a supported `sce hooks` subcommand and generated Claude settings no longer produce `SessionStart` model-attribution events. The `session_models` DB API/table and diff-trace fallback are removed from active code; upgraded databases may still contain the retired table, but runtime paths no longer read or write it. ## Explicit non-goals in the current baseline diff --git a/context/sce/opencode-agent-trace-plugin-runtime.md b/context/sce/opencode-agent-trace-plugin-runtime.md index 55584fbf8..8a70f0033 100644 --- a/context/sce/opencode-agent-trace-plugin-runtime.md +++ b/context/sce/opencode-agent-trace-plugin-runtime.md @@ -9,7 +9,7 @@ The Claude TypeScript agent-trace runtime was removed in T07 of the `claude-rust ## Event capture baseline - The plugin registers for `message`, `message.part`, `session.created`, and `session.updated` events. -- Conversation-trace handoff uses the current mixed-batch STDIN shape expected by Rust: `{ "payloads": [{ "type": "message" | "message.part", ... }] }`. The producer does not emit top-level `type` envelopes. +- Conversation-trace handoff uses the current mixed-batch STDIN shape expected by Rust: `{ "tool_name": "opencode", "payloads": [{ "type": "message" | "message.part", ... }] }`. The producer identity is hardcoded and the producer does not emit top-level `type` envelopes. - For every captured `message` event, the plugin checks for `summary.diffs` via `buildPatchConversationTracePayload`: - **When diffs exist**: builds one mixed `-patch` conversation-trace envelope containing the synthetic parent `message` item with `message_id = "${id}-patch"` plus all per-diff `message.part` patch items, then invokes `sce hooks conversation-trace` once. The original `message` event is replaced — no original `message` payload is sent. - **When no diffs exist**: builds one mixed envelope containing a single `message` item via `buildConversationTracePayload` and invokes `sce hooks conversation-trace` over STDIN JSON. @@ -18,6 +18,7 @@ The Claude TypeScript agent-trace runtime was removed in T07 of the `claude-rust - Existing diff-trace capture remains filtered to user messages with usable diffs. - When diff extraction succeeds, the plugin invokes `sce hooks diff-trace` after conversation-trace handoff and sends `{ sessionID, diff, time, model_id, tool_name, tool_version }` over STDIN JSON (`tool_name` is always `"opencode"`; `tool_version` is captured from session lifecycle events when available). `runDiffTraceHook` fails open at the plugin level (ignored stderr, unconditional resolve), so callers do not need try/catch. - The plugin no longer writes diff-trace artifacts or database rows directly; the Rust `diff-trace` hook path owns DB-only AgentTraceDb insertion, including `oc_`-prefixed stored `diff_traces.session_id` values for OpenCode payloads. +- Rust conversation-trace persistence likewise stores OpenCode message and part session IDs with an idempotent `oc_` prefix while keeping producer-facing skipped/error diagnostics on the original session ID. ## In-memory dedup cache diff --git a/context/sce/pi-extension-runtime.md b/context/sce/pi-extension-runtime.md index c5780fd1a..81db7db25 100644 --- a/context/sce/pi-extension-runtime.md +++ b/context/sce/pi-extension-runtime.md @@ -45,9 +45,9 @@ or packaging fallbacks. - Part extraction: `TextContent.text` → `part_type: "text"`, `ThinkingContent.thinking` → `part_type: "reasoning"`; string user content becomes a single text part; empty text is skipped. -- Batches are piped to `sce hooks conversation-trace` (same normalized mixed - `message` / `message.part` envelope as the OpenCode agent-trace plugin), - keyed by `ctx.sessionManager.getSessionId()` with `cwd` from `ctx.cwd`. +- Batches are piped to `sce hooks conversation-trace` with the normalized mixed + `message` / `message.part` envelope and hardcoded `tool_name: "pi"`, keyed by + `ctx.sessionManager.getSessionId()` with `cwd` from `ctx.cwd`. - Fail-open fire-and-forget spawn: stdio `["pipe", "ignore", "ignore"]`, ENOENT logs install guidance, the promise resolves on every outcome and is not awaited by the handler. @@ -69,7 +69,7 @@ or packaging fallbacks. are rewritten to the diff label only before the first `@@` marker so content lines are never touched; file creation rewrites to `--- /dev/null`. - Each diff is emitted twice, both fire-and-forget fail-open spawns: - - `sce hooks conversation-trace`: a mixed batch with a synthetic assistant + - `sce hooks conversation-trace`: a mixed batch with `tool_name: "pi"` and a synthetic assistant `message` (`message_id` = `${toolCallId}-patch`) plus one `part_type: "patch"` part, mirroring the OpenCode patch-batch shape. - `sce hooks diff-trace`: normalized `{ sessionID, diff, time, model_id, @@ -93,6 +93,10 @@ via the `"pi"` arm in `prefixed_diff_trace_session_id()` (`cli/src/services/hooks/mod.rs`). Unknown tool names still pass through unprefixed. +Rust `conversation-trace` intake applies the same idempotent `pi_` prefix to +both message and part session IDs, while skipped and batch-failure diagnostics +retain the original producer-native session ID. + ## Asset pipeline, install, and doctor coverage - For repository builds, a pre-Cargo step evaluates the canonical Pkl model and