diff --git a/docs/features/git-diff-panel/artifacts/.discovery-notes.md b/docs/features/git-diff-panel/artifacts/.discovery-notes.md new file mode 100644 index 0000000..5480ecd --- /dev/null +++ b/docs/features/git-diff-panel/artifacts/.discovery-notes.md @@ -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` D1–D18, `team-findings.md` F1–F21). 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 ...` 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`). diff --git a/docs/features/git-diff-panel/artifacts/implementation-decision-log.md b/docs/features/git-diff-panel/artifacts/implementation-decision-log.md new file mode 100644 index 0000000..c12aca6 --- /dev/null +++ b/docs/features/git-diff-panel/artifacts/implementation-decision-log.md @@ -0,0 +1,195 @@ +# Implementation Decision Log: Git Diff Panel + + + +## Trivial decisions + +- D-9: No `@boocode/contracts` change — refresh is a client-side `sessionEvents` event, not a WS frame, so the contracts package (`packages/contracts/`) is not touched and not rebuilt. — Referenced in plan: Implementation Approach → External Interfaces, Operational Readiness. +- D-10: No DB schema change, no migration, no new env var — the panel reads git state at request time and writes the project repo directly; nothing is persisted in Postgres. — Referenced in plan: Implementation Approach → Data Model and Persistence, Operational Readiness. +- D-11: Per-file expand/collapse state is local React state inside `GitDiffView`, not a shared hook — one consumer exists. — Referenced in plan: Implementation Approach → Architecture and Integration Points, Deferred (YAGNI). +- D-12: `autoSelectMode` and `canCommit` are inline pure helpers inside `git_diff.ts` (unit-tested), not separate modules. — Referenced in plan: Implementation Approach → Runtime Behavior, Testing Strategy. + +## Full decisions + +### D-1: Both git read AND git write live in apps/server + +- **Question:** Which service owns the new git read and git write operations — apps/server (BooChat, Docker), apps/coder (BooCoder, host service), or a read-in-server / write-in-coder split? +- **Decision:** Both the read route and the write routes (stage / unstage / commit / discard) live in **apps/server**. New `apps/server/src/services/git_diff.ts` holds the read logic and pure helpers plus the git-write helpers; new routes are added beside `GET /api/projects/:id/git` in `apps/server/src/routes/projects.ts`. No apps/coder changes. Single deploy surface: `docker compose up --build -d boocode`. +- **Rationale:** Writing in apps/server reuses the existing safe argv `runGit` pattern and the proven `project_bootstrap.ts` git-write precedent, and avoids a cross-service `/api/coder/*` proxy hop per write, a second deploy surface (coder restart), and making the host coder service a dependency for a plain BooChat-session commit. The "read-only" posture that motivated the split governs the **assistant's tool surface**, not the container filesystem or the user's own UI actions. +- **Evidence:** + - `docker-compose.yml:16` mounts `/opt:/opt` **read-write** into the boocode container — apps/server can already write project paths (refutes the "apps/server is read-only by posture" premise). + - `apps/server/src/services/project_bootstrap.ts` already runs git writes from apps/server: `git init` + commits via safe `execFile` with `-c user.name` / `-c user.email` (`GIT_USER_NAME='indifferentketchup'`, `GIT_USER_EMAIL='samkintop@gmail.com'` at :38-39, applied :122-123) — git-write from the server is an existing, proven pattern. + - `apps/server/src/services/git_meta.ts:30` has the safe `runGit` (discrete-argv `execFile`, no shell); `apps/coder/src/services/worktrees.ts` has only the **unsafe** `hostExec(shell string)` — writing in coder would require building a new safe wrapper there. + - Spec D8 ([decision-log.md](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=` beside `GET /api/projects/:id/git`. The route delegates to `apps/server/src/services/git_diff.ts`, which exposes pure helpers written test-first before wiring: `parseNameStatus` (porcelain → file list with change type), `splitDiffByFile` (unified diff text → per-file segments), `resolveCommittedBase` (base resolution, see D-6), `autoSelectMode`, the binary/large `classify`, and `detectInProgress` (in-progress git-state detection, see D-7). The route composes these over `runGit` argv calls. +- **Rationale:** The pure-helper-then-wire pattern is the project's established TDD precedent and keeps the parse/mode/base/classify logic unit-testable without spawning git. Slotting beside the existing git-meta route reuses its derivation and request shape. +- **Evidence:** `apps/server/src/services/git_meta.ts:44` `getGitMeta` + `apps/server/src/routes/projects.ts:426` `GET /api/projects/:id/git` (the read precedent the new route slots beside). Pure-helper TDD precedent: `apps/server/src/services/backends/turn-guard.ts`, `lifecycle-decisions.ts`, `mistake-tracker.ts` (pure module + unit test, then wire). test-engineer (T1–T12). +- **Rejected alternatives:** + - Inline all parse/mode/base logic in the route handler — rejected because it cannot be unit-tested without spawning git and breaks the project's pure-helper precedent. + - A separate module per helper — rejected (see D-12); the helpers are small and cohesive in one tested module. +- **Specialist owner:** `software-architect` +- **Revisit criterion:** If `git_diff.ts` grows past a single cohesive module (e.g. distinct read vs write concerns each exceed a file), split along that seam. +- **Driven by rounds:** R1 +- **Dependent decisions:** D-6, D-7 +- **Referenced in plan:** Implementation Approach → Architecture and Integration Points, Implementation Approach → Runtime Behavior, Decomposition and Sequencing, Testing Strategy + +### D-3: Write ops via argv-safe `runGit`/`execFile` with `--` separators + +- **Question:** How are the write operations (stage / unstage / commit / discard) invoked safely? +- **Decision:** All write ops use the discrete-argv `runGit`/`execFile` pattern with explicit `--` separators between options and user-supplied paths, plus a flag-injection guard (reject path arguments beginning with `-`). They **never** use `hostExec(shell string)` or any shell-interpolated invocation. User-supplied text (commit message, file targets) is always passed as discrete argv, never built into a command string. +- **Rationale:** Discrete argv with `--` separators closes the command-injection and flag-injection vectors that a shell string opens (F2). The server already has the safe pattern; the coder's shell pattern is exactly what the spec security posture warns against. +- **Evidence:** `apps/server/src/services/git_meta.ts:30` `runGit` (safe discrete-argv `execFile`) is the bar. `apps/coder/src/services/worktrees.ts` `hostExec(shell)` + `shellEscape` is the anti-pattern F2 flags. Spec D11/D12 ([decision-log.md](decision-log.md#d11--all-git-operations-scoped-to-the-project-repository-path)), F2 ([team-findings.md](team-findings.md)). +- **Rejected alternatives:** + - Reuse the coder `hostExec(shell)` + `shellEscape` pattern — rejected because shell interpolation lets user-supplied content (commit message, filenames with special characters) be interpreted as flags or shell syntax; the argv pattern eliminates the class. +- **Specialist owner:** `adversarial-security-analyst` +- **Revisit criterion:** If a git operation genuinely cannot be expressed in discrete argv (none in the v1 set qualifies), reopen with a documented justification. +- **Driven by rounds:** R1 +- **Dependent decisions:** D-4 +- **Referenced in plan:** Implementation Approach → Architecture and Integration Points, Security Posture, Decomposition and Sequencing + +### D-4: Path validation — repo-relative `pathGuard`, reject escape and repo-root discard + +- **Question:** How are per-file path arguments validated before a write? +- **Decision:** Every per-file argument is validated by a `pathGuard(repoRoot, file)` that resolves the path and rejects it if it is absolute, contains `..` traversal, or resolves (including via symlink) outside the repository root. Additionally, discarding the repository root itself (`.`) is rejected. The repository root is derived server-side from the session's project record, never from the request. +- **Rationale:** Per-file arguments can escape the repo root via `../` traversal or a symlink even when the root is correct, so each argument needs independent validation (F2). Rejecting a repo-root discard prevents a single confirmation from wiping the whole working tree. +- **Evidence:** `apps/server/src/services/path_guard.ts` `resolveProjectRoot(project.path)` — derives + scopes project paths from the DB project row, never from the request (the F2 precedent). Spec D11/D12, F2 ([team-findings.md](team-findings.md)). Integration-test precedent: `path_guard.test.ts`. +- **Rejected alternatives:** + - Validate only the repository root, not per-file arguments — rejected (F2): a per-file `../` or symlink argument escapes a correctly-scoped root. + - Accept a caller-supplied repository path — rejected: needless write surface, no use case; root is server-derived. +- **Specialist owner:** `adversarial-security-analyst` +- **Revisit criterion:** If a legitimate write target outside the resolved root ever appears (none in scope), reopen. +- **Driven by rounds:** R1 +- **Dependent decisions:** — +- **Referenced in plan:** Security Posture, Decomposition and Sequencing, Testing Strategy + +### D-5: Commit identity server-derived; request schema `.strict()` with no author fields + +- **Question:** Where does the commit author/committer identity come from, and what can the request set? +- **Decision:** Commit identity is server-derived via `-c user.name=… -c user.email=…` read from the repository's git config at commit time, falling back to the `project_bootstrap.ts` constants (`indifferentketchup` / `samkintop@gmail.com`) when git config yields nothing. The commit request schema is Zod `.strict()` and carries `{message, files?}` only — no `author`, `email`, `date`, or any identity field; unknown keys are rejected. +- **Rationale:** A server-derived identity passed via `-c` flags makes authorship un-spoofable from the request body (F3). `.strict()` ensures an attacker cannot smuggle an identity field. The `project_bootstrap.ts` `-c` precedent already establishes this exact mechanism for server-side commits. +- **Evidence:** `apps/server/src/services/project_bootstrap.ts:38-39,122-123` (server-side commit with `-c user.name`/`-c user.email`). Spec D6/D12, F3 ([team-findings.md](team-findings.md)). Zod is the project's request-validation library (apps/server CLAUDE.md). +- **Rejected alternatives:** + - Request-supplied commit identity — rejected (F3): allows authorship spoofing; no legitimate use in a single-user app. + - Hardcode the coder's `boocoder@local` identity — rejected: a user commit is not a coder commit; git config is the correct source for the human user, with the bootstrap constants as the fallback. + - A non-strict request schema — rejected: an unknown `author`-style key could slip through and influence the commit. +- **Specialist owner:** `adversarial-security-analyst` +- **Revisit criterion:** If a multi-identity requirement appears (e.g. co-author selection), reopen with a server-validated identity source, never a free-form request field. +- **Driven by rounds:** R1 +- **Dependent decisions:** — +- **Referenced in plan:** Implementation Approach → Architecture and Integration Points, Security Posture, Decomposition and Sequencing, Testing Strategy + +### D-6: Committed-mode base resolution via `@{upstream}` → `origin/HEAD` → null + +- **Question:** How does Committed mode resolve the base it compares the current branch against? +- **Decision:** The pure helper `resolveCommittedBase` resolves the base in order: (1) `git rev-parse --abbrev-ref --symbolic-full-name @{upstream}` (the tracking branch; non-zero exit if none), then (2) `git rev-parse --abbrev-ref origin/HEAD` (the default branch), then (3) null. On null the panel falls back to Uncommitted and labels it as a fallback. The resolved base is returned in the read response so the header can label "Git — branch vs <base>". +- **Rationale:** Tracking-branch-first matches git's own upstream model and is the right base for a contributor whose tracking branch is a fork or PR target; `origin/HEAD` is the right fallback for the default branch; a labeled Uncommitted fallback is more useful than an error or a silent swap (F11). +- **Evidence:** `apps/server/src/services/git_meta.ts:58` already uses `HEAD...@{upstream}` (the upstream-resolution precedent). Discovery notes §6 (`rev-parse --abbrev-ref origin/HEAD` for the default-branch fallback). Spec D13, F11 ([team-findings.md](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 diff --git a/docs/features/git-diff-panel/artifacts/implementation-iteration-history.md b/docs/features/git-diff-panel/artifacts/implementation-iteration-history.md new file mode 100644 index 0000000..c87002a --- /dev/null +++ b/docs/features/git-diff-panel/artifacts/implementation-iteration-history.md @@ -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 (D1–D18) + team-findings (F1–F21), 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 T1–T12: 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 (D1–D18); 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. diff --git a/docs/features/git-diff-panel/artifacts/synthesis-input.md b/docs/features/git-diff-panel/artifacts/synthesis-input.md new file mode 100644 index 0000000..4b96c07 --- /dev/null +++ b/docs/features/git-diff-panel/artifacts/synthesis-input.md @@ -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. diff --git a/docs/features/git-diff-panel/feature-implementation-plan.md b/docs/features/git-diff-panel/feature-implementation-plan.md new file mode 100644 index 0000000..a00c342 --- /dev/null +++ b/docs/features/git-diff-panel/feature-implementation-plan.md @@ -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:** D1–D18 +- **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 | T1–T12: 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=` ([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 T1–T7 | +| 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 T9–T12 | +| 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 1–4 (read + display); Phase 2 = units 5–6 (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` (T1–T12). 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 (T1–T7, pure helpers) + integration (T8–T12, 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 T1–T12 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 T1–T12; 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 3–4 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; ~40–50 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 1–4) first, then Phase 2 (units 5–6).