Hunk / Patch parsing#30
Merged
Merged
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 agent event tracing plugins and CLI hook trace capture that persist JSON traces; introduces a Rust unified-diff patch parser/composer with fixtures and tests; adds agent-trace domain logic, exposes new services, and updates flake/dev tooling and OpenCode plugin lists. Changes
Sequence DiagramsequenceDiagram
participant Hook as Hook Handler
participant Git as Git CLI
participant Tracer as Trace Writer
participant FS as File System
Hook->>Git: resolve repo root / collect GIT_* env
Hook->>Git: run git commands (e.g., head OID, diffs) as needed
Hook->>Hook: run original hook runtime (capture output or error)
Hook->>Tracer: assemble trace input (hook, git_env, head_oid/head_patch or stdin)
Hook->>Tracer: provide hook output or error
Tracer->>FS: ensure <repoRoot>/context/tmp exists
Tracer->>FS: write sanitized <timestamp>-<hook>.json with { input, output } / { input, error }
FS-->>Tracer: write complete
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 13
♻️ Duplicate comments (3)
config/automated/.opencode/opencode.json (1)
4-4: 🧹 Nitpick | 🔵 TrivialSame formatting nit as sibling config — one plugin per line.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/automated/.opencode/opencode.json` at line 4, The plugins list in opencode.json currently has two entries on the same line ("./plugins/sce-bash-policy.ts", "./plugins/sce-agent-trace.ts"); split them so each plugin path is on its own line in the array to match sibling configs and maintain consistent formatting—update the entry containing "./plugins/sce-bash-policy.ts" and "./plugins/sce-agent-trace.ts" to place each string on its own line, preserving commas and surrounding JSON array syntax.config/lib/agent-trace-plugin/opencode-sce-agent-trace-plugin.ts (1)
1-80: 🛠️ Refactor suggestion | 🟠 MajorThis is the right home for the shared implementation — but nothing imports it.
The three
.opencode/plugins/sce-agent-trace.tsfiles each reimplement this file verbatim instead of importingSceAgentTracePluginfrom here. Combined with the missingname/versionin the adjacentpackage.json, the lib package is effectively dead code today.Recommend exporting a factory so each config can opt into its own capture set:
export function createSceAgentTracePlugin(opts: { capturedEvents: ReadonlySet<string> }): Plugin { /* ... */ } export const SceAgentTracePlugin = createSceAgentTracePlugin({ capturedEvents: DEFAULT_CAPTURED_EVENTS });All the correctness issues (collision, error handling, filename sanitization) flagged on the root
.opencodecopy apply here too — and fixing them once here is exactly the point.🤖 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 - 80, The shared plugin file is never imported and is duplicated elsewhere; replace the hardcoded ALL_CAPTURED_EVENTS export with a factory and export a default instance so other configs can import and customize capture sets: add and export function createSceAgentTracePlugin(opts: { capturedEvents: ReadonlySet<string> }) that returns a Plugin using opts.capturedEvents (update shouldCaptureEvent to reference the provided set), then export const SceAgentTracePlugin = createSceAgentTracePlugin({ capturedEvents: ALL_CAPTURED_EVENTS }); also ensure buildTrace, buildTraceFileName, getTraceName remain reusable (sanitize traceName inside buildTraceFileName), add basic error handling around mkdir/writeFile, and update callers to import createSceAgentTracePlugin or SceAgentTracePlugin instead of duplicating the file.config/.opencode/opencode.json (1)
4-4: 🧹 Nitpick | 🔵 TrivialSame formatting nit — one plugin per line.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/.opencode/opencode.json` at line 4, The plugins array entry currently lists multiple plugin paths on the same line ("./plugins/sce-bash-policy.ts", "./plugins/sce-agent-trace.ts"); split them so each plugin path is its own array element on its own line (separate the two strings into individual lines) to match the one-plugin-per-line formatting convention and ensure consistency for "./plugins/sce-bash-policy.ts" and "./plugins/sce-agent-trace.ts".
🤖 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:
- Line 4: The plugins list currently has two entries on a single line
("./plugins/sce-bash-policy.ts", "./plugins/sce-agent-trace.ts"); split them so
each plugin string is on its own line in the array to produce cleaner diffs.
Update the array containing "./plugins/sce-bash-policy.ts" and
"./plugins/sce-agent-trace.ts" so each plugin appears as a separate line (one
string per line) while preserving commas and JSON syntax.
In @.opencode/plugins/sce-agent-trace.ts:
- Around line 1-87: Consolidate the duplicated plugin by moving core logic into
a single factory exported from the lib package (e.g., export function
createSceAgentTracePlugin({ capturedEvents }) that encapsulates getTraceName,
formatTimestamp, buildTraceFileName, buildTrace, shouldCaptureEvent and uses the
provided capturedEvents set), then replace the four near-identical files with
tiny re-export stubs that import the factory and call it with their desired
event set (e.g., pass REQUIRED_EVENTS or a combined set like new
Set([...REQUIRED_EVENTS, ...OPTIONAL_EVENTS]); keep the public export name
SceAgentTracePlugin in each stub). Ensure the factory accepts the ({ directory,
worktree }) args and preserves existing behavior for file writing and
traceDirectory resolution so callers of SceAgentTracePlugin need no other
changes.
- Around line 30-32: The filename builder currently interpolates unsanitized
trace names (from getTraceName) into buildTraceFileName and when constructing
the trace path, creating a path-traversal risk; sanitize the trace name before
using it by stripping or replacing path separators and parent-ref segments and
restricting to a safe character set (e.g. allowlist [A-Za-z0-9_.-] or call
path.basename then remove any '..'), ensure the sanitized name is non-empty (use
a fallback like "trace" + a unique id or timestamp), and apply this sanitization
both where getTraceName is used and inside buildTraceFileName (or immediately
after getTraceName) so filenames cannot escape the intended trace directory.
- Around line 26-32: The filename generation uses millisecond-precision ISO
strings in formatTimestamp/buildTraceFileName, causing collisions when events
occur within the same millisecond; update buildTraceFileName to append a
monotonic suffix (e.g., a process- or module-scoped incrementing counter or a
high-resolution monotonic value from process.hrtime()/performance.now()) to
guarantee uniqueness for multiple events in the same ms, and ensure
formatTimestamp remains unchanged except for where the suffix is joined; apply
the same change to the sibling copies of buildTraceFileName/formatTimestamp in
the other plugin modules.
- Around line 57-64: The buildTrace function may throw from mkdir or writeFile
and that rejection currently bubbles up; wrap the body of buildTrace in a
try/catch so trace writes are best-effort and do not break the agent session,
catching any unknown thrown value and logging a warning via
console.warn(getErrorMessage(err)) (use the existing getErrorMessage helper)
instead of letting the rejection propagate; apply the same defensive try/catch +
console.warn(getErrorMessage(...)) pattern to the other three plugin copies that
implement buildTrace.
- Line 60: Add a root .gitignore entry for context/tmp/ (or context/) and
document the sensitivity of traces produced by sce-agent-trace.ts: the
serialized trace (see const body = JSON.stringify({ input }, null, 2) which
writes full input payloads) can contain PII/credentials/proprietary code, so
update repository docs/README and the .gitignore to explicitly ignore
context/tmp/ and/or implement a safety cap and cleanup (e.g., max file size or
rotation/auto-delete) in the trace-writing logic to avoid storing full
unfiltered inputs.
In @.opencode/skills/sce-atomic-commit/SKILL.md:
- Line 48: The ordered list numbering in SKILL.md uses incremental numbers
(e.g., "2) Choose scope for each unit") which triggers MD029; change each
ordered list item that is part of the same list to use the constant "1)" prefix
(i.e., replace "2)", "3)", etc. with "1)") so the list follows the configured
1/1/1 style; locate occurrences of the ordered-list items (e.g., the line
starting "2) Choose scope for each unit" and the other similar list lines in the
same list) and update their numeric prefixes to "1)".
In `@cli/src/services/hooks.rs`:
- Around line 219-244: The current collect_git_environment function returns all
env vars starting with "GIT_" which can leak secrets into traces; update the
code so that either (a) collect_git_environment only returns a safe allow-list
of GIT_* keys the trace needs (e.g., GIT_AUTHOR_*, GIT_COMMITTER_*, GIT_DIR,
GIT_WORK_TREE) or (b) pass the full map returned by collect_git_environment
through the existing services::security::redact_sensitive_text (as used in
app.rs::write_error_diagnostic) before building the Value::Object in
build_base_hook_trace_input; adjust build_hook_trace_input_for_post_commit /
build_base_hook_trace_input to call the chosen sanitization function so no raw
GIT_* secrets are written to disk.
- Around line 51-53: The two "with_trace" wrappers
(run_pre_commit_subcommand_with_trace and run_commit_msg_subcommand_with_trace)
currently just forward to run_pre_commit_subcommand /
run_commit_msg_subcommand_in_repo and don't build input, call
persist_hook_trace, or capture outcome; either implement the trace pipeline or
remove the wrappers and call the inner functions directly. To fix: either (A)
implement the same flow used by run_post_commit_subcommand_with_trace — build
the HookTrace input, call the inner function, capture the Result/outcome and
call persist_hook_trace with the input and outcome, and remove the unused _:
&HookSubcommand parameter in run_commit_msg_subcommand_with_trace so signatures
match; or (B) delete both *_with_trace wrappers and update callers to call
run_pre_commit_subcommand and run_commit_msg_subcommand_in_repo directly,
removing the misleading names and unused parameter. Ensure referenced symbols
are updated: run_pre_commit_subcommand_with_trace,
run_commit_msg_subcommand_with_trace, run_pre_commit_subcommand,
run_commit_msg_subcommand_in_repo, run_post_commit_subcommand_with_trace, and
persist_hook_trace.
- Around line 128-136: run_post_commit_subcommand_with_trace currently discards
persist_hook_trace errors and writes unbounded timestamped JSONs into
repo/context/tmp/, causing silent failures and unlimited growth; change
persist_hook_trace calls (used by run_post_commit_subcommand_with_trace and the
other hook paths) to: attempt persistence under a non-repo state location (e.g.
$XDG_STATE_HOME/sce/traces/<repo-hash>/) or implement retention (keep last N or
TTL-cleanup) when writing into context/tmp/, include a short unique suffix (PID
+ nanoseconds or a small counter) to the timestamped filename to avoid
collisions, and capture any I/O or serde errors to emit via the existing Logger
at warn/debug level instead of using let _ = ..., ensuring the hook outcome is
not propagated as a failure.
- Around line 291-319: The helper run_git_command_capture_stdout is too strict
for commands that produce non-UTF-8 data (e.g., git show --patch); change the
implementation strategy by adding a new helper (e.g.,
run_git_command_capture_bytes(repository_root: &Path, args: &[&str],
context_message: &str) -> Result<Vec<u8>>) that returns raw bytes and keep
run_git_command_capture_stdout for callers that require valid UTF-8 by
converting with String::from_utf8(...).context(...). Update callers that capture
patches to call run_git_command_capture_bytes and then either convert with
String::from_utf8_lossy(...) when textual patch with replacement chars is
acceptable, or base64-encode the Vec<u8> when you must preserve all bytes (store
under a separate key like head_patch_base64); ensure error handling and the
existing git stderr diagnostic logic are preserved in the new helper.
- Around line 148-155: The wrapper run_post_rewrite_subcommand_with_trace
currently discards STDIN and never persists a trace; change it to mirror
run_post_commit_subcommand_with_trace by (1) constructing the base
HookTraceInput, (2) calling read_hook_stdin() and folding its Ok(payload) into
the input or storing the Err into the input as the stdin read error (do not use
and_then(|_| …) which drops the payload), (3) always call
run_post_rewrite_subcommand(repository_root, rewrite_method) regardless of stdin
result, capture its Result, and (4) invoke persist_hook_trace with the final
input and the subcommand outcome so a trace file is produced. Ensure you
reference and modify run_post_rewrite_subcommand_with_trace, read_hook_stdin,
run_post_rewrite_subcommand, and persist_hook_trace accordingly.
In `@config/lib/agent-trace-plugin/package.json`:
- Around line 1-7: Update package.json metadata and dependency classification:
keep "@opencode-ai/plugin" in "dependencies" but move "@types/bun" and
"@types/node" into "devDependencies"; add top-level fields "name", "version",
"private": true, and "type": "module" (or appropriate type for the package) so
the package has proper workspace resolution metadata and is marked
non-publishable; ensure the JSON keys are valid and that "dependencies" and
"devDependencies" reflect runtime vs dev-only packages.
---
Duplicate comments:
In `@config/.opencode/opencode.json`:
- Line 4: The plugins array entry currently lists multiple plugin paths on the
same line ("./plugins/sce-bash-policy.ts", "./plugins/sce-agent-trace.ts");
split them so each plugin path is its own array element on its own line
(separate the two strings into individual lines) to match the
one-plugin-per-line formatting convention and ensure consistency for
"./plugins/sce-bash-policy.ts" and "./plugins/sce-agent-trace.ts".
In `@config/automated/.opencode/opencode.json`:
- Line 4: The plugins list in opencode.json currently has two entries on the
same line ("./plugins/sce-bash-policy.ts", "./plugins/sce-agent-trace.ts");
split them so each plugin path is on its own line in the array to match sibling
configs and maintain consistent formatting—update the entry containing
"./plugins/sce-bash-policy.ts" and "./plugins/sce-agent-trace.ts" to place each
string on its own line, preserving commas and surrounding JSON array syntax.
In `@config/lib/agent-trace-plugin/opencode-sce-agent-trace-plugin.ts`:
- Around line 1-80: The shared plugin file is never imported and is duplicated
elsewhere; replace the hardcoded ALL_CAPTURED_EVENTS export with a factory and
export a default instance so other configs can import and customize capture
sets: add and export function createSceAgentTracePlugin(opts: { capturedEvents:
ReadonlySet<string> }) that returns a Plugin using opts.capturedEvents (update
shouldCaptureEvent to reference the provided set), then export const
SceAgentTracePlugin = createSceAgentTracePlugin({ capturedEvents:
ALL_CAPTURED_EVENTS }); also ensure buildTrace, buildTraceFileName, getTraceName
remain reusable (sanitize traceName inside buildTraceFileName), add basic error
handling around mkdir/writeFile, and update callers to import
createSceAgentTracePlugin or SceAgentTracePlugin instead of duplicating the
file.
🪄 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: 62815387-6da8-4b0f-bb60-ac3611a7cb97
⛔ Files ignored due to path filters (15)
.opencode/package-lock.jsonis excluded by!**/package-lock.jsonconfig/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/cli/patch-service.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/cli-patch-intersection-equivalent-hunks.mdis excluded by!context/**/*.mdcontext/plans/cli-patch-json-and-set-ops.mdis excluded by!context/**/*.mdcontext/plans/cli-patch-memory-service.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 (14)
.opencode/opencode.json.opencode/package.json.opencode/plugins/sce-agent-trace.ts.opencode/skills/sce-atomic-commit/SKILL.mdcli/src/app.rscli/src/services/hooks.rscli/src/services/mod.rscli/src/services/patch.rsconfig/.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
💤 Files with no reviewable changes (1)
- .opencode/package.json
Comment on lines
+291
to
+319
| fn run_git_command_capture_stdout( | ||
| repository_root: &Path, | ||
| args: &[&str], | ||
| context_message: &str, | ||
| ) -> Result<String> { | ||
| let output = Command::new("git") | ||
| .args(args) | ||
| .current_dir(repository_root) | ||
| .output() | ||
| .with_context(|| { | ||
| format!( | ||
| "{} (directory: '{}')", | ||
| context_message, | ||
| repository_root.display() | ||
| ) | ||
| })?; | ||
|
|
||
| if !output.status.success() { | ||
| let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string(); | ||
| let diagnostic = if stderr.is_empty() { | ||
| String::from("git command exited with a non-zero status") | ||
| } else { | ||
| stderr | ||
| }; | ||
| bail!("{context_message} {diagnostic}"); | ||
| } | ||
|
|
||
| String::from_utf8(output.stdout).context("git command output contained invalid UTF-8") | ||
| } |
There was a problem hiding this comment.
Strict UTF-8 decoding can drop legitimate HEAD patches.
String::from_utf8(output.stdout) is fine for git rev-parse HEAD (hex ASCII), but git show --patch can emit non-UTF-8 bytes (files in legacy encodings, author names with invalid UTF-8, etc.), in which case the entire HEAD patch is replaced with a head_patch_from_git_read_error entry and the real diff is lost.
Consider either (a) splitting the helper so the patch path uses String::from_utf8_lossy (accepting replacement chars) or (b) storing the bytes base64-encoded under a separate key for non-UTF-8 cases. Strict decoding is appropriate for rev-parse but unnecessarily strict for diff capture.
🤖 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 291 - 319, The helper
run_git_command_capture_stdout is too strict for commands that produce non-UTF-8
data (e.g., git show --patch); change the implementation strategy by adding a
new helper (e.g., run_git_command_capture_bytes(repository_root: &Path, args:
&[&str], context_message: &str) -> Result<Vec<u8>>) that returns raw bytes and
keep run_git_command_capture_stdout for callers that require valid UTF-8 by
converting with String::from_utf8(...).context(...). Update callers that capture
patches to call run_git_command_capture_bytes and then either convert with
String::from_utf8_lossy(...) when textual patch with replacement chars is
acceptable, or base64-encode the Vec<u8> when you must preserve all bytes (store
under a separate key like head_patch_base64); ensure error handling and the
existing git stderr diagnostic logic are preserved in the new helper.
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>
Introduce a standalone patch service under cli/src/services/patch.rs defining serde-serializable domain types (ParsedPatch, PatchFileChange, FileChangeKind, PatchHunk, TouchedLine, TouchedLineKind) for in-memory unified-diff representation. The model captures only touched lines (added/removed) plus minimal per-file/per-hunk metadata while excluding non-hunk headers and unchanged context lines. All types support JSON round-trip fidelity with snake_case field naming. Co-authored-by: SCE <sce@crocoder.dev>
Add parse_patch() to convert raw unified-diff text into ParsedPatch structs. Supports both Index: (SVN-style) and diff --git (git-style) formats with proper handling of /dev/null paths, a/b/ prefix stripping, multi-file patches, and hunk header parsing with optional count defaults. Co-authored-by: SCE <sce@crocoder.dev>
Co-authored-by: SCE <sce@crocoder.dev>
Add PatchLoadError type and load_patch_from_json/load_patch_from_json_bytes functions to reconstruct ParsedPatch from serialized JSON content without coupling to filesystem or database access. Callers can reload patches after reading from any storage backend. Includes comprehensive tests covering round-trip fidelity, error handling for invalid JSON, and preservation of all FileChangeKind/TouchedLineKind variants. Co-authored-by: SCE <sce@crocoder.dev>
Add `intersect_patches(a, b)` that returns a ParsedPatch containing only touched lines present in both inputs, matching files by new_path and lines by kind + line_number + content. Derive Hash on TouchedLineKind to support set-based lookup. Include 9 targeted tests covering identical overlap, no overlap, partial overlap, same-file different lines, multi-file partial overlap, empty patches, hunk metadata preservation, line identity semantics, determinism, and file matching by new_path. Co-authored-by: SCE <sce@crocoder.dev>
Add `combine_patches(patches: &[ParsedPatch]) -> ParsedPatch` to merge multiple patches into one deterministic result. Uses later-input-wins semantics for duplicate/conflicting touched-line entries. Co-authored-by: SCE <sce@crocoder.dev>
Co-authored-by: SCE <sce@crocoder.dev>
Change `intersect_patches` file matching from exact `new_path` string equality to post-change path identity based on normalized path segments. Absolute and relative paths now match when one ends with the same segment sequence as the other, enabling equivalent hunks from different Index: path spellings to intersect correctly. Co-authored-by: SCE <sce@crocoder.dev>
Co-authored-by: SCE <sce@crocoder.dev>
Move inline tests from patch.rs to a dedicated tests.rs module and add an integration test proving combine_patches → intersect_patches produces the correct result when fed real incremental patch data. The test parses seven incremental patches (with corrected file paths), combines them, intersects with the post-commit patch, and asserts the intersection equals the post-commit patch itself.
Improves patch intersection to handle line number drift between incremental diffs and canonical post-commit patches. Previously required exact matches (kind + line_number + content); now prefers exact matches but falls back to historical reconstruction (kind + content only) when line numbers differ.
Co-authored-by: SCE <sce@crocoder.dev>
Co-authored-by: SCE <sce@crocoder.dev>
There was a problem hiding this comment.
Actionable comments posted: 15
♻️ Duplicate comments (1)
.opencode/opencode.json (1)
4-4: 🧹 Nitpick | 🔵 TrivialFormat: one plugin per line for cleaner diffs.
This formatting concern has already been raised in a previous review.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.opencode/opencode.json at line 4, The plugins array currently lists multiple entries on one line ("./plugins/sce-bash-policy.ts", "./plugins/sce-agent-trace.ts"); split each plugin string into its own line in the JSON array so each plugin appears on a separate line for cleaner diffs, ensuring you keep valid JSON (commas between entries and proper array brackets) and preserve the exact plugin strings "./plugins/sce-bash-policy.ts" and "./plugins/sce-agent-trace.ts".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/src/services/patch.rs`:
- Around line 696-796: The hunk parser (parse_hunk_header_and_body) never
validates that the number of consumed old/new lines matches old_count/new_count
from the header, so implement a post-loop validation: after the while loop
compute consumed_old = old_line_num - old_start and consumed_new = new_line_num
- new_start and if consumed_old != old_count or consumed_new != new_count return
an appropriate ParseError (with contextual message referencing
old_start/old_count/new_start/new_count and the consumed values) instead of
returning Ok; use the existing ParseError type and the local symbols old_count,
new_count, old_line_num, new_line_num, old_start, new_start and ensure the error
is surfaced to callers.
- Around line 798-813: The parse_range_part function currently allows empty
start tokens (e.g. "-,7") and reports an unhelpful error quoting the whole
stripped string; update parse_range_part to validate that parts[0] is non-empty
before parsing and return a ParseError that includes the original raw range
string and the offending component (e.g. "empty hunk range start in {raw:?}" or
"invalid hunk range start '{part0:?}' in {raw:?}"). Also ensure you
normalize/keep the original raw input (the incoming s before strip_prefix) for
use in error messages and similarly validate parts[1] when present so count
parsing errors reference the specific component and raw range.
- Around line 409-418: The current grouping uses a HashMap (hunk_groups) and
then builds sorted_hunks from into_iter() and calls sort_by_key(|(meta, _)|
meta.0), which only sorts by old_start and leaves ties dependent on HashMap
iteration order; change the sort to use the full HunkMeta (which implements Ord)
so ordering is deterministic. Locate the sorted_hunks variable and replace the
sort_by_key call with a sort that compares the entire HunkMeta (e.g., sort_by or
sort using the tuple/meta directly) so that hunks with identical old_start are
ordered consistently by the remaining HunkMeta fields.
- Around line 213-246: The current single-pass matching in intersect_patches
(inside the loop over b_file.hunks) can let earlier b lines consume a candidate
via the historical matcher and block a later b line that would have been an
exact match; fix by splitting matching into two passes per b_hunk: first iterate
b_hunk.lines and attempt only exact matches using find_available_line_match with
touched_lines_match_exact and mark used_lines for successful claims, collecting
those overlapping_lines; then in a second iteration over the remaining unmatched
b_hunk.lines attempt historical matches using find_available_line_match with
touched_lines_match_historical against the still-unused entries and mark
used_lines for those claims; build the final overlapping_lines from both passes
and continue as before. Ensure you reference and update the same used_lines and
available_lines arrays and keep the b_hunk.lines iteration order unchanged.
In
`@cli/src/services/patch/fixtures/average_age_reconstruction/incremental_04.patch`:
- Around line 10-11: Rename the misspelled exported function
getAvarageAgeOptimized to getAverageAgeOptimized everywhere it appears (the
function declaration, any exports/refs, and call sites) so all fixtures stay
consistent; update occurrences in incremental_04.patch (export and function
name), and replicate the same rename in post_commit.patch, incremental_05.patch,
incremental_06.patch, and golden.json to ensure all references match the
corrected symbol getAverageAgeOptimized.
In
`@cli/src/services/patch/fixtures/average_age_reconstruction/incremental_05.patch`:
- Line 8: Rename the function getAvarageAgeOptimized to getAverageAgeOptimized
in the fixture to correct the typo; update the function declaration and any
internal or external references (calls, exports, imports, tests) that use
getAvarageAgeOptimized so they now reference getAverageAgeOptimized to keep the
API consistent.
In
`@cli/src/services/patch/fixtures/average_age_reconstruction/post_commit.patch`:
- Around line 14-122: Normalize the inconsistent function-name spelling by
choosing one canonical name (e.g., getAverageAgeOptimized and
getAverageAgeUnoptimized) and renaming all occurrences and exports accordingly
in the three diffs: the functions exported as getAvarageAgeOptimized and
getAvarageAgeUnoptimized in hunks/fib.ts (and the top-level patch block) must be
renamed to match the names used in hunks/optimized.ts and hunks/unoptimized.ts
(or vice versa), and update any corresponding export statements and callers to
the chosen spelling so content-based patch matching will succeed.
In `@config/.opencode/plugins/sce-agent-trace.ts`:
- Around line 5-11: The current indirection exposes REQUIRED_EVENTS and then
aliases it to ALL_CAPTURED_EVENTS which is misleading because there are no
optional events; replace both with a single neutral constant name
CAPTURED_EVENTS containing the current set values, remove REQUIRED_EVENTS and
ALL_CAPTURED_EVENTS, and update any usage sites such as shouldCaptureEvent to
reference CAPTURED_EVENTS instead of ALL_CAPTURED_EVENTS/REQUIRED_EVENTS so the
allowlist is inlined and clearly named.
- Around line 1-80: This file duplicates the library implementation; replace the
local implementation with a re-export of the packaged plugin: remove the
duplicated functions/constants (REQUIRED_EVENTS, ALL_CAPTURED_EVENTS,
formatTimestamp, buildTraceFileName, getTraceName, shouldCaptureEvent,
buildTrace) and the local async Plugin factory and instead import and export
SceAgentTracePlugin from '@sce/agent-trace-plugin' (keeping the exported symbol
name SceAgentTracePlugin so callers are unchanged), and ensure the project
dependency on '@sce/agent-trace-plugin' is added/updated in package.json so the
import resolves.
In `@config/automated/.opencode/plugins/sce-agent-trace.ts`:
- Line 11: ALL_CAPTURED_EVENTS is a redundant alias of REQUIRED_EVENTS; either
remove the extra binding and replace all uses of ALL_CAPTURED_EVENTS with
REQUIRED_EVENTS, or make ALL_CAPTURED_EVENTS a true superset (e.g., combine
REQUIRED_EVENTS with additional optional events) and keep it as the canonical
constant; update any references to the chosen symbol (ALL_CAPTURED_EVENTS or
REQUIRED_EVENTS) so there is a single source of truth and remove the dead
indirection to avoid drift.
- Around line 50-57: buildTrace currently creates filenames via
buildTraceFileName(getTraceName(input), now) which can collide when events occur
within the same millisecond and also never cleans up files; update buildTrace to
ensure unique filenames by appending a short random suffix or monotonic counter
(e.g., use crypto.randomBytes or a process.hrtime-based counter) to the filename
returned by buildTraceFileName/getTraceName, and add a simple rotation/cleanup
step in the same function: after mkdir but before writeFile, enumerate files in
traceDirectory and remove files older than a configurable age (e.g., X days) or
trim the directory to a configurable max count by deleting the oldest files;
keep these behaviors configurable via parameters or env vars and reference
buildTrace, buildTraceFileName, getTraceName, and traceDirectory when applying
changes.
- Around line 63-78: Replace the duplicated typeof/null checks with a single
call to getTraceName: call getTraceName(traceInput) to obtain the trace name (it
returns "unknown" as the sentinel), use that name for the capture decision
(return early if name === "unknown" or !shouldCaptureEvent(name)), and then pass
the same name into buildTrace (so the filename and capture logic are consistent)
while keeping buildTrace(traceDirectory, input) behavior intact if it already
derives the filename internally. Ensure you reference getTraceName,
shouldCaptureEvent, buildTrace, and the TraceInput cast (traceInput) when making
these changes.
In `@config/lib/agent-trace-plugin/opencode-sce-agent-trace-plugin.ts`:
- Around line 59-79: The event handler in SceAgentTracePlugin can produce
filename collisions and silently lose errors; modify the code that generates
trace filenames (used by buildTrace / buildTraceFileName or wherever
formatTimestamp is applied) to append a disambiguator such as
`${process.pid}-${counter}` or `crypto.randomUUID().slice(0,8)` to guarantee
uniqueness, and wrap the await buildTrace(traceDirectory, input) call in a
try/catch that logs failures (e.g., console.warn or processLogger.warn) so I/O
errors are surfaced; optionally implement a simple retention mechanism (trim
files by count or age under traceDirectory) later, but first add the unique
suffix and the try/catch around buildTrace.
- Around line 27-44: The getTraceName function duplicates checks already done by
the event handler before calling buildTrace; update the flow to avoid re-walking
the validated shape by either (A) change buildTrace to accept the
already-validated eventType (string) from the event handler and use that
directly instead of calling getTraceName, or (B) remove the redundant
type/length checks inside getTraceName and assume a TraceInput with a valid
event.type; reference getTraceName and buildTrace to implement the chosen
approach and update the event handler call site accordingly so only one
validation is performed.
---
Duplicate comments:
In @.opencode/opencode.json:
- Line 4: The plugins array currently lists multiple entries on one line
("./plugins/sce-bash-policy.ts", "./plugins/sce-agent-trace.ts"); split each
plugin string into its own line in the JSON array so each plugin appears on a
separate line for cleaner diffs, ensuring you keep valid JSON (commas between
entries and proper array brackets) and preserve the exact plugin strings
"./plugins/sce-bash-policy.ts" and "./plugins/sce-agent-trace.ts".
🪄 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: ad06a13f-bc77-48c0-a535-51b84dccd515
⛔ Files ignored due to path filters (17)
.opencode/package-lock.jsonis excluded by!**/package-lock.jsonconfig/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/cli/patch-service.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/cli-patch-combine-intersect-test.mdis excluded by!context/**/*.mdcontext/plans/cli-patch-fixture-reconstruction-from-tmp-hunks.mdis excluded by!context/**/*.mdcontext/plans/cli-patch-intersection-equivalent-hunks.mdis excluded by!context/**/*.mdcontext/plans/cli-patch-json-and-set-ops.mdis excluded by!context/**/*.mdcontext/plans/cli-patch-memory-service.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 (56)
.opencode/opencode.json.opencode/package.json.opencode/plugins/sce-agent-trace.tscli/src/app.rscli/src/command_surface.rscli/src/services/hooks.rscli/src/services/mod.rscli/src/services/patch.rscli/src/services/patch/fixtures/average_age_reconstruction/golden.jsoncli/src/services/patch/fixtures/average_age_reconstruction/incremental_01.patchcli/src/services/patch/fixtures/average_age_reconstruction/incremental_02.patchcli/src/services/patch/fixtures/average_age_reconstruction/incremental_03.patchcli/src/services/patch/fixtures/average_age_reconstruction/incremental_04.patchcli/src/services/patch/fixtures/average_age_reconstruction/incremental_05.patchcli/src/services/patch/fixtures/average_age_reconstruction/incremental_06.patchcli/src/services/patch/fixtures/average_age_reconstruction/incremental_07.patchcli/src/services/patch/fixtures/average_age_reconstruction/post_commit.patchcli/src/services/patch/fixtures/hello_world_reconstruction/golden.jsoncli/src/services/patch/fixtures/hello_world_reconstruction/incremental_01.patchcli/src/services/patch/fixtures/hello_world_reconstruction/post_commit.patchcli/src/services/patch/fixtures/text_file_lifecycle_reconstruction/golden.jsoncli/src/services/patch/fixtures/text_file_lifecycle_reconstruction/incremental_01.patchcli/src/services/patch/fixtures/text_file_lifecycle_reconstruction/incremental_02.patchcli/src/services/patch/fixtures/text_file_lifecycle_reconstruction/incremental_03.patchcli/src/services/patch/fixtures/text_file_lifecycle_reconstruction/incremental_04.patchcli/src/services/patch/fixtures/text_file_lifecycle_reconstruction/incremental_05.patchcli/src/services/patch/fixtures/text_file_lifecycle_reconstruction/incremental_06.patchcli/src/services/patch/fixtures/text_file_lifecycle_reconstruction/incremental_07.patchcli/src/services/patch/fixtures/text_file_lifecycle_reconstruction/incremental_08.patchcli/src/services/patch/fixtures/text_file_lifecycle_reconstruction/incremental_09.patchcli/src/services/patch/fixtures/text_file_lifecycle_reconstruction/incremental_10.patchcli/src/services/patch/fixtures/text_file_lifecycle_reconstruction/incremental_11.patchcli/src/services/patch/fixtures/text_file_lifecycle_reconstruction/incremental_12.patchcli/src/services/patch/fixtures/text_file_lifecycle_reconstruction/incremental_13.patchcli/src/services/patch/fixtures/text_file_lifecycle_reconstruction/incremental_14.patchcli/src/services/patch/fixtures/text_file_lifecycle_reconstruction/incremental_15.patchcli/src/services/patch/fixtures/text_file_lifecycle_reconstruction/incremental_16.patchcli/src/services/patch/fixtures/text_file_lifecycle_reconstruction/incremental_17.patchcli/src/services/patch/fixtures/text_file_lifecycle_reconstruction/incremental_18.patchcli/src/services/patch/fixtures/text_file_lifecycle_reconstruction/incremental_19.patchcli/src/services/patch/fixtures/text_file_lifecycle_reconstruction/incremental_20.patchcli/src/services/patch/fixtures/text_file_lifecycle_reconstruction/incremental_21.patchcli/src/services/patch/fixtures/text_file_lifecycle_reconstruction/incremental_22.patchcli/src/services/patch/fixtures/text_file_lifecycle_reconstruction/incremental_23.patchcli/src/services/patch/fixtures/text_file_lifecycle_reconstruction/incremental_24.patchcli/src/services/patch/fixtures/text_file_lifecycle_reconstruction/incremental_25.patchcli/src/services/patch/fixtures/text_file_lifecycle_reconstruction/incremental_26.patchcli/src/services/patch/fixtures/text_file_lifecycle_reconstruction/post_commit.patchcli/src/services/patch/tests.rsconfig/.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.jsonflake.nix
💤 Files with no reviewable changes (2)
- .opencode/package.json
- cli/src/command_surface.rs
Comment on lines
+213
to
+246
| let mut result_hunks: Vec<PatchHunk> = Vec::new(); | ||
| for b_hunk in &b_file.hunks { | ||
| let overlapping_lines: Vec<TouchedLine> = b_hunk | ||
| .lines | ||
| .iter() | ||
| .filter(|line| { | ||
| if let Some(index) = find_available_line_match( | ||
| &available_lines, | ||
| &used_lines, | ||
| line, | ||
| touched_lines_match_exact, | ||
| ) { | ||
| used_lines[index] = true; | ||
| return true; | ||
| } | ||
|
|
||
| if let Some(index) = find_available_line_match( | ||
| &available_lines, | ||
| &used_lines, | ||
| line, | ||
| touched_lines_match_historical, | ||
| ) { | ||
| used_lines[index] = true; | ||
| return true; | ||
| } | ||
|
|
||
| false | ||
| }) | ||
| .cloned() | ||
| .collect(); | ||
|
|
||
| if overlapping_lines.is_empty() { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Greedy single-pass matching can drop lines that have an exact match because a prior line consumed their candidate historically.
The filter in intersect_patches walks b_hunk.lines once and, per line, tries exact match first then falls back to historical. used_lines prevents a single a line from matching twice, but that means a later b line with an exact match can fail if an earlier b line already claimed the same a entry via the historical fallback.
Counter-example:
alines:[(Added, 5, "foo")]blines:[(Added, 10, "foo"), (Added, 5, "foo")]- Pass 1:
(Added, 10, "foo")has no exact hit → historical matches(Added, 5, "foo"), marks it used. - Pass 2:
(Added, 5, "foo")has an exact match to that same entry, but it is now used → dropped.
This contradicts the docstring's "Matching prefers exact touched-line identity … When no exact match exists, it falls back to historical". Do two passes — claim all exact matches first, then run the historical pass over the unclaimed a entries:
🐛 Proposed fix
- let mut result_hunks: Vec<PatchHunk> = Vec::new();
- for b_hunk in &b_file.hunks {
- let overlapping_lines: Vec<TouchedLine> = b_hunk
- .lines
- .iter()
- .filter(|line| {
- if let Some(index) = find_available_line_match(
- &available_lines,
- &used_lines,
- line,
- touched_lines_match_exact,
- ) {
- used_lines[index] = true;
- return true;
- }
-
- if let Some(index) = find_available_line_match(
- &available_lines,
- &used_lines,
- line,
- touched_lines_match_historical,
- ) {
- used_lines[index] = true;
- return true;
- }
-
- false
- })
- .cloned()
- .collect();
+ let mut result_hunks: Vec<PatchHunk> = Vec::new();
+ for b_hunk in &b_file.hunks {
+ // Pass 1: reserve all exact matches first so they cannot be stolen
+ // by a historical fallback on a neighbouring line.
+ let mut kept = vec![false; b_hunk.lines.len()];
+ for (i, line) in b_hunk.lines.iter().enumerate() {
+ if let Some(index) = find_available_line_match(
+ &available_lines,
+ &used_lines,
+ line,
+ touched_lines_match_exact,
+ ) {
+ used_lines[index] = true;
+ kept[i] = true;
+ }
+ }
+ // Pass 2: historical fallback for the remaining b lines.
+ for (i, line) in b_hunk.lines.iter().enumerate() {
+ if kept[i] {
+ continue;
+ }
+ if let Some(index) = find_available_line_match(
+ &available_lines,
+ &used_lines,
+ line,
+ touched_lines_match_historical,
+ ) {
+ used_lines[index] = true;
+ kept[i] = true;
+ }
+ }
+ let overlapping_lines: Vec<TouchedLine> = b_hunk
+ .lines
+ .iter()
+ .zip(kept.iter())
+ .filter_map(|(line, keep)| keep.then(|| line.clone()))
+ .collect();📝 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.
Suggested change
| let mut result_hunks: Vec<PatchHunk> = Vec::new(); | |
| for b_hunk in &b_file.hunks { | |
| let overlapping_lines: Vec<TouchedLine> = b_hunk | |
| .lines | |
| .iter() | |
| .filter(|line| { | |
| if let Some(index) = find_available_line_match( | |
| &available_lines, | |
| &used_lines, | |
| line, | |
| touched_lines_match_exact, | |
| ) { | |
| used_lines[index] = true; | |
| return true; | |
| } | |
| if let Some(index) = find_available_line_match( | |
| &available_lines, | |
| &used_lines, | |
| line, | |
| touched_lines_match_historical, | |
| ) { | |
| used_lines[index] = true; | |
| return true; | |
| } | |
| false | |
| }) | |
| .cloned() | |
| .collect(); | |
| if overlapping_lines.is_empty() { | |
| continue; | |
| } | |
| let mut result_hunks: Vec<PatchHunk> = Vec::new(); | |
| for b_hunk in &b_file.hunks { | |
| // Pass 1: reserve all exact matches first so they cannot be stolen | |
| // by a historical fallback on a neighbouring line. | |
| let mut kept = vec![false; b_hunk.lines.len()]; | |
| for (i, line) in b_hunk.lines.iter().enumerate() { | |
| if let Some(index) = find_available_line_match( | |
| &available_lines, | |
| &used_lines, | |
| line, | |
| touched_lines_match_exact, | |
| ) { | |
| used_lines[index] = true; | |
| kept[i] = true; | |
| } | |
| } | |
| // Pass 2: historical fallback for the remaining b lines. | |
| for (i, line) in b_hunk.lines.iter().enumerate() { | |
| if kept[i] { | |
| continue; | |
| } | |
| if let Some(index) = find_available_line_match( | |
| &available_lines, | |
| &used_lines, | |
| line, | |
| touched_lines_match_historical, | |
| ) { | |
| used_lines[index] = true; | |
| kept[i] = true; | |
| } | |
| } | |
| let overlapping_lines: Vec<TouchedLine> = b_hunk | |
| .lines | |
| .iter() | |
| .zip(kept.iter()) | |
| .filter_map(|(line, keep)| keep.then(|| line.clone())) | |
| .collect(); | |
| if overlapping_lines.is_empty() { | |
| continue; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/services/patch.rs` around lines 213 - 246, The current single-pass
matching in intersect_patches (inside the loop over b_file.hunks) can let
earlier b lines consume a candidate via the historical matcher and block a later
b line that would have been an exact match; fix by splitting matching into two
passes per b_hunk: first iterate b_hunk.lines and attempt only exact matches
using find_available_line_match with touched_lines_match_exact and mark
used_lines for successful claims, collecting those overlapping_lines; then in a
second iteration over the remaining unmatched b_hunk.lines attempt historical
matches using find_available_line_match with touched_lines_match_historical
against the still-unused entries and mark used_lines for those claims; build the
final overlapping_lines from both passes and continue as before. Ensure you
reference and update the same used_lines and available_lines arrays and keep the
b_hunk.lines iteration order unchanged.
Comment on lines
+409
to
+418
| // Group surviving lines by their hunk metadata. | ||
| let mut hunk_groups: HashMap<HunkMeta, Vec<TouchedLine>> = HashMap::new(); | ||
| for (_line_key, (line, hunk_meta)) in acc.lines { | ||
| hunk_groups.entry(hunk_meta).or_default().push(line); | ||
| } | ||
|
|
||
| // Sort hunk groups by old_start for deterministic output. | ||
| let mut sorted_hunks: Vec<_> = hunk_groups.into_iter().collect(); | ||
| sorted_hunks.sort_by_key(|(meta, _)| meta.0); | ||
|
|
There was a problem hiding this comment.
Non-deterministic hunk ordering contradicts the docstring's "deterministic" guarantee.
hunk_groups: HashMap<HunkMeta, Vec<TouchedLine>> is iterated via .into_iter() to build sorted_hunks, then sort_by_key(|(meta, _)| meta.0) sorts only by old_start. Rust's HashMap uses a randomized hasher by default, and sort_by_key is stable, so when two hunks share old_start (e.g., a file with a single canonical old_start = 1 for a new-file hunk, or re-used hunk starts across input patches) their relative order in the output depends on the HashMap's iteration order and will change from run to run. This breaks the "same inputs always produce the same result" contract and will cause flaky golden tests.
Sort by the full HunkMeta tuple (which implements Ord lexicographically):
🐛 Proposed fix
- // Sort hunk groups by old_start for deterministic output.
- let mut sorted_hunks: Vec<_> = hunk_groups.into_iter().collect();
- sorted_hunks.sort_by_key(|(meta, _)| meta.0);
+ // Sort hunk groups by full metadata tuple for deterministic output.
+ let mut sorted_hunks: Vec<_> = hunk_groups.into_iter().collect();
+ sorted_hunks.sort_by_key(|(meta, _)| *meta);📝 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.
Suggested change
| // Group surviving lines by their hunk metadata. | |
| let mut hunk_groups: HashMap<HunkMeta, Vec<TouchedLine>> = HashMap::new(); | |
| for (_line_key, (line, hunk_meta)) in acc.lines { | |
| hunk_groups.entry(hunk_meta).or_default().push(line); | |
| } | |
| // Sort hunk groups by old_start for deterministic output. | |
| let mut sorted_hunks: Vec<_> = hunk_groups.into_iter().collect(); | |
| sorted_hunks.sort_by_key(|(meta, _)| meta.0); | |
| // Group surviving lines by their hunk metadata. | |
| let mut hunk_groups: HashMap<HunkMeta, Vec<TouchedLine>> = HashMap::new(); | |
| for (_line_key, (line, hunk_meta)) in acc.lines { | |
| hunk_groups.entry(hunk_meta).or_default().push(line); | |
| } | |
| // Sort hunk groups by full metadata tuple for deterministic output. | |
| let mut sorted_hunks: Vec<_> = hunk_groups.into_iter().collect(); | |
| sorted_hunks.sort_by_key(|(meta, _)| *meta); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/services/patch.rs` around lines 409 - 418, The current grouping uses
a HashMap (hunk_groups) and then builds sorted_hunks from into_iter() and calls
sort_by_key(|(meta, _)| meta.0), which only sorts by old_start and leaves ties
dependent on HashMap iteration order; change the sort to use the full HunkMeta
(which implements Ord) so ordering is deterministic. Locate the sorted_hunks
variable and replace the sort_by_key call with a sort that compares the entire
HunkMeta (e.g., sort_by or sort using the tuple/meta directly) so that hunks
with identical old_start are ordered consistently by the remaining HunkMeta
fields.
Comment on lines
+644
to
+669
| fn parse_git_diff_header(rest: &str) -> DiffPaths { | ||
| // Format: "a/old_path b/new_path" | ||
| // The paths can contain spaces, so we need to split on " b/" carefully. | ||
| // Git format: diff --git a/path b/path | ||
| if let Some(idx) = rest.find(" b/") { | ||
| let old = rest[..idx] | ||
| .strip_prefix("a/") | ||
| .unwrap_or(&rest[..idx]) | ||
| .to_string(); | ||
| let new = rest[idx + 3..] | ||
| .strip_prefix("b/") | ||
| .unwrap_or(&rest[idx + 3..]) | ||
| .to_string(); | ||
| DiffPaths { | ||
| old_path: old, | ||
| new_path: new, | ||
| } | ||
| } else { | ||
| // Fallback: treat the whole thing as both paths | ||
| let path = rest.trim().to_string(); | ||
| DiffPaths { | ||
| old_path: path.clone(), | ||
| new_path: path, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
parse_git_diff_header mis-splits paths that contain b/.
rest.find(" b/") picks the first occurrence, which is fragile for paths like a/my b/file.ts b/my b/file.ts (real file names with a literal b/ substring) or any old path whose name contains b/. The split will land mid-old-path and produce garbage for both sides. Git itself disambiguates by the unquoted-vs-quoted form and by honoring the a//b/ prefixes — a safer heuristic here is to prefer a split that yields a/… on the left and b/… on the right, falling back to the midpoint when the paths are equal length.
Not a blocker for the fixtures currently committed (none contain spaces in paths), but worth hardening before this parser is used on arbitrary repos, especially on Windows paths or filenames with spaces.
Comment on lines
+696
to
+796
| fn parse_hunk_header_and_body<'a, I>( | ||
| rest: &str, | ||
| lines: &mut std::iter::Peekable<I>, | ||
| ) -> Result<PatchHunk, ParseError> | ||
| where | ||
| I: Iterator<Item = &'a str>, | ||
| { | ||
| // Format: -old_start[,old_count] +new_start[,new_count] @@ [optional context] | ||
| let header_end = rest.find("@@").ok_or_else(|| ParseError { | ||
| message: format!("invalid hunk header: missing closing '@@' in {rest:?}"), | ||
| })?; | ||
| let range_part = rest[..header_end].trim(); | ||
| let mut ranges = range_part.split_whitespace(); | ||
| let old_range = ranges.next().ok_or_else(|| ParseError { | ||
| message: format!("invalid hunk header: missing old range in {range_part:?}"), | ||
| })?; | ||
| let new_range = ranges.next().ok_or_else(|| ParseError { | ||
| message: format!("invalid hunk header: missing new range in {range_part:?}"), | ||
| })?; | ||
|
|
||
| let (old_start, old_count) = parse_range_part(old_range, '-')?; | ||
| let (new_start, new_count) = parse_range_part(new_range, '+')?; | ||
|
|
||
| // Consume hunk body lines until we hit a line that starts a new file | ||
| // section or another hunk header or end of input. | ||
| let mut touched_lines: Vec<TouchedLine> = Vec::new(); | ||
| let mut old_line_num = old_start; | ||
| let mut new_line_num = new_start; | ||
|
|
||
| while let Some(&line) = lines.peek() { | ||
| // Stop at file boundaries | ||
| if line.starts_with("diff --git ") || line.starts_with("Index: ") || line.starts_with("===") | ||
| { | ||
| break; | ||
| } | ||
| // Stop at next hunk header | ||
| if line.starts_with("@@ ") { | ||
| break; | ||
| } | ||
| // Stop at ---/+++ headers (next file section in Index: format) | ||
| if line.starts_with("--- ") || line.starts_with("+++ ") { | ||
| break; | ||
| } | ||
| // Stop at git metadata lines | ||
| if line.starts_with("new file mode ") | ||
| || line.starts_with("deleted file mode ") | ||
| || line.starts_with("old mode ") | ||
| || line.starts_with("new mode ") | ||
| || line.starts_with("index ") | ||
| || line.starts_with("similarity index ") | ||
| || line.starts_with("rename from ") | ||
| || line.starts_with("rename to ") | ||
| || line.starts_with("copy from ") | ||
| || line.starts_with("copy to ") | ||
| { | ||
| break; | ||
| } | ||
|
|
||
| // Consume the line | ||
| let line = lines.next().unwrap(); | ||
|
|
||
| if let Some(content) = line.strip_prefix('+') { | ||
| // Added line | ||
| touched_lines.push(TouchedLine { | ||
| kind: TouchedLineKind::Added, | ||
| line_number: new_line_num, | ||
| content: content.to_string(), | ||
| }); | ||
| new_line_num += 1; | ||
| } else if let Some(content) = line.strip_prefix('-') { | ||
| // Removed line | ||
| touched_lines.push(TouchedLine { | ||
| kind: TouchedLineKind::Removed, | ||
| line_number: old_line_num, | ||
| content: content.to_string(), | ||
| }); | ||
| old_line_num += 1; | ||
| } else if line.starts_with(' ') || line.starts_with('\t') { | ||
| // Context line — skip but advance both counters | ||
| old_line_num += 1; | ||
| new_line_num += 1; | ||
| } else if line.is_empty() { | ||
| // Empty line within a hunk body — could be a context line with | ||
| // no leading space (some diffs emit this). Treat as context. | ||
| old_line_num += 1; | ||
| new_line_num += 1; | ||
| } else if line.starts_with('\\') { | ||
| // "\ No newline at end of file" — skip | ||
| } else { | ||
| // Unknown line format within hunk — skip | ||
| } | ||
| } | ||
|
|
||
| Ok(PatchHunk { | ||
| old_start, | ||
| old_count, | ||
| new_start, | ||
| new_count, | ||
| lines: touched_lines, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Hunk body parser does not validate old_count/new_count and can silently mis-number.
parse_hunk_header_and_body advances old_line_num/new_line_num based on the leading character, but never checks the final counts against the header's old_count/new_count. If the patch was truncated, or a trailing non-hunk line (e.g. the git footer -- \n<version>\n) is mistakenly consumed as a - line (line 765's strip_prefix('-') matches -- too before the --- branch is reached, but since --- is tested earlier in the peek loop at line 736 this particular case is handled — however - followed by non-space text like a diffstat footer will still be treated as a removed line), the output's line_numbers will drift without any error signal.
At minimum, after the loop, verify old_line_num - old_start == old_count (and likewise for new) and surface a ParseError on mismatch, or log a diagnostic. This would also have caught the earlier-mentioned mis-split path bug at parse time for round-tripped fixtures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/services/patch.rs` around lines 696 - 796, The hunk parser
(parse_hunk_header_and_body) never validates that the number of consumed old/new
lines matches old_count/new_count from the header, so implement a post-loop
validation: after the while loop compute consumed_old = old_line_num - old_start
and consumed_new = new_line_num - new_start and if consumed_old != old_count or
consumed_new != new_count return an appropriate ParseError (with contextual
message referencing old_start/old_count/new_start/new_count and the consumed
values) instead of returning Ok; use the existing ParseError type and the local
symbols old_count, new_count, old_line_num, new_line_num, old_start, new_start
and ensure the error is surfaced to callers.
Comment on lines
+798
to
+813
| /// Parse a range part like `-3,7` or `+1,9` from a hunk header. | ||
| fn parse_range_part(s: &str, prefix: char) -> Result<(u64, u64), ParseError> { | ||
| let s = s.strip_prefix(prefix).unwrap_or(s).trim(); | ||
| let parts: Vec<&str> = s.splitn(2, ',').collect(); | ||
| let start: u64 = parts[0].parse().map_err(|_| ParseError { | ||
| message: format!("invalid hunk range start in {s:?}"), | ||
| })?; | ||
| let count: u64 = if parts.len() > 1 { | ||
| parts[1].parse().map_err(|_| ParseError { | ||
| message: format!("invalid hunk range count in {s:?}"), | ||
| })? | ||
| } else { | ||
| 1 | ||
| }; | ||
| Ok((start, count)) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
parse_range_part accepts -,7 and similar malformed input as start=<parse-err>.
s.splitn(2, ',').collect() means when s = "" you still get parts = [""] and parts[0].parse::<u64>() fails with the generic "invalid hunk range start" message, which is fine. But parts[0] can also be an empty string when input is ",7" (prefix stripped from "-,7"), and the error message quotes s rather than the offending component, so diagnostics are unhelpful. Minor nit — consider including the original raw range in the message and rejecting zero-length start:
♻️ Proposed diagnostics tweak
- let start: u64 = parts[0].parse().map_err(|_| ParseError {
- message: format!("invalid hunk range start in {s:?}"),
+ let start: u64 = parts[0].parse().map_err(|_| ParseError {
+ message: format!("invalid hunk range start {:?} in {s:?}", parts[0]),
})?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/services/patch.rs` around lines 798 - 813, The parse_range_part
function currently allows empty start tokens (e.g. "-,7") and reports an
unhelpful error quoting the whole stripped string; update parse_range_part to
validate that parts[0] is non-empty before parsing and return a ParseError that
includes the original raw range string and the offending component (e.g. "empty
hunk range start in {raw:?}" or "invalid hunk range start '{part0:?}' in
{raw:?}"). Also ensure you normalize/keep the original raw input (the incoming s
before strip_prefix) for use in error messages and similarly validate parts[1]
when present so count parsing errors reference the specific component and raw
range.
Add two new test scenarios for the patch reconstruction service: - `poem_write_reconstruction`: Tests file creation from empty state - `poem_edit_reconstruction`: Tests incremental edits to existing file Co-authored-by: SCE <sce@crocoder.dev>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/src/services/patch/fixtures/poem_write_reconstruction/golden.json`:
- Around line 12-128: The golden fixture is missing the added line 15, causing
new_count (24) to disagree with the 23 entries; insert an object into the
"lines" array with kind: "added", line_number: 15, content: "For every error
teaches true," so the lines array contains the intersection entry present in
both patches and keeps new_count at 24; re-run the
poem_write_reconstruction_matches_post_commit test to confirm the golden now
matches the produced intersection.
🪄 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: 44986805-51f5-49ff-b2ef-53e433ee9656
📒 Files selected for processing (8)
cli/src/services/patch/fixtures/poem_edit_reconstruction/golden.jsoncli/src/services/patch/fixtures/poem_edit_reconstruction/incremental_01.patchcli/src/services/patch/fixtures/poem_edit_reconstruction/incremental_02.patchcli/src/services/patch/fixtures/poem_edit_reconstruction/post_commit.patchcli/src/services/patch/fixtures/poem_write_reconstruction/golden.jsoncli/src/services/patch/fixtures/poem_write_reconstruction/incremental_01.patchcli/src/services/patch/fixtures/poem_write_reconstruction/post_commit.patchcli/src/services/patch/tests.rs
Streamline the SCE agent trace plugin by removing unused event captures (session.*, message.updated, file.*). The plugin now captures only message.part.updated events, reducing trace noise and simplifying processing. Add nix run .#pkl-generate as a convenience wrapper for pkl eval, providing a consistent entry point for regenerating config outputs from Pkl sources during development workflows. Co-authored-by: SCE <sce@crocoder.dev>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/src/services/agent_trace.rs`:
- Around line 82-87: The current lookup of an intersection hunk using only
old_start (in the block that sets intersection_hunk from
intersection_hunks.iter().find(|h| h.old_start == post_commit_hunk.old_start))
can mispair hunks when multiple hunks share the same old_start; update the
matching logic in the function that classifies HunkContributor so it matches on
the full hunk identity (old_start, old_count, new_start, new_count) or otherwise
consume intersection_hunks in lockstep with post_commit_hunks so each
post_commit_hunk is paired with exactly one intersection hunk; refer to
intersection_hunks, post_commit_hunk, HunkContributor::Unknown and consider the
intersect_patches behavior in patch.rs (lines around 253-259) when implementing
the stronger tuple-based key or positional pairing.
- Around line 37-43: Remove the redundant #[serde(rename_all = "snake_case")]
attribute from the structs Conversation, TraceFile, and AgentTrace in
agent_trace.rs; leave serde derives intact (Serialize/Deserialize) and keep the
attribute only on HunkContributor where it actually changes the enum variant
names (Ai/Mixed/Unknown). Update the struct definitions for Conversation,
TraceFile, and AgentTrace to drop the rename_all attribute so the code uses the
default field name mapping and avoids misleading readers.
In `@cli/src/services/patch.rs`:
- Around line 303-333: paths_refer_to_same_file currently treats any
proper-suffix match as equality which can conflate unrelated files; update it to
only accept suffix-based matches when the shared suffix has at least a minimum
number of segments (e.g., >= 2) or when one of the original paths is absolute.
Concretely, modify normalized_path_components or add a small helper to preserve
whether the original Path::is_absolute(), then in paths_refer_to_same_file call
path_has_relative_suffix only if suffix_candidate.len() >= MIN_SHARED_SEGMENTS
(set to 2) or if Path::new(path_a).is_absolute() ||
Path::new(path_b).is_absolute(); update path_has_relative_suffix to assume this
precondition (or add the length check there) so short single-segment suffixes
like "mod.rs" no longer match arbitrary files.
- Around line 559-565: The code currently silently drops hunk headers when
current_file is None; update the @@ hunk handling so that if
current_file.as_mut() is None you return a ParseError (like other header
branches) instead of ignoring the hunk. In the block handling the "@@ " prefix
(where parse_hunk_header_and_body and fb.add_hunk are called), detect the None
case and construct/return an appropriate ParseError describing a missing file
header for the hunk, mirroring the error behavior used for the `--- `/`+++ `
branches so malformed input is reported to callers.
🪄 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: da51df2c-3eab-4ee9-844f-42f8978679bc
⛔ Files ignored due to path filters (4)
context/context-map.mdis excluded by!context/**/*.mdcontext/glossary.mdis excluded by!context/**/*.mdcontext/plans/cli-agent-trace-minimal-generator.mdis excluded by!context/**/*.mdcontext/sce/agent-trace-minimal-generator.mdis excluded by!context/**/*.md
📒 Files selected for processing (8)
.opencode/plugins/sce-agent-trace.tscli/src/services/agent_trace.rscli/src/services/mod.rscli/src/services/patch.rsconfig/.opencode/plugins/sce-agent-trace.tsconfig/automated/.opencode/plugins/sce-agent-trace.tsconfig/lib/agent-trace-plugin/opencode-sce-agent-trace-plugin.tsflake.nix
Comment on lines
+82
to
+87
| let Some(intersection_hunk) = intersection_hunks | ||
| .iter() | ||
| .find(|h| h.old_start == post_commit_hunk.old_start) | ||
| else { | ||
| return HunkContributor::Unknown; | ||
| }; |
There was a problem hiding this comment.
Hunk matching by old_start alone can mis-bind when a file has multiple hunks sharing the same old_start.
For well-formed unified diffs this is essentially never the case, but hunks in intersection_patch preserve the post-commit hunk metadata (see intersect_patches at patch.rs:253-259), so if post_commit_patch itself happens to contain two hunks with the same old_start in a single file (e.g., a pathological old_start = 0 deleted/new-file layout, or a hand-crafted patch), .find(...) will return the first intersection hunk for both post-commit hunks and classify the second as Mixed/Ai based on the wrong peer. Consider indexing by the full (old_start, old_count, new_start, new_count) tuple, or consuming intersection hunks positionally as they are iterated so each intersection hunk is paired with exactly one post-commit hunk.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/services/agent_trace.rs` around lines 82 - 87, The current lookup of
an intersection hunk using only old_start (in the block that sets
intersection_hunk from intersection_hunks.iter().find(|h| h.old_start ==
post_commit_hunk.old_start)) can mispair hunks when multiple hunks share the
same old_start; update the matching logic in the function that classifies
HunkContributor so it matches on the full hunk identity (old_start, old_count,
new_start, new_count) or otherwise consume intersection_hunks in lockstep with
post_commit_hunks so each post_commit_hunk is paired with exactly one
intersection hunk; refer to intersection_hunks, post_commit_hunk,
HunkContributor::Unknown and consider the intersect_patches behavior in patch.rs
(lines around 253-259) when implementing the stronger tuple-based key or
positional pairing.
Comment on lines
+303
to
+333
| fn paths_refer_to_same_file(path_a: &str, path_b: &str) -> bool { | ||
| if path_a == path_b { | ||
| return true; | ||
| } | ||
|
|
||
| let a_components = normalized_path_components(path_a); | ||
| let b_components = normalized_path_components(path_b); | ||
|
|
||
| if a_components.is_empty() || b_components.is_empty() { | ||
| return false; | ||
| } | ||
|
|
||
| path_has_relative_suffix(&a_components, &b_components) | ||
| || path_has_relative_suffix(&b_components, &a_components) | ||
| } | ||
|
|
||
| fn normalized_path_components(path: &str) -> Vec<&str> { | ||
| Path::new(path) | ||
| .components() | ||
| .filter_map(|component| match component { | ||
| Component::Normal(part) => part.to_str(), | ||
| _ => None, | ||
| }) | ||
| .collect() | ||
| } | ||
|
|
||
| fn path_has_relative_suffix<'a>(full_path: &[&'a str], suffix_candidate: &[&'a str]) -> bool { | ||
| full_path.len() > suffix_candidate.len() | ||
| && full_path.ends_with(suffix_candidate) | ||
| && !suffix_candidate.is_empty() | ||
| } |
There was a problem hiding this comment.
Suffix-based path matching can conflate unrelated files.
paths_refer_to_same_file treats one path as a match for another whenever one's normalized components are a proper suffix of the other. That's useful for matching a/src/foo.rs to /abs/repo/src/foo.rs, but it will also match src/foo.rs to other-crate/src/foo.rs or match a short path like mod.rs to any */mod.rs, silently intersecting lines across different files. Consider requiring a minimum shared-segment count (e.g., full path or at least 2 segments) or only accept suffix matching when one of the paths is absolute.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/services/patch.rs` around lines 303 - 333, paths_refer_to_same_file
currently treats any proper-suffix match as equality which can conflate
unrelated files; update it to only accept suffix-based matches when the shared
suffix has at least a minimum number of segments (e.g., >= 2) or when one of the
original paths is absolute. Concretely, modify normalized_path_components or add
a small helper to preserve whether the original Path::is_absolute(), then in
paths_refer_to_same_file call path_has_relative_suffix only if
suffix_candidate.len() >= MIN_SHARED_SEGMENTS (set to 2) or if
Path::new(path_a).is_absolute() || Path::new(path_b).is_absolute(); update
path_has_relative_suffix to assume this precondition (or add the length check
there) so short single-segment suffixes like "mod.rs" no longer match arbitrary
files.
Comment on lines
+559
to
+565
| // Parse hunk header: @@ -old_start[,old_count] +new_start[,new_count] @@ | ||
| if let Some(rest) = line.strip_prefix("@@ ") { | ||
| if let Some(fb) = current_file.as_mut() { | ||
| let hunk = parse_hunk_header_and_body(rest, &mut lines)?; | ||
| fb.add_hunk(hunk); | ||
| } | ||
| } |
There was a problem hiding this comment.
@@ hunks silently dropped when no file header has been seen.
If a patch begins with a hunk header (or the file-header detection above misses a line), current_file.as_mut() returns None and the hunk is simply ignored with no diagnostic. Every other header branch (--- , +++ ) already raises a ParseError in this situation; the hunk branch should follow the same pattern so callers learn about malformed input instead of getting an empty patch.
🐛 Proposed fix
- if let Some(rest) = line.strip_prefix("@@ ") {
- if let Some(fb) = current_file.as_mut() {
- let hunk = parse_hunk_header_and_body(rest, &mut lines)?;
- fb.add_hunk(hunk);
- }
- }
+ if let Some(rest) = line.strip_prefix("@@ ") {
+ let fb = current_file.as_mut().ok_or_else(|| ParseError {
+ message: format!(
+ "encountered hunk header without a preceding file header: {line:?}"
+ ),
+ })?;
+ let hunk = parse_hunk_header_and_body(rest, &mut lines)?;
+ fb.add_hunk(hunk);
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/services/patch.rs` around lines 559 - 565, The code currently
silently drops hunk headers when current_file is None; update the @@ hunk
handling so that if current_file.as_mut() is None you return a ParseError (like
other header branches) instead of ignoring the hunk. In the block handling the
"@@ " prefix (where parse_hunk_header_and_body and fb.add_hunk are called),
detect the None case and construct/return an appropriate ParseError describing a
missing file header for the hunk, mirroring the error behavior used for the `---
`/`+++ ` branches so malformed input is reported to callers.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@cli/src/services/patch/fixtures/line_and_file_deletion_reconstruction/incremental_02.patch`:
- Around line 10-15: The fixture treats deletion of second.txt as an edit
because the new-file header uses "+++ second.txt"; to encode a deletion
explicitly, change the new path header to "+++ /dev/null" in
incremental_02.patch for the second.txt hunk (keep the hunk range @@ -1,1 +0,0
@@ as-is) so the parser recognizes this as a file deletion rather than an
edit-to-empty-file.
🪄 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: 1089d1a1-86ba-474f-8729-eb469d348507
⛔ Files ignored due to path filters (3)
context/cli/patch-service.mdis excluded by!context/**/*.mdcontext/context-map.mdis excluded by!context/**/*.mdcontext/plans/cli-patch-tmp-hunks-reconstruction.mdis excluded by!context/**/*.md
📒 Files selected for processing (5)
cli/src/services/patch/fixtures/line_and_file_deletion_reconstruction/golden.jsoncli/src/services/patch/fixtures/line_and_file_deletion_reconstruction/incremental_01.patchcli/src/services/patch/fixtures/line_and_file_deletion_reconstruction/incremental_02.patchcli/src/services/patch/fixtures/line_and_file_deletion_reconstruction/post_commit.patchcli/src/services/patch/tests.rs
Comment on lines
+10
to
+15
| Index: second.txt | ||
| =================================================================== | ||
| --- second.txt | ||
| +++ second.txt | ||
| @@ -1,1 +0,0 @@ | ||
| -temp |
There was a problem hiding this comment.
Encode second.txt deletion explicitly.
With +++ second.txt, this hunk is indistinguishable from editing second.txt down to an empty file, so the fixture may not exercise deleted-file parsing. Use /dev/null for the new path if the scenario is meant to cover file deletion.
🧪 Proposed fixture correction
Index: second.txt
===================================================================
--- second.txt
-+++ second.txt
++++ /dev/null
@@ -1,1 +0,0 @@
-temp📝 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.
Suggested change
| Index: second.txt | |
| =================================================================== | |
| --- second.txt | |
| +++ second.txt | |
| @@ -1,1 +0,0 @@ | |
| -temp | |
| Index: second.txt | |
| =================================================================== | |
| ++ /dev/null | |
| @@ -1,1 +0,0 @@ | |
| -temp |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@cli/src/services/patch/fixtures/line_and_file_deletion_reconstruction/incremental_02.patch`
around lines 10 - 15, The fixture treats deletion of second.txt as an edit
because the new-file header uses "+++ second.txt"; to encode a deletion
explicitly, change the new path header to "+++ /dev/null" in
incremental_02.patch for the second.txt hunk (keep the hunk range @@ -1,1 +0,0
@@ as-is) so the parser recognizes this as a file deletion rather than an
edit-to-empty-file.
Add library-only Rust seam at cli/src/services/agent_trace.rs that produces the minimal agent-trace JSON shape from patch data. Co-authored-by: SCE <sce@crocoder.dev>
Add reconstruction fixture sets and golden outputs so build_agent_trace is verified against real patch histories across multiple scenarios. Include the new fixture directory in flake sources so sandboxed validation can load the assets during nix flake check. Co-authored-by: SCE <sce@crocoder.dev>
Co-authored-by: SCE <sce@crocoder.dev>
Preserve constructed added and deleted files when intersecting patches by inferring file change kinds from hunk metadata and normalizing paths to match the inferred kind. Co-authored-by: SCE <sce@crocoder.dev>
Reshape the minimal agent-trace schema so each conversation serializes `contributor` as a nested object and emits `ranges` with derived `start_line`/`end_line` values per post-commit hunk. Co-authored-by: SCE <sce@crocoder.dev>
Add a regression test for reconstructing agent traces from a mixed sequence of incremental and post-commit patch changes. This captures a realistic edit history with empty files, additions, deletions, and mixed contributor attribution so the golden output stays stable. Co-authored-by: SCE <sce@crocoder.dev>
| { | ||
| "files": [ | ||
| { | ||
| "path": "", |
Contributor
There was a problem hiding this comment.
Path should not be empty
Comment on lines
+12
to
+13
| "start_line": 0, | ||
| "end_line": 0 |
Contributor
There was a problem hiding this comment.
start_line and end_line should be integers and minimum 1 by specs
Co-authored-by: SCE <sce@crocoder.dev>
Expand deleted `.patch` files into trace entries for their embedded files, use the correct line ranges for deletions, and skip metadata-only file entries so reconstructed traces match post-commit reality more faithfully. Co-authored-by: SCE <sce@crocoder.dev>
Cover pure rename diffs in the patch and agent trace test suites so rename-only commits are reconstructed consistently. Co-authored-by: SCE <sce@crocoder.dev>
…data Co-authored-by: SCE <sce@crocoder.dev>
davidabram
commented
Apr 23, 2026
davidabram
commented
Apr 23, 2026
davidabram
commented
Apr 23, 2026
Co-authored-by: SCE <sce@crocoder.dev>
Implement internal validation seam for Agent Trace JSON using a compiled JSON Schema embedded at compile time. Co-authored-by: SCE <sce@crocoder.dev>
Add nodePackages.vscode-json-languageserver to devShell packages to enable JSON language server support for editor integrations. Co-authored-by: SCE <sce@crocoder.dev>
Add the turso crate (0.5.3) to support local DB functionality for agent traces. Co-authored-by: SCE <sce@crocoder.dev>
Co-authored-by: SCE <sce@crocoder.dev>
Add LocalDb struct in cli/src/services/local_db.rs that wraps a Turso connection with a tokio current-thread runtime for blocking operations. Embed migrations at compile time via include_str! from cli/migrations/. Co-authored-by: SCE <sce@crocoder.dev>
Initialize the canonical local DB during setup by adding a setup bootstrap call after repo/config bootstrap, and align setup/local-db context contracts to this behavior. Co-authored-by: SCE <sce@crocoder.dev>
Align the CLI and root context with the implemented behavior by describing setup/doctor as the current local DB entrypoints and deferring command wiring to 0.4.0. The staged plan update cancels the old wiring task and records the completed documentation task. Co-authored-by: SCE <sce@crocoder.dev>
Co-authored-by: SCE <sce@crocoder.dev>
…li hooks Co-authored-by: SCE <sce@crocoder.dev>
Co-authored-by: SCE <sce@crocoder.dev>
Remove direct context/tmp event writes from the OpenCode trace plugin so captured trace data flows through the CLI hook path instead. Co-authored-by: SCE <sce@crocoder.dev>
…ames Replace loose serde_json::Value parsing in the diff-trace hook path with an explicit DiffTracePayload type that enforces u64 Unix epoch milliseconds for time, required non-empty sessionID/diff strings, and deterministic validation diagnostics. Trace artifact filenames now use atomic create-new retry semantics with zero-padded attempt suffixes so separate short-lived processes cannot overwrite same-millisecond artifacts. Co-authored-by: SCE <sce@crocoder.dev>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Tests