config: Add generated OpenCode agent-trace plugin registration#28
config: Add generated OpenCode agent-trace plugin registration#28ivke995 wants to merge 5 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new SceAgentTracePlugin (multiple placements) that writes timestamped JSON traces to a repo-local context/tmp directory, instruments CLI git hook execution to emit per-hook JSON traces, registers the plugin in OpenCode manifests, and adds a package.json for the plugin. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Agent as OpenCode Agent
participant Plugin as SceAgentTracePlugin
participant FS as Filesystem (repo/context/tmp)
Agent->>Plugin: emit event (input with event.type)
Plugin->>Plugin: build ISO timestamp & trace name
Plugin->>FS: mkdir -p context/tmp (recursive)
Plugin->>FS: write <timestamp>-<name>.json with {input}
FS-->>Plugin: write success / error
sequenceDiagram
autonumber
participant CLI as Hook Runner
participant Git as git CLI
participant HookLogic as Existing Hook Logic
participant FS as Filesystem (repo/context/tmp)
CLI->>Git: collect GIT_* env, staged diff or HEAD info, post-rewrite stdin
CLI->>HookLogic: execute hook (scoped to repository_root)
HookLogic-->>CLI: result (success/error) + output
CLI->>FS: write <timestamp>-<hook>.json with {input,output} or {input,error}
FS-->>CLI: write success / error
CLI-->>Caller: return original hook result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/.opencode/plugins/sce-agent-trace.ts`:
- Around line 3-9: The file defines SceAgentTracePlugin with a missing trailing
semicolon and duplicates the implementation found in the shared module; remove
the duplicate implementation and instead re-export the existing
SceAgentTracePlugin from the shared module
(config/lib/agent-trace-plugin/opencode-sce-agent-trace-plugin.ts) so there’s a
single source of truth, and ensure the exported declaration for
SceAgentTracePlugin ends with a semicolon.
In `@config/automated/.opencode/plugins/sce-agent-trace.ts`:
- Around line 3-9: Replace the duplicate inline implementation of
SceAgentTracePlugin (the async Plugin returning the "file.edited" handler with a
console.log and missing semicolon) by re-exporting the canonical implementation
from the shared library; remove the local handler/console.log and ensure the
export statement ends with a semicolon so the file simply re-exports
SceAgentTracePlugin from the shared lib.
In `@config/lib/agent-trace-plugin/opencode-sce-agent-trace-plugin.ts`:
- Around line 1-10: The duplicate plugin implementations in the two plugin
registration modules should re-export the shared SceAgentTracePlugin
implementation instead of duplicating code; change each registration file to
export the SceAgentTracePlugin symbol from the shared module
(opencode-sce-agent-trace-plugin) and remove the duplicated function body,
ensuring the exported symbol remains type Plugin and updating any local imports
to reference the shared export.
- Around line 3-9: The exported SceAgentTracePlugin function has inconsistent
indentation and is missing a trailing semicolon after the returned object
handler; remove the stray whitespace at the start of the function
declaration/line containing "return {" to match file indentation, and add a
semicolon after the closing brace of the returned object (the "file.edited"
handler) so the export SceAgentTracePlugin: Plugin = async () => { ... };
follows the project's TypeScript semicolon style.
In `@config/lib/agent-trace-plugin/package.json`:
- Line 3: Update the `@opencode-ai/plugin` dependency in this package's
package.json from "1.3.0" to "1.4.6" to match the version used in .opencode/ and
other plugins; edit the dependency string for "@opencode-ai/plugin" in
config/lib/agent-trace-plugin/package.json, run the package manager install
(npm/yarn/pnpm) to update lockfiles, and verify builds/tests for
AgentTracePlugin-related types or signatures (look for references to
AgentTracePlugin, plugin initialization code, or exported types) to catch any
incompatibilities.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 48c3e471-09c9-47f0-aa31-5913753d1d02
⛔ Files ignored due to path filters (10)
config/lib/agent-trace-plugin/bun.lockis excluded by!**/*.lockconfig/pkl/base/opencode.pklis excluded by!**/*.pklconfig/pkl/generate.pklis excluded by!**/*.pklconfig/pkl/renderers/common.pklis excluded by!**/*.pklcontext/architecture.mdis excluded by!context/**/*.mdcontext/context-map.mdis excluded by!context/**/*.mdcontext/glossary.mdis excluded by!context/**/*.mdcontext/overview.mdis excluded by!context/**/*.mdcontext/plans/opencode-agent-trace-plugin-generation.mdis excluded by!context/**/*.mdcontext/sce/generated-opencode-plugin-registration.mdis excluded by!context/**/*.md
📒 Files selected for processing (6)
config/.opencode/opencode.jsonconfig/.opencode/plugins/sce-agent-trace.tsconfig/automated/.opencode/opencode.jsonconfig/automated/.opencode/plugins/sce-agent-trace.tsconfig/lib/agent-trace-plugin/opencode-sce-agent-trace-plugin.tsconfig/lib/agent-trace-plugin/package.json
| export const SceAgentTracePlugin: Plugin = async () => { | ||
|
|
||
| return { | ||
| "file.edited": async (input, output) => { | ||
| console.log("input/output", input, output) | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Missing semicolon and duplicate implementation.
Line 7 is missing a trailing semicolon. Additionally, this file duplicates the implementation in config/lib/agent-trace-plugin/opencode-sce-agent-trace-plugin.ts. Consider re-exporting from the shared location instead.
Proposed fix: Re-export from shared lib
-import type { Plugin } from "@opencode-ai/plugin";
-
-export const SceAgentTracePlugin: Plugin = async () => {
-
- return {
- "file.edited": async (input, output) => {
- console.log("input/output", input, output)
- },
- };
-};
+export { SceAgentTracePlugin } from "../../lib/agent-trace-plugin/opencode-sce-agent-trace-plugin";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/.opencode/plugins/sce-agent-trace.ts` around lines 3 - 9, The file
defines SceAgentTracePlugin with a missing trailing semicolon and duplicates the
implementation found in the shared module; remove the duplicate implementation
and instead re-export the existing SceAgentTracePlugin from the shared module
(config/lib/agent-trace-plugin/opencode-sce-agent-trace-plugin.ts) so there’s a
single source of truth, and ensure the exported declaration for
SceAgentTracePlugin ends with a semicolon.
| export const SceAgentTracePlugin: Plugin = async () => { | ||
|
|
||
| return { | ||
| "file.edited": async (input, output) => { | ||
| console.log("input/output", input, output) | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Missing semicolon and duplicate implementation.
Same issues as the other plugin files. Line 7 is missing a semicolon, and this is a third copy of the same implementation. Re-export from the shared lib location.
Proposed fix: Re-export from shared lib
-import type { Plugin } from "@opencode-ai/plugin";
-
-export const SceAgentTracePlugin: Plugin = async () => {
-
- return {
- "file.edited": async (input, output) => {
- console.log("input/output", input, output)
- },
- };
-};
+export { SceAgentTracePlugin } from "../../../lib/agent-trace-plugin/opencode-sce-agent-trace-plugin";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const SceAgentTracePlugin: Plugin = async () => { | |
| return { | |
| "file.edited": async (input, output) => { | |
| console.log("input/output", input, output) | |
| }, | |
| }; | |
| export { SceAgentTracePlugin } from "../../../lib/agent-trace-plugin/opencode-sce-agent-trace-plugin"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/automated/.opencode/plugins/sce-agent-trace.ts` around lines 3 - 9,
Replace the duplicate inline implementation of SceAgentTracePlugin (the async
Plugin returning the "file.edited" handler with a console.log and missing
semicolon) by re-exporting the canonical implementation from the shared library;
remove the local handler/console.log and ensure the export statement ends with a
semicolon so the file simply re-exports SceAgentTracePlugin from the shared lib.
| import type { Plugin } from "@opencode-ai/plugin"; | ||
|
|
||
| export const SceAgentTracePlugin: Plugin = async () => { | ||
|
|
||
| return { | ||
| "file.edited": async (input, output) => { | ||
| console.log("input/output", input, output) | ||
| }, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
This shared implementation is not used by the plugin registrations.
This file in config/lib/agent-trace-plugin/ appears intended as a shared implementation, but both config/.opencode/plugins/sce-agent-trace.ts and config/automated/.opencode/plugins/sce-agent-trace.ts duplicate the code instead of importing from here. Consider having those files re-export from this shared location to maintain DRY.
Example for config/.opencode/plugins/sce-agent-trace.ts:
export { SceAgentTracePlugin } from "../../lib/agent-trace-plugin/opencode-sce-agent-trace-plugin";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/lib/agent-trace-plugin/opencode-sce-agent-trace-plugin.ts` around
lines 1 - 10, The duplicate plugin implementations in the two plugin
registration modules should re-export the shared SceAgentTracePlugin
implementation instead of duplicating code; change each registration file to
export the SceAgentTracePlugin symbol from the shared module
(opencode-sce-agent-trace-plugin) and remove the duplicated function body,
ensuring the exported symbol remains type Plugin and updating any local imports
to reference the shared export.
| @@ -0,0 +1,7 @@ | |||
| { | |||
| "dependencies": { | |||
| "@opencode-ai/plugin": "1.3.0", | |||
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🌐 Web query:
What are the breaking changes between @opencode-ai/plugin versions 1.3.0 and 1.4.6?
💡 Result:
The breaking changes between @opencode-ai/plugin versions 1.3.0 and 1.4.6 primarily occurred in the OpenCode SDK around version 1.3.x (likely corresponding to plugin ~1.3.x), with the most explicit documentation in an earlier release note around OpenCode v1.x (source 2). Key breaking changes in the SDK/plugin system: - Diff metadata: The diffs in metadata for edit and patch tools no longer contain 'to' and 'from' fields with full content. This reduces payload size due to performance issues. - User message structure: The 'variant' field is now nested under 'model' as userMessage.model.variant (previously directly on userMessage). These changes were introduced to address performance problems in the data model. Later releases up to 1.4.x include plugin improvements like custom workspace adaptors (v1.4.4), but no additional breaking changes explicitly listed between 1.3.0 and 1.4.6. The plugin package is published alongside OpenCode releases (e.g., npm shows 1.4.3 recently, versions bump with OpenCode like 1.3.3 to 1.3.4). Migration to SDK v2 was discussed in PRs but implemented non-breaking where possible. No dedicated changelog for the plugin package found; changes tracked via OpenCode releases and commits (e.g., plugin version bumps in release commits). For full details, check the monorepo packages/plugin directory commits between relevant tags.
Citations:
- 1: https://github.com/anomalyco/opencode/releases
- 2: feat(plugin)!: migrate plugin system to SDK v2 anomalyco/opencode#7639
- 3: https://www.npmjs.com/package/@opencode-ai/plugin
- 4: https://github.com/anomalyco/opencode/releases/tag/v1.3.4
- 5: anomalyco/opencode@f0a9075
🏁 Script executed:
# Check the specific file mentioned in the review
cat -n config/lib/agent-trace-plugin/package.json | head -20Repository: crocoder-dev/shared-context-engineering
Length of output: 243
🏁 Script executed:
# Check .opencode/package.json
cat -n .opencode/package.json | grep -A2 -B2 "@opencode-ai/plugin"Repository: crocoder-dev/shared-context-engineering
Length of output: 180
🏁 Script executed:
# Search for all `@opencode-ai/plugin` references across the codebase
rg "@opencode-ai/plugin" --type json -A 1Repository: crocoder-dev/shared-context-engineering
Length of output: 377
🏁 Script executed:
# Check if .opencode/package-lock.json exists and what it says about `@opencode-ai/plugin`
if [ -f .opencode/package-lock.json ]; then
cat .opencode/package-lock.json | jq '.dependencies["@opencode-ai/plugin"].version' 2>/dev/null || rg '"@opencode-ai/plugin"' .opencode/package-lock.json -A 3 | head -20
fiRepository: crocoder-dev/shared-context-engineering
Length of output: 87
🏁 Script executed:
# Check if the plugins are actually dependencies or just local packages
# See if they have their own lock files
ls -la config/lib/agent-trace-plugin/ | grep -E "lock|package"
ls -la config/lib/bash-policy-plugin/ | grep -E "lock|package"Repository: crocoder-dev/shared-context-engineering
Length of output: 324
🏁 Script executed:
# Check if these are actual npm packages being imported or just local package definitions
rg "agent-trace-plugin|bash-policy-plugin" --type json --type yaml --type ts --type js -l | head -10Repository: crocoder-dev/shared-context-engineering
Length of output: 93
🏁 Script executed:
# Check root package.json for workspace configuration
cat -n package.json 2>/dev/null | head -30Repository: crocoder-dev/shared-context-engineering
Length of output: 65
🏁 Script executed:
# Check for monorepo configuration files
ls -la | grep -E "lerna|yarn|npm|workspace"Repository: crocoder-dev/shared-context-engineering
Length of output: 137
🏁 Script executed:
# Check if .opencode is a published package or part of the monorepo
head -20 .opencode/package.jsonRepository: crocoder-dev/shared-context-engineering
Length of output: 145
🏁 Script executed:
# Check the implementation of agent-trace-plugin
cat -n config/lib/agent-trace-plugin/index.ts 2>/dev/null || find config/lib/agent-trace-plugin -name "*.ts" -o -name "*.js" | head -5Repository: crocoder-dev/shared-context-engineering
Length of output: 147
🏁 Script executed:
# Check the implementation of bash-policy-plugin
cat -n config/lib/bash-policy-plugin/index.ts 2>/dev/null || find config/lib/bash-policy-plugin -name "*.ts" -o -name "*.js" | head -5Repository: crocoder-dev/shared-context-engineering
Length of output: 254
🏁 Script executed:
# Check if these plugins reference the structures that changed (diff metadata, userMessage.variant)
rg "variant|from.*to|patch.*diff" config/lib/agent-trace-plugin config/lib/bash-policy-plugin -A 2 -B 2Repository: crocoder-dev/shared-context-engineering
Length of output: 65
🏁 Script executed:
# Read the agent-trace-plugin implementation
cat -n config/lib/agent-trace-plugin/opencode-sce-agent-trace-plugin.ts | head -100Repository: crocoder-dev/shared-context-engineering
Length of output: 384
🏁 Script executed:
# Read the bash-policy-plugin implementation
cat -n config/lib/bash-policy-plugin/opencode-bash-policy-plugin.ts | head -100Repository: crocoder-dev/shared-context-engineering
Length of output: 1389
🏁 Script executed:
# Check if .opencode imports or uses these plugins
rg "agent-trace-plugin|bash-policy-plugin" .opencode -A 2 -B 2Repository: crocoder-dev/shared-context-engineering
Length of output: 65
🏁 Script executed:
# Check the types used in the plugins more carefully
cat -n config/lib/agent-trace-plugin/opencode-sce-agent-trace-plugin.ts
cat -n config/lib/bash-policy-plugin/opencode-bash-policy-plugin.tsRepository: crocoder-dev/shared-context-engineering
Length of output: 1691
🏁 Script executed:
# Check if there are any imports of these plugins elsewhere
rg "SceAgentTracePlugin|SceBashPolicyPlugin" --type ts --type jsRepository: crocoder-dev/shared-context-engineering
Length of output: 345
Align @opencode-ai/plugin versions across packages.
This package and config/lib/bash-policy-plugin/ specify @opencode-ai/plugin@1.3.0, while .opencode/ specifies 1.4.6. Although the documented breaking changes between these versions (diff metadata structure and user message nesting) don't directly impact the current plugin implementations, keeping versions consistent is a best practice to avoid potential type incompatibilities or signature mismatches.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/lib/agent-trace-plugin/package.json` at line 3, Update the
`@opencode-ai/plugin` dependency in this package's package.json from "1.3.0" to
"1.4.6" to match the version used in .opencode/ and other plugins; edit the
dependency string for "@opencode-ai/plugin" in
config/lib/agent-trace-plugin/package.json, run the package manager install
(npm/yarn/pnpm) to update lockfiles, and verify builds/tests for
AgentTracePlugin-related types or signatures (look for references to
AgentTracePlugin, plugin initialization code, or exported types) to catch any
incompatibilities.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.opencode/opencode.json:
- Around line 3-5: Change the "plugin" array in .opencode/opencode.json to use a
multiline format with each plugin string on its own line and include a trailing
comma for readability and consistency; specifically update the "plugin" array
definition (the array containing "./plugins/sce-bash-policy.ts" and
"./plugins/sce-agent-trace.ts") so each entry is on its own line and the array
is written across multiple lines.
In @.opencode/plugins/sce-agent-trace.ts:
- Around line 1-81: The file duplicates the SceAgentTracePlugin implementation;
remove the local implementation (functions like ensureTmpDir, writeTraceToFile,
safeWrite and the exported SceAgentTracePlugin) and instead re-export
SceAgentTracePlugin from the shared implementation in
config/lib/agent-trace-plugin using the correct relative import (from
.opencode/plugins use ../../ to reach the shared module) so the plugin is
imported and re-exported rather than duplicated.
In @.opencode/skills/sce-atomic-commit/SKILL.md:
- Around line 43-70: The ordered list items starting with "1) Analyze the staged
diff...", "2) Choose scope...", "3) Write subject...", etc., must use the
consistent "1." (or consistently "1)") markdown ordered-list style required by
markdownlint MD029; update each list item in SKILL.md so every ordered list line
uses the same numeric prefix (e.g., change "2) Choose scope..." through "7)
Validate each proposed message" to start with "1." to produce the repeating "1."
style, keeping the existing text and punctuation otherwise unchanged.
In `@config/lib/agent-trace-plugin/opencode-sce-agent-trace-plugin.ts`:
- Around line 1-4: Reorder the import block so Node builtins come first: group
"fs" (promises), "node:crypto" (randomUUID) and "path" before external package
imports; then import the external type Plugin from "@opencode-ai/plugin". Locate
the imports in opencode-sce-agent-trace-plugin.ts (references: promises as fs,
randomUUID, path, Plugin) and move the Plugin import below the builtins while
preserving the existing named imports and types.
- Around line 31-40: The code calls new Date() twice causing slight timestamp
drift between the filename and trace.recorded_at; capture a single Date instance
(e.g., const now = new Date()) and reuse it when computing timestamp via
formatTimestamp(now) and when setting trace.recorded_at = now.toISOString(),
keeping the rest of the logic that builds filename, traceId, safeEventName,
filename and filepath unchanged (look for timestamp, formatTimestamp,
trace.recorded_at and the TraceEnvelope construction).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ff16efe7-92e9-4a50-8399-06b346e24e2e
⛔ Files ignored due to path filters (3)
.opencode/package-lock.jsonis excluded by!**/package-lock.jsoncontext/plans/agent-trace-file-writer.mdis excluded by!context/**/*.mdcontext/sce/generated-opencode-plugin-registration.mdis excluded by!context/**/*.md
📒 Files selected for processing (7)
.opencode/opencode.json.opencode/package.json.opencode/plugins/sce-agent-trace.ts.opencode/skills/sce-atomic-commit/SKILL.mdconfig/.opencode/plugins/sce-agent-trace.tsconfig/automated/.opencode/plugins/sce-agent-trace.tsconfig/lib/agent-trace-plugin/opencode-sce-agent-trace-plugin.ts
💤 Files with no reviewable changes (1)
- .opencode/package.json
| "plugin": [ | ||
| "./plugins/sce-bash-policy.ts" | ||
| "./plugins/sce-bash-policy.ts", "./plugins/sce-agent-trace.ts" | ||
| ] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider multiline format for better readability.
The plugin array with two entries would be clearer with one entry per line, aligning with the coding guideline for trailing commas in multiline structures.
♻️ Proposed formatting
"plugin": [
- "./plugins/sce-bash-policy.ts", "./plugins/sce-agent-trace.ts"
+ "./plugins/sce-bash-policy.ts",
+ "./plugins/sce-agent-trace.ts"
]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "plugin": [ | |
| "./plugins/sce-bash-policy.ts" | |
| "./plugins/sce-bash-policy.ts", "./plugins/sce-agent-trace.ts" | |
| ] | |
| "plugin": [ | |
| "./plugins/sce-bash-policy.ts", | |
| "./plugins/sce-agent-trace.ts" | |
| ] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.opencode/opencode.json around lines 3 - 5, Change the "plugin" array in
.opencode/opencode.json to use a multiline format with each plugin string on its
own line and include a trailing comma for readability and consistency;
specifically update the "plugin" array definition (the array containing
"./plugins/sce-bash-policy.ts" and "./plugins/sce-agent-trace.ts") so each entry
is on its own line and the array is written across multiple lines.
| import type { Plugin } from "@opencode-ai/plugin"; | ||
| import { promises as fs } from "fs"; | ||
| import { randomUUID } from "node:crypto"; | ||
| import path from "path"; | ||
|
|
||
| function formatTimestamp(date: Date): string { | ||
| return date.toISOString().replace(/:/g, "-"); | ||
| } | ||
|
|
||
| function makeTraceId(): string { | ||
| return randomUUID(); | ||
| } | ||
|
|
||
| async function ensureTmpDir(rootDir: string): Promise<string> { | ||
| const tmpDir = path.join(rootDir, "tmp"); | ||
| await fs.mkdir(tmpDir, { recursive: true }); | ||
| return tmpDir; | ||
| } | ||
|
|
||
| type TraceEnvelope = { | ||
| version: "0.1.0"; | ||
| trace_id: string; | ||
| event: string; | ||
| recorded_at: string; | ||
| cwd: string; | ||
| payload: unknown; | ||
| }; | ||
|
|
||
| async function writeTraceToFile(rootDir: string, eventName: string, payload: unknown): Promise<string> { | ||
| const tmpDir = await ensureTmpDir(rootDir); | ||
| const timestamp = formatTimestamp(new Date()); | ||
| const traceId = makeTraceId(); | ||
| const safeEventName = eventName.replace(/[^\w.-]/g, "_"); | ||
| const filename = `${timestamp}-${safeEventName}-${traceId}.json`; | ||
| const filepath = path.join(tmpDir, filename); | ||
| const trace: TraceEnvelope = { | ||
| version: "0.1.0", | ||
| trace_id: traceId, | ||
| event: eventName, | ||
| recorded_at: new Date().toISOString(), | ||
| cwd: rootDir, | ||
| payload, | ||
| }; | ||
|
|
||
| await fs.writeFile(filepath, JSON.stringify(trace, null, 2), "utf8"); | ||
|
|
||
| return filepath; | ||
| } | ||
|
|
||
| async function safeWrite(rootDir: string, eventName: string, payload: unknown): Promise<void> { | ||
| try { | ||
| const filepath = await writeTraceToFile(rootDir, eventName, payload); | ||
| console.log(`[sce-agent-trace] ${eventName} -> ${filepath}`); | ||
| } catch (error) { | ||
| console.error(`[sce-agent-trace] Failed to write ${eventName}:`, error); | ||
| } | ||
| } | ||
|
|
||
| export const SceAgentTracePlugin: Plugin = async ({ directory, worktree }) => { | ||
| const rootDir = worktree ?? directory ?? process.cwd(); | ||
|
|
||
| return { | ||
| "session.created": async () => { | ||
| try { | ||
| const tmpDir = await ensureTmpDir(rootDir); | ||
| console.log(`[sce-agent-trace] Ready at ${tmpDir}`); | ||
| } catch (error) { | ||
| console.error("[sce-agent-trace] Failed to initialize tmp directory:", error); | ||
| } | ||
| }, | ||
| "file.edited": async (...args) => { | ||
| await safeWrite(rootDir, "file.edited", { args }); | ||
| }, | ||
| "file.watcher.updated": async (...args) => { | ||
| await safeWrite(rootDir, "file.watcher.updated", { args }); | ||
| }, | ||
| "tool.execute.after": async (...args) => { | ||
| await safeWrite(rootDir, "tool.execute.after", { args }); | ||
| }, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the shared implementation location relative to .opencode/plugins/
fd -t f "opencode-sce-agent-trace-plugin.ts"Repository: crocoder-dev/shared-context-engineering
Length of output: 147
🏁 Script executed:
cat -n config/lib/agent-trace-plugin/opencode-sce-agent-trace-plugin.tsRepository: crocoder-dev/shared-context-engineering
Length of output: 3101
🏁 Script executed:
#!/bin/bash
# Verify the directory structure and relative path
echo "=== Directory structure from .opencode/plugins/ ==="
cd .opencode/plugins/
pwd
echo ""
echo "=== Try to navigate using proposed path ../config/lib/agent-trace-plugin/ ==="
ls -la ../config/lib/agent-trace-plugin/ 2>&1 || echo "Path not found"
echo ""
echo "=== Correct path should be ../../config/lib/agent-trace-plugin/ ==="
ls -la ../../config/lib/agent-trace-plugin/ 2>&1 || echo "Path not found"Repository: crocoder-dev/shared-context-engineering
Length of output: 731
Correct relative path for re-export from shared lib: use ../../ not ../.
This file duplicates the full implementation found in config/lib/agent-trace-plugin/opencode-sce-agent-trace-plugin.ts. Re-export from there to maintain DRY. The proposed relative path in the original suggestion is incorrect—from .opencode/plugins/, you need two levels up (../../) to reach the root, then access config/lib/.
♻️ Proposed fix: Re-export from shared lib
-import type { Plugin } from "@opencode-ai/plugin";
-import { promises as fs } from "fs";
-import { randomUUID } from "node:crypto";
-import path from "path";
-
-function formatTimestamp(date: Date): string {
- return date.toISOString().replace(/:/g, "-");
-}
-
-function makeTraceId(): string {
- return randomUUID();
-}
-
-async function ensureTmpDir(rootDir: string): Promise<string> {
- const tmpDir = path.join(rootDir, "tmp");
- await fs.mkdir(tmpDir, { recursive: true });
- return tmpDir;
-}
-
-type TraceEnvelope = {
- version: "0.1.0";
- trace_id: string;
- event: string;
- recorded_at: string;
- cwd: string;
- payload: unknown;
-};
-
-async function writeTraceToFile(rootDir: string, eventName: string, payload: unknown): Promise<string> {
- const tmpDir = await ensureTmpDir(rootDir);
- const timestamp = formatTimestamp(new Date());
- const traceId = makeTraceId();
- const safeEventName = eventName.replace(/[^\w.-]/g, "_");
- const filename = `${timestamp}-${safeEventName}-${traceId}.json`;
- const filepath = path.join(tmpDir, filename);
- const trace: TraceEnvelope = {
- version: "0.1.0",
- trace_id: traceId,
- event: eventName,
- recorded_at: new Date().toISOString(),
- cwd: rootDir,
- payload,
- };
-
- await fs.writeFile(filepath, JSON.stringify(trace, null, 2), "utf8");
-
- return filepath;
-}
-
-async function safeWrite(rootDir: string, eventName: string, payload: unknown): Promise<void> {
- try {
- const filepath = await writeTraceToFile(rootDir, eventName, payload);
- console.log(`[sce-agent-trace] ${eventName} -> ${filepath}`);
- } catch (error) {
- console.error(`[sce-agent-trace] Failed to write ${eventName}:`, error);
- }
-}
-
-export const SceAgentTracePlugin: Plugin = async ({ directory, worktree }) => {
- const rootDir = worktree ?? directory ?? process.cwd();
-
- return {
- "session.created": async () => {
- try {
- const tmpDir = await ensureTmpDir(rootDir);
- console.log(`[sce-agent-trace] Ready at ${tmpDir}`);
- } catch (error) {
- console.error("[sce-agent-trace] Failed to initialize tmp directory:", error);
- }
- },
- "file.edited": async (...args) => {
- await safeWrite(rootDir, "file.edited", { args });
- },
- "file.watcher.updated": async (...args) => {
- await safeWrite(rootDir, "file.watcher.updated", { args });
- },
- "tool.execute.after": async (...args) => {
- await safeWrite(rootDir, "tool.execute.after", { args });
- },
- };
-};
+export { SceAgentTracePlugin } from "../../config/lib/agent-trace-plugin/opencode-sce-agent-trace-plugin";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import type { Plugin } from "@opencode-ai/plugin"; | |
| import { promises as fs } from "fs"; | |
| import { randomUUID } from "node:crypto"; | |
| import path from "path"; | |
| function formatTimestamp(date: Date): string { | |
| return date.toISOString().replace(/:/g, "-"); | |
| } | |
| function makeTraceId(): string { | |
| return randomUUID(); | |
| } | |
| async function ensureTmpDir(rootDir: string): Promise<string> { | |
| const tmpDir = path.join(rootDir, "tmp"); | |
| await fs.mkdir(tmpDir, { recursive: true }); | |
| return tmpDir; | |
| } | |
| type TraceEnvelope = { | |
| version: "0.1.0"; | |
| trace_id: string; | |
| event: string; | |
| recorded_at: string; | |
| cwd: string; | |
| payload: unknown; | |
| }; | |
| async function writeTraceToFile(rootDir: string, eventName: string, payload: unknown): Promise<string> { | |
| const tmpDir = await ensureTmpDir(rootDir); | |
| const timestamp = formatTimestamp(new Date()); | |
| const traceId = makeTraceId(); | |
| const safeEventName = eventName.replace(/[^\w.-]/g, "_"); | |
| const filename = `${timestamp}-${safeEventName}-${traceId}.json`; | |
| const filepath = path.join(tmpDir, filename); | |
| const trace: TraceEnvelope = { | |
| version: "0.1.0", | |
| trace_id: traceId, | |
| event: eventName, | |
| recorded_at: new Date().toISOString(), | |
| cwd: rootDir, | |
| payload, | |
| }; | |
| await fs.writeFile(filepath, JSON.stringify(trace, null, 2), "utf8"); | |
| return filepath; | |
| } | |
| async function safeWrite(rootDir: string, eventName: string, payload: unknown): Promise<void> { | |
| try { | |
| const filepath = await writeTraceToFile(rootDir, eventName, payload); | |
| console.log(`[sce-agent-trace] ${eventName} -> ${filepath}`); | |
| } catch (error) { | |
| console.error(`[sce-agent-trace] Failed to write ${eventName}:`, error); | |
| } | |
| } | |
| export const SceAgentTracePlugin: Plugin = async ({ directory, worktree }) => { | |
| const rootDir = worktree ?? directory ?? process.cwd(); | |
| return { | |
| "session.created": async () => { | |
| try { | |
| const tmpDir = await ensureTmpDir(rootDir); | |
| console.log(`[sce-agent-trace] Ready at ${tmpDir}`); | |
| } catch (error) { | |
| console.error("[sce-agent-trace] Failed to initialize tmp directory:", error); | |
| } | |
| }, | |
| "file.edited": async (...args) => { | |
| await safeWrite(rootDir, "file.edited", { args }); | |
| }, | |
| "file.watcher.updated": async (...args) => { | |
| await safeWrite(rootDir, "file.watcher.updated", { args }); | |
| }, | |
| "tool.execute.after": async (...args) => { | |
| await safeWrite(rootDir, "tool.execute.after", { args }); | |
| }, | |
| }; | |
| }; | |
| export { SceAgentTracePlugin } from "../../config/lib/agent-trace-plugin/opencode-sce-agent-trace-plugin"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.opencode/plugins/sce-agent-trace.ts around lines 1 - 81, The file
duplicates the SceAgentTracePlugin implementation; remove the local
implementation (functions like ensureTmpDir, writeTraceToFile, safeWrite and the
exported SceAgentTracePlugin) and instead re-export SceAgentTracePlugin from the
shared implementation in config/lib/agent-trace-plugin using the correct
relative import (from .opencode/plugins use ../../ to reach the shared module)
so the plugin is imported and re-exported rather than duplicated.
| 1) Analyze the staged diff for coherent units | ||
| - Infer the main reason(s) for the staged change from the diff first. | ||
| - Use optional notes only to refine wording, not to override the staged truth. | ||
| - Identify whether staged changes represent one coherent unit or multiple unrelated goals. | ||
|
|
||
| 2) Choose scope | ||
| 2) Choose scope for each unit | ||
| - Use the smallest stable subsystem/module name recognizable in the repo. | ||
| - If unclear, use the primary directory/package of the change. | ||
|
|
||
| 3) Write subject | ||
| 3) Write subject for each unit | ||
| - Pattern: `<scope>: <Imperative verb> <specific technical summary>` | ||
| - Keep concrete and targeted. | ||
|
|
||
| 4) Add body when needed | ||
| - Explain what was wrong/missing, why it matters, what changed conceptually, and impact. | ||
| - Add issue references on separate lines. | ||
|
|
||
| 5) Validate atomicity | ||
| - Reverting the commit should cleanly remove one logical change. | ||
| - Review should not require unrelated context. | ||
| - No drive-by formatting or unrelated refactors. | ||
|
|
||
| If atomicity fails, split further. | ||
|
|
||
| ## Split guidance | ||
| 5) Apply the plan-update body rule when needed | ||
| - Check whether staged changes include `context/plans/*.md`. | ||
| - If yes, cite the affected plan slug(s) and updated task ID(s) in the body. | ||
| - If the staged plan diff is ambiguous, stop with actionable guidance asking the user to stage or clarify the plan/task reference explicitly. | ||
|
|
||
| Split when you see: | ||
| - renames with behavior changes | ||
| - pure refactors bundled with fixes | ||
| - formatting-only changes mixed with logic | ||
| - multiple features shipped together | ||
| 6) Propose split guidance when appropriate | ||
| - If staged changes mix unrelated goals (for example: a feature change plus unrelated refactoring), propose separate commit messages for each coherent unit. | ||
| - Explain why the split is recommended and which files belong to each proposed commit. | ||
| - If staged changes represent one coherent unit, propose a single commit message. | ||
|
|
||
| Default split order: | ||
| 1. mechanical rename/refactor | ||
| 2. behavior change | ||
| 3. tests | ||
| 4. docs | ||
| 7) Validate each proposed message |
There was a problem hiding this comment.
Fix ordered-list numbering to satisfy markdownlint MD029
Lines 43-70 use 1) ... 2) ... 3) ..., but your linter expects 1/1/1 style (all items prefixed with 1. or 1)). This currently produces warnings at Line 48, Line 52, Line 56, Line 60, Line 65, and Line 70.
Suggested diff
-1) Analyze the staged diff for coherent units
+1) Analyze the staged diff for coherent units
...
-2) Choose scope for each unit
+1) Choose scope for each unit
...
-3) Write subject for each unit
+1) Write subject for each unit
...
-4) Add body when needed
+1) Add body when needed
...
-5) Apply the plan-update body rule when needed
+1) Apply the plan-update body rule when needed
...
-6) Propose split guidance when appropriate
+1) Propose split guidance when appropriate
...
-7) Validate each proposed message
+1) Validate each proposed message🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 48-48: Ordered list item prefix
Expected: 1; Actual: 2; Style: 1/1/1
(MD029, ol-prefix)
[warning] 52-52: Ordered list item prefix
Expected: 1; Actual: 3; Style: 1/1/1
(MD029, ol-prefix)
[warning] 56-56: Ordered list item prefix
Expected: 1; Actual: 4; Style: 1/1/1
(MD029, ol-prefix)
[warning] 60-60: Ordered list item prefix
Expected: 1; Actual: 5; Style: 1/1/1
(MD029, ol-prefix)
[warning] 65-65: Ordered list item prefix
Expected: 1; Actual: 6; Style: 1/1/1
(MD029, ol-prefix)
[warning] 70-70: Ordered list item prefix
Expected: 1; Actual: 7; Style: 1/1/1
(MD029, ol-prefix)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.opencode/skills/sce-atomic-commit/SKILL.md around lines 43 - 70, The
ordered list items starting with "1) Analyze the staged diff...", "2) Choose
scope...", "3) Write subject...", etc., must use the consistent "1." (or
consistently "1)") markdown ordered-list style required by markdownlint MD029;
update each list item in SKILL.md so every ordered list line uses the same
numeric prefix (e.g., change "2) Choose scope..." through "7) Validate each
proposed message" to start with "1." to produce the repeating "1." style,
keeping the existing text and punctuation otherwise unchanged.
davidabram
force-pushed
the
feat/agent-trace-plugin
branch
from
5d51ab7 to
fe5c27e
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.opencode/plugins/sce-agent-trace.ts:
- Around line 43-72: buildTrace currently can throw (JSON.stringify, mkdir,
writeFile) and the event/"tool.execute.before" handlers await it directly,
allowing hook rejection; wrap the call sites or the buildTrace implementation in
a try/catch that prevents exceptions from propagating, log the error (using the
same logging approach as the other trace-plugin variants in this PR) and return
void so the handlers never reject. Specifically, add a try/catch around the body
of async function buildTrace(traceDirectory, input, output) (or around the await
buildTrace(...) calls in the event and "tool.execute.before" handlers) to catch
synchronous JSON.stringify errors and async fs errors, call the module's
logger/console with a clear message including the caught error and the trace
name (use getTraceName(input) or filePath for context), and ensure the function
returns normally on error.
- Around line 12-18: The current timestamp-based filenames from
formatTimestamp/buildTraceFileName can collide for events in the same
millisecond; modify buildTraceFileName to append a short unique suffix (e.g., a
random 6–8 character alphanumeric token or a monotonic counter) to the filename
to guarantee uniqueness, e.g. by calling a small helper (generateRandomSuffix or
incrementing counter) and returning
`${formatTimestamp(date)}-${traceName}-${suffix}.json`; ensure the suffix
generation is deterministic per-process if using a counter or cryptographically
safe if using randomness and that traceName sanitization is preserved.
In `@cli/src/services/hooks.rs`:
- Around line 51-60: Multiple near-identical wrappers (e.g.,
run_pre_commit_subcommand_with_trace, run_post_rewrite_subcommand_with_trace)
all perform "build input → run → let _ = persist_hook_trace(...) → return
outcome"; extract a single helper (e.g., with_trace) that takes repository_root:
&Path, subcommand: HookSubcommand, an input or input factory (Value or FnOnce()
-> Value to cover post-rewrite pre-run side effects), and a run closure F:
FnOnce() -> Result<String>, then inside call run(), call
persist_hook_trace(repository_root, &subcommand, &input, &outcome) ignoring
errors, and return outcome; replace each specific *_with_trace to call this
helper (use build_hook_trace_input_for_pre_commit, run_pre_commit_subcommand,
run_post_rewrite_subcommand, etc.) so the persist logic is centralized and the
post-rewrite case can pass an input factory when needed.
- Around line 227-233: The filename generation in build_hook_trace_file_name
(which uses hook_trace_name) can collide at ms granularity; modify
build_hook_trace_file_name to append a short collision-resistant suffix (e.g., a
per-process atomic counter or a small random hex/uuid fragment) after the
timestamp and hook name so filenames are unique for rapid/parallel invocations;
ensure the chosen approach is thread-safe (use an AtomicUsize counter or
thread_rng for randomness) and keep the suffix short (4–8 hex chars) to avoid
overly long filenames.
- Around line 244-302: Add unit tests adjacent to the hooks code that exercise
the new trace input/persistence behavior: create tests named
persist_hook_trace_writes_input_and_output_on_success and
persist_hook_trace_writes_input_and_error_on_failure that invoke the persist
function used by the trace-wrapper (persist_hook_trace or equivalent), simulate
a successful run vs a failing run, and assert the persisted JSON contains an
"input" object and either an "output" field on success or an "error" field on
failure; create build_hook_trace_input_for_commit_msg_includes_message_from_git
which calls build_hook_trace_input_for_commit_msg with a temporary message file
(and another case where reading fails) and asserts presence of
"message_from_git" or "message_from_git_read_error"; create
parse_post_rewrite_pairs_handles_malformed_lines which calls
parse_post_rewrite_pairs (and build_hook_trace_input_for_post_rewrite) with
well-formed and malformed stdin payloads and asserts malformed lines are ignored
or recorded as expected; and add
build_hook_trace_file_name_uses_hook_name_for_each_variant to call the filename
builder (build_hook_trace_file_name or equivalent) for each hook variant and
assert the returned file names include the hook name. Use temp dirs/files and
the existing test scaffolding/matchers pattern in the repo and reference the
functions build_hook_trace_input_for_commit_msg,
build_hook_trace_input_for_post_rewrite, parse_post_rewrite_pairs, and the
persist function in your assertions.
- Around line 162-180: read_hook_stdin() errors are currently short-circuiting
the hook run because outcome is computed as stdin_payload.and_then(|_|
run_post_rewrite_subcommand(...)). Change run_post_rewrite_subcommand_with_trace
so it first captures stdin_result = read_hook_stdin() (best-effort), constructs
input via build_hook_trace_input_for_post_rewrite(...,
stdin_result.as_deref().unwrap_or_default()), then always call
run_post_rewrite_subcommand(repository_root, rewrite_method) to produce outcome
(regardless of stdin_result), and finally call
persist_hook_trace(repository_root, subcommand, &input, &outcome) while treating
any trace write/read errors as diagnostic-only; return the outcome from
run_post_rewrite_subcommand.
- Around line 304-323: The hook tracing currently writes sensitive git env and
diffs unconditionally via build_base_hook_trace_input and
collect_git_environment and leaves files in context/tmp/ indefinitely; change it
so trace persistence is disabled by default and only enabled when an explicit
opt-in env var (e.g., SCE_HOOK_TRACE=1) is present, avoid serializing PII by
filtering out keys like GIT_AUTHOR_EMAIL, GIT_COMMITTER_EMAIL, GIT_AUTHOR_NAME,
GIT_COMMITTER_NAME in collect_git_environment, and implement retention limits
for the trace directory (either rotate by max total size or delete files older
than a configurable age) so traces in context/tmp/ are automatically pruned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3f33803b-108f-4e13-9281-8fdddb5e42b0
📒 Files selected for processing (3)
.opencode/plugins/sce-agent-trace.tscli/src/app.rscli/src/services/hooks.rs
| function formatTimestamp(date: Date): string { | ||
| return date.toISOString().replaceAll(":", "-").replaceAll(".", "-"); | ||
| } | ||
|
|
||
| function buildTraceFileName(traceName: string, date: Date): string { | ||
| return `${formatTimestamp(date)}-${traceName}.json`; | ||
| } |
There was a problem hiding this comment.
Filename collision risk: ms-granularity timestamp + traceName can overwrite prior traces.
Two events in the same millisecond with the same traceName (e.g. repeated tool.execute.before with identical tool string, or bursty events of the same type) will produce the same filename and silently overwrite the earlier trace. A short random suffix (or a monotonic counter) makes traces safe to retain.
♻️ Proposed fix
-import { mkdir, writeFile } from "node:fs/promises";
-import path from "node:path";
-import type { Plugin } from "@opencode-ai/plugin";
+import { mkdir, writeFile } from "node:fs/promises";
+import { randomUUID } from "node:crypto";
+import path from "node:path";
+import type { Plugin } from "@opencode-ai/plugin";
@@
-function buildTraceFileName(traceName: string, date: Date): string {
- return `${formatTimestamp(date)}-${traceName}.json`;
-}
+function buildTraceFileName(traceName: string, date: Date): string {
+ return `${formatTimestamp(date)}-${traceName}-${randomUUID().slice(0, 8)}.json`;
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.opencode/plugins/sce-agent-trace.ts around lines 12 - 18, The current
timestamp-based filenames from formatTimestamp/buildTraceFileName can collide
for events in the same millisecond; modify buildTraceFileName to append a short
unique suffix (e.g., a random 6–8 character alphanumeric token or a monotonic
counter) to the filename to guarantee uniqueness, e.g. by calling a small helper
(generateRandomSuffix or incrementing counter) and returning
`${formatTimestamp(date)}-${traceName}-${suffix}.json`; ensure the suffix
generation is deterministic per-process if using a counter or cryptographically
safe if using randomness and that traceName sanitization is preserved.
| async function buildTrace(traceDirectory: string, input: unknown, output?: unknown): Promise<void> { | ||
| const now = new Date(); | ||
| const filePath = path.join(traceDirectory, buildTraceFileName(getTraceName(input), now)); | ||
| const body = JSON.stringify( | ||
| output === undefined | ||
| ? { input } | ||
| : { | ||
| output, | ||
| input, | ||
| }, | ||
| null, | ||
| 2, | ||
| ); | ||
|
|
||
| await mkdir(traceDirectory, { recursive: true }); | ||
| await writeFile(filePath, body, "utf8"); | ||
| } | ||
|
|
||
| export const SceAgentTracePlugin: Plugin = async ({ directory, worktree }) => { | ||
| const repoRoot = worktree ?? directory ?? process.cwd(); | ||
| const traceDirectory = path.join(repoRoot, "context", "tmp"); | ||
|
|
||
| return { | ||
| event: async (input, output) => { | ||
| await buildTrace(traceDirectory, input, output); | ||
| }, | ||
| "tool.execute.before": async (input, output) => { | ||
| await buildTrace(traceDirectory, input, output); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Missing error handling — handler promises will reject on any fs/JSON failure.
The AI summary explicitly calls out that, unlike the sibling trace-plugin implementations added in this PR, buildTrace here has no internal try/catch. mkdir, writeFile, and JSON.stringify (e.g. on a value containing a BigInt or circular reference from a tool payload) can all throw. When they do, the event / tool.execute.before handlers reject, and OpenCode will see a failing hook on every tool call or event — a diagnostic plugin should never be able to break the host session.
Wrap buildTrace (or the handler bodies) in try/catch and log failures, consistent with the other trace-plugin variants in this PR.
🛡️ Proposed fix
-async function buildTrace(traceDirectory: string, input: unknown, output?: unknown): Promise<void> {
- const now = new Date();
- const filePath = path.join(traceDirectory, buildTraceFileName(getTraceName(input), now));
- const body = JSON.stringify(
- output === undefined
- ? { input }
- : {
- output,
- input,
- },
- null,
- 2,
- );
-
- await mkdir(traceDirectory, { recursive: true });
- await writeFile(filePath, body, "utf8");
-}
+async function buildTrace(traceDirectory: string, input: unknown, output?: unknown): Promise<void> {
+ try {
+ const now = new Date();
+ const filePath = path.join(traceDirectory, buildTraceFileName(getTraceName(input), now));
+ const body = JSON.stringify(
+ output === undefined ? { input } : { output, input },
+ null,
+ 2,
+ );
+
+ await mkdir(traceDirectory, { recursive: true });
+ await writeFile(filePath, body, "utf8");
+ } catch (error) {
+ console.error("[sce-agent-trace] Failed to persist trace:", error);
+ }
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.opencode/plugins/sce-agent-trace.ts around lines 43 - 72, buildTrace
currently can throw (JSON.stringify, mkdir, writeFile) and the
event/"tool.execute.before" handlers await it directly, allowing hook rejection;
wrap the call sites or the buildTrace implementation in a try/catch that
prevents exceptions from propagating, log the error (using the same logging
approach as the other trace-plugin variants in this PR) and return void so the
handlers never reject. Specifically, add a try/catch around the body of async
function buildTrace(traceDirectory, input, output) (or around the await
buildTrace(...) calls in the event and "tool.execute.before" handlers) to catch
synchronous JSON.stringify errors and async fs errors, call the module's
logger/console with a clear message including the caught error and the trace
name (use getTraceName(input) or filePath for context), and ensure the function
returns normally on error.
| fn run_post_rewrite_subcommand_with_trace( | ||
| repository_root: &Path, | ||
| subcommand: &HookSubcommand, | ||
| rewrite_method: &str, | ||
| ) -> Result<String> { | ||
| let stdin_payload = read_hook_stdin(); | ||
| let input = build_hook_trace_input_for_post_rewrite( | ||
| repository_root, | ||
| rewrite_method, | ||
| stdin_payload.as_deref().unwrap_or_default(), | ||
| ); | ||
| let outcome = | ||
| stdin_payload.and_then(|_| run_post_rewrite_subcommand(repository_root, rewrite_method)); | ||
|
|
||
| // Trace persistence is diagnostic only; hook execution should not fail if local trace writing fails. | ||
| let _ = persist_hook_trace(repository_root, subcommand, &input, &outcome); | ||
|
|
||
| outcome | ||
| } |
There was a problem hiding this comment.
post-rewrite stdin-read failure silently skips real hook work.
If read_hook_stdin() returns Err, outcome = stdin_payload.and_then(|_| run_post_rewrite_subcommand(...)) short-circuits with the stdin error and the actual hook runner never runs. Previously (pre-refactor) a stdin failure was the only failure mode too, but now a diagnostic-only concern (failing to capture stdin for the trace) has been elevated to abort the hook. Consider decoupling: capture stdin best-effort for the trace, then still invoke run_post_rewrite_subcommand so hook semantics are unchanged when tracing fails.
♻️ Proposed decoupling
- let stdin_payload = read_hook_stdin();
- let input = build_hook_trace_input_for_post_rewrite(
- repository_root,
- rewrite_method,
- stdin_payload.as_deref().unwrap_or_default(),
- );
- let outcome =
- stdin_payload.and_then(|_| run_post_rewrite_subcommand(repository_root, rewrite_method));
+ let stdin_payload = read_hook_stdin().unwrap_or_default();
+ let input = build_hook_trace_input_for_post_rewrite(
+ repository_root,
+ rewrite_method,
+ &stdin_payload,
+ );
+ let outcome = run_post_rewrite_subcommand(repository_root, rewrite_method);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn run_post_rewrite_subcommand_with_trace( | |
| repository_root: &Path, | |
| subcommand: &HookSubcommand, | |
| rewrite_method: &str, | |
| ) -> Result<String> { | |
| let stdin_payload = read_hook_stdin(); | |
| let input = build_hook_trace_input_for_post_rewrite( | |
| repository_root, | |
| rewrite_method, | |
| stdin_payload.as_deref().unwrap_or_default(), | |
| ); | |
| let outcome = | |
| stdin_payload.and_then(|_| run_post_rewrite_subcommand(repository_root, rewrite_method)); | |
| // Trace persistence is diagnostic only; hook execution should not fail if local trace writing fails. | |
| let _ = persist_hook_trace(repository_root, subcommand, &input, &outcome); | |
| outcome | |
| } | |
| fn run_post_rewrite_subcommand_with_trace( | |
| repository_root: &Path, | |
| subcommand: &HookSubcommand, | |
| rewrite_method: &str, | |
| ) -> Result<String> { | |
| let stdin_payload = read_hook_stdin().unwrap_or_default(); | |
| let input = build_hook_trace_input_for_post_rewrite( | |
| repository_root, | |
| rewrite_method, | |
| &stdin_payload, | |
| ); | |
| let outcome = run_post_rewrite_subcommand(repository_root, rewrite_method); | |
| // Trace persistence is diagnostic only; hook execution should not fail if local trace writing fails. | |
| let _ = persist_hook_trace(repository_root, subcommand, &input, &outcome); | |
| outcome | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/services/hooks.rs` around lines 162 - 180, read_hook_stdin() errors
are currently short-circuiting the hook run because outcome is computed as
stdin_payload.and_then(|_| run_post_rewrite_subcommand(...)). Change
run_post_rewrite_subcommand_with_trace so it first captures stdin_result =
read_hook_stdin() (best-effort), constructs input via
build_hook_trace_input_for_post_rewrite(...,
stdin_result.as_deref().unwrap_or_default()), then always call
run_post_rewrite_subcommand(repository_root, rewrite_method) to produce outcome
(regardless of stdin_result), and finally call
persist_hook_trace(repository_root, subcommand, &input, &outcome) while treating
any trace write/read errors as diagnostic-only; return the outcome from
run_post_rewrite_subcommand.
| fn build_hook_trace_file_name(subcommand: &HookSubcommand) -> String { | ||
| format!( | ||
| "{}-{}.json", | ||
| Utc::now().format("%Y-%m-%dT%H-%M-%S-%3fZ"), | ||
| hook_trace_name(subcommand) | ||
| ) | ||
| } |
There was a problem hiding this comment.
Filename collision within the same millisecond.
%Y-%m-%dT%H-%M-%S-%3fZ-{hook}.json grants ms granularity; back-to-back invocations (e.g. rapid post-rewrite during a rebase of many commits, or parallel hooks in a monorepo) can collide and overwrite a prior trace. Add a short random suffix or a per-process counter.
♻️ Proposed fix
fn build_hook_trace_file_name(subcommand: &HookSubcommand) -> String {
format!(
- "{}-{}.json",
+ "{}-{}-{}.json",
Utc::now().format("%Y-%m-%dT%H-%M-%S-%3fZ"),
- hook_trace_name(subcommand)
+ hook_trace_name(subcommand),
+ std::process::id()
)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/services/hooks.rs` around lines 227 - 233, The filename generation in
build_hook_trace_file_name (which uses hook_trace_name) can collide at ms
granularity; modify build_hook_trace_file_name to append a short
collision-resistant suffix (e.g., a per-process atomic counter or a small random
hex/uuid fragment) after the timestamp and hook name so filenames are unique for
rapid/parallel invocations; ensure the chosen approach is thread-safe (use an
AtomicUsize counter or thread_rng for randomness) and keep the suffix short (4–8
hex chars) to avoid overly long filenames.
| fn build_hook_trace_input_for_pre_commit(repository_root: &Path) -> Value { | ||
| let mut input = build_base_hook_trace_input("pre-commit"); | ||
| insert_staged_changes_from_git(repository_root, &mut input); | ||
| Value::Object(input) | ||
| } | ||
|
|
||
| fn build_hook_trace_input_for_commit_msg(repository_root: &Path, message_file: &Path) -> Value { | ||
| let mut input = build_base_hook_trace_input("commit-msg"); | ||
| insert_staged_changes_from_git(repository_root, &mut input); | ||
| input.insert( | ||
| "message_file".to_string(), | ||
| Value::String(message_file.display().to_string()), | ||
| ); | ||
|
|
||
| match fs::read_to_string(message_file) { | ||
| Ok(message_from_git) => { | ||
| input.insert( | ||
| "message_from_git".to_string(), | ||
| Value::String(message_from_git), | ||
| ); | ||
| } | ||
| Err(error) => { | ||
| input.insert( | ||
| "message_from_git_read_error".to_string(), | ||
| Value::String(error.to_string()), | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| Value::Object(input) | ||
| } | ||
|
|
||
| fn build_hook_trace_input_for_post_commit(repository_root: &Path) -> Value { | ||
| let mut input = build_base_hook_trace_input("post-commit"); | ||
| insert_head_commit_from_git(repository_root, &mut input); | ||
| Value::Object(input) | ||
| } | ||
|
|
||
| fn build_hook_trace_input_for_post_rewrite( | ||
| repository_root: &Path, | ||
| rewrite_method: &str, | ||
| stdin_payload: &str, | ||
| ) -> Value { | ||
| let mut input = build_base_hook_trace_input("post-rewrite"); | ||
| input.insert( | ||
| "rewrite_method".to_string(), | ||
| Value::String(rewrite_method.trim().to_string()), | ||
| ); | ||
| input.insert( | ||
| "stdin_from_git".to_string(), | ||
| Value::String(stdin_payload.to_string()), | ||
| ); | ||
| input.insert( | ||
| "rewritten_commits_from_git".to_string(), | ||
| Value::Array(parse_post_rewrite_pairs(repository_root, stdin_payload)), | ||
| ); | ||
|
|
||
| Value::Object(input) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Missing unit tests for the new trace input/persistence surface.
The trace-wrapper path is entirely new behavior (env-var capture, git command capture, stdin parsing for post-rewrite, JSON body shape with output vs error) and this file has substantial test scaffolding elsewhere. Per the repo's guideline to "add unit tests close to the code they exercise" with "focused behavioral test names", please add tests such as:
persist_hook_trace_writes_input_and_output_on_successpersist_hook_trace_writes_input_and_error_on_failurebuild_hook_trace_input_for_commit_msg_includes_message_from_gitparse_post_rewrite_pairs_handles_malformed_linesbuild_hook_trace_file_name_uses_hook_name_for_each_variant
As per coding guidelines: "Add unit tests close to the code they exercise" and "Match the repo's current pattern of focused behavioral test names".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/services/hooks.rs` around lines 244 - 302, Add unit tests adjacent to
the hooks code that exercise the new trace input/persistence behavior: create
tests named persist_hook_trace_writes_input_and_output_on_success and
persist_hook_trace_writes_input_and_error_on_failure that invoke the persist
function used by the trace-wrapper (persist_hook_trace or equivalent), simulate
a successful run vs a failing run, and assert the persisted JSON contains an
"input" object and either an "output" field on success or an "error" field on
failure; create build_hook_trace_input_for_commit_msg_includes_message_from_git
which calls build_hook_trace_input_for_commit_msg with a temporary message file
(and another case where reading fails) and asserts presence of
"message_from_git" or "message_from_git_read_error"; create
parse_post_rewrite_pairs_handles_malformed_lines which calls
parse_post_rewrite_pairs (and build_hook_trace_input_for_post_rewrite) with
well-formed and malformed stdin payloads and asserts malformed lines are ignored
or recorded as expected; and add
build_hook_trace_file_name_uses_hook_name_for_each_variant to call the filename
builder (build_hook_trace_file_name or equivalent) for each hook variant and
assert the returned file names include the hook name. Use temp dirs/files and
the existing test scaffolding/matchers pattern in the repo and reference the
functions build_hook_trace_input_for_commit_msg,
build_hook_trace_input_for_post_rewrite, parse_post_rewrite_pairs, and the
persist function in your assertions.
| fn build_base_hook_trace_input(hook_name: &str) -> serde_json::Map<String, Value> { | ||
| let mut input = serde_json::Map::new(); | ||
| input.insert("hook".to_string(), Value::String(hook_name.to_string())); | ||
| input.insert( | ||
| "git_env".to_string(), | ||
| Value::Object( | ||
| collect_git_environment() | ||
| .into_iter() | ||
| .map(|(key, value)| (key, Value::String(value))) | ||
| .collect(), | ||
| ), | ||
| ); | ||
| input | ||
| } | ||
|
|
||
| fn collect_git_environment() -> BTreeMap<String, String> { | ||
| std::env::vars() | ||
| .filter(|(key, _)| key.starts_with("GIT_")) | ||
| .collect() | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify context/tmp is covered by a gitignore rule shipped with setup.
echo '--- repo-root .gitignore ---'
[ -f .gitignore ] && cat .gitignore || echo '(no .gitignore)'
echo
echo '--- any gitignore referencing context/tmp ---'
rg -n --hidden -S 'context/tmp' -g '**/.gitignore' -g '**/*.gitignore.tmpl' -g '**/*.template' -g '**/*.json' -g '**/*.rs' -g '**/*.ts'
echo
echo '--- setup-owned gitignore writers ---'
rg -nP -C3 '\.gitignore|gitignore' cli/src/services/setup 2>/dev/null | head -120Repository: crocoder-dev/shared-context-engineering
Length of output: 348
🏁 Script executed:
#!/bin/bash
echo '=== Check for .gitignore in context/ subdirectories ==='
find . -path './context*/.gitignore' -o -path './context*/.gitignore*' 2>/dev/null | head -20
echo
echo '=== Check if context/ directory exists and what git knows about it ==='
git ls-files | grep -E '^context/' | head -20
echo
echo '=== Look for context/ in .git/config or documentation ==='
[ -d context ] && echo "context/ directory exists" || echo "context/ does not exist"
[ -d .sce ] && echo ".sce/ directory exists" || echo ".sce/ does not exist"
echo
echo '=== Check templates or setup scripts for gitignore creation ==='
find . -name '*.template' -o -name '*.tmpl' 2>/dev/null | head -20
echo
echo '=== Examine the full hooks.rs file for trace persistence ==='
wc -l cli/src/services/hooks.rsRepository: crocoder-dev/shared-context-engineering
Length of output: 1231
🏁 Script executed:
#!/bin/bash
echo '=== Search for where context/tmp is used in the codebase ==='
rg -n 'context/tmp' --type rs --type ts
echo
echo '=== Search for trace file writing patterns ==='
rg -n -A3 'serde_json|write_' cli/src/services/hooks.rs | head -100Repository: crocoder-dev/shared-context-engineering
Length of output: 3846
🏁 Script executed:
#!/bin/bash
echo '=== Contents of context/tmp/.gitignore ==='
cat -n ./context/tmp/.gitignore
echo
echo '=== Search for persist_hook_trace function definition ==='
rg -n -A20 'fn persist_hook_trace' cli/src/services/hooks.rsRepository: crocoder-dev/shared-context-engineering
Length of output: 925
🏁 Script executed:
#!/bin/bash
echo '=== Search for trace file path construction ==='
rg -n 'trace_dir|file_path|hook.*json' cli/src/services/hooks.rs | head -40
echo
echo '=== Examine the persist_hook_trace and surrounding code (lines 200-230) ==='
sed -n '200,240p' cli/src/services/hooks.rsRepository: crocoder-dev/shared-context-engineering
Length of output: 1925
🏁 Script executed:
#!/bin/bash
echo '=== Search for cleanup/rotation of hook traces ==='
rg -n 'context/tmp|trace.*clean|retain|rotate|age|size' cli/src/services/ --type rs
echo
echo '=== Search for trace directory management in setup ==='
rg -n -B3 -A3 'context.*tmp|tmp.*gitignore|gitignore.*context' cli/src/services/setup.rs
echo
echo '=== Check if context/tmp/.gitignore is tracked by git ==='
git ls-files | grep 'context/tmp'
echo
echo '=== Look for any TODO or FIXME comments in hooks.rs about persistence ==='
rg -n 'TODO|FIXME|XXX' cli/src/services/hooks.rsRepository: crocoder-dev/shared-context-engineering
Length of output: 390
🏁 Script executed:
#!/bin/bash
echo '=== Verify the exact data being collected in git_env ==='
sed -n '315,330p' cli/src/services/hooks.rs
echo
echo '=== Check what other git output is being captured ==='
sed -n '330,370p' cli/src/services/hooks.rsRepository: crocoder-dev/shared-context-engineering
Length of output: 1854
Unbounded trace accumulation: Hook invocations persist indefinitely to context/tmp/.
Each hook invocation serializes sensitive data to disk:
GIT_AUTHOR_EMAIL,GIT_COMMITTER_EMAIL,GIT_AUTHOR_NAME,GIT_COMMITTER_NAMEviacollect_git_environment()— PII- Full
git diff --cached --patchandgit show --format= --patch HEAD— may contain secrets, API keys, or sensitive data in patched files - Raw commit messages and post-rewrite patches between arbitrary OIDs
Mitigation: The repository ships context/tmp/.gitignore with pattern * excluding all trace files from version control, so accidental commit via git add . is prevented.
Remaining concern: Traces accumulate unboundedly on the local filesystem with no retention/rotation policy. This directory grows with every commit and retains PII indefinitely. Consider implementing a size or age cap, or gating trace persistence behind an explicit opt-in flag (e.g., SCE_HOOK_TRACE=1) so this diagnostic feature is not enabled by default.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/services/hooks.rs` around lines 304 - 323, The hook tracing currently
writes sensitive git env and diffs unconditionally via
build_base_hook_trace_input and collect_git_environment and leaves files in
context/tmp/ indefinitely; change it so trace persistence is disabled by default
and only enabled when an explicit opt-in env var (e.g., SCE_HOOK_TRACE=1) is
present, avoid serializing PII by filtering out keys like GIT_AUTHOR_EMAIL,
GIT_COMMITTER_EMAIL, GIT_AUTHOR_NAME, GIT_COMMITTER_NAME in
collect_git_environment, and implement retention limits for the trace directory
(either rotate by max total size or delete files older than a configurable age)
so traces in context/tmp/ are automatically pruned.
fe5c27e to
eea0402
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
cli/src/services/hooks.rs (2)
304-323:⚠️ Potential issue | 🟠 MajorPII/secret exposure via unbounded trace persistence remains unaddressed.
build_base_hook_trace_input+collect_git_environmentstill serialize everyGIT_*env var (includingGIT_AUTHOR_EMAIL,GIT_COMMITTER_EMAIL,GIT_AUTHOR_NAME,GIT_COMMITTER_NAME, and potentially credential-adjacent ones likeGIT_ASKPASS/GIT_SSH_COMMAND) plus full staged/HEAD/rewrite patches tocontext/tmp/on every hook invocation, with no retention policy and no opt-in. Even thoughcontext/tmp/.gitignoreprevents accidental commits, this leaves PII/diff content accumulating on disk indefinitely by default.At minimum, please gate persistence behind an explicit opt-in (e.g.,
SCE_HOOK_TRACE=1), filter identity-bearingGIT_*keys out ofcollect_git_environment, and add a rotation/size/age cap oncontext/tmp/.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/services/hooks.rs` around lines 304 - 323, The hook tracing code unconditionally collects and persists all GIT_* env vars (via collect_git_environment) and writes full diffs to context/tmp, risking PII/secret exposure; change build_base_hook_trace_input to only run when an explicit opt-in env var (e.g., SCE_HOOK_TRACE="1") is present, update collect_git_environment to exclude identity/credential-related keys (at minimum filter out GIT_AUTHOR_EMAIL, GIT_COMMITTER_EMAIL, GIT_AUTHOR_NAME, GIT_COMMITTER_NAME, GIT_ASKPASS, GIT_SSH_COMMAND and any other sensitive names), and implement a rotation/retention policy for context/tmp (e.g., cap total size, remove files older than N days, or keep only the last M traces) so traces are not retained indefinitely; locate these changes around the functions build_base_hook_trace_input and collect_git_environment and add the env-check and retention logic where traces are written.
167-180:⚠️ Potential issue | 🟠 Major
post-rewritestdin read failure still aborts the hook (unchanged from prior review).
outcome = stdin_payload.and_then(|_| run_post_rewrite_subcommand(...))still elevates a diagnostic-only STDIN capture failure into a hook failure. Decouple tracing from execution: capture STDIN best-effort and always invokerun_post_rewrite_subcommand, so hook semantics are unaffected when tracing fails.♻️ Proposed decoupling
- let stdin_payload = read_hook_stdin(); - let input = build_hook_trace_input_for_post_rewrite( - repository_root, - rewrite_method, - stdin_payload.as_deref().unwrap_or_default(), - ); - let outcome = - stdin_payload.and_then(|_| run_post_rewrite_subcommand(repository_root, rewrite_method)); + let stdin_payload = read_hook_stdin().unwrap_or_default(); + let input = build_hook_trace_input_for_post_rewrite( + repository_root, + rewrite_method, + &stdin_payload, + ); + let outcome = run_post_rewrite_subcommand(repository_root, rewrite_method);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/services/hooks.rs` around lines 167 - 180, The current code uses stdin_payload.and_then(|_| run_post_rewrite_subcommand(...)) which aborts hook execution when the diagnostic STDIN capture fails; instead, call read_hook_stdin and build_hook_trace_input_for_post_rewrite as you already do, but always invoke run_post_rewrite_subcommand(repository_root, rewrite_method) to produce outcome regardless of stdin_payload, then call persist_hook_trace(repository_root, subcommand, &input, &outcome) best-effort; in short, replace the and_then-based outcome assignment with a direct call to run_post_rewrite_subcommand so tracing remains diagnostic-only while hook execution always runs (referencing read_hook_stdin, build_hook_trace_input_for_post_rewrite, run_post_rewrite_subcommand, and persist_hook_trace).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/lib/agent-trace-plugin/opencode-sce-agent-trace-plugin.ts`:
- Around line 65-72: The event handler currently calls
buildTrace(traceDirectory, input) for every OpenCode event which creates a file
per event and can flood context/tmp; change the logic inside the event handler
(the anonymous event async function which calls buildTrace) to first filter
input.type against a small allow-list of event types you actually care about
(e.g., "tool.execute", "session.start", etc.), then update buildTrace usage to
append events into a single rolling NDJSON file in traceDirectory (instead of
creating one file per event) and add a retention/enforcement step in the same
module to trim older entries or rotate files when total files/bytes exceed
configured maxima; also apply the same allow-list/filtering for the
"tool.execute.before" hook or narrow it to only call buildTrace when output is
present. Ensure references: event handler, "tool.execute.before", buildTrace,
traceDirectory/context/tmp.
---
Duplicate comments:
In `@cli/src/services/hooks.rs`:
- Around line 304-323: The hook tracing code unconditionally collects and
persists all GIT_* env vars (via collect_git_environment) and writes full diffs
to context/tmp, risking PII/secret exposure; change build_base_hook_trace_input
to only run when an explicit opt-in env var (e.g., SCE_HOOK_TRACE="1") is
present, update collect_git_environment to exclude identity/credential-related
keys (at minimum filter out GIT_AUTHOR_EMAIL, GIT_COMMITTER_EMAIL,
GIT_AUTHOR_NAME, GIT_COMMITTER_NAME, GIT_ASKPASS, GIT_SSH_COMMAND and any other
sensitive names), and implement a rotation/retention policy for context/tmp
(e.g., cap total size, remove files older than N days, or keep only the last M
traces) so traces are not retained indefinitely; locate these changes around the
functions build_base_hook_trace_input and collect_git_environment and add the
env-check and retention logic where traces are written.
- Around line 167-180: The current code uses stdin_payload.and_then(|_|
run_post_rewrite_subcommand(...)) which aborts hook execution when the
diagnostic STDIN capture fails; instead, call read_hook_stdin and
build_hook_trace_input_for_post_rewrite as you already do, but always invoke
run_post_rewrite_subcommand(repository_root, rewrite_method) to produce outcome
regardless of stdin_payload, then call persist_hook_trace(repository_root,
subcommand, &input, &outcome) best-effort; in short, replace the and_then-based
outcome assignment with a direct call to run_post_rewrite_subcommand so tracing
remains diagnostic-only while hook execution always runs (referencing
read_hook_stdin, build_hook_trace_input_for_post_rewrite,
run_post_rewrite_subcommand, and persist_hook_trace).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0a169f46-98ec-40dc-87fc-3839c950b682
📒 Files selected for processing (6)
.opencode/plugins/sce-agent-trace.tscli/src/app.rscli/src/services/hooks.rsconfig/.opencode/plugins/sce-agent-trace.tsconfig/automated/.opencode/plugins/sce-agent-trace.tsconfig/lib/agent-trace-plugin/opencode-sce-agent-trace-plugin.ts
| return { | ||
| event: async (input) => { | ||
| await buildTrace(traceDirectory, input); | ||
| }, | ||
| "tool.execute.before": async (input, output) => { | ||
| await buildTrace(traceDirectory, input, output); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Unbounded trace volume: event handler writes a file for every OpenCode event.
The event hook fires for all event types (file watcher updates, session events, etc.), not just ones of interest. Combined with per-invocation file writes to <repoRoot>/context/tmp, a single session can produce thousands of JSON files with no rotation, size cap, or event-type filter. This is both an I/O hot path on every event and an unbounded disk-growth risk.
Consider:
- Filtering
event.typeto a known allow-list before persisting. - Batching/appending to a single rolling file (e.g., NDJSON) rather than one file per event.
- Enforcing a retention policy (max files / max bytes) on
context/tmp.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/lib/agent-trace-plugin/opencode-sce-agent-trace-plugin.ts` around
lines 65 - 72, The event handler currently calls buildTrace(traceDirectory,
input) for every OpenCode event which creates a file per event and can flood
context/tmp; change the logic inside the event handler (the anonymous event
async function which calls buildTrace) to first filter input.type against a
small allow-list of event types you actually care about (e.g., "tool.execute",
"session.start", etc.), then update buildTrace usage to append events into a
single rolling NDJSON file in traceDirectory (instead of creating one file per
event) and add a retention/enforcement step in the same module to trim older
entries or rotate files when total files/bytes exceed configured maxima; also
apply the same allow-list/filtering for the "tool.execute.before" hook or narrow
it to only call buildTrace when output is present. Ensure references: event
handler, "tool.execute.before", buildTrace, traceDirectory/context/tmp.
eea0402 to
98419f4
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (10)
config/lib/agent-trace-plugin/opencode-sce-agent-trace-plugin.ts (3)
23-25:⚠️ Potential issue | 🟡 MinorFilename collision risk remains — ms timestamp +
traceNameis not unique.Append a short random suffix (
randomUUID().slice(0, 8)) tobuildTraceFileNameso concurrent same-ms events of the same type don't overwrite each other.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/lib/agent-trace-plugin/opencode-sce-agent-trace-plugin.ts` around lines 23 - 25, The filename generation in buildTraceFileName uses only formatTimestamp(date) and traceName which can collide for concurrent events; update buildTraceFileName to append a short random suffix by importing/using randomUUID() and adding randomUUID().slice(0,8) (e.g., `${formatTimestamp(date)}-${traceName}-${randomUUID().slice(0,8)}.json`) so same-ms same-type events produce unique filenames; ensure the randomUUID import is added where needed.
50-57:⚠️ Potential issue | 🟠 MajorWrap
buildTracein try/catch so host event hooks never reject.As flagged on previous commits,
mkdir/writeFile/JSON.stringifycan throw, and theeventhandler awaits this directly — a diagnostic plugin must not be able to break the host. Catch and log viaconsole.error, converting the unknown with agetErrorMessage-style helper.Based on learnings: "Convert unknown thrown values with helper functions like
getErrorMessagebefore logging or persisting in TypeScript".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/lib/agent-trace-plugin/opencode-sce-agent-trace-plugin.ts` around lines 50 - 57, The buildTrace function can throw from JSON.stringify, mkdir, or writeFile and must not let the host event handler reject; wrap the entire body of buildTrace (the code that computes filePath, serializes body, calls mkdir and writeFile) in a try/catch, and on error call console.error with a descriptive message plus the converted error string using a getErrorMessage-style helper (convert unknown thrown values before logging). Ensure the function still returns a resolved Promise (no rethrow) so the event hooks never reject.
59-80: 🛠️ Refactor suggestion | 🟠 MajorShared-lib file is not actually imported by the three registration copies.
The three plugin-registration files (
.opencode/plugins/sce-agent-trace.ts,config/.opencode/plugins/sce-agent-trace.ts,config/automated/.opencode/plugins/sce-agent-trace.ts) still duplicate the implementation rather than re-exporting from this shared module, defeating the purpose of having a shared lib. Update them to re-exportSceAgentTracePluginfrom here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/lib/agent-trace-plugin/opencode-sce-agent-trace-plugin.ts` around lines 59 - 80, The three registration files that currently duplicate the plugin implementation should be changed to re-export the shared SceAgentTracePlugin instead of duplicating code; locate the duplicated plugin modules and replace their bodies with a single re-export of the named export SceAgentTracePlugin from this shared module so consumers import the plugin implementation from this file only.config/automated/.opencode/plugins/sce-agent-trace.ts (2)
1-80: 🛠️ Refactor suggestion | 🟠 MajorThird copy of the same implementation — re-export from
config/lib/agent-trace-plugin/...instead.This file is identical to
config/.opencode/plugins/sce-agent-trace.tsand to the sharedconfig/lib/agent-trace-plugin/opencode-sce-agent-trace-plugin.ts. Replace the body with a single re-export so bug fixes (error handling, filename collisions, allowlist tuning) only need to land in one place.♻️ Proposed fix
-import { mkdir, writeFile } from "node:fs/promises"; -import path from "node:path"; -import type { Plugin } from "@opencode-ai/plugin"; -// ... (rest of duplicated implementation) +export { SceAgentTracePlugin } from "../../../lib/agent-trace-plugin/opencode-sce-agent-trace-plugin";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/automated/.opencode/plugins/sce-agent-trace.ts` around lines 1 - 80, This file duplicates the SceAgentTracePlugin implementation; replace the duplicate by re-exporting the canonical implementation from the shared module so fixes land in one place: remove the local implementations (getTraceName, buildTraceFileName, buildTrace, shouldCaptureEvent, formatTimestamp, ALL_CAPTURED_EVENTS, REQUIRED_EVENTS) and export SceAgentTracePlugin from the shared module path (the module that currently contains opencode-sce-agent-trace-plugin.ts), ensuring the exported symbol name remains SceAgentTracePlugin so callers keep working.
50-57:⚠️ Potential issue | 🟠 MajorSame missing error handling on
buildTrace.See the detailed comment on
.opencode/plugins/sce-agent-trace.ts— any fs/serialization failure will reject theeventhook. Wrap in try/catch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/automated/.opencode/plugins/sce-agent-trace.ts` around lines 50 - 57, The buildTrace function can throw on JSON.stringify or fs ops and currently will reject the caller; wrap the entire body of buildTrace (including calls to getTraceName, buildTraceFileName, mkdir, writeFile and JSON.stringify) in a try/catch, and in the catch log the error (including the error object) via the existing logger or console with a clear message like "buildTrace failed" and then swallow the error so the event hook is not rejected; keep the function signature and ensure you still await mkdir/writeFile inside the try block.config/.opencode/plugins/sce-agent-trace.ts (2)
50-57:⚠️ Potential issue | 🟠 MajorMissing error handling — same issue as the root copy.
buildTracelacks try/catch; any fs orJSON.stringifyfailure rejects theeventhook. See the detailed comment on.opencode/plugins/sce-agent-trace.ts— this copy has the same defect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/.opencode/plugins/sce-agent-trace.ts` around lines 50 - 57, The buildTrace function currently has no error handling and can reject the calling event hook on JSON or filesystem failures; wrap the body of buildTrace (including getTraceName, buildTraceFileName, JSON.stringify, mkdir, and writeFile calls) in a try/catch, and in the catch log the error (including contextual info such as traceDirectory and the result of getTraceName(input)) and gracefully return without throwing so the event hook is not rejected; ensure the catch uses the project's logger or console as used elsewhere to keep behavior consistent.
1-80: 🛠️ Refactor suggestion | 🟠 MajorEliminate duplication — re-export from the shared library.
This file is byte-identical to
config/automated/.opencode/plugins/sce-agent-trace.tsandconfig/lib/agent-trace-plugin/opencode-sce-agent-trace-plugin.ts, and near-identical to.opencode/plugins/sce-agent-trace.ts. Maintaining 3–4 copies guarantees drift (the allowlists already diverge between the root andconfig/copies). Replace this file's body with a re-export from the shared lib so there is a single source of truth.♻️ Proposed fix
-import { mkdir, writeFile } from "node:fs/promises"; -import path from "node:path"; -import type { Plugin } from "@opencode-ai/plugin"; - -const REQUIRED_EVENTS = new Set([ - "session.diff", - "message.updated", - "message.part.updated", -]); -// ... (rest of duplicated implementation) +export { SceAgentTracePlugin } from "../../lib/agent-trace-plugin/opencode-sce-agent-trace-plugin";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/.opencode/plugins/sce-agent-trace.ts` around lines 1 - 80, This file is a duplicate of the shared agent-trace implementation; remove the local implementation and replace it with a single re-export that forwards the canonical SceAgentTracePlugin symbol from the shared agent-trace module (i.e., delete the functions formatTimestamp/getTraceName/buildTrace/shouldCaptureEvent and the local export and instead export SceAgentTracePlugin from the shared module), so there is one source of truth for SceAgentTracePlugin..opencode/plugins/sce-agent-trace.ts (3)
30-32:⚠️ Potential issue | 🟡 MinorFilename collision: ms-granularity timestamp +
traceNamecan overwrite prior traces.Two events in the same millisecond with the same derived
traceName(e.g. burstymessage.part.updated) produce identical filenames and silently overwrite each other. Append a short random suffix (randomUUID().slice(0, 8)) or monotonic counter to guarantee uniqueness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.opencode/plugins/sce-agent-trace.ts around lines 30 - 32, The current buildTraceFileName(traceName: string, date: Date) can produce colliding filenames when two events share the same millisecond and traceName; update buildTraceFileName to append a short unique suffix (e.g. randomUUID().slice(0,8) or a monotonic counter) to the returned string so filenames are unique, and ensure you import or reference the generator you choose (e.g. randomUUID) and include the suffix before the ".json" extension (keep formatTimestamp(date) and traceName intact).
57-64:⚠️ Potential issue | 🟠 MajorMissing error handling — rejections from
buildTracesurface to the OpenCode host.
mkdir,writeFile, andJSON.stringify(e.g. on a payload containing aBigIntor circular reference) can all throw. TheeventhandlerawaitsbuildTracedirectly (Line 84), so any fs or serialization failure rejects the hook on every matching event. A diagnostic plugin should never be able to break the host session — wrapbuildTracein try/catch and log viaconsole.error(converting the unknown via agetErrorMessage-style helper), consistent with other trace-plugin variants.🛡️ Proposed fix
async function buildTrace(traceDirectory: string, input: unknown): Promise<void> { - const now = new Date(); - const filePath = path.join(traceDirectory, buildTraceFileName(getTraceName(input), now)); - const body = JSON.stringify({ input }, null, 2); - - await mkdir(traceDirectory, { recursive: true }); - await writeFile(filePath, body, "utf8"); + try { + const now = new Date(); + const filePath = path.join(traceDirectory, buildTraceFileName(getTraceName(input), now)); + const body = JSON.stringify({ input }, null, 2); + + await mkdir(traceDirectory, { recursive: true }); + await writeFile(filePath, body, "utf8"); + } catch (error) { + console.error(`[sce-agent-trace] Failed to persist trace: ${error instanceof Error ? error.message : String(error)}`); + } }Based on learnings: "Convert unknown thrown values with helper functions like
getErrorMessagebefore logging or persisting in TypeScript".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.opencode/plugins/sce-agent-trace.ts around lines 57 - 64, buildTrace currently lets JSON.stringify, mkdir, and writeFile throw and those rejections bubble up to the host; wrap the contents of buildTrace (including JSON.stringify and the fs calls) in a try/catch so failures are caught, convert the caught unknown to a string using a getErrorMessage-style helper, log the error with console.error (including trace name from getTraceName/buildTraceFileName for context), and ensure the function does not rethrow so the event handler that awaits buildTrace cannot break the host session.
66-87:⚠️ Potential issue | 🟠 MajorUnbounded trace volume on
context/tmp.Even with the allowlist, high-frequency events like
message.part.updatedcan produce thousands of JSON files per session with no rotation, size cap, or retention. Consider appending to a rolling NDJSON file or enforcing a retention policy (max files / max bytes).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.opencode/plugins/sce-agent-trace.ts around lines 66 - 87, SceAgentTracePlugin currently writes unbounded per-event JSON files into traceDirectory causing runaway disk usage; change the event handler/buildTrace flow to append each TraceInput as a single NDJSON record into a rolling log (e.g., traces.ndjson) instead of creating one file per event and implement rotation/retention (rotate when file > MAX_BYTES or keep only MAX_FILES/retain latest N days). Modify buildTrace (or add a new appendTrace helper called from the event handler) to atomically append newline-delimited JSON, perform size checks and rotate by renaming/compressing old file or truncating, and add configurable constants (MAX_BYTES, MAX_FILES) and a cleanup routine; keep shouldCaptureEvent filtering unchanged and ensure traceDirectory is created before writing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.opencode/plugins/sce-agent-trace.ts:
- Around line 5-18: Multiple identical plugin copies define event sets
(REQUIRED_EVENTS, OPTIONAL_EVENTS, ALL_CAPTURED_EVENTS) instead of reusing the
shared library which contains the canonical 7-event set; either update the three
duplicated plugin files to import and re-export the shared definitions
(REQUIRED_EVENTS, OPTIONAL_EVENTS, ALL_CAPTURED_EVENTS) from the shared module
so they all use the same source of truth, or if the shared module is unused
delete it and keep a single canonical definition; also add a short comment near
the root version's sets explaining why it intentionally includes the two extra
events (file.edited, file.watcher.updated) if that divergence is required for
dev/debug scenarios.
In `@cli/src/services/hooks.rs`:
- Around line 51-53: The three passthrough wrappers
run_pre_commit_subcommand_with_trace, run_commit_msg_subcommand_with_trace, and
run_post_rewrite_subcommand_with_trace should consistently persist traces like
run_post_commit_subcommand_with_trace: build the proper hook trace input
(implement or call build_hook_trace_input_for_pre_commit,
build_hook_trace_input_for_commit_msg, and
build_hook_trace_input_for_post_rewrite), ensure
run_commit_msg_subcommand_with_trace uses its subcommand parameter (not `_`),
and for post-rewrite use the captured stdin_payload as the input; then call
persist_hook_trace with that input before returning the underlying subcommand
result so the `_with_trace` name accurately reflects behavior.
In `@config/.opencode/plugins/sce-agent-trace.ts`:
- Line 11: The alias ALL_CAPTURED_EVENTS simply duplicates REQUIRED_EVENTS and
adds unnecessary indirection; update shouldCaptureEvent to reference
REQUIRED_EVENTS directly and remove the ALL_CAPTURED_EVENTS constant (or
alternatively add an OPTIONAL_EVENTS set and build ALL_CAPTURED_EVENTS from
REQUIRED_EVENTS ∪ OPTIONAL_EVENTS if optional events are intended). Modify the
code that currently imports/uses ALL_CAPTURED_EVENTS to use REQUIRED_EVENTS (or
switch to the combined set if you add OPTIONAL_EVENTS), and delete the redundant
const ALL_CAPTURED_EVENTS = REQUIRED_EVENTS declaration.
In `@config/lib/agent-trace-plugin/opencode-sce-agent-trace-plugin.ts`:
- Line 11: Remove the redundant alias ALL_CAPTURED_EVENTS and update code that
references it (notably shouldCaptureEvent) to use REQUIRED_EVENTS directly;
delete the declaration "const ALL_CAPTURED_EVENTS = REQUIRED_EVENTS" and replace
any uses of ALL_CAPTURED_EVENTS with REQUIRED_EVENTS so the code no longer
maintains a dead abstraction unless/ until an OPTIONAL_EVENTS set is introduced.
---
Duplicate comments:
In @.opencode/plugins/sce-agent-trace.ts:
- Around line 30-32: The current buildTraceFileName(traceName: string, date:
Date) can produce colliding filenames when two events share the same millisecond
and traceName; update buildTraceFileName to append a short unique suffix (e.g.
randomUUID().slice(0,8) or a monotonic counter) to the returned string so
filenames are unique, and ensure you import or reference the generator you
choose (e.g. randomUUID) and include the suffix before the ".json" extension
(keep formatTimestamp(date) and traceName intact).
- Around line 57-64: buildTrace currently lets JSON.stringify, mkdir, and
writeFile throw and those rejections bubble up to the host; wrap the contents of
buildTrace (including JSON.stringify and the fs calls) in a try/catch so
failures are caught, convert the caught unknown to a string using a
getErrorMessage-style helper, log the error with console.error (including trace
name from getTraceName/buildTraceFileName for context), and ensure the function
does not rethrow so the event handler that awaits buildTrace cannot break the
host session.
- Around line 66-87: SceAgentTracePlugin currently writes unbounded per-event
JSON files into traceDirectory causing runaway disk usage; change the event
handler/buildTrace flow to append each TraceInput as a single NDJSON record into
a rolling log (e.g., traces.ndjson) instead of creating one file per event and
implement rotation/retention (rotate when file > MAX_BYTES or keep only
MAX_FILES/retain latest N days). Modify buildTrace (or add a new appendTrace
helper called from the event handler) to atomically append newline-delimited
JSON, perform size checks and rotate by renaming/compressing old file or
truncating, and add configurable constants (MAX_BYTES, MAX_FILES) and a cleanup
routine; keep shouldCaptureEvent filtering unchanged and ensure traceDirectory
is created before writing.
In `@config/.opencode/plugins/sce-agent-trace.ts`:
- Around line 50-57: The buildTrace function currently has no error handling and
can reject the calling event hook on JSON or filesystem failures; wrap the body
of buildTrace (including getTraceName, buildTraceFileName, JSON.stringify,
mkdir, and writeFile calls) in a try/catch, and in the catch log the error
(including contextual info such as traceDirectory and the result of
getTraceName(input)) and gracefully return without throwing so the event hook is
not rejected; ensure the catch uses the project's logger or console as used
elsewhere to keep behavior consistent.
- Around line 1-80: This file is a duplicate of the shared agent-trace
implementation; remove the local implementation and replace it with a single
re-export that forwards the canonical SceAgentTracePlugin symbol from the shared
agent-trace module (i.e., delete the functions
formatTimestamp/getTraceName/buildTrace/shouldCaptureEvent and the local export
and instead export SceAgentTracePlugin from the shared module), so there is one
source of truth for SceAgentTracePlugin.
In `@config/automated/.opencode/plugins/sce-agent-trace.ts`:
- Around line 1-80: This file duplicates the SceAgentTracePlugin implementation;
replace the duplicate by re-exporting the canonical implementation from the
shared module so fixes land in one place: remove the local implementations
(getTraceName, buildTraceFileName, buildTrace, shouldCaptureEvent,
formatTimestamp, ALL_CAPTURED_EVENTS, REQUIRED_EVENTS) and export
SceAgentTracePlugin from the shared module path (the module that currently
contains opencode-sce-agent-trace-plugin.ts), ensuring the exported symbol name
remains SceAgentTracePlugin so callers keep working.
- Around line 50-57: The buildTrace function can throw on JSON.stringify or fs
ops and currently will reject the caller; wrap the entire body of buildTrace
(including calls to getTraceName, buildTraceFileName, mkdir, writeFile and
JSON.stringify) in a try/catch, and in the catch log the error (including the
error object) via the existing logger or console with a clear message like
"buildTrace failed" and then swallow the error so the event hook is not
rejected; keep the function signature and ensure you still await mkdir/writeFile
inside the try block.
In `@config/lib/agent-trace-plugin/opencode-sce-agent-trace-plugin.ts`:
- Around line 23-25: The filename generation in buildTraceFileName uses only
formatTimestamp(date) and traceName which can collide for concurrent events;
update buildTraceFileName to append a short random suffix by importing/using
randomUUID() and adding randomUUID().slice(0,8) (e.g.,
`${formatTimestamp(date)}-${traceName}-${randomUUID().slice(0,8)}.json`) so
same-ms same-type events produce unique filenames; ensure the randomUUID import
is added where needed.
- Around line 50-57: The buildTrace function can throw from JSON.stringify,
mkdir, or writeFile and must not let the host event handler reject; wrap the
entire body of buildTrace (the code that computes filePath, serializes body,
calls mkdir and writeFile) in a try/catch, and on error call console.error with
a descriptive message plus the converted error string using a
getErrorMessage-style helper (convert unknown thrown values before logging).
Ensure the function still returns a resolved Promise (no rethrow) so the event
hooks never reject.
- Around line 59-80: The three registration files that currently duplicate the
plugin implementation should be changed to re-export the shared
SceAgentTracePlugin instead of duplicating code; locate the duplicated plugin
modules and replace their bodies with a single re-export of the named export
SceAgentTracePlugin from this shared module so consumers import the plugin
implementation from this file only.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4306e573-a424-428d-9df5-48832546398c
📒 Files selected for processing (6)
.opencode/plugins/sce-agent-trace.tscli/src/app.rscli/src/services/hooks.rsconfig/.opencode/plugins/sce-agent-trace.tsconfig/automated/.opencode/plugins/sce-agent-trace.tsconfig/lib/agent-trace-plugin/opencode-sce-agent-trace-plugin.ts
| const REQUIRED_EVENTS = new Set([ | ||
| "session.created", | ||
| "session.updated", | ||
| "session.diff", | ||
| "message.updated", | ||
| "message.part.updated", | ||
| ]); | ||
|
|
||
| const OPTIONAL_EVENTS = new Set([ | ||
| "file.edited", | ||
| "file.watcher.updated", | ||
| ]); | ||
|
|
||
| const ALL_CAPTURED_EVENTS = new Set([...REQUIRED_EVENTS, ...OPTIONAL_EVENTS]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
find . -name "*sce-agent-trace*" -type f 2>/dev/null | head -20Repository: crocoder-dev/shared-context-engineering
Length of output: 290
🏁 Script executed:
git ls-files | grep -E "(\.opencode|config)" | head -30Repository: crocoder-dev/shared-context-engineering
Length of output: 1215
🏁 Script executed:
echo "=== Root file ===" && cat -n .opencode/plugins/sce-agent-trace.ts && echo -e "\n=== config/ file ===" && cat -n config/.opencode/plugins/sce-agent-trace.ts && echo -e "\n=== config/automated/ file ===" && cat -n config/automated/.opencode/plugins/sce-agent-trace.ts && echo -e "\n=== Shared lib file ===" && cat -n config/lib/agent-trace-plugin/opencode-sce-agent-trace-plugin.tsRepository: crocoder-dev/shared-context-engineering
Length of output: 11058
🏁 Script executed:
rg "agent-trace-plugin/opencode-sce-agent-trace-plugin" --type ts --type js 2>/dev/null | head -20Repository: crocoder-dev/shared-context-engineering
Length of output: 65
🏁 Script executed:
rg -i "trace.*plugin\|plugin.*trace" --type md config/ .opencode/ | grep -i "intentional\|drift\|diverge\|development\|distributed\|config" | head -20Repository: crocoder-dev/shared-context-engineering
Length of output: 65
🏁 Script executed:
find config -name "*.md" -o -name "*.txt" | xargs grep -l "sce-agent-trace\|trace.*plugin" 2>/dev/nullRepository: crocoder-dev/shared-context-engineering
Length of output: 67
Consolidate three duplicate sce-agent-trace plugin copies to use shared library.
The root version intentionally captures 7 events (5 required + 2 optional: file.edited, file.watcher.updated) while config/.opencode/plugins/sce-agent-trace.ts, config/automated/.opencode/plugins/sce-agent-trace.ts, and config/lib/agent-trace-plugin/opencode-sce-agent-trace-plugin.ts capture only 3 required events. However, the three config versions are identical duplicates, and the shared library is not imported or referenced anywhere. Either consolidate the config copies to use the shared library to eliminate duplication, or remove the shared library if it's dead code. Document the root version's intentional divergence if it's meant for development/debug scenarios.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.opencode/plugins/sce-agent-trace.ts around lines 5 - 18, Multiple identical
plugin copies define event sets (REQUIRED_EVENTS, OPTIONAL_EVENTS,
ALL_CAPTURED_EVENTS) instead of reusing the shared library which contains the
canonical 7-event set; either update the three duplicated plugin files to import
and re-export the shared definitions (REQUIRED_EVENTS, OPTIONAL_EVENTS,
ALL_CAPTURED_EVENTS) from the shared module so they all use the same source of
truth, or if the shared module is unused delete it and keep a single canonical
definition; also add a short comment near the root version's sets explaining why
it intentionally includes the two extra events (file.edited,
file.watcher.updated) if that divergence is required for dev/debug scenarios.
| fn run_pre_commit_subcommand_with_trace(repository_root: &Path) -> Result<String> { | ||
| run_pre_commit_subcommand(repository_root) | ||
| } |
There was a problem hiding this comment.
Inconsistent/incomplete trace persistence across hook wrappers.
Only run_post_commit_subcommand_with_trace actually invokes persist_hook_trace. The three other _with_trace wrappers do not:
- Lines 51-53:
run_pre_commit_subcommand_with_traceis a pure passthrough torun_pre_commit_subcommand. - Lines 111-117:
run_commit_msg_subcommand_with_traceis a pure passthrough; thesubcommand: &HookSubcommandparameter is unused (_:). - Lines 148-155:
run_post_rewrite_subcommand_with_tracereads STDIN but never builds input or callspersist_hook_trace; the captured payload is discarded viaand_then(|_| ...).
Given the AI summary/PR objectives claim traces are emitted "per-hook" and the OpenCode plugin counterpart writes traces for multiple hooks, this looks like an incomplete implementation rather than an intentional post-commit-only trace. Either:
- Wire up
persist_hook_trace+ dedicatedbuild_hook_trace_input_for_*builders for pre-commit, commit-msg, and post-rewrite (the post-rewrite case should usestdin_payloadas its input per the AI summary), or - If only post-commit tracing is intended for this PR, remove the other
_with_tracewrappers and call the underlying functions directly so the naming doesn't overstate behavior.
As per coding guidelines: "Prefer names that encode intent, not implementation trivia" — *_with_trace currently misleads on three of four hooks.
Also applies to: 111-117, 128-136, 148-155
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/services/hooks.rs` around lines 51 - 53, The three passthrough
wrappers run_pre_commit_subcommand_with_trace,
run_commit_msg_subcommand_with_trace, and run_post_rewrite_subcommand_with_trace
should consistently persist traces like run_post_commit_subcommand_with_trace:
build the proper hook trace input (implement or call
build_hook_trace_input_for_pre_commit, build_hook_trace_input_for_commit_msg,
and build_hook_trace_input_for_post_rewrite), ensure
run_commit_msg_subcommand_with_trace uses its subcommand parameter (not `_`),
and for post-rewrite use the captured stdin_payload as the input; then call
persist_hook_trace with that input before returning the underlying subcommand
result so the `_with_trace` name accurately reflects behavior.
| "message.part.updated", | ||
| ]); | ||
|
|
||
| const ALL_CAPTURED_EVENTS = REQUIRED_EVENTS; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Dead abstraction: ALL_CAPTURED_EVENTS = REQUIRED_EVENTS.
Without an OPTIONAL_EVENTS set (unlike the root-level copy), this alias adds indirection without value. Either drop the alias and reference REQUIRED_EVENTS directly in shouldCaptureEvent, or align the file with the root-level structure if optional events are planned.
♻️ Proposed fix
-const ALL_CAPTURED_EVENTS = REQUIRED_EVENTS;
-
-// ...
-
function shouldCaptureEvent(eventType: string): boolean {
- return ALL_CAPTURED_EVENTS.has(eventType);
+ return REQUIRED_EVENTS.has(eventType);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/.opencode/plugins/sce-agent-trace.ts` at line 11, The alias
ALL_CAPTURED_EVENTS simply duplicates REQUIRED_EVENTS and adds unnecessary
indirection; update shouldCaptureEvent to reference REQUIRED_EVENTS directly and
remove the ALL_CAPTURED_EVENTS constant (or alternatively add an OPTIONAL_EVENTS
set and build ALL_CAPTURED_EVENTS from REQUIRED_EVENTS ∪ OPTIONAL_EVENTS if
optional events are intended). Modify the code that currently imports/uses
ALL_CAPTURED_EVENTS to use REQUIRED_EVENTS (or switch to the combined set if you
add OPTIONAL_EVENTS), and delete the redundant const ALL_CAPTURED_EVENTS =
REQUIRED_EVENTS declaration.
| "message.part.updated", | ||
| ]); | ||
|
|
||
| const ALL_CAPTURED_EVENTS = REQUIRED_EVENTS; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Dead abstraction: ALL_CAPTURED_EVENTS is just an alias for REQUIRED_EVENTS.
Drop the alias and use REQUIRED_EVENTS directly in shouldCaptureEvent, unless an OPTIONAL_EVENTS set is planned here too.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/lib/agent-trace-plugin/opencode-sce-agent-trace-plugin.ts` at line 11,
Remove the redundant alias ALL_CAPTURED_EVENTS and update code that references
it (notably shouldCaptureEvent) to use REQUIRED_EVENTS directly; delete the
declaration "const ALL_CAPTURED_EVENTS = REQUIRED_EVENTS" and replace any uses
of ALL_CAPTURED_EVENTS with REQUIRED_EVENTS so the code no longer maintains a
dead abstraction unless/ until an OPTIONAL_EVENTS set is introduced.
Co-authored-by: SCE <sce@crocoder.dev>
Register the new OpenCode agent trace plugin and update the sce-atomic-commit skill to use staged-diff-first commit message proposals. This aligns local agent tracing with a stricter atomic-commit workflow based on intentionally staged changes. Co-authored-by: SCE <sce@crocoder.dev>
Persist timestamped JSON traces for OpenCode events and Git hook invocations to make local debugging easier. Keep hook trace writes best-effort so trace persistence failures do not interrupt normal hook execution. Co-authored-by: SCE <sce@crocoder.dev>
Stop capturing diagnostic trace payloads for pre-commit, commit-msg, and post-rewrite hooks, and narrow the agent trace plugin to the required runtime events only. Co-authored-by: SCE <sce@crocoder.dev>
ivke995
force-pushed
the
feat/agent-trace-plugin
branch
from
37b5ba4 to
3687dd7
Compare
Co-authored-by: SCE <sce@crocoder.dev>
2 participants
Summary by CodeRabbit