Skip to content

feat: centralize workspace diff review - #203

Merged
oratis merged 32 commits into
mainfrom
codex/diff-review-protocol
Aug 2, 2026
Merged

feat: centralize workspace diff review#203
oratis merged 32 commits into
mainfrom
codex/diff-review-protocol

Conversation

@oratis

@oratis oratis commented Aug 1, 2026

Copy link
Copy Markdown
Owner

Summary

  • add capability-negotiated workspace/diff bound to a canonical thread cwd
  • return bounded file/hunk/line DTOs for staged, unstaged, renamed, deleted, and untracked changes
  • invoke Git without a shell, strip inherited GIT_* redirects, disable external diff/textconv, and cap files/bytes
  • refuse to read untracked symlink targets or binary content
  • make VS Code review consume the canonical DTO and expose the same method through Desktop and LSP
  • document the remaining inline feedback and per-finding apply/revert work separately

Validation

  • pnpm format:check
  • pnpm lint (one pre-existing warning)
  • pnpm typecheck
  • pnpm test (1010 passed, 12 skipped)
  • pnpm docs:check
  • pnpm build
  • pnpm --filter @deepcode/desktop test:e2e (4 passed)
  • sidecar and VS Code child CJS syntax checks
  • VSIX: 8 files, 175.57 KB

Adversarial coverage

  • paths containing spaces and rename metadata
  • tracked modification/deletion and untracked additions
  • untracked binary files
  • symlinks targeting files outside the repository
  • non-repository workspaces

t added 30 commits August 1, 2026 13:32
@oratis
oratis changed the base branch from codex/structured-tracing to main August 2, 2026 06:36
@oratis
oratis marked this pull request as ready for review August 2, 2026 06:45
@oratis

oratis commented Aug 2, 2026

Copy link
Copy Markdown
Owner Author

Review: approved ✅ — the Git invocation hardening is the notable part

Verified directly in apps/server/src/workspace-diff.ts:

  • execFile, not exec — no shell, so branch/path names containing shell metacharacters are arguments rather than code. Everything else here would be moot without that.
  • Inherited GIT_* redirects are stripped. This one is worth dwelling on: this repo has already been burned by leaked GIT_DIR — a test harness leaked it and re-inited the real repository as bare. A diff endpoint that inherits GIT_DIR/GIT_WORK_TREE from an ambient environment reads (or worse, operates on) the wrong repository entirely. Sanitizing the env is the correct fix, not a nicety.
  • GIT_OPTIONAL_LOCKS=0 so a read-only diff can't contend for the index lock against a concurrent user Git command.
  • GIT_PAGER=cat, LC_ALL=C — no pager hang, stable machine-readable output regardless of the user's locale.
  • External diff and textconv disabled. Both are repo-controlled config that execute arbitrary programs; leaving them enabled would let a hostile .gitattributes/.git/config run code during a "read-only" diff. Good catch.
  • 32 MiB maxBuffer plus file/byte caps, and untracked symlink targets and binary content are refused. Symlink refusal blocks the ln -s ~/.ssh/id_rsa → "untracked file" read primitive; without it this endpoint would be an arbitrary-file-read.

On the DTO: bounded file/hunk/line shape covering staged, unstaged, renamed, deleted, and untracked means VS Code, Desktop, and LSP consume one canonical result instead of three parsers of git diff text. Given how many edge cases the above list represents, three parsers would have meant three of these hardening steps missing.

Validation: CI green; apps/server tests pass locally at the stack tip.

@oratis
oratis merged commit 1f8f57a into main Aug 2, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant