docs: add ADR 0026 git review flow

coding-agent/docs/adr/0026-remote-result-publication-v2-git-review-flow.md

@@ -0,0 +1,313 @@
+# ADR-0026: Harden remote result publication as the Git review flow
+
+**Status**: Proposed
+**Date**: 2026-05-15
+**Decision owner**: repository maintainer / current product owner
+
+## Context
+
+ADR-0024 changed the remote workspace product model from local snapshot
+overwrite to remote result publication. It added Git-backed workspace sources,
+session result, workspace diff, workspace patch, branch publication, GitHub PR
+publication, and result-first CLI output. ADR-0025 then made session/workspace
+metadata durable so publication refs and workspace lifecycle can be inspected
+after runtime shutdown and server restart.
+
+Dogfood of ADR-0024 and ADR-0025 confirmed the direction: a remote cloud agent
+should not make "download the result back into the local checkout" the normal
+success path. The remote workspace is the execution site. The result should be
+reviewed and applied through remote Git artifacts: diff, patch, branch, and PR.
+The local machine should normally consume the result later through ordinary Git
+operations such as reviewing a PR, checking out a branch, or pulling after merge.
+
+The current ADR-0024 implementation is a working baseline, but it still needs a
+production-quality review flow. In particular, review artifacts must faithfully
+represent what publication will commit, publication failures must not destroy
+the ability to inspect/retry results, and server-side Git clone/publish
+operations must fail closed before using untrusted transports or hosts.
+
+This ADR strengthens ADR-0024. It does not replace ADR-0024's API names or move
+remote operations into a new product shape.
+
+## Decision
+
+Adopt **Git Review Flow v2** for remote result publication:
+
+1. **Remote Git artifacts are the primary result path.** A completed remote
+   coding session should expose result, diff, patch, branch, and PR artifacts
+   from the remote workspace. Local archive download remains an explicit export
+   fallback.
+2. **Review artifacts and publication must be content-consistent.** The diff and
+   patch a user reviews must describe the same file set that branch publication
+   would commit.
+3. **Publication must be retry-safe and inspectable.** Validation failures,
+   credential failures, host allowlist failures, and provider API failures must
+   not leave the workspace in a state where diff/patch disappear without an
+   explicit publication state explaining what happened.
+4. **Server-side Git operations must be allowlisted before I/O.** Git source
+   clone and publication remotes must validate scheme, host, URL shape, and
+   credential policy before running `git clone`, `git fetch`, `git push`, or
+   provider API calls.
+5. **CLI remains a thin client.** The HTTP API is the product contract; CLI
+   commands only call the API, stream events, and display next actions.
+
+The default user story is:
+
+```text
+remote run against a clean Git-backed source
+→ inspect session result / diff / patch
+→ explicitly publish branch or PR
+→ review/merge through Git provider
+→ local checkout receives changes later through normal Git workflows
+```
+
+`remote download` and archive overwrite remain available for local-only repos,
+smoke tests, and operational recovery. They must be documented and displayed as
+fallback export commands, not as the default cloud-agent result application
+path.
+
+### Diff, patch, and publish consistency
+
+For Git-backed workspaces, diff and patch generation must include all changes
+that branch publication would commit:
+
+- tracked modifications;
+- deletions;
+- untracked added files;
+- binary file markers;
+- renames when Git can detect them.
+
+Implementations should not use plain `git diff HEAD --` as the final source for
+review artifacts because it omits untracked files. The preferred mechanism is a
+temporary index:
+
+1. copy or create an isolated `GIT_INDEX_FILE`;
+2. run `git add -A` against that temporary index;
+3. produce summary with `git diff --cached --name-status --numstat HEAD --`;
+4. produce patch with `git diff --cached --binary HEAD --`;
+5. leave the real workspace index and working tree untouched.
+
+Branch publication may still use the real index when committing, but its commit
+input must match the same "add all current workspace changes" semantics used by
+the reviewed diff/patch.
+
+### Publication mutation ordering
+
+Publication must perform all validations that can be done without mutating the
+workspace before any `git add`, `git commit`, branch checkout, or push:
+
+- requested mode is supported;
+- branch name is valid and allowed;
+- remote URL exists and is valid;
+- remote scheme and host are allowlisted;
+- remote URL contains no query/fragment credential material unless explicitly
+  supported;
+- publication is enabled;
+- Git author identity is configured;
+- required token environment variables exist and are non-empty;
+- PR provider configuration exists when strict PR publication is requested.
+
+If validation fails, the workspace must remain inspectable through result,
+diff, and patch APIs.
+
+If mutation succeeds locally but a later push or PR API call fails, the response
+must report explicit partial state, such as local branch name, local commit SHA,
+push status, and PR error. The system must not pretend nothing happened, and it
+must not turn the result into an empty diff without exposing the local
+publication state.
+
+### Git source clone safety
+
+Git-backed workspace creation must validate client-provided source URLs before
+server-side clone/fetch operations:
+
+- allow only configured schemes, initially `https`;
+- require a hostname;
+- require host membership in `[remote_sources.git].allowed_hosts`;
+- reject `file://`, `ssh://`, scp-like `git@host:repo`, relative paths, and
+  local filesystem paths unless a later ADR explicitly enables them;
+- reject username/password, query, and fragment in source URLs unless a later
+  ADR defines a redaction and credential policy.
+
+This is separate from publication remote validation. Source clone allowlists
+control where the server may read from; publication allowlists control where it
+may write to.
+
+### Publication response and durable metadata
+
+Publication responses and ADR-0025 result refs should be rich enough for humans,
+CI, Web UI, and future editor integrations:
+
+- session id and workspace id;
+- mode: `branch` or `pr`;
+- status: `published`, `partial`, or `failed`;
+- base ref and base SHA when known;
+- branch name;
+- commit SHA when a local commit exists;
+- pushed ref when push succeeds;
+- redacted remote URL without query or fragment;
+- PR URL when PR creation succeeds;
+- provider error summary when PR creation fails;
+- retry guidance when the operation is retryable.
+
+Cleaned workspace records may retain publication refs after workspace files are
+gone. APIs must clearly distinguish durable metadata from provider-local
+exports that require a still-existing workspace.
+
+### Layering decision
+
+ADR-0026 keeps Git review flow v2 in `coding_agent`.
+
+The implementation may shape result and publication metadata so it can later be
+backed by an `agentkit` session-result/artifact-ref contract, but this ADR does
+not migrate runtime storage, workspace retention, Git publication, or remote CLI
+behavior into `agentkit`.
+
+Future downshift candidates for a separate ADR include:
+
+- `SessionResult` / `TurnResult`;
+- `VerificationSummary` / `FailureSummary`;
+- `ArtifactRef` / `ResultRef`;
+- result reducers over `agentkit.tape.extract.TurnTrace`;
+- provider-neutral metadata-only artifact store protocols.
+
+The following must remain in `coding_agent` for this ADR:
+
+- Git-backed workspace source;
+- Git clone/diff/patch/branch publication execution;
+- GitHub PR integration;
+- Docker workspace provider behavior;
+- remote workspace retention, `provider_instance_id`, pin/unpin, and GC;
+- HTTP remote operations and CLI commands.
+
+This avoids turning Git Review Flow v2 into an `agentkit` refactor. A later
+ADR-0027 may define `agentkit` session result and artifact reference models.
+
+## Non-goals
+
+- Do not implement live sync or background bidirectional sync.
+- Do not automatically apply remote changes to the local checkout.
+- Do not automatically merge remote changes into protected branches.
+- Do not resolve Git conflicts.
+- Do not add a broad Git provider abstraction before a second provider is real.
+- Do not move Docker workspace, Git publication, workspace retention, or remote
+  CLI behavior into `agentkit`.
+- Do not store workspace files, Git checkouts, or archive blobs in PostgreSQL.
+- Do not remove archive download fallback.
+
+## Consequences
+
+- The product boundary remains cloud-agent-like: review remote results remotely,
+  then consume them through Git.
+- Git remains responsible for review, merge, rollback, CI, and local checkout
+  update; coding-agent avoids inventing a competing sync system.
+- Diff/patch generation becomes slightly more complex because it must model
+  "what publish would commit" without mutating the workspace.
+- Publication code must distinguish preflight validation failures from partial
+  publication failures.
+- Future `agentkit` extraction stays possible because result refs and artifact
+  metadata are shaped as provider-neutral references, but that migration is not
+  on the critical path for Git Review Flow v2.
+
+## Implementation Plan
+
+### PR 1: ADR and tests for review/publish consistency
+
+- Add this ADR.
+- Add regression tests in
+  `tests/coding_agent/environment/test_docker_workspace_provider.py` proving:
+  - untracked text files appear in workspace diff;
+  - untracked binary files appear as added binary files;
+  - patch export contains new text file content;
+  - diff/patch generation does not mutate the real Git index.
+
+### PR 2: Temporary-index diff and patch generation
+
+- Update `src/coding_agent/environment/docker_workspace_provider.py` so
+  Git-backed workspace diff and patch use a temporary index with `git add -A`.
+- Preserve existing provider method signatures in
+  `src/coding_agent/environment/workspace_provider.py`.
+- Keep snapshot-only behavior explicit; do not pretend archive export is a
+  patch.
+
+### PR 3: Git source clone URL allowlist
+
+- Add pre-clone validation for `workspace_source.kind = "git"` in Docker
+  workspace provisioning.
+- Use `[remote_sources.git].allowed_hosts` as the source clone host allowlist.
+- Reject local paths, `file://`, SSH URLs, scp-like syntax, query strings,
+  fragments, and username/password in source URLs.
+- Add tests proving disallowed source URLs fail before `subprocess.run`.
+
+### PR 4: Publication preflight and retry-safe mutation
+
+- Move branch name, remote URL, host allowlist, author identity, and token env
+  checks before `git add`, `git commit`, branch checkout, or push.
+- Add tests proving unsafe remote hosts and missing token env do not mutate the
+  workspace into an empty diff.
+- If local commit succeeds but push/PR fails, return explicit partial
+  publication metadata instead of hiding the local state.
+
+### PR 5: Response metadata and durable refs
+
+- Extend `src/coding_agent/ui/schemas.py` publication response models with
+  publication status, base ref/SHA when known, redacted remote URL, partial
+  state, and retry guidance.
+- Persist the richer publication metadata through ADR-0025 workspace result refs.
+- Ensure redacted remote URLs drop query and fragment.
+
+### PR 6: CLI and documentation polish
+
+- Update `src/coding_agent/__main__.py` output so remote run and publish
+  messages reinforce Git review flow as the main path and archive download as
+  fallback.
+- Update `docs/remote-sandbox-production.md` with Git Review Flow v2,
+  preflight/partial publication semantics, and local download fallback wording.
+
+## Alternatives Rejected
+
+- **Make local safe sync the next milestone** — rejected because the product
+  direction is remote review and Git consumption, not local checkout mutation.
+- **Move result/publication into `agentkit` immediately** — rejected because it
+  would mix a product hardening milestone with a runtime abstraction refactor.
+- **Use plain `git diff HEAD --` for review artifacts** — rejected because it
+  omits untracked files and can make review output differ from publish output.
+- **Commit before validating publication config** — rejected because failed
+  preflight should not mutate workspace state or erase inspectable diff/patch.
+- **Allow arbitrary Git source URLs** — rejected because server-side clone is
+  network and filesystem access initiated by user input.
+- **Remove archive download** — rejected because snapshot fallback, local-only
+  repos, dogfood smoke, and recovery workflows still need explicit export.
+
+## Acceptance Criteria
+
+- [ ] `test_git_workspace_diff_includes_untracked_text_file`
+- [ ] `test_git_workspace_patch_includes_untracked_text_file`
+- [ ] `test_git_workspace_diff_marks_untracked_binary_file`
+- [ ] `test_git_workspace_diff_and_patch_do_not_mutate_index`
+- [ ] `test_git_workspace_source_rejects_disallowed_clone_host_before_subprocess`
+- [ ] `test_git_workspace_source_rejects_file_and_ssh_clone_urls`
+- [ ] `test_publish_preflight_missing_token_does_not_commit`
+- [ ] `test_publish_preflight_disallowed_remote_host_does_not_commit`
+- [ ] `test_publish_push_failure_returns_partial_local_commit_state`
+- [ ] `test_publish_response_redacts_remote_query_and_fragment`
+- [ ] `uv run pytest tests/ui/test_http_server.py tests/cli/test_remote_client.py tests/coding_agent/environment/test_docker_workspace_provider.py -k "git_workspace or workspace_diff or workspace_patch or publish" -v`
+- [ ] `uv run basedpyright --level error src/coding_agent/ui src/coding_agent/environment src/coding_agent/remote/client.py src/coding_agent/__main__.py`
+- [ ] `uv run ruff format --check src/coding_agent/ui src/coding_agent/environment src/coding_agent/remote/client.py src/coding_agent/__main__.py tests/ui/test_http_server.py tests/cli/test_remote_client.py tests/coding_agent/environment/test_docker_workspace_provider.py`
+
+## References
+
+- `docs/adr/0021-remote-session-and-workspace-operations-api.md`
+- `docs/adr/0024-remote-result-publication.md`
+- `docs/adr/0025-durable-remote-session-and-workspace-retention.md`
+- `docs/remote-sandbox-production.md`
+- `src/agentkit/tape/extract.py`
+- `src/coding_agent/ui/http_server.py`
+- `src/coding_agent/ui/schemas.py`
+- `src/coding_agent/environment/workspace_provider.py`
+- `src/coding_agent/environment/docker_workspace_provider.py`
+- `src/coding_agent/remote/client.py`
+- `src/coding_agent/__main__.py`
+- `tests/ui/test_http_server.py`
+- `tests/cli/test_remote_client.py`
+- `tests/coding_agent/environment/test_docker_workspace_provider.py`