boocode/docs/features/git-diff-panel/artifacts/implementation-decision-log.md

Implementation Decision Log: Git Diff Panel

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): 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), F2 (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). 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). 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 <base>".
• 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).
• 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).
• 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).
• 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).
• 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