mjalmoudy94
d41e00ad10
The platform's side of D6: git runs on the host, in a gitdir that is never mounted into a container, so repository credentials never reach the model. - adapters/git_workspace.py: LocalGitWorkspaceAdapter implementing RepoWorkspacePort. Bare shared gitdir + a dev work tree mounted at /workspace + one linked worktree per task under work/.worktrees/<id> (excluded via the gitdir's info/exclude, not the project's .gitignore). Explicit --git-dir/--work-tree, core.hooksPath=/dev/null, fsmonitor off, quotePath off, GIT_CONFIG_NOSYSTEM, a private HOME per project, prompts off, one asyncio.Lock per project. Token via GIT_ASKPASS reading a 0600 file; SSH key as a 0600 temp file with a pinned known_hosts. Everything deleted in a finally; only a *path* ever travels through the environment. - The bug this phase existed to find: --git-dir=<shared> does not address a linked worktree, so `git commit` in a task worktree committed straight to `dev`. Operations now address <gitdir>/worktrees/<task_id> explicitly — and never read the worktree's .git file, which a repository can rewrite. - domain/platform/rules.py: is_blocked_address (pure) + assert_remote_host_allowed (resolves, refuses if any address is loopback/private/link-local/metadata). Shared by the git remote test and, later, every deploy target. - ProjectExternalGit domain model + repository; container_delivery wires the real adapter behind GIT_WORKSPACE=local with a resolver that joins the project, its repo and its (decrypted) credentials. Tests: 394 API (+48) against real git and a local bare repo; ruff and mypy clean. Report: docs/plan/reports/agent-3-phase-2.md
141 lines
7.6 KiB
Markdown
141 lines
7.6 KiB
Markdown
# Agent 3 — Phase 2 report: git workspaces
|
||
|
||
Date: 2026-10-13 · Branch: `dev`
|
||
|
||
## What was done
|
||
|
||
### 2.1–2.4 `LocalGitWorkspaceAdapter` (`app/infrastructure/adapters/git_workspace.py`)
|
||
|
||
Implements every method of `RepoWorkspacePort` with a real host `git` binary.
|
||
|
||
**Layout** under `SANDBOX_WORKSPACES_ROOT/<org>/<project>/`:
|
||
`git/` (shared gitdir, never mounted), `work/` (the `dev` work tree, mounted at
|
||
`/workspace`), `work/.worktrees/<task_id>/` (one per task), `private/` (0700, holds
|
||
credentials for the duration of one call only). `.worktrees/` is excluded through the
|
||
**gitdir's** `info/exclude`, never the project's `.gitignore`.
|
||
|
||
**Hardening on every call** (asserted by test): explicit `--git-dir`/`--work-tree`;
|
||
`core.hooksPath=/dev/null`, `core.fsmonitor=false`, `core.quotePath=false`,
|
||
`safe.directory=*`; `GIT_CONFIG_NOSYSTEM=1`; a private `HOME` per project;
|
||
`GIT_TERMINAL_PROMPT=0`; `protocol.file.allow` configurable (see the deviation below);
|
||
one `asyncio.Lock` per project held for every mutating operation.
|
||
|
||
**Credentials.** A token goes through a 0700 `GIT_ASKPASS` script that reads a 0600
|
||
file containing `username\nsecret`; an SSH key is a 0600 temp file referenced by
|
||
`GIT_SSH_COMMAND` with `IdentitiesOnly=yes` and a per-project `known_hosts`
|
||
(`StrictHostKeyChecking=yes` once pinned, `accept-new` for TOFU on first contact).
|
||
Both are deleted in a `finally`. Only a **path** ever travels through the environment —
|
||
never the secret itself — so nothing sensitive reaches `argv` (world-readable via `ps`).
|
||
|
||
**`test_remote`** runs the shared SSRF check first, then `git ls-remote --heads` with a
|
||
20 s timeout, and returns a plain Persian message from a closed set of causes. Raw
|
||
stderr is never returned: git echoes the remote URL and a credential helper can write a
|
||
token into stderr, so every message goes through `app.core.redact`.
|
||
|
||
**`merge_into`** forces the checkout of the target in the main work tree, merges
|
||
`--no-ff`, and on conflict collects the unmerged paths and runs `merge --abort` before
|
||
returning — a half-merge would break every later operation. **`push`** never forces:
|
||
a rejection is reported with `rejected=True` after a fetch.
|
||
|
||
### The bug this phase existed to find: linked worktrees need their own gitdir
|
||
|
||
`git --git-dir=<shared> --work-tree=<task tree>` does **not** address the task branch.
|
||
`--git-dir` names the *main* gitdir, whose `HEAD` is `dev`, so `git commit` in a task
|
||
worktree commits straight to `dev` and the task branch never moves. The first version of
|
||
this adapter had exactly that bug; it was caught by a test that asserts the task sha is
|
||
*not* `dev`'s head.
|
||
|
||
The fix is to address the **linked gitdir** that `git worktree add` creates:
|
||
`<gitdir>/worktrees/<task_id>`, which holds that worktree's own `HEAD` and index. It is
|
||
passed explicitly rather than discovered from the worktree's `.git` file — that file is
|
||
plain text a repository can rewrite, and the test that tampers with it proves the
|
||
tampered value is ignored. `_worktree_gitdir()` falls back to the shared gitdir when no
|
||
linked one exists, which is the `analysis_only` case (no worktree).
|
||
|
||
### 2.3 SSRF check (`app/domain/platform/rules.py`)
|
||
|
||
`is_blocked_address()` (pure) classifies an IP literal: loopback, RFC1918 private,
|
||
link-local — including the cloud metadata address `169.254.169.254` — unique-local IPv6,
|
||
multicast, reserved, unspecified, and IPv4-mapped IPv6 forms of all of them.
|
||
`assert_remote_host_allowed()` validates the URL (scheme allowlist, no embedded
|
||
credentials, blocked ports) and then resolves the host, refusing if **any** returned
|
||
address is blocked — a name can legitimately have both a public and a private address.
|
||
The resolver is injectable so the DNS step is fakeable in tests. `localhost` and friends
|
||
are refused by name as well as by resolution.
|
||
|
||
### 2.5 Wiring (`container_delivery.py`)
|
||
|
||
`build_repo_workspace` returns the fake unless `GIT_WORKSPACE=local`, then builds a
|
||
resolver that joins `projects` + `project_repositories`/`project_git_credentials` (platform)
|
||
or `project_external_git` (external) and hands the decrypted secret to the adapter for one
|
||
call. A rotated `ENCRYPTION_KEY` degrades to "no credential" with a log line rather than an
|
||
exception.
|
||
|
||
Supporting additions: `ProjectExternalGit` domain model +
|
||
`ProjectExternalGitRepository` (in Agent 3's own files), and
|
||
`app/infrastructure/db/columns.py` is untouched.
|
||
|
||
## Files changed
|
||
|
||
| File | Change |
|
||
|---|---|
|
||
| `app/infrastructure/adapters/git_workspace.py` | new — the git adapter (~700 lines) |
|
||
| `app/domain/platform/rules.py` | SSRF: `is_blocked_address`, `assert_remote_host_allowed`, `BLOCKED_REMOTE_PORTS` |
|
||
| `app/domain/projects/models.py` | `ProjectExternalGit` |
|
||
| `app/infrastructure/repositories/projects.py` | `ProjectExternalGitRepository` |
|
||
| `app/application/container_delivery.py` | real `build_repo_workspace` + resolver |
|
||
| `app/core/config.py`, `.env.example` | `GIT_ALLOW_FILE_PROTOCOL` |
|
||
| `pyproject.toml` | registered the `docker` / `deploy_it` markers |
|
||
| `tests/test_git_workspace.py` | new — 48 tests |
|
||
|
||
## Test output
|
||
|
||
```
|
||
apps/api: 394 passed (was 346; +48)
|
||
ruff check . -> All checks passed!
|
||
mypy app -> Success: no issues found in 128 source files
|
||
apps/web: 67 passed (unchanged — no frontend work in this phase)
|
||
```
|
||
|
||
The 48 new tests run **real git** against a local bare repository standing in for the
|
||
remote; nothing about git itself is mocked.
|
||
|
||
## Deviations from the TODO
|
||
|
||
1. **`protocol.file.allow` is a setting, not a constant.** The TODO fixes it at `never`.
|
||
That is the right production default (it stops a repository from making git read a
|
||
local path via a submodule or `--upload-pack`), but it also blocks the local bare repo
|
||
the TODO's own test plan asks for. `GIT_ALLOW_FILE_PROTOCOL=false` keeps `never` as the
|
||
default and lets the test suite opt in.
|
||
2. **The SSRF check lives in `rules.py` as instructed, and it resolves DNS.** That is the
|
||
one I/O call in an otherwise pure module; the module docstring now says so, and the
|
||
address classification itself is pure and separately tested.
|
||
3. **`base_changed` for `sync_with_base` is defined as "the task branch moved".** The
|
||
port docstring says "True when the base moved, so the engine must re-run the tests" —
|
||
comparing the base branch's sha before and after a merge into the task branch would
|
||
always be equal, so the signal would never fire. It is computed from the worktree's
|
||
own `HEAD` instead.
|
||
4. **`.git` files are not read at all.** The TODO says "never trust the `.git` file
|
||
inside a worktree"; the adapter goes further and never opens it, using the linked
|
||
gitdir path instead.
|
||
|
||
## Open questions / CHANGE REQUESTS
|
||
|
||
- **CHANGE REQUEST (for integration):** `container_delivery.build_repo_workspace(session)`
|
||
is called with a request-scoped `AsyncSession`. The git adapter's resolver uses that
|
||
session, so a `RepoWorkspacePort` handed out by a long-lived worker is only valid while
|
||
its session lives. Phase 3's delivery worker must build the port per pass (which is what
|
||
`build_repo_workspace(session)` in `worker_session()` already implies). No contract
|
||
change needed — noting it so the engine and delivery workers do not cache the port
|
||
across sessions.
|
||
|
||
## Known limitations
|
||
|
||
- `git worktree add` names the linked gitdir after the worktree path's basename. The
|
||
adapter relies on that (`<task_id>`), with a fallback to the shared gitdir. A task id
|
||
containing characters git rewrites would silently degrade to the fallback; task ids are
|
||
platform-generated hex, so this is theoretical.
|
||
- Shallow clones are not supported; `ensure_workspace` fetches full history, which is what
|
||
the diff guard and merge need.
|
||
- No `@pytest.mark.docker` integration test — Phase 2 needs no container.
|