boocode/docs/features/git-diff-panel/artifacts/team-findings.md

Team findings — Git diff panel

Review-team findings on feature-specification.md and resolutions. Team (medium): junior-developer, user-experience-designer, adversarial-security-analyst, on-call-engineer. Dispatched 2026-06-02. All findings resolved by evidence/convention/design-judgment; none required new user input beyond the three foundational answers already given. Shared F# counter. All resolutions applied to spec and decision log 2026-06-02.

Major findings

F1 — Assistant could drive the new git-write endpoints via a rendered artifact (security)

• Raised by: adversarial-security-analyst (D8). Highest priority.
• Concern: D8 says the read-only-assistant rule covers the AI's tools, not the user's UI. But the AI can emit HTML artifacts; if an artifact could POST to the new stage/commit/discard endpoints using the user's Authelia cookie, the assistant would gain a write path it is forbidden.
• Resolved by: evidence. The HTML artifact iframe is sandboxed with connect-src 'none' (per BOOCHAT.md output-format section — fetch/WebSocket do not work in artifacts), so an artifact cannot reach the endpoints. Spec gains an explicit commitment: the git-write actions are user-initiated UI actions only, are never registered as an assistant tool, and the artifact sandbox prevents an artifact from invoking them.
• Affected decisions: D8 (expanded), D12 (new).
• Affected tech-notes: —
• Changed in spec: Actors and triggers, Coordinations.

F2 — D11 scoping needs explicit derivation + argument-safety commitments (security)

• Raised by: adversarial-security-analyst (D11).
• Concern: "confined to the project path" is a destination constraint that doesn't say the path is derived server-side, that per-file arguments are validated to resolve inside the repo, or that the commit message and file arguments are passed as discrete arguments (not shell-interpolated).
• Resolved by: evidence (the existing project path-scoping guard derives roots from the project record, never from the request). Spec/D11 gain three commitments: repository root derived server-side from the session's project; per-file arguments validated as repo-relative and rejected if they escape; commit message and file arguments passed as discrete arguments, never built into a shell string.
• Affected decisions: D11 (expanded), D12 (new).
• Affected tech-notes: —
• Changed in spec: Coordinations.

F3 — Commit author identity must be server-derived, not request-supplied (security + clarity)

• Raised by: adversarial-security-analyst, junior-developer (JD-004; Open items).
• Concern: identity was deferred entirely to implementation; an unauthenticated local request could set an arbitrary author, and the codebase already hardcodes differing identities elsewhere.
• Resolved by: evidence + commitment. Spec commits: a panel commit's author/committer is derived from a server-side source (host git config or a configured value); the request body cannot set or influence it. The exact source is left to plan-implementation.
• Affected decisions: D6 (expanded), D12 (new).
• Affected tech-notes: —
• Changed in spec: Primary flow, Open items (closed).

F4 — Discard semantics: own "irrecoverable", distinguish tracked vs untracked, separate the affordance

• Raised by: adversarial-security-analyst (D7), on-call-engineer (OCE-002), user-experience-designer (UX-005).
• Concern: the spec calls discard "irrecoverable" but defers the git mechanic, creating a tension; a tracked-file revert and an untracked-file permanent delete are different losses; and Discard sitting next to Stage at equal weight invites accidental taps on mobile.
• Resolved by: design-judgment. Discard hard-deletes (own the word "irrecoverable"). The confirmation uses two variants — "Discard changes to X?" (tracked, reverts to committed content) vs "Delete X? It has never been committed and cannot be recovered" (untracked). The Discard affordance is separated from Stage/Unstage (an overflow/secondary placement), not an equal-weight sibling button.
• Affected decisions: D7 (expanded), D15 (new).
• Affected tech-notes: —
• Changed in spec: Alternate flows and states, User interactions.

F5 — Index-lock contention with concurrent agent turns is unnamed (resilience)

• Raised by: on-call-engineer (OCE-001).
• Resolved by: evidence. Spec names the case: when a write fails because the repository is busy (its index is locked by another process, e.g. a concurrent agent turn), the panel communicates "the repository is busy, try again" rather than a raw git error.
• Affected decisions: —
• Affected tech-notes: —
• Changed in spec: Edge cases and failure modes.

F6 — "Leave state unchanged" is unenforceable for partial failures (resilience wording)

• Raised by: on-call-engineer (OCE-002).
• Resolved by: reword. "Leaves the repository state unchanged" → "leaves the repository as close to its pre-action state as the git layer allows, and the list refreshes to reflect the repository's true state"; an untracked-directory discard that fails partway may leave a partially-removed tree, surfaced on refresh.
• Affected decisions: —
• Affected tech-notes: —
• Changed in spec: Edge cases and failure modes.

F7 — No deadline on a hanging git read (resilience)

• Raised by: on-call-engineer (OCE-003).
• Resolved by: add commitment. If a git read does not complete within a deadline, the panel leaves the loading state, shows an error, and offers Refresh (distinct from the large-result cap in D5).
• Affected decisions: D5 (expanded).
• Affected tech-notes: —
• Changed in spec: Edge cases and failure modes.

F8 — Concurrent refresh triggers have no coalescence commitment (resilience)

• Raised by: on-call-engineer (OCE-004).
• Resolved by: add commitment. Concurrent refresh triggers are coalesced — a refresh already in flight absorbs later triggers instead of spawning a second concurrent read; the panel settles to a single final snapshot.
• Affected decisions: D10 (expanded).
• Affected tech-notes: —
• Changed in spec: Alternate flows and states.

F9 — In-progress git states (merge/rebase/cherry-pick/bisect) make writes fail opaquely (resilience)

• Raised by: on-call-engineer (OCE-005).
• Resolved by: add commitment. When the repository is mid-operation (merge, rebase, cherry-pick, or bisect), the panel disables its write affordances and shows the repository's state rather than letting stage/commit/ discard fail with raw git errors.
• Affected decisions: —
• Affected tech-notes: —
• Changed in spec: Edge cases and failure modes.

F10 — The Changes tab is undiscoverable and collides with "Pending Changes" naming (UX)

• Raised by: user-experience-designer (UX-001, UX-008), junior-developer (JD-001).
• Resolved by: design-judgment. (a) The new tab is named Git (Files / Git), distinct from the existing "Pending Changes" panel; the existing panel is NOT renamed (out of scope). (b) An ambient indicator on the file-panel toggle/header signals the repository is dirty (derived from the refresh data already gathered), so the tab is findable. (c) When the Git view is empty but the session has unapplied pending changes, the empty state hints that those live in the pending-changes panel.
• Affected decisions: D16 (new tab name), D17 (new dirty indicator + empty-state hint).
• Affected tech-notes: —
• Changed in spec: Actors and triggers, Alternate flows and states, User interactions, Coordinations, Out of scope.

F11 — Committed-mode base is undefined and unlabeled (UX + correctness)

• Raised by: user-experience-designer (UX-002), junior-developer (JD-002).
• Resolved by: decision. Committed mode compares the current branch against its base — the upstream tracking branch when set, otherwise the repository's default branch (main/master). The view labels the base it used ("Git — branch vs <base>"). When no base resolves, the panel shows uncommitted changes and labels the mode as falling back, rather than silently swapping.
• Affected decisions: D2 (expanded), D13 (new).
• Affected tech-notes: —
• Changed in spec: Primary flow, Edge cases and failure modes, User interactions.

F12 — Auto-mode selection silently dislocates the view on refresh (UX)

• Raised by: user-experience-designer (UX-003).
• Resolved by: design-judgment. Auto mode-selection applies on first open only; once the user picks a mode it is pinned for the session and refreshes do not override it. A refresh that would change the mode (e.g. the tree went clean) briefly notes the change rather than swapping silently.
• Affected decisions: D3 (expanded), D14 (new).
• Affected tech-notes: —
• Changed in spec: Primary flow, Alternate flows and states.

F13 — Staged vs unstaged distinction must not be color-only (UX/accessibility)

• Raised by: user-experience-designer (UX-004).
• Resolved by: add commitment. Staged and unstaged files are distinguished by more than color (a label/icon, and grouping into staged/unstaged sections); each stage/unstage control carries an accessible name that includes the file path.
• Affected decisions: —
• Affected tech-notes: —
• Changed in spec: Primary flow, User interactions.

F14 — Discard's availability across modes is unspecified (clarity)

• Raised by: junior-developer (JD-003).
• Resolved by: decision. Stage, unstage, commit, and discard are available only in Uncommitted mode; Committed mode is read-only review (no per-file revert of committed history in v1).
• Affected decisions: D6 (scoped), D15 (new).
• Affected tech-notes: —
• Changed in spec: User interactions, Alternate flows and states.

F15 — Mobile fit + tap-target convention for the new tab and controls (UX)

• Raised by: user-experience-designer (UX-009), junior-developer (JD-008).
• Resolved by: convention. All interactive controls in the panel follow the app's existing mobile tap-target minimum; the Files / Git tab strip and header fit one line without horizontal scroll or wrapping (the project's toolbar-fit rule), condensing existing header elements if needed.
• Affected decisions: D18 (new).
• Affected tech-notes: —
• Changed in spec: User interactions.

Minor edits

• F16: Successful commit shows a brief, non-blocking success confirmation (not just files disappearing) — user-experience-designer (UX-006) — Affected decisions: — Affected tech-notes: — Changed in spec: Primary flow.
• F17: Error placement — commit-area errors appear by the commit control, per-file action errors appear in the affected file row — user-experience-designer (UX-007) — Affected decisions: — Affected tech-notes: — Changed in spec: Edge cases and failure modes.
• F18: Which service runs the git read vs write operations (read-only server vs write-capable host service) is an architecture/module-boundary decision routed to plan-implementation; the spec stays behavioral ("the system performs…") — junior-developer (JD-005) — Affected decisions: — Affected tech-notes: — Changed in spec: — (plan-implementation input).
• F19: The git-write surface is the larger half of v1; implementation should sequence diff-display before the write actions even within v1 — junior-developer (JD-007) — Affected decisions: — Affected tech-notes: — Changed in spec: — (plan-implementation note).
• F20: The refresh-on-pending-apply and refresh-on-turn-complete triggers require an event/frame wiring that must follow the project's WS-frame / sessionEvents parity steps — junior-developer (JD-009) — Affected decisions: — Affected tech-notes: — Changed in spec: — (plan-implementation note).
• F21: D8's commit-button-in-a-read-only-session affordance needs no extra label; the "Git" tab name and dirty indicator make it clearly the user's own git surface, not assistant output — junior-developer (JD-006) — Affected decisions: D8 confirmed, no extra label needed — Affected tech-notes: — Changed in spec: — (D8 confirmed).