🤖 refactor: extract the task git patch engine from the apply tool - #4008
🤖 refactor: extract the task git patch engine from the apply tool#4008ibetitsmike wants to merge 4 commits into
Conversation
Audit-driven cleanup: the engine no longer imports tools/toolUtils (inlined workspaceId assert, engine-named assert messages) and returns typed result literals instead of re-validating its own output through the result schema. The workflow adapter passes allowedPathPrefixes on the dry-run precheck too, so policy violations fail fast before the worktree dance.
This comment has been minimized.
This comment has been minimized.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a1d5d9c36
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This comment has been minimized.
This comment has been minimized.
Codex P2: git C-quotes each header side independently, so an ASCII to non-ASCII rename emits a mixed quoted/unquoted path pair that the consolidated parser treated as unparseable, falsely rejecting valid renames under allowedPathPrefixes. Handle both mixed directions and cover them with a real-repo rename round trip.
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This comment has been minimized.
This comment has been minimized.
1 participant
Summary
Extracts the git patch engine out of
tools/task_apply_git_patch.tsbehind a 3-methodTaskGitPatchEngineseam (applyPatch,findPatch,generatePatch) insrc/node/services/taskGitPatchEngine.ts. The model tool and the workflow adapter become thin adapters that own their own locking, and the engine has notaskServicedependency.Net LOC delta vs
main(f04e0f8): production -192 (+2113/-2305), tests -364 (+2125/-2489), total -556. No irreducible net additions: every new line is the seam's interface surface, offset by deleted duplication.Background
From the 2026-08-29 architecture review: the 1,995-line tool file was a full git diff parser, dirty-tree detector, and 3-way merge engine with a 9-line tool adapter at the bottom.
WorkflowTaskServiceAdapterimported those internals and passedtaskService: undefinedto dodge the non-reentrant artifact lock (an inverted seam where the engine knew about the lock owner and callers had to know to disarm it), while carrying its own duplicate git patch-path parser forallowedPathPrefixesvalidation. The rawgit format-patchinvocation lived inline ingitPatchArtifactService.Implementation
src/node/services/taskGitPatchEngine.ts(moved withgit mvto retain history): the apply pipeline, parsers, dirty-tree overlap detection, pending-generation wait, and ancestor artifact lookup, exposed asapplyPatch/findPatch/generatePatch. The engine validates untrusted args at entry but returns typed result literals instead of re-validating its own output; it no longer imports tool-layer utilities.tools/task_apply_git_patch.tsis now a 17-line adapter: schema wiring plus lock acquisition viaconfig.taskService.withGitPatchArtifactOperationLockwhen present.WorkflowTaskServiceAdapter.applyPatchholds its existing lock and calls the engine directly; thetaskService: undefinedhack, its duplicate parser (~103 lines), and the artifact-lookup helpers are deleted.allowedPathPrefixesis enforced inside the engine and rides the dry-run precheck, so policy violations fail before any worktree is created.gitPatchArtifactServicekeeps all artifact bookkeeping and callsengine.generatePatchfor the rawgit format-patchstream.findPatchcurrently has no production caller outside the engine (the adapter consumesPick<TaskGitPatchEngine, "applyPatch">); it stays on the seam intentionally as the lookup capability of the engine and is covered directly by the engine tests.A Codex review round caught a real regression in the consolidated parser (the old adapter parser handled mixed quoted/unquoted
diff --githeader sides; the consolidation did not); fixed with a real-repo rename round trip covering both mixed-quoting directions.Tests: the 2,394-line tool test was moved into
taskGitPatchEngine.test.ts(2,015 lines) and reworked to exercise the engine directly on real temp git repos, including a generate-then-apply round trip; the tool test shrank to a 52-line dispatch test (lock acquisition and delegation); adapter patch tests became seam tests against a fake engine.Validation
make static-checkgreen; 94 tests across the five touched suites pass.Risks
Behavior-preserving refactor of intricate logic (patch application). The two intentional behavior deltas are internal and narrow: prefix-policy violations in workflow applies now fail at the dry-run precheck instead of after it (same error, earlier), and engine results are no longer re-parsed through the result schema at runtime (they are typed literals; the schema remains the tool contract). Severity if wrong: patch application in sub-agent and workflow flows, covered by the real-repo suites above.
Generated with
xum• Model:anthropic:claude-fable-5• Thinking:xhigh• Cost:$51.58