feat(git): add worktree_diff method for read-only working-tree diff (#1708) #245
Loading…
Reference in a new issue
No description provided.
Delete branch "worktree-1708"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
worktree_diffmethod to@swamp/gitthat compares a base ref (default HEAD) against the working tree, including staged, unstaged, and untracked non-ignored filesnameOnly,stat, and path filter options; makes no index, worktree, or network mutationsgit diff --no-indexfor untracked file patches, keeping all operations through theexecGitrunnerCloses #1708
Changes
_lib/schemas.tsWorktreeDiffArgsSchemaandWorktreeDiffResultSchema_lib/operations.tsrunWorktreeDiff— runsgit diff <base>,git ls-files --others --exclude-standard, andgit diff --no-indexfor each untracked filegit.tsgit_test.tsmanifest.yamlREADME.mdDesign decisions
ephemerallifetime with GC 50 instead ofworkflowlifetime —workflowis unused in this repo and unvalidated as a runtime value. GC 50 matchesupstreamStateResultprecedent for longer evidence retention.git diff --no-index /dev/null <file>for untracked content instead ofDeno.readTextFile— keeps all operations throughexecGit, consistent with existing patterns, testable via mock executor.files(tracked, populated in nameOnly mode) +untrackedFiles(always populated) +raw(combined diff output) to give callers both structured path lists and reviewable patches.Test plan
deno check extensions/models/git.ts— passdeno lint extensions/models/— passdeno fmt --check extensions/models/— passdeno test— 191/191 pass (14 new worktree_diff tests)deno install --frozen— pass🤖 Generated with Claude Code
Code Review
Blocking Issues
countis semantically undefined in non-nameOnly mode (operations.ts:1687–1707)trackedFilesis computed as:It is always
[]whennameOnly=false. The result object then sets:In full-diff and stat modes:
files = []andcount = untrackedFiles.length. A repo with 10 tracked changed files and zero untracked files will producecount = 0andfiles = [], even thoughrawcontains a substantial diff. Downstream consumers checkingresult.count === 0orresult.files.length === 0to decide whether changes exist will get a false negative.The fix depends on intent:
nameOnly", thencountshould benull/omitted in non-nameOnly mode (or documented as "untracked file count only") and the schema and README updated to reflect that.countis meant to be a total change count, always rungit diff --name-onlyas a secondary call to get the tracked file list, or usegit diff --name-onlyunconditionally and keep the full patch separately.The inconsistency between
count != files.lengthin non-nameOnly mode (count is untracked count, files is empty) makes the result shape ambiguous and easy to misuse. This needs to be resolved before merge — either by fixing the count semantics or by clearly constraining the schema.Suggestions
pathsarray entries have no dash-check (schemas.ts:437)Other schemas in this file also use bare
z.array(z.string()).optional()for path filters (lines 65, 76, 83, 96, 107), so this is consistent with existing patterns. The--separator in bothgit diffandgit ls-filescalls (operations.ts:1627,1647) prevents path strings from being interpreted as flags by git. No code injection is possible via subprocess argv. Not blocking, but worth noting as a defence-in-depth opportunity if the team ever adds a linting rule for it across the whole schema file.Untracked file patches omitted from
countin stat mode (operations.ts:1666)In stat mode,
--no-indexdiffs for untracked files are skipped (correct —--statoutput for new untracked files is not collected), but those untracked files are still inuntrackedFilesand contribute tocountvia the same formula. Minor: consumers usingstat: trueto get a stat summary won't see untracked file stats inraw, even though they appear incount/untrackedFiles. Consider documenting this limitation or collecting--stat --no-indexfor untracked files too.Test for full-diff mode doesn't assert
count(git_test.ts:3280–3314)The "full diff includes untracked file patches" test verifies
rawcontent and call count but never assertswrites[0].data.countorwrites[0].data.files. Adding those assertions would have caught the blocking issue above.Adversarial Review
Medium
countfield is a misleading partial count in non-nameOnlymode —git/extensions/models/_lib/operations.ts:1700-1707When
nameOnly=false,trackedFilesis always[](line 1687-1689), so the count formulatrackedFiles.length + untrackedFiles.lengthreduces to justuntrackedFiles.length. This means:countis0even thoughrawcontains diff output. A consumer checkingcount === 0to decide "clean working tree" would get a false positive.countdiverges fromfiles.length—filesis[]butcountcould be non-zero (when untracked files exist). The siblingdiffmethod always hascount === files.length, making this inconsistency surprising.Breaking example: Working tree has 3 modified tracked files, no untracked files.
nameOnly=false. Result:files=[], untrackedFiles=[], count=0, raw="<3 file diffs>". Thecountsays "nothing changed" butrawdisagrees.Suggested fix: Either (a) always set
count: allFiles.lengthand document thatcountis only meaningful innameOnlymode (matchingdiff's pattern where count=0 in non-nameOnly mode), or (b) parse--name-onlyoutput in a separate git call to always populate a correct count. Option (a) is simpler and makes the behavior consistent — the current formula is equivalent toallFiles.lengthin all cases anyway, but the branch in the ternary is misleading.Note: the test suite doesn't cover non-
nameOnlymode with tracked-only changes, so this inconsistency isn't exercised.Low
Sequential subprocess spawning per untracked file —
git/extensions/models/_lib/operations.ts:1667-1679When
nameOnly=falseandstat=false, each untracked file spawns a separategit diff --no-indexsubprocess sequentially. In a CI environment where a build step generates many untracked files (100+), this creates 100+ sequential subprocess invocations. Unlikely to hit in the documented use case (pre-commit review), but worth noting for robustness.TOCTOU between
ls-filesanddiff --no-index—git/extensions/models/_lib/operations.ts:1651-1679An untracked file enumerated by
git ls-files --otherscould be deleted beforegit diff --no-index -- /dev/null <file>runs. The--no-indexcall would fail with a non-0/non-1 exit code, throwing an error. In practice this requires concurrent file deletion during the method execution, which is unlikely but possible in CI with parallel cleanup tasks.Verdict
PASS — The new
worktree_diffmethod is well-structured, follows established patterns (span lifecycle, error handling, input validation viasafeRef,--separators for pathspecs), and has comprehensive test coverage. Thecountinconsistency (Medium #1) is a design wart, not a correctness bug in the primarynameOnly=truecode path that CI consumers will use. The credential scrubbing, dash-prefix rejection, and signal threading are all properly applied. No blocking issues.Code Review
Blocking Issues
None.
Suggestions
rawfield is inconsistent withfilesinnameOnlymode (operations.ts:1643): WhennameOnly: true,rawis set tonameOnlyResult.stdout— the output ofgit diff --name-only, which lists only tracked changed files. Butfilesincludes both tracked and untracked files. A caller usingrawas their file list in nameOnly mode will silently miss untracked files. Consider either appending untracked file names torawin nameOnly mode, or adding a note to theWorktreeDiffResultSchemadescribing this asymmetry.nameOnly+statconflict is undocumented (schemas.ts:433): When bothnameOnly: trueandstat: trueare supplied,nameOnlysilently wins andstatis ignored (seeoperations.ts:1643). Since these flags are mutually exclusive in practice, a brief note on the schema description forstat(e.g., "ignored whennameOnlyis true") would prevent user confusion.README method-count jump (
README.md:376): The table previously said "all 14 methods" forgit-available, but the model had 16 methods before this PR (the test suite atgit_test.tswas already asserting 16). This PR corrects it to 17, which is accurate now, but skips acknowledging the pre-existing off-by-two. This is a pre-existing documentation gap, not introduced by this PR, so no action is required — just noting it for awareness.Adversarial Review
Critical / High
None.
Medium
None.
Low
git/extensions/models/_lib/operations.ts:1703—worktree_diffcan produce duplicate file entries in a narrow edge case.The
filesarray is built by concatenating tracked changed files (git diff --name-only <base>) with untracked files (git ls-files --others --exclude-standard):If a file has been removed from the index via
git rm --cachedand then modified on disk, it appears in both lists:git diff --name-only HEADsees it because HEAD has the file and the on-disk content differs.git ls-files --otherssees it because it's no longer in the index.Breaking example:
git rm --cached foo.ts, editfoo.ts, then callworktree_diff→filescontainsfoo.tstwice,countis inflated by 1.Why this is LOW: The primary use case is CI change detection where
git rm --cachedmid-workflow is atypical. Therawdiff output is unaffected — only thefilesarray andcountare wrong. A deduplicated set ([...new Set(allFiles)]) would fix it, but the scenario is uncommon enough that this is informational.git/extensions/models/_lib/operations.ts:159-162—diffmethod reportscount: 0and emptyfileswhennameOnlyis false.When a consumer calls
diffwithstat: trueor with neithernameOnlynorstat(full patch mode), the resource always reportscount: 0andfiles: [], whileworktree_diffalways populates them (per the recent fix in3c7245ccd). A consumer checkingcountto detect "any changes?" would get a false negative fromdiffin non-nameOnlymode.Why this is LOW: The README examples consistently pair file-counting with
nameOnly: true, so this is a documented contract rather than a bug. But the inconsistency withworktree_diff(which always populatesfiles/count) could surprise a consumer who switches between the two methods.Notes (not findings)
safeRefrefinements rejecting leading dashes. All path arguments are placed after--separators. Credential scrubbing covers error messages, resource data, tags, and logs. The token-in-URL pattern is standard for CI git clones.git logformat (%H%x00%an%x00%aI%x00%s%x00) handles multi-commit output with inter-record newlines correctly viatrim()on the SHA field..meta({ sensitive: true })on the token field,.refine()for cross-field validation, andz.input<>for pre-default types are all valid v4 patterns.Verdict
PASS — The code is well-structured, defensively written, and comprehensively tested. No correctness, security, or data integrity issues that would affect production use. The two LOW findings are edge-case inconsistencies, not blocking problems.