Compare commits

...

5 Commits

Author SHA1 Message Date
d10d79399b fix(docker): trust bind-mounted repos via git safe.directory
The container runs as root over uid-1000-owned host repos; git's dubious-
ownership guard made every project read as not-a-repo, hiding the git diff
panel's Git tab and nulling the branch indicator. Bakes safe.directory='*'
into the runtime image. Applied live to the running container too.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-06-03 08:29:33 +00:00
aeb2777ad4 docs: changelog for v2.7.14-backlog-hardening + v2.7.15-git-diff-panel
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-06-03 03:42:40 +00:00
2c58f2b3d3 Merge epic-backlog-and-gitdiff: v2.7.14 backlog hardening + v2.7.15 git diff panel
Two plans delivered via paseo-epic in an isolated worktree, audited green:
- v2.7.14: post-review backlog (external task-cancel + finalization, tool-call
  parser prune + pino logging, BooChat stall-timeout, view_session_history MCP
  tool, retire the :9502 fallback SPA).
- v2.7.15: git diff panel (Files/Git tab in the file browser with stage/commit/
  discard, server-side argv-safe git, sessionEvent-driven refresh).
2026-06-03 03:41:12 +00:00
ca028a4024 docs: add git-diff-panel implementation planning artifacts
Implementation decision log, iteration history, synthesis input, the
implementation plan, and discovery notes for the git-diff-panel feature.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-06-03 02:26:04 +00:00
3e7115afad docs: record @boocode/contracts SSOT + schema-migration learnings in CLAUDE.md
Add the packages/contracts package to the monorepo list, a consolidated
@boocode/contracts conventions bullet (subpaths, build-first, web-local strict
WsFrame union, built-dist consumption), and the `SELECT *` view / DROP COLUMN
(2BP01) schema gotcha that crash-looped boocoder.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-06-03 02:25:59 +00:00
8 changed files with 683 additions and 8 deletions

View File

@@ -2,6 +2,18 @@
All notable changes per release tag. Most recent on top, ordered by tag creation date (which matches the git history). Tag names follow `vMAJOR.MINOR.PATCH-slug` — the slug describes what shipped, so the tag name alone is enough to recall the batch. All notable changes per release tag. Most recent on top, ordered by tag creation date (which matches the git history). Tag names follow `vMAJOR.MINOR.PATCH-slug` — the slug describes what shipped, so the tag name alone is enough to recall the batch.
## v2.7.16-container-git-safedir — 2026-06-03
Hotfix that makes the `v2.7.15-git-diff-panel` work in production. The `boocode` container runs as root but bind-mounts host project repos owned by uid 1000, so git rejected them with "detected dubious ownership" and the diff route reported every project as not-a-repo — which hid the Git tab entirely (and had been silently nulling the existing branch indicator too). Adds `git config --system --add safe.directory '*'` to the Dockerfile runtime stage so the container's git trusts the mounted repos; applied live to the running container and baked into the image for future rebuilds. Surfaced by a live smoke immediately after the v2.7.14/v2.7.15 deploy.
## v2.7.15-git-diff-panel — 2026-06-03
A Files / Git tab in the right-side file panel (the file-browser sidebar) that shows the project repository's git diff and lets the user stage, unstage, commit, and discard whole files in-session — modeled on Paseo's diff view, scoped and planned through the `plan-a-feature``plan-implementation` skills, then built and audited via `paseo-epic` in an isolated worktree. Two comparison modes (Uncommitted vs HEAD, and the current branch vs its base — the upstream tracking branch else `origin/HEAD`), auto-selected by repo dirty-state on first open and pinned after an explicit choice; per-file expand/collapse with lazy Shiki `lang:'diff'` highlighting, +/- stats, and binary/too-large placeholders. All git read and write logic lives in `apps/server` (new `git_diff.ts` + routes on `projects.ts`) — the read-only-server posture governs the assistant's tools, not the user's own actions, and the container already mounts `/opt` read-write while `project_bootstrap` already commits via `execFile`. Every write uses the safe `execFile` argv pattern (never a shell string) with `--` operand separators, per-file `pathGuard` + realpath symlink-escape validation, server-derived `-c` commit identity (the request body is `.strict()` and carries no author fields), and the write endpoints are deliberately absent from the assistant tool registry. Reads are bounded (30s deadline, 10MB); an index lock or an in-progress merge/rebase/cherry-pick/bisect surfaces as "repository busy" and disables writes. The panel stays current via a client `git_diff_refresh` sessionEvent (no new wire contract) coalesced across tab-open, mutations, turn completion, and pending-change apply; discard is an irrecoverable hard-delete behind a plain confirm distinguishing a tracked revert from an untracked delete. New `git_diff` pure-helper + temp-repo integration tests (59 cases); server 630 tests green, web tsc clean. Pairs with `v2.7.14-backlog-hardening` (shipped together).
## v2.7.14-backlog-hardening — 2026-06-03
Five independent items from the second external-code-review backlog (`boocode_code_review_v2.md`), each built and audited as its own phase via `paseo-epic`. **External task-cancel** now actually works: Stop on an opencode/goose/qwen/claude task aborts the running child via a per-task `AbortController` registry reachable from the cancel route and finalizes the assistant message as `cancelled` — fixing two latent bugs (catch blocks left the message `streaming`; warm success-paths wrote `complete` on an aborted turn); warm pools/worktrees are preserved (abort the prompt only, never the pooled process) and the native boocode path is unchanged. **Parser prune**: the tool-call parser drops to its two load-bearing exports (eight zero-caller symbols unexported, a gate test added for the `<invoke>`-as-text fallback) with no live-path behavior change, and placeholder-rejection logging moves to pino. **BooChat stall-timeout**: a 90s per-chunk deadline wraps native inference's `fullStream` via `AbortSignal.any` so a hung local stream finalizes the message instead of hanging — no retry, since re-running re-emits already-streamed deltas (a pure `classifyStreamError` helper is added). **view_session_history**: a read-only MCP tool returning the newest-N transcript (role≠system) in chronological order. **Retire :9502**: the unused `apps/coder/web` fallback SPA is removed (package, static-serve block, build step, Dockerfile copy, `@fastify/static`), keeping every API/WS/health/MCP route. F1 added an optional `status` field to the shared `message_complete` contracts frame (so a deploy rebuilds `@boocode/contracts` first, as the sequence already does). Server 630 / coder 360 tests green.
## v2.7.13-contracts-ssot — 2026-06-02 ## v2.7.13-contracts-ssot — 2026-06-02
Creates `@boocode/contracts` (`packages/contracts`), a new workspace package that becomes the single source of truth for every cross-app wire contract — reversing the decision recorded in `v2.5.12-provider-lifecycle-phase4` that declined a shared types package as not worth the Docker/build-order risk at solo scale; a live `AgentSessionConfig` drift that had since appeared between `apps/coder` and `apps/web` justified the investment. Six contracts are now defined exactly once: the `WsFrameSchema` Zod runtime schema, the provider snapshot types (`ProviderSnapshotEntry` and family), the Zod provider-config schemas, `MessageMetadata` + `ErrorReason`, `AgentSessionConfig`, and `WorktreeRiskReport`; both Zod-backed contracts use `z.infer` so validator and type derive from the same definition and cannot drift independently. All four consumers — `apps/server`, `apps/web`, `apps/coder`, and the fallback SPA `apps/coder/web` — import via `workspace:*` through a per-subpath exports map consuming built dist only (no tsconfig project references); the hand-synced copies and their parity tests (`provider-types-parity.test.ts`; the ws-frames byte-parity assertion) are deleted while the KNOWN_FRAME_TYPES drift test and broker fail-closed tests are preserved. Build order is inverted in the root build script, Dockerfile, and coder deploy docs; `apps/coder/web`'s migration also removed dead `pending_change_*` reducer arms (no frame publisher exists for these — pending changes are HTTP-delivered), closing a latent missing-default-arm crash, and reconciled field-type conflicts with the canonical `WsFrame`; zod is pinned to a single version across the workspace. Server 543 / coder 293 / contracts 11 tests passing; human smoke verified on the live stack 2026-06-02. Creates `@boocode/contracts` (`packages/contracts`), a new workspace package that becomes the single source of truth for every cross-app wire contract — reversing the decision recorded in `v2.5.12-provider-lifecycle-phase4` that declined a shared types package as not worth the Docker/build-order risk at solo scale; a live `AgentSessionConfig` drift that had since appeared between `apps/coder` and `apps/web` justified the investment. Six contracts are now defined exactly once: the `WsFrameSchema` Zod runtime schema, the provider snapshot types (`ProviderSnapshotEntry` and family), the Zod provider-config schemas, `MessageMetadata` + `ErrorReason`, `AgentSessionConfig`, and `WorktreeRiskReport`; both Zod-backed contracts use `z.infer` so validator and type derive from the same definition and cannot drift independently. All four consumers — `apps/server`, `apps/web`, `apps/coder`, and the fallback SPA `apps/coder/web` — import via `workspace:*` through a per-subpath exports map consuming built dist only (no tsconfig project references); the hand-synced copies and their parity tests (`provider-types-parity.test.ts`; the ws-frames byte-parity assertion) are deleted while the KNOWN_FRAME_TYPES drift test and broker fail-closed tests are preserved. Build order is inverted in the root build script, Dockerfile, and coder deploy docs; `apps/coder/web`'s migration also removed dead `pending_change_*` reducer arms (no frame publisher exists for these — pending changes are HTTP-delivered), closing a latent missing-default-arm crash, and reconciled field-type conflicts with the canonical `WsFrame`; zod is pinned to a single version across the workspace. Server 543 / coder 293 / contracts 11 tests passing; human smoke verified on the live stack 2026-06-02.

View File

@@ -23,13 +23,12 @@ pnpm -C apps/server build # server only (tsc + copy schema.sql)
pnpm -C apps/web build # web only (vite) pnpm -C apps/web build # web only (vite)
# Type checking (no emit) # Type checking (no emit)
npx tsc --noEmit # project references (root) # Per-app is authoritative. There is NO root tsconfig.json (only tsconfig.base.json),
npx tsc -p apps/web/tsconfig.app.json --noEmit # web app specifically # so a bare `npx tsc --noEmit` at root compiles nothing.
npx tsc -p apps/web/tsconfig.app.json --noEmit # web (authoritative)
# IMPORTANT: root tsc --noEmit uses project references and can miss errors pnpm -C apps/server build # server typecheck (tsc + copy schema)
# that the per-app tsconfig catches. Always verify with the per-app command pnpm -C apps/coder build # coder typecheck
# when editing web code. The server build (pnpm -C apps/server build) is pnpm -C apps/booterm typecheck # booterm typecheck
# authoritative for server code.
# Production # Production
docker compose build --no-cache boocode && docker compose up -d docker compose build --no-cache boocode && docker compose up -d
@@ -39,7 +38,7 @@ Tests: `pnpm -C apps/server test` (vitest); `apps/coder` has its own suite — `
## Architecture ## Architecture
**Monorepo**: pnpm workspaces with `apps/server` (Fastify + postgres), `apps/web` (React + Vite), `apps/booterm` (Fastify + node-pty + tmux), `apps/coder` (BooCoder, host service). **Monorepo**: pnpm workspaces with `apps/server` (Fastify + postgres), `apps/web` (React + Vite), `apps/booterm` (Fastify + node-pty + tmux), `apps/coder` (BooCoder, host service), `packages/contracts` (`@boocode/contracts`, cross-app wire-contract SSOT — builds FIRST).
### Per-app deep references ### Per-app deep references
@@ -69,6 +68,8 @@ Schema CHECK migration order when renaming allowed values: (1) `ALTER TABLE ...
**`CREATE OR REPLACE VIEW` can't reorder/rename columns** (Postgres `42P16`): append a new `messages_with_parts` column at the END of the SELECT — a mid-list insert shifts an existing column → crash-loops boot. Add it to each explicit read SELECT too (`routes/messages.ts`/`chats.ts`/`ws.ts`). **`CREATE OR REPLACE VIEW` can't reorder/rename columns** (Postgres `42P16`): append a new `messages_with_parts` column at the END of the SELECT — a mid-list insert shifts an existing column → crash-loops boot. Add it to each explicit read SELECT too (`routes/messages.ts`/`chats.ts`/`ws.ts`).
**A `SELECT *` view pins every column** (`2BP01`): `DROP COLUMN` on the table fails while such a view exists. `human_inbox` is `SELECT * FROM tasks` — to drop a `tasks` column, `DROP VIEW IF EXISTS human_inbox` first, drop the column(s), then recreate the view (idempotent). Bites existing DBs only; a fresh DB never had the column, so fresh-DB testing misses it.
## Environment ## Environment
Required: `DATABASE_URL`, `LLAMA_SWAP_URL`. Optional: `PORT` (3000), `HOST` (0.0.0.0), `PROJECT_ROOT_WHITELIST` (/opt, read-only add-existing scope), `BOOTSTRAP_ROOT` (/opt/projects, writable bootstrap mkdir target — host must `mkdir -p` it before container start), `DEFAULT_MODEL`, `LOG_LEVEL`, `SEARXNG_URL` (default `http://100.114.205.53:8888` — internal Tailscale; the public host is behind Authelia, unusable from server context), `BOOCODE_TOOLS` (`core`|`standard`|`all`, default `all`; a ceiling, never expands an agent's whitelist), `MCP_CONFIG_PATH` (default `/data/mcp.json`, opencode `mcpServers` shape; missing = no MCP), `CONTEXT7_API_KEY` (the Context7 MCP key, referenced from `data/mcp.json` as `"{env:CONTEXT7_API_KEY}"`). `data/mcp.json` is **gitignored** but no longer holds secrets — string values support opencode-style `{env:VAR}` substitution (`mcp-config.ts:substituteEnvVars`, applied before Zod validation; unset var → `''` + warn), so real keys live in `.env`; template `data/mcp.example.json`. A config-only edit there needs only `docker compose restart boocode` (data/ is bind-mounted); changing a referenced secret edits `.env`. MCP loads at server startup with per-server graceful degradation; the coder does NOT load MCP (BooChat only). Required: `DATABASE_URL`, `LLAMA_SWAP_URL`. Optional: `PORT` (3000), `HOST` (0.0.0.0), `PROJECT_ROOT_WHITELIST` (/opt, read-only add-existing scope), `BOOTSTRAP_ROOT` (/opt/projects, writable bootstrap mkdir target — host must `mkdir -p` it before container start), `DEFAULT_MODEL`, `LOG_LEVEL`, `SEARXNG_URL` (default `http://100.114.205.53:8888` — internal Tailscale; the public host is behind Authelia, unusable from server context), `BOOCODE_TOOLS` (`core`|`standard`|`all`, default `all`; a ceiling, never expands an agent's whitelist), `MCP_CONFIG_PATH` (default `/data/mcp.json`, opencode `mcpServers` shape; missing = no MCP), `CONTEXT7_API_KEY` (the Context7 MCP key, referenced from `data/mcp.json` as `"{env:CONTEXT7_API_KEY}"`). `data/mcp.json` is **gitignored** but no longer holds secrets — string values support opencode-style `{env:VAR}` substitution (`mcp-config.ts:substituteEnvVars`, applied before Zod validation; unset var → `''` + warn), so real keys live in `.env`; template `data/mcp.example.json`. A config-only edit there needs only `docker compose restart boocode` (data/ is bind-mounted); changing a referenced secret edits `.env`. MCP loads at server startup with per-server graceful degradation; the coder does NOT load MCP (BooChat only).
@@ -116,6 +117,7 @@ Cross-cutting only. Per-app conventions live in the matching `apps/*/CLAUDE.md`.
- **Adding a new WS frame type** (cross-app): add it to `WsFrameSchema` in `packages/contracts/src/ws-frames.ts` (single source of truth; rebuild with `pnpm -C packages/contracts build`). The server's `InferenceFrame` loose union (`services/inference/turn.ts`) and the web's strict `WsFrame` discriminated union (`apps/web/src/api/types.ts`) still exist separately and also need updating. Server publish is permissive; the frontend type is the wire-format gate — missing the web side silently drops the frame at JSON-parse. - **Adding a new WS frame type** (cross-app): add it to `WsFrameSchema` in `packages/contracts/src/ws-frames.ts` (single source of truth; rebuild with `pnpm -C packages/contracts build`). The server's `InferenceFrame` loose union (`services/inference/turn.ts`) and the web's strict `WsFrame` discriminated union (`apps/web/src/api/types.ts`) still exist separately and also need updating. Server publish is permissive; the frontend type is the wire-format gate — missing the web side silently drops the frame at JSON-parse.
- **Sentinels** (cross-app) are `role='system'` rows with structured `metadata.kind` (`cap_hit`, `doom_loop`). UI-only — `buildMessagesPayload` strips them via `isAnySentinel` so the LLM never sees them. `MessageMetadata` is single-sourced in `@boocode/contracts` (`packages/contracts/src/message-metadata.ts`). A new kind requires updating that file and rebuilding the package, plus a render branch in `apps/web/src/components/MessageBubble.tsx`. - **Sentinels** (cross-app) are `role='system'` rows with structured `metadata.kind` (`cap_hit`, `doom_loop`). UI-only — `buildMessagesPayload` strips them via `isAnySentinel` so the LLM never sees them. `MessageMetadata` is single-sourced in `@boocode/contracts` (`packages/contracts/src/message-metadata.ts`). A new kind requires updating that file and rebuilding the package, plus a render branch in `apps/web/src/components/MessageBubble.tsx`.
- **Provider snapshot types** (`ProviderSnapshotEntry`, `ProviderModel`, `ProviderMode`, `ThinkingOption`, `AgentCommand`, `ProviderSnapshotStatus`) are single-sourced in `@boocode/contracts` (`packages/contracts/src/provider-snapshot.ts`); `apps/coder/src/services/provider-types.ts` re-exports them. Edit the package source; there is no hand-synced web copy to update. - **Provider snapshot types** (`ProviderSnapshotEntry`, `ProviderModel`, `ProviderMode`, `ThinkingOption`, `AgentCommand`, `ProviderSnapshotStatus`) are single-sourced in `@boocode/contracts` (`packages/contracts/src/provider-snapshot.ts`); `apps/coder/src/services/provider-types.ts` re-exports them. Edit the package source; there is no hand-synced web copy to update.
- **`@boocode/contracts`** single-sources cross-app wire contracts via per-subpath built-dist exports, consumed by all four apps (incl. `apps/coder/web`): `./ws-frames`, `./provider-snapshot`, `./provider-config` (Zod schemas), `./message-metadata` (`MessageMetadata`/`ErrorReason`/`AgentSessionConfig`), `./worktree-risk`. It builds BEFORE every consumer (root build, Dockerfile, coder deploy). Its `WsFrame` is the loose `z.infer` of `WsFrameSchema` (payloads `unknown`); the web's richer strict `WsFrame` union is **deliberately web-local** (`apps/web/src/api/types.ts`), bridged to the validated frame by a cast — don't move it into the package. Consume built `dist` via the exports map; never add the package to a tsconfig `references` array.
- **JSONB columns**: use `sql.json(value as never)` — NOT `${JSON.stringify(value)}::jsonb` which double-serializes (stores a JSON string instead of an object/array). Pattern in `parts.ts`, `settings.ts`. - **JSONB columns**: use `sql.json(value as never)` — NOT `${JSON.stringify(value)}::jsonb` which double-serializes (stores a JSON string instead of an object/array). Pattern in `parts.ts`, `settings.ts`.
- Skills live in `data/skills/<vendor>/`; Sam's own namespace is `boocode/` (`committing-changes`, `using-worktrees`, `improving-boocode-guidance`, `systematic-debugging`) — `SKILL.md` + optional `eval.yaml` (gerund names; eval = `skill:` + `tasks:` of `prompt`+`grader`, incl. a negative-trigger task). `data/skills/` is canonical; a divergent mirror at `/opt/skills/` exists. - Skills live in `data/skills/<vendor>/`; Sam's own namespace is `boocode/` (`committing-changes`, `using-worktrees`, `improving-boocode-guidance`, `systematic-debugging`) — `SKILL.md` + optional `eval.yaml` (gerund names; eval = `skill:` + `tasks:` of `prompt`+`grader`, incl. a negative-trigger task). `data/skills/` is canonical; a divergent mirror at `/opt/skills/` exists.

View File

@@ -24,6 +24,9 @@ RUN pnpm deploy --filter=@boocode/server --prod --legacy /out/server
FROM node:20-alpine AS runtime FROM node:20-alpine AS runtime
RUN apk add --no-cache ripgrep git openssh-client RUN apk add --no-cache ripgrep git openssh-client
# The container runs as root but bind-mounts host project repos owned by uid 1000;
# trust them so git read/write tools (git_status, the git diff panel) work over the mount.
RUN git config --system --add safe.directory '*'
RUN mkdir -p /root/.ssh && ssh-keyscan -p 2222 -H 100.114.205.53 git.indifferentketchup.com >> /root/.ssh/known_hosts && chmod 700 /root/.ssh && chmod 600 /root/.ssh/known_hosts RUN mkdir -p /root/.ssh && ssh-keyscan -p 2222 -H 100.114.205.53 git.indifferentketchup.com >> /root/.ssh/known_hosts && chmod 700 /root/.ssh && chmod 600 /root/.ssh/known_hosts
WORKDIR /app WORKDIR /app

View File

@@ -0,0 +1,97 @@
# Discovery notes — git-diff-panel implementation
Single source of truth for project context. Specialists: read this first; do NOT re-grep what is here.
Source spec: `../feature-specification.md` (+ `decision-log.md` D1D18, `team-findings.md` F1F21). No
`feature-technical-notes.md` (no load-bearing mechanic qualified at spec time).
## Tech stack
- pnpm monorepo. `apps/web` (React 18 + Vite SPA), `apps/server` (BooChat — Fastify + postgres, native
inference, **read-only** file/git tools), `apps/coder` (BooCoder — host systemd service, port 9502,
write-capable, runs git writes today), `apps/booterm`. TS strict, NodeNext (`.js` import suffixes) on
server + coder. `@boocode/contracts` package single-sources WS frames + provider/message types.
- Tests: vitest ^3. server `pnpm -C apps/server test`; coder `pnpm -C apps/coder test` (`globals:false`
import describe/it/expect). Include glob `src/**/__tests__/**/*.test.ts`. No web test harness. DB-integration
tests opt-in via `DATABASE_URL` + `describe.runIf`.
- Deploy by surface: apps/coder change → `sudo systemctl restart boocoder`; apps/web|server change →
`docker compose up --build -d boocode` (rebuilds web+server from working tree; web HMR live on dev only).
- Shiki `^1.29.2` already in apps/web (`CodeBlock.tsx`, `FileViewerOverlay.tsx`); `lang:'diff'` is valid —
the path of least resistance for rendering a unified diff. No react-diff-viewer / diff2html installed.
## ADRs / coding standards
- No `docs/adr/`. Decisions live in `boocode_roadmap.md` (Decisions log) + per-app `CLAUDE.md` (auto-loaded
when editing that subtree) + `openspec/changes/archived/`.
- Coding standards: `docs/coding-standards/` (canonical), surfaced via `.claude/rules/coding-standards/`
path-scoped indexes. Cross-cutting conventions in root `CLAUDE.md`.
## Code touch points
### Git data sources today (read)
- `apps/server/src/services/git_meta.ts:44` `getGitMeta(rootPath)` — runs `runGit([...argv], rootPath)`
(SAFE: discrete argv, no shell), returns `{branch,is_dirty,ahead,behind}` only (NO diff text), 30s cache.
This is the read-side precedent for the new read route and the F2 argv-safety bar. Uses
`rev-list --left-right --count HEAD...@{upstream}` (the upstream-resolution precedent for D2/D13's base).
- `apps/server/src/routes/projects.ts:426` `GET /api/projects/:id/git` — returns the GitMeta shape (no diff).
`api.projects.git(id)` (`apps/web/src/api/client.ts:154`), polled 30s by `useProjectGit`. The new read
route slots beside this.
- `apps/coder/src/services/worktrees.ts:46` `diffWorktree(worktreePath, projectPath, {baseRef})` — produces
a real unified `git diff <base>...<head>` but via `hostExec(shell string)` + `shellEscape`, and commits
with per-invocation `-c user.email=boocoder@local -c user.name=BooCoder`. **Caution:** this is the SHELL
pattern F2 warns against; the new git-write ops should follow `git_meta.ts`'s argv `runGit`, not this.
But it IS the precedent that the coder/host can run git writes and inject identity per-invocation.
- `pending_changes.diff` is unified only for external-agent edits; native BooCode edits store `{old,new}` JSON.
The new Git panel is project-repo git state, complementary to the pending-changes panel (spec Coordinations).
### The "file browser" host (apps/web)
- `apps/web/src/components/RightRail.tsx` — the right-side file panel (NOT a workspace pane; the legacy
`file_browser` pane kind is dead). Header at ~:209 renders a static "Files" label + 2 icon buttons; desktop
`w-64`, mobile drawer `w-[85vw] max-w-sm` via `useRightRailDrawer`. Already applies `max-md:min-h-[44px]`
to header buttons (the 44px convention for D18). Fetches tree via `api.projects.files/listDir/viewFile`.
**The Files / Git tab (D1, D16) is added to THIS header** — and must fit one line (toolbar-fit rule, D18).
`open_file_in_browser` sessionEvent already opens the panel programmatically.
- Rendered in `App.tsx:~89` for every `/session/:id` (so the panel — and D8's Git tab — appears in all
session types). `Session.tsx:~397` mobile `FolderTree` toggle button (the dirty-indicator host for D17).
### Refresh-trigger plumbing (F20, D10)
- `message_complete` WS frame = "agent turn complete" trigger. Coder pending-changes refresh precedent:
`usePendingChanges` in `CoderPane.tsx:~786` refetches on message-complete. Pending-apply/discard has no
named frame — driven by the `refresh()` callback. Adding a new event/frame requires the CLAUDE.md parity
steps: a new WS frame → BOTH server/contracts `WsFrameSchema` AND web `WsFrame` (`apps/web/src/api/types.ts`);
a new sessionEvent → a `case` in `useSidebar.ts` `applyEvent`.
### Security surfaces
- `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 "server-derived root + relative-arg
validation" precedent). `secret_guard.ts` deny-list applies to the assistant's read tools (not the user's
git panel — spec D8). HTML artifacts run in a sandboxed iframe with `connect-src 'none'` (BOOCHAT.md) — the
evidence behind F1 (an artifact cannot POST to the new write endpoints). No app-layer auth (Authelia at the
proxy; `'default'` user key).
- BooCoder proxy: apps/server forwards `/api/coder/*` to apps/coder (`coder-proxy.ts`) — the route by which
a web client reaches coder (host) endpoints.
## Recent activity / precedent
- HEAD ~v2.7.11. Pure-helper + TDD precedent: `backends/turn-guard.ts`, `lifecycle-decisions.ts`,
`mistake-tracker.ts` (pure module + unit test, then wire). The diff-parse / base-resolution / mode-decision
logic should follow it (testable pure helpers).
- Sibling backlog plan at `docs/plans/post-review-backlog/` is the format precedent for the plan files.
## Enumerated gaps / open implementation questions for the team
1. **THE architecture decision (F18 / JD-005):** which service owns the new git operations? Options: (a) all
in apps/server (read + write) — but apps/server is "read-only" by posture (the git-write would be a new
write surface there); (b) read in apps/server (consistent with git_meta), write in apps/coder (the
write-capable host service, already runs git writes) via the `/api/coder/*` proxy; (c) all in apps/coder.
Note apps/coder runs on the host (can git-write to the project path); apps/server runs in Docker. The diff
panel appears in ALL sessions incl. plain BooChat — does that constrain which service answers? software-architect to settle.
2. No HTTP route returns a full working-tree git diff today — a new read route is needed regardless.
3. The diff-parse + per-file expand/collapse + staged/unstaged grouping + Shiki `lang:'diff'` rendering is
net-new UI in `RightRail.tsx`; no unified-diff renderer exists yet.
4. F2 argv-safety: the new write path must use discrete-argv git (like `git_meta.runGit`), NOT the
`hostExec(shell)` pattern `worktrees.ts` uses — concrete bar for the security/structural recommendation.
5. Committer identity (D6/D12/F3): server-derived. Precedents differ — `worktrees.ts` injects
`-c user.email=boocoder@local`; `project_bootstrap.ts` hardcodes `samkintop@gmail.com`. The plan must pick
the source for a USER commit (host git config vs a configured value) — not request-supplied.
6. Base resolution (D2/D13): `@{upstream}` precedent exists in git_meta; default-branch fallback unspecified
in code (needs `git symbolic-ref refs/remotes/origin/HEAD` or `rev-parse --abbrev-ref origin/HEAD`).

View File

@@ -0,0 +1,195 @@
# 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 (D1D18) 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 (T1T12).
- **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

View File

@@ -0,0 +1,64 @@
# Implementation iteration history — Git diff panel
Round-by-round record of the `plan-implementation` discussion for the git-diff-panel feature. The primary
plan is [`../feature-implementation-plan.md`](../feature-implementation-plan.md); decisions are in
[`implementation-decision-log.md`](implementation-decision-log.md). No `feature-technical-notes.md` exists
(no T# notes), so the spec-maturity gate reduces to the spec-level threshold alone.
Team size: **medium** (round cap 2). Converged in **1 round** — every open question resolved from evidence;
the spec-maturity gate did not trip.
---
## R1 — Parallel specialist review
**Specialists engaged:** software-architect, adversarial-security-analyst, on-call-engineer, test-engineer,
junior-developer (all sonnet, parallel). project-manager synthesized (Step 8).
**New input provided:** the feature spec + decision-log (D1D18) + team-findings (F1F21), the discovery
notes (`.discovery-notes.md`), and domain-scoped briefs. Mid-round the orchestrator verified the Docker
mount and project_bootstrap git-write precedent, which refuted the architect's service-split premise.
### Claim ledger
| # | Claim | State | Spec-maturity | Supporting |
|---|-------|-------|---------------|-----------|
| C1 | Read + write both in apps/server; architect's write-in-coder premise refuted by `/opt` rw mount + `project_bootstrap.ts` git-write precedent | Evidenced | plan-level | junior (coupling flag) + evidence; security (safe runGit only in server) |
| C2 | Read route + `git_diff.ts` pure helpers (parse, base-resolve, mode-select, classify, in-progress) — TDD-first | Evidenced | plan-level | architect, test-engineer |
| C3 | Write ops via argv-safe `runGit`/`execFile` + `--` separators; never `hostExec(shell)` | Evidenced | plan-level | architect, security |
| C4 | Path validation via `pathGuard` (reject `..`/abs/symlink-escape + repo-root discard) | Evidenced | plan-level | security |
| C5 | Commit identity server-derived (`-c` from git config, bootstrap fallback); `.strict()` request, no author fields | Evidenced | plan-level | security, on-call |
| C6 | Refresh = client `git_diff_refresh` sessionEvent + in-flight coalescence ref (no WS frame, no contracts rebuild) | Evidenced | plan-level | architect, on-call |
| C7 | Read deadline 30s + `maxBuffer` 10MB (distinct from D5 display cap) | Evidenced | plan-level | on-call |
| C8 | Index-lock → HTTP 409 "repository busy"; no server retry | Evidenced | plan-level | on-call |
| C9 | In-progress detection via `.git` sentinel `stat`s folded into read response → disable writes | Evidenced | plan-level | on-call, test-engineer |
| C10 | Write endpoints excluded from `ALL_TOOLS`; artifact sandbox `connect-src 'none'` blocks artifact→endpoint | Evidenced | plan-level | security |
| C11 | RightRail Files/Git tab + `GitDiffView` (Shiki `lang:'diff'` lazy-on-expand) + dirty dot from `useProjectGit` | Evidenced | plan-level | architect, junior |
| C12 | Two-phase build: read/display first, writes second (same deploy surface) | Evidenced | plan-level | architect, junior |
| C13 | Test plan T1T12: pure-helper units + temp-repo integration; skip Shiki/layout (no web harness) | Evidenced | plan-level | test-engineer |
### Open Questions and resolutions
| OQ | Question | Resolution source | Outcome |
|----|----------|-------------------|---------|
| OQ-1 | Which service owns the git routes? (raised by all five) | evidence | Read + write both in apps/server (D-1) — `/opt` rw mount + project_bootstrap precedent + D8 logic refute the coder-split premise |
| OQ-2 | Refresh-wiring mechanism across CoderPane↔RightRail subtrees | evidence | Client `git_diff_refresh` sessionEvent; no WS frame, no contracts rebuild (D-8) |
| OQ-6 | Commit identity source | evidence | Server-derived `-c` from git config, project_bootstrap constants fallback; request has no author fields (D-5) |
| OQ-7 | Committed-mode base resolution command | evidence | `@{upstream}``origin/HEAD` → null→Uncommitted fallback (D-6) |
| OQ-3/8/9 | Header condensation / dirty-indicator placement / D14 notification | evidence | Tab strip replaces "Files" label; FilePlus on Files tab only; dirty dot from `useProjectGit`; D14 = inline non-blocking line, not a toast (D-15) |
| OQ-4 | Is the write half gated/sequenced? | evidence | Two-phase build, read/display then writes (D-14) |
| OQ-5 | Shiki lazy vs eager | evidence | Lazy highlight on expand, per-file loading state (D-15) |
### Spec-maturity gate
**NOT tripped.** Zero spec-level findings — the spec committed every behavior (D1D18); all Round-1 findings
are plan-level HOW choices, each resolved from codebase evidence. No T#-contradictions (no T# notes exist).
### Next-step recommendation
**Go to synthesis.** All open questions resolved by evidence; no handoffs requested; no plan-level question
left unresolved.
**Decisions produced:** D-1 through D-15.
**Changed in plan:** Implementation Approach, Decomposition and Sequencing, Security Posture, On-Call
Resilience Posture, Operational Readiness, Testing Strategy, Deferred (YAGNI), UX Notes.

View File

@@ -0,0 +1,105 @@
# Synthesis input — Round 1 aggregation + resolutions (git-diff-panel impl)
Deterministic aggregation of Round-1 (software-architect, adversarial-security-analyst, on-call-engineer,
test-engineer, junior-developer). Team size: medium (round cap 2; converged in 1 — all open questions
resolved from evidence). Spec-maturity gate: NOT tripped (0 spec-level findings — the spec committed all
behaviors; every finding is a plan-level HOW). Next step: go to synthesis.
## THE corrected key decision (D-1)
**Both the git READ and the git WRITE routes live in apps/server, NOT a read-server/write-coder split.**
The software-architect (A1) recommended read-in-server / write-in-coder, on the premise that apps/server is
"read-only by posture" and adding writes there is a new surface. The junior-developer flagged the resulting
coupling (a plain BooChat session reaching the coder write service). Evidence REFUTES the architect's premise:
- `docker-compose.yml:16` mounts `/opt:/opt` **read-write** into the boocode container — apps/server can
already write project paths.
- `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` flags (`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 — not new.
- D8's own logic: "read-only" governs the **assistant's tools**, not the container's filesystem or the
user's own UI actions. A user-driven git-write route is not an assistant tool.
- apps/server has the safe `runGit` (`git_meta.ts:30`, `execFile` discrete argv); apps/coder has only the
UNSAFE `hostExec(shell string)` (`worktrees.ts`). Writing in coder would require building a NEW safe
wrapper there; writing in server reuses the existing safe one and the project_bootstrap `-c` precedent.
- Writing in server avoids: a cross-service `/api/coder/*` proxy hop per write, a second deploy surface
(coder restart), and making coder a dependency for a BooChat-session commit.
**Decision:** read + write both in apps/server. New `apps/server/src/services/git_diff.ts` (read) +
git-write helpers, new routes beside `GET /api/projects/:id/git` in `routes/projects.ts`. No apps/coder
changes. Single deploy surface (`docker compose up --build -d boocode`). Rejected: A1's read-server/
write-coder split (premise refuted); all-in-coder (needs a new safe wrapper, adds coupling).
## Resolved open questions (all plan-level, settled by evidence)
- **OQ-1 service split** → D-1 above (read+write in apps/server).
- **OQ-2 refresh wiring (F20/D10)** → a client-side `sessionEvents` event `git_diff_refresh`, NOT a new WS
frame. The five triggers are all client-side (tab open, post-mutation, message_complete, pending-apply,
on-demand); no server push needed → no `@boocode/contracts` rebuild. Emit from the `message_complete`
handler + CoderPane apply/discard callbacks; `useGitDiff` subscribes; add a no-op `case` in
`useSidebar.ts:applyEvent` per the apps/web CLAUDE.md rule. (architect A4.)
- **OQ-6 commit identity (D12/F3)** → server-derived `-c user.name=… -c user.email=…`, read from git config
at commit time, fallback to the project_bootstrap constants. Request schema is `.strict()`,
`{message, files?}` only — no author/email/date fields. (security #4, project_bootstrap precedent.)
- **OQ-7 base resolution (D13)** → `git rev-parse --abbrev-ref --symbolic-full-name @{upstream}` (tracking
branch; non-zero if none) → else `git rev-parse --abbrev-ref origin/HEAD` (default branch) → else null →
fall back to Uncommitted, labeled. Pure helper `resolveCommittedBase`. (architect A2.)
- **OQ-3/8/9 UX placement** → adopt architect A4: the Files/Git tab strip replaces the static "Files" label;
the FilePlus button shows only on the Files tab (keeps header one line, D18); dirty dot on the Git tab
button + the `Session.tsx` FolderTree toggle, fed by `useProjectGit`'s existing `is_dirty` (no new fetch,
D17); D14's "briefly notes the change" is a non-blocking inline line inside the panel (NOT a toast —
avoids mobile-drawer z-index collision).
- **OQ-4 sequencing (F19)** → adopt architect A5 two-phase: Phase 1 = read + display (git_diff.ts pure
helpers TDD-first → read route → client + RightRail tab + GitDiffView read-only + refresh wiring); Phase 2
= write actions (write helpers + routes + stage/commit/discard affordances + in-progress disable). Both
phases are now the SAME deploy surface (server+web) since writes are in apps/server.
- **OQ-5 Shiki (D9)** → lazy: highlight a file's diff only when expanded, with a per-file loading state;
"expand all" triggers lazy highlight per file as rendered. (junior OQ-5.)
## Claim ledger (consolidated)
| # | Claim | State | Spec-maturity | Supporting |
|---|-------|-------|---------------|-----------|
| C1 | Read+write both in apps/server; architect's write-in-coder premise refuted by /opt rw mount + project_bootstrap git-write precedent | Evidenced | plan-level | junior (flag) + corrected via evidence; security (safe-runGit-only-in-server) |
| C2 | Read route + git_diff.ts pure helpers (parseNameStatus, splitDiffByFile, resolveCommittedBase, autoSelectMode, classify binary/large, detectInProgress) — TDD-first | Evidenced | plan-level | architect, test-engineer |
| C3 | Write ops via argv-safe runGit/execFile with `--` separators; NEVER hostExec(shell) | Evidenced | plan-level | architect, security |
| C4 | Path validation: pathGuard(repoRoot, file) rejects ../ + absolute + symlink-escape; also reject discard of repo-root (`.`) | Evidenced | plan-level | security |
| C5 | Commit identity server-derived (-c from git config, bootstrap fallback); request `.strict()` no author fields | Evidenced | plan-level | security, on-call OQ4 |
| C6 | Refresh = sessionEvent git_diff_refresh + client in-flight coalescence ref (no WS frame, no debounce) | Evidenced | plan-level | architect, on-call |
| C7 | Read deadline 30s + maxBuffer 10MB (distinct from D5 size cap); timeout→error+Refresh | Evidenced | plan-level | on-call |
| C8 | Index-lock → HTTP 409 "repository busy", NO server retry (user-driven retry) | Evidenced | plan-level | on-call |
| C9 | In-progress detection via .git sentinel stats (MERGE_HEAD/rebase-merge/CHERRY_PICK_HEAD/BISECT_LOG) folded into read response → disable writes | Evidenced | plan-level | on-call, test-engineer |
| C10 | Write endpoints NOT in the assistant tool registry (ALL_TOOLS); artifact sandbox connect-src 'none' already blocks artifact→endpoint | Evidenced | plan-level | security |
| C11 | RightRail Files/Git tab + GitDiffView (Shiki lang:'diff' lazy-on-expand) + dirty dot from useProjectGit | Evidenced | plan-level | architect, junior |
| C12 | Two-phase build: read/display first, writes second (same deploy surface) | Evidenced | plan-level | architect, junior |
| C13 | Test plan T1-T12: pure-helper units + temp-repo integration (mkdtemp/git init, like path_guard.test.ts); skip Shiki/layout (no web harness) | Evidenced | plan-level | test-engineer |
## YAGNI ledger
- **Shared `packages/` runGit extraction** → DEFER. Rule of Three not met (now only apps/server consumes it
for this feature; coder untouched). Reopen: a third consumer needs git ops. Source: architect.
- **New WS frame for refresh** → REPLACE with the simpler client `sessionEvents` event (no server push, no
contracts rebuild). Source: architect/junior OQ-2.
- **Server-side retry on index-lock** → DEFER. "Try again" is user-driven; a server retry hides the busy
state and adds timer state. Reopen: lock contention observed frequent in practice. Source: on-call F5.
- **Streaming diff reader (cap mid-stream)** → DEFER. `execFile` maxBuffer 10MB covers any realistic
single-user working-tree diff; streaming is ~40-50 LoC for a transient memory spike that doesn't matter at
this scale. Reopen: diffs routinely exceed 10MB. Source: on-call §6.
- **CSRF custom-header on the write routes** → DEFER. `connect-src 'none'` on artifacts + SameSite=Lax
Authelia cookie + same-origin SPA covers it at single-user scale. Reopen: routes exposed to a less-
controlled origin. Source: security #3.
- **Separate per-file expand-state hook** → REPLACE with local state in GitDiffView (one consumer). Reopen:
a second component needs the same expand state. Source: architect.
- **`autoSelectMode` / `canCommit` as separate files** → keep as inline pure helpers in git_diff.ts (tested),
not separate modules. Source: architect/test-engineer.
## Notes for synthesis
- No `feature-technical-notes.md` (no T# notes) — omit all T# handling.
- Honor: deploy-by-surface (now single surface: docker rebuild), stage-commits-by-path, WS-frame/sessionEvent
parity steps (only a sessionEvent here — `useSidebar.ts` case), pure-helper-then-wire TDD precedent,
`globals:false` + `.js` import suffixes + `src/**/__tests__/**` glob for tests.
- Security posture section: real content (argv-safety, path-traversal, identity, not-in-tool-registry,
artifact-sandbox). On-call resilience section: real content (deadline, index-lock, coalescence,
in-progress, partial-failure). Operational readiness: single deploy surface, no migration.

View File

@@ -0,0 +1,197 @@
# Feature Implementation Plan: Git Diff Panel
A Files / Git tab in the right-side file panel that reads the project repository's diff (two modes) and stages, unstages, commits, and discards whole files — built entirely in `apps/server` ([D-1](artifacts/implementation-decision-log.md#d-1-both-git-read-and-git-write-live-in-appsserver)), shipped via a single docker rebuild, in two phases (read/display, then write actions) with no schema change, no migration, and no `@boocode/contracts` change.
## Source Specification
- **Feature specification:** [feature-specification.md](feature-specification.md)
- **Specification decision log:** [artifacts/decision-log.md](artifacts/decision-log.md)
- **Specification team findings:** [artifacts/team-findings.md](artifacts/team-findings.md)
- **Specification decisions this plan inherits:** D1D18
- **Specification open items this plan must respect or resolve:** None — the spec's only open item (commit identity) was settled at spec time (F3, D12).
## Outcome
When this plan is executed, the right-side file panel gains a Files / Git tab. Selecting **Git** shows the project repository's changed files (Uncommitted or Committed mode), each expandable to a syntax-highlighted unified diff. The user can stage, unstage, commit (with a server-derived identity), and discard whole files without leaving the session. The work is delivered by a new `apps/server/src/services/git_diff.ts` (read logic + git-write helpers + pure helpers), new read and write routes in `apps/server/src/routes/projects.ts`, and new `apps/web` UI (`GitDiffView`, a tab in `RightRail.tsx`, a `useGitDiff` hook, and a `git_diff_refresh` sessionEvent). No apps/coder change, no Postgres schema change, no new env var.
## Context
- **Driving constraint:** A direct user request (2026-06-02) to add a Paseo-style git diff panel shown "instead of the file browser." Not deadline-bound; scoped to ship as a coherent v1.
- **Stakeholders:** The single session user (reviews and commits their own repo changes in-session). The project's security posture (the write surface must not become an assistant-reachable path). On-call/operability (the panel must not stall on large or slow repos or fail opaquely under index contention).
- **Future-state concern:** The git-write surface now lives in `apps/server`. Watch for a third consumer of git ops (would trigger a shared-`packages/` extraction, [D-1](artifacts/implementation-decision-log.md#d-1-both-git-read-and-git-write-live-in-appsserver)) and for working-tree diffs routinely exceeding the 10MB read buffer ([D-7](artifacts/implementation-decision-log.md#d-7-read-deadline-30s--maxbuffer-10mb-index-lock--http-409-in-progress-detection-disables-writes)).
- **Out-of-scope boundary:** No remote operations (push/pull/PR/merge), no side-by-side layout, no per-hunk staging, no continuous file-watch streaming, no rename of the pending-changes panel, no per-line review/re-prompt surface — all deferred in the spec under YAGNI.
## Team Composition and Participation
| Specialist | Status | Key Input |
|------------|--------|-----------|
| `project-manager` | Coordinator | Facilitated R1, corrected the service-owner decision against evidence, synthesized the plan. |
| `software-architect` | Active | Recommended a read-server/write-coder split (R1); the split's premise was refuted by evidence and recorded as the rejected alternative on [D-1](artifacts/implementation-decision-log.md#d-1-both-git-read-and-git-write-live-in-appsserver). Owns the read shape, base resolution, refresh wiring, two-phase sequencing. |
| `adversarial-security-analyst` | Active | Argv-safety, path-traversal, server-derived identity, tool-registry exclusion, artifact-sandbox evidence ([D-3](artifacts/implementation-decision-log.md#d-3-write-ops-via-argv-safe-rungitexecfile-with----separators)[D-5](artifacts/implementation-decision-log.md#d-5-commit-identity-server-derived-request-schema-strict-with-no-author-fields), [D-13](artifacts/implementation-decision-log.md#d-13-write-endpoints-excluded-from-the-assistant-tool-registry-artifact-sandbox-closes-the-indirect-path)). |
| `on-call-engineer` | Active | Read deadline + maxBuffer, index-lock 409, refresh coalescence, in-progress detection, partial-failure honesty ([D-7](artifacts/implementation-decision-log.md#d-7-read-deadline-30s--maxbuffer-10mb-index-lock--http-409-in-progress-detection-disables-writes), [D-8](artifacts/implementation-decision-log.md#d-8-refresh-via-client-git_diff_refresh-sessionevent-coalesced--not-a-ws-frame)). |
| `test-engineer` | Active | T1T12: pure-helper units + temp-repo integration; skip Shiki/layout (no web harness). |
| `junior-developer` | Reframer | Flagged the cross-service coupling of the write-in-coder split, which seeded the evidence correction on [D-1](artifacts/implementation-decision-log.md#d-1-both-git-read-and-git-write-live-in-appsserver); confirmed the read-only-session Git tab needs no extra label (F21). |
## Implementation Approach
Both the git read and the git write operations live in `apps/server` ([D-1](artifacts/implementation-decision-log.md#d-1-both-git-read-and-git-write-live-in-appsserver)) — reusing the existing safe argv `runGit` and the `project_bootstrap.ts` server-side git-write precedent, and avoiding a cross-service proxy hop, a second deploy surface, and coder coupling. The feature reuses Shiki (`lang:'diff'`) already in `apps/web`, the `useProjectGit` dirty signal, and the existing `RightRail.tsx` file-panel host; it introduces one new server service, new routes, one new web view + hook, and one new client `sessionEvents` event.
### Architecture and Integration Points
- **New `apps/server/src/services/git_diff.ts`** — read logic, git-write helpers, and TDD-first pure helpers (`parseNameStatus`, `splitDiffByFile`, `resolveCommittedBase`, `autoSelectMode`, `classify`, `detectInProgress`) ([D-2](artifacts/implementation-decision-log.md#d-2-read-route--git_diffts-pure-helpers-tdd-first)). `autoSelectMode` and `canCommit` are inline tested helpers, not separate modules ([D-12](artifacts/implementation-decision-log.md#trivial-decisions)).
- **New routes in `apps/server/src/routes/projects.ts`**, beside `GET /api/projects/:id/git`: read `GET /api/projects/:id/git/diff?mode=<uncommitted|committed>` ([D-2](artifacts/implementation-decision-log.md#d-2-read-route--git_diffts-pure-helpers-tdd-first)); writes for stage / unstage / commit / discard ([D-3](artifacts/implementation-decision-log.md#d-3-write-ops-via-argv-safe-rungitexecfile-with----separators)). The repository root is derived server-side via `path_guard.ts` `resolveProjectRoot`, never from the request ([D-4](artifacts/implementation-decision-log.md#d-4-path-validation--repo-relative-pathguard-reject-escape-and-repo-root-discard)).
- **Frontend** — a Files / Git tab in the existing `RightRail.tsx` header (replacing the static "Files" label, fitting one line); a new `GitDiffView` in the same slot as the file tree; a `useGitDiff` hook; the dirty dot fed by `useProjectGit`'s existing `is_dirty` ([D-15](artifacts/implementation-decision-log.md#d-15-frontend--rightrail-filesgit-tab-gitdiffview-with-lazy-shiki-dirty-dot-from-useprojectgit)). Per-file expand/collapse is local state in `GitDiffView`, not a shared hook ([D-11](artifacts/implementation-decision-log.md#trivial-decisions)).
### Data Model and Persistence
None. The panel reads git state at request time and writes the project repository directly via git; nothing is persisted in Postgres — no schema change, no migration, no new env var ([D-10](artifacts/implementation-decision-log.md#trivial-decisions)).
### Runtime Behavior
- **Read:** the read route runs argv `runGit` calls under a 30s deadline and a 10MB `maxBuffer` ([D-7](artifacts/implementation-decision-log.md#d-7-read-deadline-30s--maxbuffer-10mb-index-lock--http-409-in-progress-detection-disables-writes)); `parseNameStatus` builds the file list, `splitDiffByFile` segments the unified diff, `classify` marks binary/large bodies, `detectInProgress` reads `.git` sentinels, and the response carries the resolved base label and the in-progress flag.
- **Mode/base:** `autoSelectMode` picks Uncommitted (dirty) or Committed (clean) on first open only ([D-12](artifacts/implementation-decision-log.md#trivial-decisions)); `resolveCommittedBase` resolves `@{upstream}``origin/HEAD` → null, falling back to a labeled Uncommitted on null ([D-6](artifacts/implementation-decision-log.md#d-6-committed-mode-base-resolution-via-upstream--originhead--null)).
- **Refresh:** the client `git_diff_refresh` sessionEvent fires on the five triggers, coalesced behind an in-flight ref so a running refresh absorbs later triggers ([D-8](artifacts/implementation-decision-log.md#d-8-refresh-via-client-git_diff_refresh-sessionevent-coalesced--not-a-ws-frame)).
- **Diff render:** `GitDiffView` highlights a file's diff via Shiki `lang:'diff'` lazily on expand, with a per-file loading state ([D-15](artifacts/implementation-decision-log.md#d-15-frontend--rightrail-filesgit-tab-gitdiffview-with-lazy-shiki-dirty-dot-from-useprojectgit)).
### External Interfaces
New HTTP routes only (read + four writes) under `/api/projects/:id/git/…`. Refresh is a client-side `sessionEvents` event (`git_diff_refresh`), **not** a WS frame, so `@boocode/contracts` is not touched or rebuilt; the only parity step is a no-op `case 'git_diff_refresh'` in `useSidebar.ts` `applyEvent` ([D-8](artifacts/implementation-decision-log.md#d-8-refresh-via-client-git_diff_refresh-sessionevent-coalesced--not-a-ws-frame), [D-9](artifacts/implementation-decision-log.md#trivial-decisions)).
## Decomposition and Sequencing
Two phases on a single deploy surface ([D-14](artifacts/implementation-decision-log.md#d-14-two-phase-build--readdisplay-phase-1-then-write-actions-phase-2)); both ship via `docker compose up --build -d boocode`.
| # | Work Unit | Delivers | Depends On | Verification |
|---|-----------|----------|------------|--------------|
| 1 | `git_diff.ts` pure helpers (TDD-first) | `parseNameStatus`, `splitDiffByFile`, `resolveCommittedBase`, `autoSelectMode`, `classify`, `detectInProgress` ([D-2](artifacts/implementation-decision-log.md#d-2-read-route--git_diffts-pure-helpers-tdd-first), [D-6](artifacts/implementation-decision-log.md#d-6-committed-mode-base-resolution-via-upstream--originhead--null)) | — | Unit tests T1T7 |
| 2 | Read route `GET /api/projects/:id/git/diff?mode=` | Diff payload (files, counts, base label, in-progress flag) under 30s/10MB bounds ([D-2](artifacts/implementation-decision-log.md#d-2-read-route--git_diffts-pure-helpers-tdd-first), [D-7](artifacts/implementation-decision-log.md#d-7-read-deadline-30s--maxbuffer-10mb-index-lock--http-409-in-progress-detection-disables-writes)) | 1 | Temp-repo integration T8 |
| 3 | `useGitDiff` + RightRail Files/Git tab + dirty dot | Read-only panel, tab switch, dirty indicator from `useProjectGit` ([D-15](artifacts/implementation-decision-log.md#d-15-frontend--rightrail-filesgit-tab-gitdiffview-with-lazy-shiki-dirty-dot-from-useprojectgit)) | 2 | Manual (no web harness) |
| 4 | `GitDiffView` read-only + lazy Shiki + refresh wiring | Per-file expand, lazy `lang:'diff'`, `git_diff_refresh` sessionEvent + `useSidebar.ts` no-op case, coalescence ref ([D-8](artifacts/implementation-decision-log.md#d-8-refresh-via-client-git_diff_refresh-sessionevent-coalesced--not-a-ws-frame), [D-15](artifacts/implementation-decision-log.md#d-15-frontend--rightrail-filesgit-tab-gitdiffview-with-lazy-shiki-dirty-dot-from-useprojectgit)) | 3 | Manual |
| 5 | Write helpers + routes (stage/unstage/commit/discard) | Argv-safe writes with `--` separators, `pathGuard`, server-derived `-c` identity, `.strict()` commit schema, 409 on index-lock ([D-3](artifacts/implementation-decision-log.md#d-3-write-ops-via-argv-safe-rungitexecfile-with----separators), [D-4](artifacts/implementation-decision-log.md#d-4-path-validation--repo-relative-pathguard-reject-escape-and-repo-root-discard), [D-5](artifacts/implementation-decision-log.md#d-5-commit-identity-server-derived-request-schema-strict-with-no-author-fields), [D-7](artifacts/implementation-decision-log.md#d-7-read-deadline-30s--maxbuffer-10mb-index-lock--http-409-in-progress-detection-disables-writes)); endpoints kept out of `ALL_TOOLS` ([D-13](artifacts/implementation-decision-log.md#d-13-write-endpoints-excluded-from-the-assistant-tool-registry-artifact-sandbox-closes-the-indirect-path)) | 1, 2 | Temp-repo integration T9T12 |
| 6 | Write affordances + in-progress disable | Stage/unstage/commit/discard UI, tracked/untracked discard confirmation, in-progress disable ([D-7](artifacts/implementation-decision-log.md#d-7-read-deadline-30s--maxbuffer-10mb-index-lock--http-409-in-progress-detection-disables-writes), [D-15](artifacts/implementation-decision-log.md#d-15-frontend--rightrail-filesgit-tab-gitdiffview-with-lazy-shiki-dirty-dot-from-useprojectgit)) | 4, 5 | Manual |
Phase 1 = units 14 (read + display); Phase 2 = units 56 (write actions).
## RAID Log
### Assumptions
| ID | Assumption | What Changes If Wrong | Verifier | Status |
|----|------------|-----------------------|----------|--------|
| A1 | The boocode container keeps the `/opt:/opt` read-write mount, so apps/server can write project paths. | [D-1](artifacts/implementation-decision-log.md#d-1-both-git-read-and-git-write-live-in-appsserver) reopens — writes would need a host service. | `docker-compose.yml:16` | Verified (R1) |
| A2 | A realistic single-user working-tree diff stays under 10MB. | The read truncates/errors; the streaming reader reopens (Deferred YAGNI). | `on-call-engineer` | Unverified — bounded by [D-7](artifacts/implementation-decision-log.md#d-7-read-deadline-30s--maxbuffer-10mb-index-lock--http-409-in-progress-detection-disables-writes) |
### Dependencies
| ID | Dependency | Owner | Status |
|----|------------|-------|--------|
| Dep1 | Shiki `^1.29.2` (`lang:'diff'`) already in `apps/web`. | `apps/web` | Present (no install) |
| Dep2 | `useProjectGit` already returns `is_dirty` for the dirty dot. | `apps/web` | Present (no new fetch) |
## Testing Strategy
Sourced from `test-engineer` (T1T12). Server-side only — `apps/web` has no test harness, so Shiki rendering and layout/tab behavior are verified manually, not in the suite. Vitest conventions: `globals:false` (import `describe`/`it`/`expect`), `.js` import suffixes, include glob `src/**/__tests__/**/*.test.ts`.
- **Observable behaviors to test:** porcelain → file-list parse with change types (T1); unified-diff split per file (T2); base resolution `@{upstream}``origin/HEAD` → null (T3); auto-select mode by dirty/clean (T4); binary/large classify (T5); in-progress sentinel detection (T6); `canCommit` gating (T7); read route over a temp repo (T8); stage/unstage round-trip (T9); commit with server-derived identity (T10); discard tracked vs untracked (T11); path-guard rejection of `..`/absolute/symlink-escape and repo-root discard (T12).
- **Test doubles posture:** pure helpers tested directly with fixture strings (no git spawn); route/write tests use a real temp repo via `mkdtemp` + `git init` (the `path_guard.test.ts` integration pattern).
- **Edge cases requiring coverage:** binary file, oversized diff, no resolvable base, in-progress state, index-lock 409, untracked-discard partial failure, path-escape attempts ([D-4](artifacts/implementation-decision-log.md#d-4-path-validation--repo-relative-pathguard-reject-escape-and-repo-root-discard), [D-7](artifacts/implementation-decision-log.md#d-7-read-deadline-30s--maxbuffer-10mb-index-lock--http-409-in-progress-detection-disables-writes)).
- **Test levels:** unit (T1T7, pure helpers) + integration (T8T12, temp-repo). No end-to-end / web-layer automation.
## Security Posture
`adversarial-security-analyst` contributed the full write-surface posture. Threat vectors and the mitigations this plan commits to:
- **Command/flag injection** — every git invocation uses discrete argv with explicit `--` separators between options and user-supplied paths and a flag-injection guard (reject path args starting with `-`); never `hostExec(shell)` ([D-3](artifacts/implementation-decision-log.md#d-3-write-ops-via-argv-safe-rungitexecfile-with----separators)). User text (commit message, file targets) is always discrete argv.
- **Path traversal** — `pathGuard(repoRoot, file)` resolves each per-file argument and rejects absolute paths, `..` traversal, and symlink escape outside the server-derived root; discarding the repo root (`.`) is rejected ([D-4](artifacts/implementation-decision-log.md#d-4-path-validation--repo-relative-pathguard-reject-escape-and-repo-root-discard)). The root is derived from the project record via `path_guard.ts`, never the request.
- **Identity spoofing** — commit identity is server-derived (`-c user.name`/`-c user.email` from git config, falling back to the `project_bootstrap.ts` constants); the commit request schema is Zod `.strict()` with `{message, files?}` only — no author/email/date field can be supplied ([D-5](artifacts/implementation-decision-log.md#d-5-commit-identity-server-derived-request-schema-strict-with-no-author-fields)).
- **Assistant-driven invocation** — the write endpoints are never registered in `ALL_TOOLS`, so no assistant tool surface reaches them; the indirect artifact path is already closed by the artifact iframe's `connect-src 'none'` sandbox (F1), so no new CSRF mitigation is built ([D-13](artifacts/implementation-decision-log.md#d-13-write-endpoints-excluded-from-the-assistant-tool-registry-artifact-sandbox-closes-the-indirect-path)).
## Operational Readiness
- **Deploy surface:** single — `docker compose up --build -d boocode` (rebuilds web + server from the working tree). No `sudo systemctl restart boocoder`, because no apps/coder code changes ([D-1](artifacts/implementation-decision-log.md#d-1-both-git-read-and-git-write-live-in-appsserver)).
- **Schema / migration / env:** none — no Postgres change, no migration, no new env var ([D-10](artifacts/implementation-decision-log.md#trivial-decisions)).
- **Contracts package:** not touched — refresh is a client `sessionEvents` event, not a WS frame, so `packages/contracts/` is not rebuilt ([D-9](artifacts/implementation-decision-log.md#trivial-decisions)).
- **Feature flag / rollout:** none — the panel is additive and inert until the Git tab is opened; rollback is reverting the build.
## On-Call Resilience Posture
`on-call-engineer` contributed; each commitment maps to a flagged failure mode.
- **Timeouts and deadlines:** the read path runs under 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 ([D-7](artifacts/implementation-decision-log.md#d-7-read-deadline-30s--maxbuffer-10mb-index-lock--http-409-in-progress-detection-disables-writes)).
- **Retry strategy:** none on the server. An index-lock write failure returns **HTTP 409 "repository busy"**; retry is user-driven, so no timer state or hidden busy state is introduced ([D-7](artifacts/implementation-decision-log.md#d-7-read-deadline-30s--maxbuffer-10mb-index-lock--http-409-in-progress-detection-disables-writes)).
- **Concurrency / coalescence:** the client holds an in-flight coalescence ref so concurrent `git_diff_refresh` triggers absorb into the running refresh — one read at a time, settling to a single final snapshot ([D-8](artifacts/implementation-decision-log.md#d-8-refresh-via-client-git_diff_refresh-sessionevent-coalesced--not-a-ws-frame)).
- **Graceful degradation:** `detectInProgress` (`.git` MERGE_HEAD / rebase / CHERRY_PICK_HEAD / BISECT_LOG sentinels) folds an in-progress flag into the read response; the client disables write affordances when set, so stage/commit/discard never fail with raw git errors mid-operation ([D-7](artifacts/implementation-decision-log.md#d-7-read-deadline-30s--maxbuffer-10mb-index-lock--http-409-in-progress-detection-disables-writes)).
- **Data integrity:** an untracked-file discard that fails partway reports honest partial failure on the next refresh rather than claiming an unenforceable "state unchanged" ([D-7](artifacts/implementation-decision-log.md#d-7-read-deadline-30s--maxbuffer-10mb-index-lock--http-409-in-progress-detection-disables-writes)).
## Definition of Done
- [ ] Opening the Git tab shows the project repo's changed files in the auto-selected mode; switching to Files restores the tree in the same slot ([D-15](artifacts/implementation-decision-log.md#d-15-frontend--rightrail-filesgit-tab-gitdiffview-with-lazy-shiki-dirty-dot-from-useprojectgit)).
- [ ] Committed mode labels its resolved base (`@{upstream}``origin/HEAD` → labeled Uncommitted fallback) ([D-6](artifacts/implementation-decision-log.md#d-6-committed-mode-base-resolution-via-upstream--originhead--null)).
- [ ] Stage / unstage / commit / discard operate at whole-file granularity in Uncommitted mode; discard prompts the tracked vs untracked confirmation; Committed mode is read-only ([D-3](artifacts/implementation-decision-log.md#d-3-write-ops-via-argv-safe-rungitexecfile-with----separators), [D-15](artifacts/implementation-decision-log.md#d-15-frontend--rightrail-filesgit-tab-gitdiffview-with-lazy-shiki-dirty-dot-from-useprojectgit)).
- [ ] Commit identity is server-derived; the request cannot set it; the schema is `.strict()` ([D-5](artifacts/implementation-decision-log.md#d-5-commit-identity-server-derived-request-schema-strict-with-no-author-fields)).
- [ ] Path-escape and repo-root discard are rejected; writes use argv + `--` separators ([D-3](artifacts/implementation-decision-log.md#d-3-write-ops-via-argv-safe-rungitexecfile-with----separators), [D-4](artifacts/implementation-decision-log.md#d-4-path-validation--repo-relative-pathguard-reject-escape-and-repo-root-discard)).
- [ ] Write endpoints are absent from `ALL_TOOLS` ([D-13](artifacts/implementation-decision-log.md#d-13-write-endpoints-excluded-from-the-assistant-tool-registry-artifact-sandbox-closes-the-indirect-path)).
- [ ] Read respects the 30s/10MB bounds; index-lock returns 409; in-progress disables writes; refresh coalesces ([D-7](artifacts/implementation-decision-log.md#d-7-read-deadline-30s--maxbuffer-10mb-index-lock--http-409-in-progress-detection-disables-writes), [D-8](artifacts/implementation-decision-log.md#d-8-refresh-via-client-git_diff_refresh-sessionevent-coalesced--not-a-ws-frame)).
- [ ] Tests T1T12 pass (`pnpm -C apps/server test`).
- [ ] Phase 1 ships and is reviewable before Phase 2 lands ([D-14](artifacts/implementation-decision-log.md#d-14-two-phase-build--readdisplay-phase-1-then-write-actions-phase-2)).
- [ ] Post-ship owner: the session user (single-user app); no separate on-call rotation.
## Specialist Handoffs for Implementation
- **`test-engineer`** — dispatch at the start of unit 1 to own T1T12; needs the helper signatures from `git_diff.ts` and the temp-repo integration pattern (`path_guard.test.ts`).
- **`adversarial-security-analyst`** — dispatch to review unit 5 before merge; needs the write-route handlers and the `.strict()` commit schema to confirm argv-safety, path-guard coverage, and identity derivation.
- **`user-experience-designer`** — dispatch during units 34 and 6; needs the `RightRail.tsx` header layout to confirm one-line fit (D18), tap-target minimum, and the tracked/untracked discard wording.
## Deferred (YAGNI)
### Shared `packages/` runGit extraction
- **Why deferred:** Rule of Three not met — only apps/server consumes the git ops for this feature (coder is untouched); a single-consumer abstraction is premature.
- **Reopen when:** a third consumer needs the same git ops.
- **Source:** R1, software-architect.
### New WS frame for refresh
- **Why deferred:** Replaced by the simpler client `sessionEvents` event `git_diff_refresh` — no trigger needs a server push, so a WS frame and a `@boocode/contracts` rebuild are unnecessary (simpler-version test).
- **Reopen when:** a refresh trigger genuinely originates server-side (an out-of-band repo mutation the client cannot observe).
- **Source:** R1, software-architect / junior-developer.
### Server-side retry on index-lock
- **Why deferred:** "Try again" is user-driven; a server retry hides the busy state and adds timer state (evidence test — no observed contention).
- **Reopen when:** index-lock contention is observed frequent in practice.
- **Source:** R1, on-call-engineer (F5).
### Streaming diff reader (cap mid-stream)
- **Why deferred:** `execFile` maxBuffer 10MB covers any realistic single-user working-tree diff; ~4050 LoC for a transient memory spike that does not matter at this scale (simpler-version test).
- **Reopen when:** working-tree diffs routinely exceed 10MB.
- **Source:** R1, on-call-engineer.
### CSRF custom-header on the write routes
- **Why deferred:** `connect-src 'none'` on artifacts + the SameSite=Lax Authelia cookie + a same-origin SPA cover it at single-user scale (evidence test — no exposed cross-origin path).
- **Reopen when:** the write routes are exposed to a less-controlled origin.
- **Source:** R1, adversarial-security-analyst.
### Separate per-file expand-state hook
- **Why deferred:** Replaced by local state in `GitDiffView` — one consumer (single-implementation abstraction).
- **Reopen when:** a second component needs the same expand state.
- **Source:** R1, software-architect.
### `autoSelectMode` / `canCommit` as separate modules
- **Why deferred:** Kept as inline tested pure helpers in `git_diff.ts` — splitting into modules adds files without a second consumer (simpler-version test).
- **Reopen when:** a second module needs to import them independently.
- **Source:** R1, software-architect / test-engineer.
## Open Items
- None. Every Round-1 open question resolved from evidence; the spec's only open item (commit identity) was settled at spec time. The plan is shippable as written.
## Summary
- **Outcome delivered:** A Files / Git tab in the right-side file panel that reads and (whole-file) stages, unstages, commits, and discards the project repository's changes, built entirely in `apps/server`.
- **Team size:** 6 specialists — see [artifacts/implementation-iteration-history.md](artifacts/implementation-iteration-history.md)
- **Rounds of facilitation:** 1 — see [artifacts/implementation-iteration-history.md](artifacts/implementation-iteration-history.md)
- **Decisions committed:** 15 — see [artifacts/implementation-decision-log.md](artifacts/implementation-decision-log.md)
- **Decisions settled by evidence:** 15 — see [artifacts/implementation-decision-log.md](artifacts/implementation-decision-log.md)
- **Decisions settled by junior-developer reframing:** 0 (the junior-developer's coupling flag seeded the evidence correction on D-1, but the resolution was evidence) — see [artifacts/implementation-decision-log.md](artifacts/implementation-decision-log.md)
- **Decisions settled by user input:** 0 — see [artifacts/implementation-decision-log.md](artifacts/implementation-decision-log.md)
- **Rejected alternatives recorded:** 22 — see [artifacts/implementation-decision-log.md](artifacts/implementation-decision-log.md)
- **Open items remaining:** 0
- **Recommendation:** Ship as planned — build Phase 1 (units 14) first, then Phase 2 (units 56).