84da774212
Three rounds of adversarial review (Chief Architect / Principal Dev /
Senior Test Engineer) on commits 3ca794be7..db12f45ac surfaced ~35
issues. This commit addresses 25+ across criticals, highs, and
mediums, and adds 40 new tests covering the changes plus key gaps the
review identified.
CRITICALS (M1):
- CA1: stale {role}_output.json from a prior attempt on the same
per-PR workspace was readable as "fresh" output of the new attempt.
agent_runner now unlinks the MCP-canonical path AND every fallback
path BEFORE the session runs.
- CA2/PD5: opencode.json-registered MCP subprocesses persist across
OpenCode sessions, but BuilderState was module-singleton. Added
reset_for_new_attempt() + cross-session detection (compare
identity.attempt_id) to every *_start; force-resets with WARN if
prior attempt was interrupted (timeout / lost lock).
- PD3: inline-JSON callback could overwrite an MCP-written canonical
V1 file with adapted-from-prose garbage. Callback now inspects
existing files and skips when V1 is already present.
- PD4: FORGEJO_URL = .rstrip("/api/v1") is a character-set strip —
catastrophic for hosts whose path contains /v1 in the middle.
Replaced with explicit endswith()-based suffix strip.
- CA10: clone URL embedded $FORGEJO_TOKEN, persisted into
.git/config where any agent could cat it. Token now sourced via
local credential.helper at clone-time, URL kept clean.
- CA12: state.finalized was set BEFORE the file write, so disk-full
/ OSError left the agent unable to retry finalize. Reordered.
HIGHS (M2):
- CA3/PD12: output_path validation (NUL-byte rejection, must be
absolute, parent-not-file check) in finalize_and_emit.
- CA6: ci_status_poll SELECT only considered implementer attempts;
conflict_resolver also pushes commits. SQL now unions both roles.
- PD9: ci_status_poll could advance on a stale "resolved" SHA from a
blocked attempt (whose head_sha_after == head_sha_before). Added
outcome='resolved' filter.
- CA8: cancelled/stale CI states mapped to ci_red_retry_same_tier,
burning pickup_count on healthy PRs. Both now wait (treated as
operator/system action, not failure). timed_out stays red.
- TE9: unknown Forgejo CI states now WARN-log instead of silently
being treated as pending — operators see new state strings.
- PD8: ci_status_poll event_type strings standardized to match the
state-machine event names (ci_green / ci_red_retry_same_tier)
instead of legacy ci-green / ci-red.
- CA7: inline-JSON callback now checks lost_lock_check BEFORE write
so a file isn't staged after lock loss.
- PD10: atomic .tmp + os.replace writes in both MCP finalize and
inline callback so the poller never sees a half-written file.
- PD16: inline_output_callback exceptions now re-raise as WorkerError
instead of being silently logged (root cause was buried 30s later
in a canonical-output timeout).
- CA9: WorkerConfig manual rebuild on --max-concurrent/--poll-interval
silently dropped new fields. Use dataclasses.replace, matching
round-4 P5 fix in master/__main__.py.
MEDIUMS (M3) — legacy_adapter quality upgrades:
- PD1: unrecognized confidence values now WARN instead of silently
defaulting to "medium" — surfaces agent prompt drift.
- PD2: estimator recommended_tier clamped to {0,1,2} so an out-of-
range int doesn't bypass the adapter's whole purpose.
- PD7: reviewer blocking_issues list-of-strings coerced into the
list-of-BlockingIssue-dict shape strict_parse requires.
- PD13: conflict_resolver prompt defaults tier=1 + warns instead of
raising; the scheduler always sets it but defends against drift.
- PD14: summarizer summary < 50 chars padded with a clear marker so
strict_parse accepts it (and the truncation is visible).
- PD15: implementer blockers capped at 4096 chars each so a buggy
agent can't blow up audit log / DB column.
- PD17: launch script accepts either FORGEJO_TOKEN or GITEA_TOKEN
with a clear error if both are unset.
- PD22: conflict_resolver adapter accepts singular commit_sha
fallback, matching implementer.
- CA4: every adapter invocation logs role + payload key fingerprint
so operators can measure agent-migration progress.
- estimator + summarizer now have explicit _start tools (the prompts
already referenced them; previously absent → first call would fail).
TESTS (M4) — added 40 tests in test_post_review_fixes.py:
- Cross-session MCP state reset (implementer + reviewer + estimator
+ summarizer; intra-session double-start still rejected).
- finalize_and_emit output_path precedence (arg > env > stdout),
parent-dir creation, rejection of relative/NUL paths, failed-write
leaves state retryable.
- legacy_adapter quality: tier clamping, blocker cap, non-string
commit warning, blocking_issues string coercion, conflict_resolver
full roundtrip + non-resolved head clearing, summarizer padding,
confidence warning, V1-passthrough no-log.
- opencode.json registration parity: every MCP the prompts name is
registered with the correct module path.
- Per-role prompts mention {role}_output.json (canonical poller path)
+ the "DO NOT emit chat-JSON" directive.
- FORGEJO_URL suffix-strip parametrized table.
- agent_runner stale-file cleanup: prior-attempt file is unlinked
before a new session can read it as phantom output.
Also updated 2 pre-existing tests for the CA8 / PD8 / PD13 behavior
changes (cancelled→wait, event_type renaming, conflict_resolver
default-tier warning).
Total: 741 → 781 tests, 0 regressions.
DEFERRED (M5 follow-up — non-trial-blocking):
- CA5: head_sha verification via git cat-file (requires subprocess).
- CA11: discovery_interval_s wall-time cadence (vs iteration count).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
246 lines
8.3 KiB
Python
246 lines
8.3 KiB
Python
"""Per-PR workspace management.
|
|
|
|
Per plan v6/v9: each attempt operates in a per-PR umbrella directory
|
|
at ``/tmp/cleveragents-controller/pr-{owner}-{repo}-{N}/`` containing
|
|
the editable worktree, the ``worker.session`` sidecar, and (later)
|
|
attempt log archives.
|
|
|
|
At attempt start the worker MUST:
|
|
1. Ensure workspace + clone (if absent) or reuse (if present)
|
|
2. ``git fetch origin --prune``
|
|
3. Validate ``input_payload.head_sha == current HEAD on origin/<ref>``
|
|
— if mismatched, raise ``StaleInputError`` so the runner reports
|
|
``outcome='stale-input'`` (master re-prefetches without pickup
|
|
penalty, per v6).
|
|
4. ``git reset --hard <expected_head_sha>`` (cleans residue from any
|
|
previous attempt's mid-rebase / mid-edit state on this workspace).
|
|
|
|
The workspace is per-PR, not per-attempt — multiple attempts on the
|
|
same PR share the same on-disk dir. Crash-recovery: workspace dirs
|
|
whose owning attempt is no longer in flight are deleted by the
|
|
startup janitor (janitor.py).
|
|
"""
|
|
from __future__ import annotations
|
|
|
|
import logging
|
|
import os
|
|
import shutil
|
|
import subprocess
|
|
from dataclasses import dataclass
|
|
from pathlib import Path
|
|
|
|
logger = logging.getLogger(__name__)
|
|
|
|
|
|
# Per plan v6: per-PR umbrella dir.
|
|
DEFAULT_WORKSPACE_ROOT = Path(
|
|
os.environ.get(
|
|
"CONTROLLER_WORKSPACE_ROOT",
|
|
"/tmp/cleveragents-controller",
|
|
)
|
|
)
|
|
|
|
|
|
class StaleInputError(RuntimeError):
|
|
"""Raised when the input_payload's head_sha doesn't match the
|
|
repo's current state. Runner maps this to ``WorkerError(
|
|
outcome='stale-input')`` so the master re-prefetches without
|
|
bumping ``pickup_count``."""
|
|
|
|
def __init__(self, expected: str, actual: str, head_ref: str):
|
|
self.expected = expected
|
|
self.actual = actual
|
|
self.head_ref = head_ref
|
|
super().__init__(
|
|
f"stale input: enqueued head_sha={expected!r} for ref "
|
|
f"{head_ref!r}, but current head_sha is {actual!r}"
|
|
)
|
|
|
|
|
|
@dataclass(frozen=True)
|
|
class WorkspaceIdentity:
|
|
"""The (owner, repo, entity_number) tuple that uniquely names a
|
|
workspace dir. Matches the v6 path convention."""
|
|
|
|
owner: str
|
|
repo: str
|
|
entity_number: int
|
|
|
|
def dir_name(self) -> str:
|
|
return f"pr-{self.owner}-{self.repo}-{self.entity_number}"
|
|
|
|
|
|
class PerPRWorkspace:
|
|
"""One PR's on-disk workspace.
|
|
|
|
The workspace dir layout::
|
|
|
|
/tmp/cleveragents-controller/pr-{owner}-{repo}-{N}/
|
|
worker.session (sidecar; written by the worker on spawn)
|
|
worktree/ (editable git checkout)
|
|
attempts/ (per-attempt log archive)
|
|
|
|
Tests inject a custom ``workspace_root`` so they don't need to
|
|
write to ``/tmp/``.
|
|
"""
|
|
|
|
def __init__(
|
|
self,
|
|
identity: WorkspaceIdentity,
|
|
*,
|
|
workspace_root: Path | None = None,
|
|
clone_url: str | None = None,
|
|
):
|
|
self.identity = identity
|
|
self.workspace_root = workspace_root or DEFAULT_WORKSPACE_ROOT
|
|
self.clone_url = clone_url
|
|
|
|
@property
|
|
def workspace_dir(self) -> Path:
|
|
return self.workspace_root / self.identity.dir_name()
|
|
|
|
@property
|
|
def worktree_dir(self) -> Path:
|
|
return self.workspace_dir / "worktree"
|
|
|
|
@property
|
|
def session_sidecar(self) -> Path:
|
|
return self.workspace_dir / "worker.session"
|
|
|
|
@property
|
|
def attempts_dir(self) -> Path:
|
|
return self.workspace_dir / "attempts"
|
|
|
|
def ensure_present(self) -> None:
|
|
"""Create the workspace skeleton if absent. Idempotent."""
|
|
self.workspace_dir.mkdir(parents=True, exist_ok=True)
|
|
self.attempts_dir.mkdir(exist_ok=True)
|
|
|
|
# Credential-helper invocation that sources the Forgejo token from
|
|
# the ``FORGEJO_TOKEN`` env var at fetch/push time. Keeps the token
|
|
# out of ``.git/config`` (where any agent with filesystem read
|
|
# could ``cat .git/config`` to exfiltrate). The helper script is
|
|
# quoted because git's credential.helper accepts shell strings —
|
|
# see ``git help credentials``. ``test "$1" = get`` ensures we
|
|
# only respond to the get action; store/erase become no-ops.
|
|
_CREDENTIAL_HELPER = (
|
|
'!f() { test "$1" = "get" && '
|
|
'echo "username=x" && '
|
|
'echo "password=${FORGEJO_TOKEN:-}"; }; f'
|
|
)
|
|
|
|
def clone_if_absent(self) -> None:
|
|
"""``git clone`` into ``worktree/`` if not already cloned.
|
|
|
|
Idempotent — a re-call on an already-cloned worktree is a
|
|
no-op. The clone uses ``--no-single-branch`` so subsequent
|
|
``git fetch`` can pick up new branches.
|
|
|
|
Token-handling: the clone_url MUST NOT contain credentials.
|
|
A local ``credential.helper`` is configured immediately after
|
|
clone so subsequent fetch/push operations source the token
|
|
from ``$FORGEJO_TOKEN`` at runtime — keeps the token out of
|
|
``.git/config``.
|
|
|
|
Tests can pre-populate ``worktree/`` (e.g., via ``git init``)
|
|
and skip this call.
|
|
"""
|
|
if (self.worktree_dir / ".git").exists():
|
|
return
|
|
if self.clone_url is None:
|
|
raise RuntimeError(
|
|
f"clone_url not set; cannot clone into {self.worktree_dir}"
|
|
)
|
|
self.ensure_present()
|
|
# Remove any partial state before cloning.
|
|
if self.worktree_dir.exists():
|
|
shutil.rmtree(self.worktree_dir)
|
|
# Use -c credential.helper at clone time so the initial fetch
|
|
# can authenticate; then bake the same helper into the local
|
|
# repo config for subsequent git ops.
|
|
_git_run(
|
|
[
|
|
"git", "-c", f"credential.helper={self._CREDENTIAL_HELPER}",
|
|
"clone", "--no-single-branch",
|
|
self.clone_url, str(self.worktree_dir),
|
|
],
|
|
cwd=None,
|
|
)
|
|
_git_run(
|
|
[
|
|
"git", "config", "--local",
|
|
"credential.helper", self._CREDENTIAL_HELPER,
|
|
],
|
|
cwd=self.worktree_dir,
|
|
)
|
|
|
|
def fetch_and_validate(
|
|
self, *, expected_head_sha: str, head_ref: str
|
|
) -> None:
|
|
"""Refresh remote state + verify the workspace is at the
|
|
expected head_sha.
|
|
|
|
- ``git fetch origin --prune`` to pull latest refs.
|
|
- Read ``origin/<head_ref>``; if it differs from
|
|
``expected_head_sha`` raise ``StaleInputError``.
|
|
- ``git reset --hard <expected_head_sha>`` + ``git clean -fdx``
|
|
to wipe any worktree residue.
|
|
|
|
v6 stale-input fix: the worker reports the mismatch as
|
|
``outcome='stale-input'`` so the master re-prefetches without
|
|
bumping ``pickup_count``.
|
|
"""
|
|
# 1. Fetch.
|
|
_git_run(["git", "fetch", "origin", "--prune"], cwd=self.worktree_dir)
|
|
|
|
# 2. Compare expected vs current.
|
|
current = _git_run(
|
|
["git", "rev-parse", f"origin/{head_ref}"],
|
|
cwd=self.worktree_dir,
|
|
).strip()
|
|
if current != expected_head_sha:
|
|
raise StaleInputError(
|
|
expected=expected_head_sha, actual=current, head_ref=head_ref
|
|
)
|
|
|
|
# 3. Reset worktree cleanly to the expected head.
|
|
_git_run(
|
|
["git", "reset", "--hard", expected_head_sha],
|
|
cwd=self.worktree_dir,
|
|
)
|
|
_git_run(["git", "clean", "-fdx"], cwd=self.worktree_dir)
|
|
|
|
def remove(self) -> None:
|
|
"""Recursively delete the workspace dir. Used on terminal
|
|
workflow transitions and by the startup janitor."""
|
|
if self.workspace_dir.exists():
|
|
shutil.rmtree(self.workspace_dir)
|
|
|
|
|
|
def _git_run(cmd: list[str], cwd: Path | None) -> str:
|
|
"""Run a git command; return stdout. Raises on non-zero exit."""
|
|
try:
|
|
result = subprocess.run(
|
|
cmd, cwd=str(cwd) if cwd else None,
|
|
capture_output=True, text=True, check=True, timeout=120,
|
|
)
|
|
except subprocess.CalledProcessError as exc:
|
|
raise RuntimeError(
|
|
f"git command failed: {' '.join(cmd)}\n"
|
|
f" stderr: {exc.stderr.strip()}\n"
|
|
f" stdout: {exc.stdout.strip()}"
|
|
) from exc
|
|
except subprocess.TimeoutExpired as exc:
|
|
raise RuntimeError(
|
|
f"git command timed out (120s): {' '.join(cmd)}"
|
|
) from exc
|
|
return result.stdout
|
|
|
|
|
|
__all__ = [
|
|
"DEFAULT_WORKSPACE_ROOT",
|
|
"PerPRWorkspace",
|
|
"StaleInputError",
|
|
"WorkspaceIdentity",
|
|
]
|