feat(git): add worktree_diff method for read-only working-tree diff (#1708) #245

Merged
stack72 merged 2 commits from worktree-1708 into main 2026-08-31 05:12:41 +00:00
Owner

Summary

  • Add a new worktree_diff method to @swamp/git that compares a base ref (default HEAD) against the working tree, including staged, unstaged, and untracked non-ignored files
  • Supports nameOnly, stat, and path filter options; makes no index, worktree, or network mutations
  • Uses git diff --no-index for untracked file patches, keeping all operations through the execGit runner

Closes #1708

Changes

File What changed
_lib/schemas.ts New WorktreeDiffArgsSchema and WorktreeDiffResultSchema
_lib/operations.ts New runWorktreeDiff — runs git diff <base>, git ls-files --others --exclude-standard, and git diff --no-index for each untracked file
git.ts Registered method, resource (ephemeral/GC 50), checks, version bump 2026.08.30.1 + upgrade entry
git_test.ts 14 new tests (191 total) — schema validation, operation behavior, error handling, signal threading
manifest.yaml Version bump, method table entry
README.md Usage docs, method/resource table entries

Design decisions

  • ephemeral lifetime with GC 50 instead of workflow lifetime — workflow is unused in this repo and unvalidated as a runtime value. GC 50 matches upstreamStateResult precedent for longer evidence retention.
  • git diff --no-index /dev/null <file> for untracked content instead of Deno.readTextFile — keeps all operations through execGit, consistent with existing patterns, testable via mock executor.
  • Result schema uses 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 — pass
  • deno lint extensions/models/ — pass
  • deno fmt --check extensions/models/ — pass
  • deno test — 191/191 pass (14 new worktree_diff tests)
  • deno install --frozen — pass
  • Pre-PR verification workflows blocked by pre-existing bugs (#1905, #1906)

🤖 Generated with Claude Code

## Summary - Add a new `worktree_diff` method to `@swamp/git` that compares a base ref (default HEAD) against the working tree, including staged, unstaged, and untracked non-ignored files - Supports `nameOnly`, `stat`, and path filter options; makes no index, worktree, or network mutations - Uses `git diff --no-index` for untracked file patches, keeping all operations through the `execGit` runner Closes #1708 ## Changes | File | What changed | | ---- | ------------ | | `_lib/schemas.ts` | New `WorktreeDiffArgsSchema` and `WorktreeDiffResultSchema` | | `_lib/operations.ts` | New `runWorktreeDiff` — runs `git diff <base>`, `git ls-files --others --exclude-standard`, and `git diff --no-index` for each untracked file | | `git.ts` | Registered method, resource (ephemeral/GC 50), checks, version bump 2026.08.30.1 + upgrade entry | | `git_test.ts` | 14 new tests (191 total) — schema validation, operation behavior, error handling, signal threading | | `manifest.yaml` | Version bump, method table entry | | `README.md` | Usage docs, method/resource table entries | ## Design decisions - **`ephemeral` lifetime with GC 50** instead of `workflow` lifetime — `workflow` is unused in this repo and unvalidated as a runtime value. GC 50 matches `upstreamStateResult` precedent for longer evidence retention. - **`git diff --no-index /dev/null <file>`** for untracked content instead of `Deno.readTextFile` — keeps all operations through `execGit`, consistent with existing patterns, testable via mock executor. - **Result schema** uses `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 - [x] `deno check extensions/models/git.ts` — pass - [x] `deno lint extensions/models/` — pass - [x] `deno fmt --check extensions/models/` — pass - [x] `deno test` — 191/191 pass (14 new worktree_diff tests) - [x] `deno install --frozen` — pass - [ ] Pre-PR verification workflows blocked by pre-existing bugs (#1905, #1906) 🤖 Generated with [Claude Code](https://claude.com/claude-code)
feat(git): add worktree_diff method for read-only working-tree diff (#1708)
Some checks failed
CI: Extensions / cve/researcher - fmt (pull_request) Has been skipped
CI: Extensions / cve/researcher - lint (pull_request) Has been skipped
CI: Extensions / cve/researcher - test (pull_request) Has been skipped
CI: Extensions / cve/dirtyfrag - lockfile up to date (pull_request) Has been skipped
CI: Extensions / cve/mini-shai-hulud - lockfile up to date (pull_request) Has been skipped
CI: Extensions / cve/researcher - lockfile up to date (pull_request) Has been skipped
CI: Extensions / software-factory - check (pull_request) Has been skipped
CI: Extensions / software-factory - fmt (pull_request) Has been skipped
CI: Extensions / software-factory - lint (pull_request) Has been skipped
CI: Extensions / software-factory - test (pull_request) Has been skipped
CI: Extensions / software-factory - lockfile up to date (pull_request) Has been skipped
CI: Extensions / container-image - check (pull_request) Has been skipped
CI: Extensions / container-image - fmt (pull_request) Has been skipped
CI: Extensions / container-image - lint (pull_request) Has been skipped
CI: Extensions / container-image - test (pull_request) Has been skipped
CI: Extensions / container-image - lockfile up to date (pull_request) Has been skipped
CI: Reviews / Detect Changes (pull_request) Successful in 17s
CI: Reviews / CI Security Review (pull_request) Has been skipped
CI / Actions Audit (pull_request) Successful in 22s
CI: Extensions / git - check (pull_request) Successful in 24s
CI / Dependency Audit (pull_request) Successful in 2m26s
CI: Extensions / git - fmt (pull_request) Successful in 25s
CI: Extensions / git - lint (pull_request) Successful in 23s
CI: Extensions / git - test (pull_request) Successful in 25s
CI: Extensions / git - lockfile up to date (pull_request) Successful in 24s
CI / Gate: Audit (pull_request) Successful in 0s
CI: Extensions / Gate: Extensions (pull_request) Successful in 0s
CI: Reviews / Claude Code Review (pull_request) Failing after 3m32s
CI: Reviews / Adversarial Code Review (pull_request) Successful in 4m35s
CI: Reviews / Gate: Reviews (pull_request) Failing after 0s
c17c577a76
Add a new worktree_diff method to @swamp/git that compares a base ref
against the working tree, including staged, unstaged, and untracked
non-ignored files. Unlike the existing diff (ref-to-ref only) and status
(paths without patches), worktree_diff provides reviewable patch output
covering the full working-tree state.

Supports nameOnly, stat, and path filter options. Makes no index,
worktree, or network mutations. Uses git diff --no-index for untracked
file patches, keeping all operations through the execGit runner.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Author
Owner

Code Review

Blocking Issues

  1. count is semantically undefined in non-nameOnly mode (operations.ts:1687–1707)

    trackedFiles is computed as:

    const trackedFiles = args.nameOnly
      ? trackedRaw.split("\n").filter((l) => l.trim().length > 0)
      : [];
    

    It is always [] when nameOnly=false. The result object then sets:

    files: args.nameOnly ? allFiles : trackedFiles,  // [] in full-diff/stat modes
    count: args.nameOnly
      ? allFiles.length
      : trackedFiles.length + untrackedFiles.length, // 0 + untrackedFiles.length
    

    In full-diff and stat modes: files = [] and count = untrackedFiles.length. A repo with 10 tracked changed files and zero untracked files will produce count = 0 and files = [], even though raw contains a substantial diff. Downstream consumers checking result.count === 0 or result.files.length === 0 to decide whether changes exist will get a false negative.

    The fix depends on intent:

    • If the design is "file enumeration requires nameOnly", then count should be null/omitted in non-nameOnly mode (or documented as "untracked file count only") and the schema and README updated to reflect that.
    • If count is meant to be a total change count, always run git diff --name-only as a secondary call to get the tracked file list, or use git diff --name-only unconditionally and keep the full patch separately.

    The inconsistency between count != files.length in 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

  1. paths array 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 both git diff and git ls-files calls (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.

  2. Untracked file patches omitted from count in stat mode (operations.ts:1666)

    In stat mode, --no-index diffs for untracked files are skipped (correct — --stat output for new untracked files is not collected), but those untracked files are still in untrackedFiles and contribute to count via the same formula. Minor: consumers using stat: true to get a stat summary won't see untracked file stats in raw, even though they appear in count/untrackedFiles. Consider documenting this limitation or collecting --stat --no-index for untracked files too.

  3. Test for full-diff mode doesn't assert count (git_test.ts:3280–3314)

    The "full diff includes untracked file patches" test verifies raw content and call count but never asserts writes[0].data.count or writes[0].data.files. Adding those assertions would have caught the blocking issue above.

## Code Review ### Blocking Issues 1. **`count` is semantically undefined in non-nameOnly mode** (`operations.ts:1687–1707`) `trackedFiles` is computed as: ```typescript const trackedFiles = args.nameOnly ? trackedRaw.split("\n").filter((l) => l.trim().length > 0) : []; ``` It is **always `[]` when `nameOnly=false`**. The result object then sets: ```typescript files: args.nameOnly ? allFiles : trackedFiles, // [] in full-diff/stat modes count: args.nameOnly ? allFiles.length : trackedFiles.length + untrackedFiles.length, // 0 + untrackedFiles.length ``` In full-diff and stat modes: `files = []` and `count = untrackedFiles.length`. A repo with 10 tracked changed files and zero untracked files will produce `count = 0` and `files = []`, even though `raw` contains a substantial diff. Downstream consumers checking `result.count === 0` or `result.files.length === 0` to decide whether changes exist will get a false negative. The fix depends on intent: - If the design is "file enumeration requires `nameOnly`", then `count` should be `null`/omitted in non-nameOnly mode (or documented as "untracked file count only") and the schema and README updated to reflect that. - If `count` is meant to be a total change count, always run `git diff --name-only` as a secondary call to get the tracked file list, or use `git diff --name-only` unconditionally and keep the full patch separately. The inconsistency between `count != files.length` in 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 1. **`paths` array 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 both `git diff` and `git ls-files` calls (`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. 2. **Untracked file patches omitted from `count` in stat mode** (`operations.ts:1666`) In stat mode, `--no-index` diffs for untracked files are skipped (correct — `--stat` output for new untracked files is not collected), but those untracked files are still in `untrackedFiles` and contribute to `count` via the same formula. Minor: consumers using `stat: true` to get a stat summary won't see untracked file stats in `raw`, even though they appear in `count`/`untrackedFiles`. Consider documenting this limitation or collecting `--stat --no-index` for untracked files too. 3. **Test for full-diff mode doesn't assert `count`** (`git_test.ts:3280–3314`) The "full diff includes untracked file patches" test verifies `raw` content and call count but never asserts `writes[0].data.count` or `writes[0].data.files`. Adding those assertions would have caught the blocking issue above.
Author
Owner

Adversarial Review

Medium

  1. count field is a misleading partial count in non-nameOnly modegit/extensions/models/_lib/operations.ts:1700-1707

    When nameOnly=false, trackedFiles is always [] (line 1687-1689), so the count formula trackedFiles.length + untrackedFiles.length reduces to just untrackedFiles.length. This means:

    • If the working tree has only tracked changes (modified/staged files, no untracked files), count is 0 even though raw contains diff output. A consumer checking count === 0 to decide "clean working tree" would get a false positive.
    • count diverges from files.lengthfiles is [] but count could be non-zero (when untracked files exist). The sibling diff method always has count === 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>". The count says "nothing changed" but raw disagrees.

    Suggested fix: Either (a) always set count: allFiles.length and document that count is only meaningful in nameOnly mode (matching diff's pattern where count=0 in non-nameOnly mode), or (b) parse --name-only output 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 to allFiles.length in all cases anyway, but the branch in the ternary is misleading.

    Note: the test suite doesn't cover non-nameOnly mode with tracked-only changes, so this inconsistency isn't exercised.

Low

  1. Sequential subprocess spawning per untracked filegit/extensions/models/_lib/operations.ts:1667-1679

    When nameOnly=false and stat=false, each untracked file spawns a separate git diff --no-index subprocess 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.

  2. TOCTOU between ls-files and diff --no-indexgit/extensions/models/_lib/operations.ts:1651-1679

    An untracked file enumerated by git ls-files --others could be deleted before git diff --no-index -- /dev/null <file> runs. The --no-index call 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_diff method is well-structured, follows established patterns (span lifecycle, error handling, input validation via safeRef, -- separators for pathspecs), and has comprehensive test coverage. The count inconsistency (Medium #1) is a design wart, not a correctness bug in the primary nameOnly=true code path that CI consumers will use. The credential scrubbing, dash-prefix rejection, and signal threading are all properly applied. No blocking issues.

## Adversarial Review ### Medium 1. **`count` field is a misleading partial count in non-`nameOnly` mode** — `git/extensions/models/_lib/operations.ts:1700-1707` When `nameOnly=false`, `trackedFiles` is always `[]` (line 1687-1689), so the count formula `trackedFiles.length + untrackedFiles.length` reduces to just `untrackedFiles.length`. This means: - If the working tree has **only tracked changes** (modified/staged files, no untracked files), `count` is `0` even though `raw` contains diff output. A consumer checking `count === 0` to decide "clean working tree" would get a false positive. - `count` diverges from `files.length` — `files` is `[]` but `count` could be non-zero (when untracked files exist). The sibling `diff` method always has `count === 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>"`. The `count` says "nothing changed" but `raw` disagrees. **Suggested fix:** Either (a) always set `count: allFiles.length` and document that `count` is only meaningful in `nameOnly` mode (matching `diff`'s pattern where count=0 in non-nameOnly mode), or (b) parse `--name-only` output 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 to `allFiles.length` in all cases anyway, but the branch in the ternary is misleading. Note: the test suite doesn't cover non-`nameOnly` mode with tracked-only changes, so this inconsistency isn't exercised. ### Low 2. **Sequential subprocess spawning per untracked file** — `git/extensions/models/_lib/operations.ts:1667-1679` When `nameOnly=false` and `stat=false`, each untracked file spawns a separate `git diff --no-index` subprocess 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. 3. **TOCTOU between `ls-files` and `diff --no-index`** — `git/extensions/models/_lib/operations.ts:1651-1679` An untracked file enumerated by `git ls-files --others` could be deleted before `git diff --no-index -- /dev/null <file>` runs. The `--no-index` call 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_diff` method is well-structured, follows established patterns (span lifecycle, error handling, input validation via `safeRef`, `--` separators for pathspecs), and has comprehensive test coverage. The `count` inconsistency (Medium #1) is a design wart, not a correctness bug in the primary `nameOnly=true` code path that CI consumers will use. The credential scrubbing, dash-prefix rejection, and signal threading are all properly applied. No blocking issues.
fix(git): always enumerate tracked files for accurate count in worktree_diff
All checks were successful
CI: Extensions / cve/researcher - fmt (pull_request) Has been skipped
CI: Extensions / cve/researcher - lint (pull_request) Has been skipped
CI: Extensions / cve/researcher - test (pull_request) Has been skipped
CI: Extensions / cve/dirtyfrag - lockfile up to date (pull_request) Has been skipped
CI: Extensions / cve/mini-shai-hulud - lockfile up to date (pull_request) Has been skipped
CI: Extensions / cve/researcher - lockfile up to date (pull_request) Has been skipped
CI: Extensions / software-factory - check (pull_request) Has been skipped
CI: Extensions / software-factory - fmt (pull_request) Has been skipped
CI: Extensions / software-factory - lint (pull_request) Has been skipped
CI: Extensions / software-factory - test (pull_request) Has been skipped
CI: Extensions / software-factory - lockfile up to date (pull_request) Has been skipped
CI: Extensions / container-image - check (pull_request) Has been skipped
CI: Extensions / container-image - fmt (pull_request) Has been skipped
CI: Extensions / container-image - lint (pull_request) Has been skipped
CI: Extensions / container-image - test (pull_request) Has been skipped
CI: Extensions / container-image - lockfile up to date (pull_request) Has been skipped
CI: Reviews / Detect Changes (pull_request) Successful in 20s
CI: Reviews / CI Security Review (pull_request) Has been skipped
CI / Actions Audit (pull_request) Successful in 25s
CI: Extensions / git - check (pull_request) Successful in 32s
CI: Extensions / git - fmt (pull_request) Successful in 23s
CI / Dependency Audit (pull_request) Successful in 2m30s
CI: Extensions / git - lint (pull_request) Successful in 23s
CI: Extensions / git - test (pull_request) Successful in 26s
CI: Extensions / git - lockfile up to date (pull_request) Successful in 25s
CI: Reviews / Claude Code Review (pull_request) Successful in 2m31s
CI / Gate: Audit (pull_request) Successful in 0s
CI: Extensions / Gate: Extensions (pull_request) Successful in 0s
CI: Reviews / Adversarial Code Review (pull_request) Successful in 6m16s
CI: Reviews / Gate: Reviews (pull_request) Successful in 0s
3c7245ccd4
The previous implementation only populated files/count in nameOnly mode.
In full-diff and stat modes, trackedFiles was always [], making count
equal to just untrackedFiles.length — a repo with 10 tracked changes and
zero untracked files would report count=0.

Fix: always run git diff --name-only as the first call to get the
tracked file list, then run the primary diff (patch/stat) as a separate
call when needed. files and count are now accurate in all modes.

Add test for tracked-only changes in full-diff mode to exercise the
previously broken path.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Author
Owner

Code Review

Blocking Issues

None.

Suggestions

  1. raw field is inconsistent with files in nameOnly mode (operations.ts:1643): When nameOnly: true, raw is set to nameOnlyResult.stdout — the output of git diff --name-only, which lists only tracked changed files. But files includes both tracked and untracked files. A caller using raw as their file list in nameOnly mode will silently miss untracked files. Consider either appending untracked file names to raw in nameOnly mode, or adding a note to the WorktreeDiffResultSchema describing this asymmetry.

  2. nameOnly + stat conflict is undocumented (schemas.ts:433): When both nameOnly: true and stat: true are supplied, nameOnly silently wins and stat is ignored (see operations.ts:1643). Since these flags are mutually exclusive in practice, a brief note on the schema description for stat (e.g., "ignored when nameOnly is true") would prevent user confusion.

  3. README method-count jump (README.md:376): The table previously said "all 14 methods" for git-available, but the model had 16 methods before this PR (the test suite at git_test.ts was 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.

## Code Review ### Blocking Issues None. ### Suggestions 1. **`raw` field is inconsistent with `files` in `nameOnly` mode** (`operations.ts:1643`): When `nameOnly: true`, `raw` is set to `nameOnlyResult.stdout` — the output of `git diff --name-only`, which lists only tracked changed files. But `files` includes both tracked and untracked files. A caller using `raw` as their file list in nameOnly mode will silently miss untracked files. Consider either appending untracked file names to `raw` in nameOnly mode, or adding a note to the `WorktreeDiffResultSchema` describing this asymmetry. 2. **`nameOnly` + `stat` conflict is undocumented** (`schemas.ts:433`): When both `nameOnly: true` and `stat: true` are supplied, `nameOnly` silently wins and `stat` is ignored (see `operations.ts:1643`). Since these flags are mutually exclusive in practice, a brief note on the schema description for `stat` (e.g., "ignored when `nameOnly` is true") would prevent user confusion. 3. **README method-count jump** (`README.md:376`): The table previously said "all 14 methods" for `git-available`, but the model had 16 methods before this PR (the test suite at `git_test.ts` was 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.
Author
Owner

Adversarial Review

Critical / High

None.

Medium

None.

Low

  1. git/extensions/models/_lib/operations.ts:1703worktree_diff can produce duplicate file entries in a narrow edge case.

    The files array is built by concatenating tracked changed files (git diff --name-only <base>) with untracked files (git ls-files --others --exclude-standard):

    const allFiles = [...trackedFiles, ...untrackedFiles];
    

    If a file has been removed from the index via git rm --cached and then modified on disk, it appears in both lists:

    • git diff --name-only HEAD sees it because HEAD has the file and the on-disk content differs.
    • git ls-files --others sees it because it's no longer in the index.

    Breaking example: git rm --cached foo.ts, edit foo.ts, then call worktree_difffiles contains foo.ts twice, count is inflated by 1.

    Why this is LOW: The primary use case is CI change detection where git rm --cached mid-workflow is atypical. The raw diff output is unaffected — only the files array and count are wrong. A deduplicated set ([...new Set(allFiles)]) would fix it, but the scenario is uncommon enough that this is informational.

  2. git/extensions/models/_lib/operations.ts:159-162diff method reports count: 0 and empty files when nameOnly is false.

    const files = args.nameOnly
      ? raw.split("\n").filter((l) => l.trim().length > 0)
      : [];
    

    When a consumer calls diff with stat: true or with neither nameOnly nor stat (full patch mode), the resource always reports count: 0 and files: [], while worktree_diff always populates them (per the recent fix in 3c7245ccd). A consumer checking count to detect "any changes?" would get a false negative from diff in non-nameOnly mode.

    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 with worktree_diff (which always populates files/count) could surprise a consumer who switches between the two methods.

Notes (not findings)

  • Security posture is strong. All user-facing ref/branch/remote inputs use safeRef refinements 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.
  • Test coverage is thorough. 90+ test cases cover happy paths, error paths, schema validation, flag injection rejection, signal threading, resource name sanitization, credential scrubbing, and edge cases like detached HEAD, sparse-fetch repos, and orphan branches.
  • Log parsing is correct. The NUL-delimited git log format (%H%x00%an%x00%aI%x00%s%x00) handles multi-commit output with inter-record newlines correctly via trim() on the SHA field.
  • Zod v4 usage is correct. .meta({ sensitive: true }) on the token field, .refine() for cross-field validation, and z.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.

## Adversarial Review ### Critical / High None. ### Medium None. ### Low 1. **`git/extensions/models/_lib/operations.ts:1703` — `worktree_diff` can produce duplicate file entries in a narrow edge case.** The `files` array is built by concatenating tracked changed files (`git diff --name-only <base>`) with untracked files (`git ls-files --others --exclude-standard`): ```typescript const allFiles = [...trackedFiles, ...untrackedFiles]; ``` If a file has been removed from the index via `git rm --cached` and then modified on disk, it appears in **both** lists: - `git diff --name-only HEAD` sees it because HEAD has the file and the on-disk content differs. - `git ls-files --others` sees it because it's no longer in the index. **Breaking example:** `git rm --cached foo.ts`, edit `foo.ts`, then call `worktree_diff` → `files` contains `foo.ts` twice, `count` is inflated by 1. **Why this is LOW:** The primary use case is CI change detection where `git rm --cached` mid-workflow is atypical. The `raw` diff output is unaffected — only the `files` array and `count` are wrong. A deduplicated set (`[...new Set(allFiles)]`) would fix it, but the scenario is uncommon enough that this is informational. 2. **`git/extensions/models/_lib/operations.ts:159-162` — `diff` method reports `count: 0` and empty `files` when `nameOnly` is false.** ```typescript const files = args.nameOnly ? raw.split("\n").filter((l) => l.trim().length > 0) : []; ``` When a consumer calls `diff` with `stat: true` or with neither `nameOnly` nor `stat` (full patch mode), the resource always reports `count: 0` and `files: []`, while `worktree_diff` always populates them (per the recent fix in `3c7245ccd`). A consumer checking `count` to detect "any changes?" would get a false negative from `diff` in non-`nameOnly` mode. **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 with `worktree_diff` (which always populates `files`/`count`) could surprise a consumer who switches between the two methods. ### Notes (not findings) - **Security posture is strong.** All user-facing ref/branch/remote inputs use `safeRef` refinements 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. - **Test coverage is thorough.** 90+ test cases cover happy paths, error paths, schema validation, flag injection rejection, signal threading, resource name sanitization, credential scrubbing, and edge cases like detached HEAD, sparse-fetch repos, and orphan branches. - **Log parsing is correct.** The NUL-delimited `git log` format (`%H%x00%an%x00%aI%x00%s%x00`) handles multi-commit output with inter-record newlines correctly via `trim()` on the SHA field. - **Zod v4 usage is correct.** `.meta({ sensitive: true })` on the token field, `.refine()` for cross-field validation, and `z.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.
stack72 deleted branch worktree-1708 2026-08-31 05:12:41 +00:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
swamp-club/swamp-extensions!245
No description provided.