# Implementation Decision Log: Git Diff Panel

<!--
This file records every implementation decision committed while planning the Git Diff Panel.
Behavioral and implementation statements live in [../feature-implementation-plan.md](../feature-implementation-plan.md) —
this file captures the question, rationale, evidence, and rejected alternatives for each decision.
Round-by-round history lives in [implementation-iteration-history.md](implementation-iteration-history.md).

Shared D-N counter across trivial and full sections. Decisions here are implementation HOW
decisions; the spec WHAT decisions (D1–D18) live in [decision-log.md](decision-log.md) and are
inherited, not restated.
-->

## Trivial decisions

- D-9: No `@boocode/contracts` change — refresh is a client-side `sessionEvents` event, not a WS frame, so the contracts package (`packages/contracts/`) is not touched and not rebuilt. — Referenced in plan: Implementation Approach → External Interfaces, Operational Readiness.
- D-10: No DB schema change, no migration, no new env var — the panel reads git state at request time and writes the project repo directly; nothing is persisted in Postgres. — Referenced in plan: Implementation Approach → Data Model and Persistence, Operational Readiness.
- D-11: Per-file expand/collapse state is local React state inside `GitDiffView`, not a shared hook — one consumer exists. — Referenced in plan: Implementation Approach → Architecture and Integration Points, Deferred (YAGNI).
- D-12: `autoSelectMode` and `canCommit` are inline pure helpers inside `git_diff.ts` (unit-tested), not separate modules. — Referenced in plan: Implementation Approach → Runtime Behavior, Testing Strategy.

## Full decisions

### D-1: Both git read AND git write live in apps/server

- **Question:** Which service owns the new git read and git write operations — apps/server (BooChat, Docker), apps/coder (BooCoder, host service), or a read-in-server / write-in-coder split?
- **Decision:** Both the read route and the write routes (stage / unstage / commit / discard) live in **apps/server**. New `apps/server/src/services/git_diff.ts` holds the read logic and pure helpers plus the git-write helpers; new routes are added beside `GET /api/projects/:id/git` in `apps/server/src/routes/projects.ts`. No apps/coder changes. Single deploy surface: `docker compose up --build -d boocode`.
- **Rationale:** Writing in apps/server reuses the existing safe argv `runGit` pattern and the proven `project_bootstrap.ts` git-write precedent, and avoids a cross-service `/api/coder/*` proxy hop per write, a second deploy surface (coder restart), and making the host coder service a dependency for a plain BooChat-session commit. The "read-only" posture that motivated the split governs the **assistant's tool surface**, not the container filesystem or the user's own UI actions.
- **Evidence:**
  - `docker-compose.yml:16` mounts `/opt:/opt` **read-write** into the boocode container — apps/server can already write project paths (refutes the "apps/server is read-only by posture" premise).
  - `apps/server/src/services/project_bootstrap.ts` already runs git writes from apps/server: `git init` + commits via safe `execFile` with `-c user.name` / `-c user.email` (`GIT_USER_NAME='indifferentketchup'`, `GIT_USER_EMAIL='samkintop@gmail.com'` at :38-39, applied :122-123) — git-write from the server is an existing, proven pattern.
  - `apps/server/src/services/git_meta.ts:30` has the safe `runGit` (discrete-argv `execFile`, no shell); `apps/coder/src/services/worktrees.ts` has only the **unsafe** `hostExec(shell string)` — writing in coder would require building a new safe wrapper there.
  - Spec D8 ([decision-log.md](decision-log.md#d8--git-write-is-a-user-action-not-an-assistant-tool)): the read-only invariant is defined over the assistant's tool surface; the panel's actions are the human user's own UI actions.
- **Rejected alternatives:**
  - Read-in-server / write-in-coder split (the Round-1 software-architect A1 recommendation) — rejected because its premise (apps/server is read-only and a write there is a new surface) is refuted by the `/opt` rw mount and the `project_bootstrap.ts` git-write precedent; the split adds a cross-service hop, a second deploy surface, and coder coupling for a BooChat-session commit.
  - All-in-coder — rejected because it requires building a new safe argv git wrapper in coder (which only has the unsafe `hostExec`) and adds the same coder coupling and second deploy surface.
- **Specialist owner:** `software-architect`
- **Revisit criterion:** If a third consumer (beyond this feature in apps/server) needs the same git ops, reopen the shared-`packages/` extraction (see Deferred YAGNI); if apps/server's container ever loses the `/opt` rw mount, reopen the service-owner question.
- **Dissent (if any):** The Round-1 software-architect (A1) recommended the write-in-coder split; recorded under disagree-and-commit. Revisit if the refuting evidence (rw mount + bootstrap precedent) changes.
- **Driven by rounds:** R1
- **Dependent decisions:** D-2, D-3, D-4, D-5, D-8
- **Referenced in plan:** Implementation Approach → Architecture and Integration Points, Decomposition and Sequencing, Operational Readiness, Summary

### D-2: Read route + `git_diff.ts` pure helpers, TDD-first

- **Question:** What is the read-side shape, and how is the diff-parse / mode / base logic structured and tested?
- **Decision:** Add `GET /api/projects/:id/git/diff?mode=<uncommitted|committed>` beside `GET /api/projects/:id/git`. The route delegates to `apps/server/src/services/git_diff.ts`, which exposes pure helpers written test-first before wiring: `parseNameStatus` (porcelain → file list with change type), `splitDiffByFile` (unified diff text → per-file segments), `resolveCommittedBase` (base resolution, see D-6), `autoSelectMode`, the binary/large `classify`, and `detectInProgress` (in-progress git-state detection, see D-7). The route composes these over `runGit` argv calls.
- **Rationale:** The pure-helper-then-wire pattern is the project's established TDD precedent and keeps the parse/mode/base/classify logic unit-testable without spawning git. Slotting beside the existing git-meta route reuses its derivation and request shape.
- **Evidence:** `apps/server/src/services/git_meta.ts:44` `getGitMeta` + `apps/server/src/routes/projects.ts:426` `GET /api/projects/:id/git` (the read precedent the new route slots beside). Pure-helper TDD precedent: `apps/server/src/services/backends/turn-guard.ts`, `lifecycle-decisions.ts`, `mistake-tracker.ts` (pure module + unit test, then wire). test-engineer (T1–T12).
- **Rejected alternatives:**
  - Inline all parse/mode/base logic in the route handler — rejected because it cannot be unit-tested without spawning git and breaks the project's pure-helper precedent.
  - A separate module per helper — rejected (see D-12); the helpers are small and cohesive in one tested module.
- **Specialist owner:** `software-architect`
- **Revisit criterion:** If `git_diff.ts` grows past a single cohesive module (e.g. distinct read vs write concerns each exceed a file), split along that seam.
- **Driven by rounds:** R1
- **Dependent decisions:** D-6, D-7
- **Referenced in plan:** Implementation Approach → Architecture and Integration Points, Implementation Approach → Runtime Behavior, Decomposition and Sequencing, Testing Strategy

### D-3: Write ops via argv-safe `runGit`/`execFile` with `--` separators

- **Question:** How are the write operations (stage / unstage / commit / discard) invoked safely?
- **Decision:** All write ops use the discrete-argv `runGit`/`execFile` pattern with explicit `--` separators between options and user-supplied paths, plus a flag-injection guard (reject path arguments beginning with `-`). They **never** use `hostExec(shell string)` or any shell-interpolated invocation. User-supplied text (commit message, file targets) is always passed as discrete argv, never built into a command string.
- **Rationale:** Discrete argv with `--` separators closes the command-injection and flag-injection vectors that a shell string opens (F2). The server already has the safe pattern; the coder's shell pattern is exactly what the spec security posture warns against.
- **Evidence:** `apps/server/src/services/git_meta.ts:30` `runGit` (safe discrete-argv `execFile`) is the bar. `apps/coder/src/services/worktrees.ts` `hostExec(shell)` + `shellEscape` is the anti-pattern F2 flags. Spec D11/D12 ([decision-log.md](decision-log.md#d11--all-git-operations-scoped-to-the-project-repository-path)), F2 ([team-findings.md](team-findings.md)).
- **Rejected alternatives:**
  - Reuse the coder `hostExec(shell)` + `shellEscape` pattern — rejected because shell interpolation lets user-supplied content (commit message, filenames with special characters) be interpreted as flags or shell syntax; the argv pattern eliminates the class.
- **Specialist owner:** `adversarial-security-analyst`
- **Revisit criterion:** If a git operation genuinely cannot be expressed in discrete argv (none in the v1 set qualifies), reopen with a documented justification.
- **Driven by rounds:** R1
- **Dependent decisions:** D-4
- **Referenced in plan:** Implementation Approach → Architecture and Integration Points, Security Posture, Decomposition and Sequencing

### D-4: Path validation — repo-relative `pathGuard`, reject escape and repo-root discard

- **Question:** How are per-file path arguments validated before a write?
- **Decision:** Every per-file argument is validated by a `pathGuard(repoRoot, file)` that resolves the path and rejects it if it is absolute, contains `..` traversal, or resolves (including via symlink) outside the repository root. Additionally, discarding the repository root itself (`.`) is rejected. The repository root is derived server-side from the session's project record, never from the request.
- **Rationale:** Per-file arguments can escape the repo root via `../` traversal or a symlink even when the root is correct, so each argument needs independent validation (F2). Rejecting a repo-root discard prevents a single confirmation from wiping the whole working tree.
- **Evidence:** `apps/server/src/services/path_guard.ts` `resolveProjectRoot(project.path)` — derives + scopes project paths from the DB project row, never from the request (the F2 precedent). Spec D11/D12, F2 ([team-findings.md](team-findings.md)). Integration-test precedent: `path_guard.test.ts`.
- **Rejected alternatives:**
  - Validate only the repository root, not per-file arguments — rejected (F2): a per-file `../` or symlink argument escapes a correctly-scoped root.
  - Accept a caller-supplied repository path — rejected: needless write surface, no use case; root is server-derived.
- **Specialist owner:** `adversarial-security-analyst`
- **Revisit criterion:** If a legitimate write target outside the resolved root ever appears (none in scope), reopen.
- **Driven by rounds:** R1
- **Dependent decisions:** —
- **Referenced in plan:** Security Posture, Decomposition and Sequencing, Testing Strategy

### D-5: Commit identity server-derived; request schema `.strict()` with no author fields

- **Question:** Where does the commit author/committer identity come from, and what can the request set?
- **Decision:** Commit identity is server-derived via `-c user.name=… -c user.email=…` read from the repository's git config at commit time, falling back to the `project_bootstrap.ts` constants (`indifferentketchup` / `samkintop@gmail.com`) when git config yields nothing. The commit request schema is Zod `.strict()` and carries `{message, files?}` only — no `author`, `email`, `date`, or any identity field; unknown keys are rejected.
- **Rationale:** A server-derived identity passed via `-c` flags makes authorship un-spoofable from the request body (F3). `.strict()` ensures an attacker cannot smuggle an identity field. The `project_bootstrap.ts` `-c` precedent already establishes this exact mechanism for server-side commits.
- **Evidence:** `apps/server/src/services/project_bootstrap.ts:38-39,122-123` (server-side commit with `-c user.name`/`-c user.email`). Spec D6/D12, F3 ([team-findings.md](team-findings.md)). Zod is the project's request-validation library (apps/server CLAUDE.md).
- **Rejected alternatives:**
  - Request-supplied commit identity — rejected (F3): allows authorship spoofing; no legitimate use in a single-user app.
  - Hardcode the coder's `boocoder@local` identity — rejected: a user commit is not a coder commit; git config is the correct source for the human user, with the bootstrap constants as the fallback.
  - A non-strict request schema — rejected: an unknown `author`-style key could slip through and influence the commit.
- **Specialist owner:** `adversarial-security-analyst`
- **Revisit criterion:** If a multi-identity requirement appears (e.g. co-author selection), reopen with a server-validated identity source, never a free-form request field.
- **Driven by rounds:** R1
- **Dependent decisions:** —
- **Referenced in plan:** Implementation Approach → Architecture and Integration Points, Security Posture, Decomposition and Sequencing, Testing Strategy

### D-6: Committed-mode base resolution via `@{upstream}` → `origin/HEAD` → null

- **Question:** How does Committed mode resolve the base it compares the current branch against?
- **Decision:** The pure helper `resolveCommittedBase` resolves the base in order: (1) `git rev-parse --abbrev-ref --symbolic-full-name @{upstream}` (the tracking branch; non-zero exit if none), then (2) `git rev-parse --abbrev-ref origin/HEAD` (the default branch), then (3) null. On null the panel falls back to Uncommitted and labels it as a fallback. The resolved base is returned in the read response so the header can label "Git — branch vs &lt;base&gt;".
- **Rationale:** Tracking-branch-first matches git's own upstream model and is the right base for a contributor whose tracking branch is a fork or PR target; `origin/HEAD` is the right fallback for the default branch; a labeled Uncommitted fallback is more useful than an error or a silent swap (F11).
- **Evidence:** `apps/server/src/services/git_meta.ts:58` already uses `HEAD...@{upstream}` (the upstream-resolution precedent). Discovery notes §6 (`rev-parse --abbrev-ref origin/HEAD` for the default-branch fallback). Spec D13, F11 ([team-findings.md](team-findings.md)).
- **Rejected alternatives:**
  - Always compare against the default branch, ignoring the tracking branch — rejected (F11): wrong for contributors tracking a fork or PR target.
  - Error when no base resolves — rejected (D13): leaves the panel useless; a labeled Uncommitted fallback is more helpful.
  - `git symbolic-ref refs/remotes/origin/HEAD` — equivalent but `rev-parse --abbrev-ref origin/HEAD` is the simpler form already aligned with the git_meta precedent.
- **Specialist owner:** `software-architect`
- **Revisit criterion:** If repos without `origin/HEAD` set become common and the Uncommitted fallback is observed to confuse users, reopen the default-branch heuristic.
- **Driven by rounds:** R1
- **Dependent decisions:** —
- **Referenced in plan:** Implementation Approach → Runtime Behavior, Decomposition and Sequencing, Testing Strategy

### D-7: Read deadline 30s + maxBuffer 10MB; index-lock → HTTP 409; in-progress detection disables writes

- **Question:** What resilience bounds and failure semantics does the git surface commit to?
- **Decision:** (a) The read path uses a 30s execution deadline and a 10MB `maxBuffer` — both distinct from the D5 per-file display-size cap; a read that exceeds the deadline exits loading, shows an error, and offers Refresh. (b) A write that fails because the index is locked returns **HTTP 409 "repository busy"** with no server-side retry (retry is user-driven). (c) `detectInProgress` reads `.git` sentinel stats (`MERGE_HEAD`, `rebase-merge`/`rebase-apply`, `CHERRY_PICK_HEAD`, `BISECT_LOG`) and folds an in-progress flag into the read response; the client disables write affordances when set. (d) Untracked-file discard reports honest partial failure on refresh rather than claiming an unenforceable "state unchanged."
- **Rationale:** A slow git process can stall the panel even with small files, so a deadline distinct from the size cap is needed (F7). A server retry on a lock hides the busy state and adds timer state; a 409 lets the user retry (F5). Detecting in-progress states up front prevents raw git errors on stage/commit/discard (F9). Partial-failure honesty matches what the git layer can actually guarantee (F6).
- **Evidence:** `git_meta.ts:6-10` (the existing 2s timeout + 1MB buffer pattern this scales up for diff payloads). Spec D5/F7 (deadline distinct from size cap), F5 (index-lock), F9 (in-progress states), F6 (partial-failure wording) ([team-findings.md](team-findings.md)).
- **Rejected alternatives:**
  - Server-side retry on index-lock — rejected (F5, YAGNI): "try again" is user-driven; a server retry hides the busy state and adds timer state. Reopen if lock contention is observed frequent.
  - Combine the deadline and the display-size cap into one mechanism — rejected (F7/D5): they address different failure modes (slow process vs large output).
  - Streaming diff reader that caps mid-stream — rejected (YAGNI): `execFile` maxBuffer 10MB covers any realistic single-user working-tree diff.
- **Specialist owner:** `on-call-engineer`
- **Revisit criterion:** Reopen the buffer/deadline if working-tree diffs routinely exceed 10MB or reads routinely exceed 30s; reopen the retry decision if index-lock contention is observed frequent in practice.
- **Driven by rounds:** R1
- **Dependent decisions:** —
- **Referenced in plan:** Implementation Approach → Runtime Behavior, On-Call Resilience Posture, Decomposition and Sequencing, Testing Strategy

### D-8: Refresh via client `git_diff_refresh` sessionEvent, coalesced — not a WS frame

- **Question:** How is the panel's refresh wired across its five triggers, and does it need a new WS frame?
- **Decision:** Refresh is a client-side `sessionEvents` event named `git_diff_refresh`, **not** a new WS frame. All five triggers are client-side: tab open, post-mutation, message_complete (from the existing WS frame), pending-change apply/discard, and the explicit Refresh control. The event is emitted from the `message_complete` handler and the CoderPane apply/discard callbacks; `useGitDiff` subscribes. A no-op `case 'git_diff_refresh'` is added to `useSidebar.ts` `applyEvent` per the apps/web parity rule. The client holds an in-flight coalescence ref so a refresh already running absorbs later triggers (no second concurrent read, no debounce).
- **Rationale:** No trigger requires a server push — `message_complete` already arrives as a WS frame and the rest are local UI events — so a new WS frame and a `@boocode/contracts` rebuild are unnecessary. A client coalescence ref settles the panel to a single final snapshot (F8).
- **Evidence:** Discovery notes §"Refresh-trigger plumbing": `message_complete` WS frame = turn-complete trigger; `usePendingChanges` in `CoderPane.tsx:~786` refetches on message-complete; new sessionEvent → a `case` in `useSidebar.ts` `applyEvent` (apps/web parity rule). Spec D10/F8 (coalescence), F20 (parity steps) ([team-findings.md](team-findings.md)).
- **Rejected alternatives:**
  - A new WS frame for refresh — rejected (YAGNI): replaced by the simpler client `sessionEvents` event; no server push is needed, avoiding a `@boocode/contracts` rebuild and the dual server/web frame-type update.
  - Per-trigger debounce instead of an in-flight coalescence ref — rejected (F8): a debounce delays the first read and can still spawn overlapping reads under bursts.
- **Specialist owner:** `software-architect`
- **Revisit criterion:** If a future trigger genuinely originates server-side (e.g. an out-of-band repo mutation the client cannot observe), reopen the WS-frame option.
- **Driven by rounds:** R1
- **Dependent decisions:** —
- **Referenced in plan:** Implementation Approach → Runtime Behavior, Implementation Approach → External Interfaces, Decomposition and Sequencing, On-Call Resilience Posture

### D-13: Write endpoints excluded from the assistant tool registry; artifact sandbox closes the indirect path

- **Question:** What prevents the assistant (directly or via a rendered artifact) from reaching the git-write endpoints?
- **Decision:** The git-write endpoints are HTTP routes only and are **never** registered in the assistant tool registry (`ALL_TOOLS` in `services/tools.ts`). The indirect path — an AI-emitted HTML artifact POSTing to the endpoints with the user's cookie — is already closed by the artifact iframe's `connect-src 'none'` sandbox. No new mitigation is built for the artifact path; the plan records the existing control as the evidence.
- **Rationale:** The panel's writes are the user's own UI actions (spec D8); keeping them out of `ALL_TOOLS` means no assistant tool surface exists for them, and the existing artifact sandbox closes the only indirect path (F1).
- **Evidence:** `apps/server/src/services/tools.ts` `ALL_TOOLS` (tool registry the endpoints are kept out of). Artifact iframe `connect-src 'none'` per BOOCHAT.md output-format section (the F1 evidence). Spec D8/D12, F1 ([team-findings.md](team-findings.md)).
- **Rejected alternatives:**
  - Session-type gating (restrict the panel to write-capable sessions) — rejected (F1/D8): session type is the wrong layer; the artifact sandbox closes the actual indirect path, and the user's UI actions should not be gated by the assistant's permissions.
  - A custom CSRF header on the write routes — rejected (YAGNI): `connect-src 'none'` on artifacts + the SameSite=Lax Authelia cookie + a same-origin SPA cover it at single-user scale. Reopen if the routes are exposed to a less-controlled origin.
- **Specialist owner:** `adversarial-security-analyst`
- **Revisit criterion:** If the write routes are ever exposed to an origin outside the same-origin SPA, reopen the CSRF-header decision.
- **Driven by rounds:** R1
- **Dependent decisions:** —
- **Referenced in plan:** Security Posture, Decomposition and Sequencing

### D-14: Two-phase build — read/display (Phase 1) then write actions (Phase 2)

- **Question:** How is v1 sequenced, given the write surface is the larger half?
- **Decision:** Two phases on the same deploy surface. **Phase 1 (read + display):** `git_diff.ts` pure helpers TDD-first → read route → client `useGitDiff` + RightRail Files/Git tab + `GitDiffView` read-only + the `git_diff_refresh` refresh wiring. **Phase 2 (write actions):** write helpers + write routes + stage/unstage/commit/discard affordances + in-progress disable. Both phases ship via `docker compose up --build -d boocode` (no coder restart).
- **Rationale:** The write surface is the larger, higher-risk half (F19); landing read/display first delivers reviewable value and de-risks the write phase. Because both read and write live in apps/server (D-1), the two phases share one deploy surface, so the split is sequencing, not two deploy targets.
- **Evidence:** Spec F19 (sequence diff-display before write actions within v1). D-1 (both in apps/server → single surface). junior-developer (coupling flag corrected to single-surface).
- **Rejected alternatives:**
  - Ship read and write together as one slice — rejected (F19): the write surface is the larger half and benefits from landing display first for review.
  - Phase the split across deploy surfaces — rejected (D-1): both halves are in apps/server, so there is one surface.
- **Specialist owner:** `software-architect`
- **Revisit criterion:** If Phase 1 review surfaces a read-path change that blocks the write design, fold the affected piece forward; otherwise the order holds.
- **Driven by rounds:** R1
- **Dependent decisions:** —
- **Referenced in plan:** Decomposition and Sequencing, Definition of Done

### D-15: Frontend — RightRail Files/Git tab, `GitDiffView` with lazy Shiki, dirty dot from `useProjectGit`

- **Question:** How does the panel fit into the existing right-rail file browser, and how are diffs rendered?
- **Decision:** The Files/Git tab strip is added to the existing `RightRail.tsx` header (replacing the static "Files" label), fitting one line (D18); the FilePlus button shows only on the Files tab. `GitDiffView` occupies the same slot as the file tree and renders unified diffs via Shiki `lang:'diff'`, highlighting a file's diff lazily only when expanded (per-file loading state; "expand all" lazily highlights per file as rendered). The dirty dot on the Git tab button and the `Session.tsx` FolderTree toggle is fed by `useProjectGit`'s existing `is_dirty` (no new fetch). D14's "briefly notes the change" is a non-blocking inline line inside the panel, not a toast (avoids mobile-drawer z-index collision).
- **Rationale:** The right rail is the file-browser host the spec scopes the panel to (D1); reusing its header and the existing `useProjectGit` dirty signal avoids new plumbing and a new fetch (D17). Lazy Shiki avoids highlighting unopened diffs (D9, perf). An inline note avoids the toast/drawer z-index collision on mobile.
- **Evidence:** `apps/web/src/components/RightRail.tsx` (~:209 header with static "Files" label + 2 icon buttons; `max-md:min-h-[44px]` already applied — the D18 44px convention). `Session.tsx:~397` mobile FolderTree toggle (D17 dirty host). `useProjectGit` polls `GET /api/projects/:id/git` 30s and already returns `is_dirty` (D17 reuse). Shiki `^1.29.2` already in apps/web (`CodeBlock.tsx`, `FileViewerOverlay.tsx`); `lang:'diff'` valid (discovery notes). Spec D1, D9, D16, D17, D18.
- **Rejected alternatives:**
  - Eager-highlight all diffs on load — rejected (D9, perf): highlights diffs the user never expands.
  - A new fetch for the dirty indicator — rejected (D17): `useProjectGit` already produces `is_dirty`.
  - A toast for D14's mode-change note — rejected: collides with the mobile-drawer z-index; an inline panel line is non-blocking and in-context.
  - A separate per-file expand-state hook — rejected (see D-11): one consumer; local state suffices.
- **Specialist owner:** `user-experience-designer`
- **Revisit criterion:** If a second component needs the diff-render or expand state, reopen the shared-hook extraction.
- **Driven by rounds:** R1
- **Dependent decisions:** —
- **Referenced in plan:** Implementation Approach → Architecture and Integration Points, Decomposition and Sequencing, Testing Strategy