diff --git a/CHANGELOG.md b/CHANGELOG.md index 7fe7dc22..edef3315 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -94,6 +94,8 @@ ### Added +- Added a clone-based direct code review workflow with a `git-clone` + prepare hook and bundled `code-review-clone` assignment. - Added first-class run groups across manifests, CLI, daemon APIs, and the web dashboard. Fresh child runs inherit the parent run group by default, `--group-id` overrides fresh run/init grouping, and @@ -329,6 +331,10 @@ ### Fixed +- Hardened the `git-clone` hook against option-like clone inputs, rejected + credential-bearing HTTPS URLs before manifest persistence, made managed + checkouts reusable across initialized reconfigure runs, and removed managed + clone checkouts when runs are deleted. - Daemon-connected fresh `init --message-file` and fresh `run --message-file` now forward the file contents in `overrides.message` instead of snapshotting overrides before the diff --git a/README.md b/README.md index 3f30e6de..601c0cad 100644 --- a/README.md +++ b/README.md @@ -504,12 +504,13 @@ Agents (under `agents/`): Assignments (under `assignments/`): - `repo-orientation`, `test`, `plan-feature`, `plan-review`, - `code-review`, `code-review-direct`, `doc-review`, `familiarize` + `code-review`, `code-review-direct`, `code-review-clone`, + `doc-review`, `familiarize` Shared task definitions (under `tasks/`): - reusable `review/architecture` through `review/docs-drift` - code-review dimensions used by both code-review assignments + code-review dimensions used by the bundled code-review assignments Walkthrough in [docs/examples.md](docs/examples.md). diff --git a/apps/web/src/app.test.tsx b/apps/web/src/app.test.tsx index c8543619..5e9c8932 100644 --- a/apps/web/src/app.test.tsx +++ b/apps/web/src/app.test.tsx @@ -1,7 +1,11 @@ import { readFileSync } from "node:fs"; import { join } from "node:path"; import type { RunAttachment } from "@task-runner/core/contracts/attachments.js"; -import type { RunAuditHistory, RunTimelineHistory } from "@task-runner/core/contracts/events.js"; +import type { + RunAuditHistory, + RunTimelineAttempt, + RunTimelineHistory, +} from "@task-runner/core/contracts/events.js"; import type { RunInputSurface } from "@task-runner/core/contracts/run-input-surface.js"; import type { RunDetail, RunSummary } from "@task-runner/core/contracts/runs.js"; import { cleanup, fireEvent, render, screen, waitFor, within } from "@testing-library/react"; @@ -2028,6 +2032,15 @@ describe("web app", () => { }), }); + await waitFor(() => { + expect(screen.getByRole("tab", { name: "Response" })).toHaveAttribute( + "aria-selected", + "true", + ); + }); + expect(screen.queryByRole("tab", { name: "Live" })).not.toBeInTheDocument(); + expect(screen.getByText("Waiting for live response text…")).toBeInTheDocument(); + let timelineSource: MockEventSource | undefined; await waitFor(() => { timelineSource = MockEventSource.instances.find((candidate) => @@ -2155,6 +2168,173 @@ describe("web app", () => { expect(screen.getByText("Waiting for live response text…")).toBeInTheDocument(); }); + it("refreshes and selects a resumed attempt when detail reports the run live first", async () => { + const completedSession = { + sessionIndex: 0, + status: "success" as const, + startedAt: "2026-04-13T05:00:00.000Z", + endedAt: "2026-04-13T05:02:00.000Z", + exitCode: 0, + message: null, + firstAttemptNumber: 1, + lastAttemptNumber: 1, + attemptCount: 1, + maxAttemptsPerSession: 3, + backendSessionIdAtStart: "thread-1", + backendSessionIdAtEnd: "thread-1", + }; + const resumedSession = { + sessionIndex: 1, + status: "running" as const, + startedAt: "2026-04-13T05:03:00.000Z", + endedAt: null, + exitCode: null, + message: "Continue with the fix.", + firstAttemptNumber: 2, + lastAttemptNumber: 2, + attemptCount: 1, + maxAttemptsPerSession: 3, + backendSessionIdAtStart: "thread-2", + backendSessionIdAtEnd: null, + }; + const initialDetail = makeDetail({ + status: "success", + effectiveStatus: "success", + isLive: false, + endedAt: "2026-04-13T05:02:00.000Z", + exitCode: 0, + totalAttemptCount: 1, + totalSessionCount: 1, + currentSession: null, + lastSession: completedSession, + sessions: [completedSession], + }); + const resumedDetail = makeDetail({ + status: "running", + effectiveStatus: "running", + isLive: true, + endedAt: null, + exitCode: null, + totalAttemptCount: 1, + totalSessionCount: 2, + currentSession: resumedSession, + lastSession: resumedSession, + sessions: [completedSession, resumedSession], + }); + const completedAttempt: RunTimelineAttempt = { + attemptNumber: 1, + attemptIndexInSession: 0, + sessionIndex: 0, + startedAt: "2026-04-13T05:00:00.000Z", + endedAt: "2026-04-13T05:02:00.000Z", + prompt: "Initial prompt", + transcript: "Attempt one output\n", + notices: "", + exitCode: 0, + timedOut: false, + live: false, + }; + const resumedAttempt: RunTimelineAttempt = { + attemptNumber: 2, + attemptIndexInSession: 0, + sessionIndex: 1, + startedAt: "2026-04-13T05:03:00.000Z", + endedAt: null, + prompt: "## Resume prompt", + transcript: "", + notices: "", + exitCode: null, + timedOut: false, + live: true, + }; + const state = { + runs: [ + makeRun({ + status: "success", + effectiveStatus: "success", + endedAt: "2026-04-13T05:02:00.000Z", + totalAttemptCount: 1, + totalSessionCount: 1, + currentSession: null, + lastSession: completedSession, + }), + ], + details: { "run-1": initialDetail }, + timelineHistories: { + "run-1": { + runId: "run-1", + lastCursor: 1, + attempts: [completedAttempt], + }, + }, + }; + const fetchMock = installFetchMock(state); + + const user = userEvent.setup(); + await renderApp(); + await user.click(await findRunCard("Build dashboard")); + await user.click(screen.getByRole("button", { name: "Attempts" })); + + await user.click(await screen.findByRole("tab", { name: "Prompt" })); + expect(screen.getByRole("region", { name: "Attempt prompt" })).toHaveTextContent( + "Initial prompt", + ); + expect(fetchCallCount(fetchMock, (url) => url.endsWith("/api/runs/run-1/timeline"))).toBe(1); + + state.details["run-1"] = resumedDetail; + state.timelineHistories["run-1"] = { + runId: "run-1", + lastCursor: 2, + attempts: [completedAttempt, resumedAttempt], + }; + const detailSource = findEventSource("/api/runs/run-1/events/detail"); + detailSource.emitMessage({ type: "detail_updated", detail: resumedDetail }); + + await waitFor(() => { + expect(screen.getByRole("tab", { name: "Response" })).toHaveAttribute( + "aria-selected", + "true", + ); + }); + expect(screen.queryByRole("tab", { name: "Live" })).not.toBeInTheDocument(); + expect(screen.getByText("Waiting for live response text…")).toBeInTheDocument(); + + let timelineSource: MockEventSource | undefined; + await waitFor(() => { + timelineSource = MockEventSource.instances.find((candidate) => + candidate.url.endsWith("/api/runs/run-1/events/timeline"), + ); + expect(timelineSource).toBeDefined(); + }); + if (!timelineSource) { + throw new Error("expected timeline EventSource after resume start"); + } + timelineSource.emitOpen(); + + await waitFor(() => { + expect(fetchCallCount(fetchMock, (url) => url.endsWith("/api/runs/run-1/timeline"))).toBe(2); + }); + await waitFor(() => { + expect(screen.getByRole("tab", { name: "Session 2" })).toHaveAttribute( + "aria-selected", + "true", + ); + }); + expect(screen.getByRole("tab", { name: "Response" })).toHaveAttribute("aria-selected", "true"); + expect(screen.getByText("Waiting for live response text…")).toBeInTheDocument(); + + timelineSource.emitMessage({ + runId: "run-1", + cursor: 3, + event: { + type: "agent_message_delta", + text: "streamed after resume", + }, + }); + + expect(await screen.findByText("streamed after resume")).toBeInTheDocument(); + }); + it("separates transcript and backend notices instead of gluing them together", async () => { installFetchMock({ runs: [makeRun()], diff --git a/apps/web/src/components/run-detail-drawer.tsx b/apps/web/src/components/run-detail-drawer.tsx index fbac3cbd..4f5f0751 100644 --- a/apps/web/src/components/run-detail-drawer.tsx +++ b/apps/web/src/components/run-detail-drawer.tsx @@ -67,7 +67,7 @@ import { RunTaskList } from "./run-task-list.js"; import { StatusBadge } from "./status-badge.js"; type TimelineTab = "message" | "prompt" | "response" | "diagnostics"; -type AttemptSelection = number | "pending" | null; +type AttemptSelection = number | "live" | "pending" | null; type SummaryRow = readonly [label: string, value: string]; type DataTab = "vars" | "hookState"; type AuditFilter = "all" | "hooks" | "tasks" | "run"; @@ -683,18 +683,33 @@ export function RunDetailDrawer({ (run.status === "initialized" || run.status === "ready") && run.totalAttemptCount === 0 && timelineAttempts.length === 0; + const currentSessionHasTimelineAttempt = + run.currentSession !== null && + timelineAttempts.some((attempt) => attempt.sessionIndex === run.currentSession?.sessionIndex); + const liveAttemptPendingAvailable = + run.status === "running" && + run.currentSession !== null && + run.currentSession.status === "running" && + !currentSessionHasTimelineAttempt && + !timelineAttempts.some((attempt) => attempt.live); const selectedAttemptRecord = - (typeof selectedAttempt === "number" + typeof selectedAttempt === "number" ? timelineAttempts.find((attempt) => attempt.attemptNumber === selectedAttempt) - : null) ?? - timelineAttempts[timelineAttempts.length - 1] ?? - null; + : selectedAttempt === null || + (selectedAttempt === "pending" && !pendingAttemptAvailable) || + (selectedAttempt === "live" && !liveAttemptPendingAvailable) + ? timelineAttempts[timelineAttempts.length - 1] + : null; const selectedPendingAttempt = pendingAttemptAvailable && selectedAttemptRecord === null; + const selectedLiveAttempt = + liveAttemptPendingAvailable && selectedAttempt === "live" && selectedAttemptRecord === null; const selectedAttemptNumber = selectedAttemptRecord?.attemptNumber ?? null; const selectedAttemptResponse = selectedAttemptRecord?.transcript ?? ""; const selectedAttemptDiagnostics = selectedAttemptRecord?.notices ?? ""; const selectedAttemptLive = selectedAttemptRecord?.live ?? false; - const selectedSessionIndex = selectedAttemptRecord?.sessionIndex ?? null; + const selectedSessionIndex = + selectedAttemptRecord?.sessionIndex ?? + (selectedLiveAttempt ? run.currentSession?.sessionIndex : null); const selectedTimelineSession = selectedSessionIndex === null ? null @@ -1094,6 +1109,17 @@ export function RunDetailDrawer({ setSelectedAttempt(timelineAttempts[timelineAttempts.length - 1]?.attemptNumber ?? null); return; } + if (selectedAttempt === "live") { + if (liveAttemptPendingAvailable) { + return; + } + setSelectedAttempt(timelineAttempts[timelineAttempts.length - 1]?.attemptNumber ?? null); + return; + } + if (liveAttemptPendingAvailable) { + setSelectedAttempt("live"); + return; + } if (selectedAttempt !== null && availableAttempts.has(selectedAttempt)) { return; } @@ -1102,7 +1128,7 @@ export function RunDetailDrawer({ return; } setSelectedAttempt(timelineAttempts[timelineAttempts.length - 1]?.attemptNumber ?? null); - }, [pendingAttemptAvailable, selectedAttempt, timelineAttempts]); + }, [liveAttemptPendingAvailable, pendingAttemptAvailable, selectedAttempt, timelineAttempts]); useEffect(() => { const latestAttempt = timelineAttempts[timelineAttempts.length - 1]?.attemptNumber ?? null; @@ -1110,6 +1136,12 @@ export function RunDetailDrawer({ latestAttemptRef.current = latestAttempt; return; } + if (liveAttemptPendingAvailable) { + latestAttemptRef.current = latestAttempt; + setSelectedAttempt("live"); + setTimelineTab("response"); + return; + } if (latestAttempt === null) { latestAttemptRef.current = null; return; @@ -1121,7 +1153,7 @@ export function RunDetailDrawer({ return; } latestAttemptRef.current = latestAttempt; - }, [activeSection, timelineAttempts]); + }, [activeSection, liveAttemptPendingAvailable, timelineAttempts]); useEffect(() => { if ( @@ -2670,10 +2702,11 @@ export function RunDetailDrawer({

No attempt history is available for this run yet.

) : null} - {selectedAttemptRecord || selectedPendingAttempt ? ( + {selectedAttemptRecord || selectedPendingAttempt || selectedLiveAttempt ? (
- {selectedPendingAttempt || timelineAttempts.length > 1 ? ( + {selectedPendingAttempt || + (!selectedLiveAttempt && timelineAttempts.length > 1) ? (
{selectedPendingAttempt ? (
@@ -2888,6 +2921,8 @@ export function RunDetailDrawer({ No prompt preview is available for this run yet.

) + ) : selectedLiveAttempt ? ( +

Waiting for attempt prompt…

) : selectedAttemptRecord?.prompt ? (
No response yet — this run has not started.

+ ) : selectedLiveAttempt ? ( +

Waiting for live response text…

) : selectedAttemptResponse ? (
No diagnostics yet — this run has not started.

+ ) : selectedLiveAttempt ? ( +

No diagnostics have arrived yet.

) : selectedAttemptDiagnostics ? (
(null); const reloadCountRef = useRef(0); const previousRunIdRef = useRef(undefined); + const previousRunIsLiveRef = useRef(false); useEffect(() => { historyRef.current = state.history; @@ -168,6 +169,7 @@ export function useRunTimelineState({ bufferRef.current = []; reloadCountRef.current = 0; previousRunIdRef.current = undefined; + previousRunIsLiveRef.current = false; setState({ history: null, isLoading: false, stale: false }); return; } @@ -253,7 +255,9 @@ export function useRunTimelineState({ // ends), keep the already-loaded history so the drawer doesn't flash // through an empty "Loading…" state. const sameRunId = previousRunIdRef.current === runId; + const becameLive = sameRunId && !previousRunIsLiveRef.current && runIsLive; previousRunIdRef.current = runId; + previousRunIsLiveRef.current = runIsLive; if (!sameRunId) { historyRef.current = null; staleRef.current = false; @@ -272,8 +276,16 @@ export function useRunTimelineState({ }; } + const timelineAttemptCount = historyRef.current?.attempts.length ?? 0; + const hasLoadedAttempts = bootstrappedRef.current && timelineAttemptCount > 0; + // A resumed run can flip detail back to live before the replayed timeline + // attempt reaches this hook; refresh once so history can project it. + const shouldRefreshOnLiveStart = becameLive && hasLoadedAttempts; const shouldLoadHistory = - !bootstrappedRef.current || staleRef.current || historyRef.current === null; + !bootstrappedRef.current || + staleRef.current || + historyRef.current === null || + shouldRefreshOnLiveStart; if (sameRunId) { setState((current) => ({ @@ -299,7 +311,7 @@ export function useRunTimelineState({ if (disposed) { return; } - if (!bootstrappedRef.current || staleRef.current) { + if (shouldLoadHistory) { void loadHistory(); } }, diff --git a/assignments/code-review-clone/assignment.md b/assignments/code-review-clone/assignment.md new file mode 100644 index 00000000..63276d79 --- /dev/null +++ b/assignments/code-review-clone/assignment.md @@ -0,0 +1,260 @@ +--- +schemaVersion: 1 +name: code-review-clone +vars: + repo_url: + type: string + required: true + sources: [cli, web] + description: Git SSH/HTTP URL to clone for review. HTTPS URLs with embedded userinfo are rejected. + ref: + type: string + required: false + sources: [cli, web] + description: Optional branch, tag, or commit to check out before review. + range: + type: string + required: false + sources: [cli, web] + default: full + description: | + What scope to review. One of: + - `full` - entire codebase (default) + - `unstaged` - `git diff` (working tree) + - `staged` - `git diff --cached` + - `last commit` - `git show HEAD` + - `HEAD~N..HEAD` - N commits back to HEAD + - `main..` - branch divergence + - any other git range spec - passed through to git +hooks: + prepare: + - builtin: git-clone + with: + repo_url: "{{repo_url}}" + ref: "{{ref}}" +callerInstructions: | + This run produces a structured clone-based direct code review in the + task notes, gated by an explicit ship / no-ship decision at the end. + Use it for user-launched or Web UI reviews that should clone the + target repository from `repo_url` instead of requiring an existing + local checkout. + + The reviewer's synthesis in `synthesis.notes` is the ranked + top-findings list. Each shared review task (`review/architecture`, + `review/concurrency`, ..., `review/surface-completeness`) carries + the raw findings for its dimension with severity tags. The final + decision lives in `approval.notes`. + + **Exit code carries the decision.** The run exits `success` + (code 0) only if the reviewer approved the cloned direct review + target in `approval`; `blocked` (code 2) means the reviewer could + not approve and a delta pass is needed after fixes. Scripts should + gate on the terminal status, not on the existence of a synthesis + block. +tasks: + - id: orient + title: Clone-based direct review orientation and scope resolution + body: | + First, understand the cloned project. This is a clone-based + direct/ad hoc review. The repository was cloned from + `{{repo_url}}`; the review root is `{{cwd}}`; the requested + checkout target is `{{ref}}` when that value is non-empty, + otherwise the remote default branch selected by git; and the + checked-out commit is `{{commit_sha}}`. Do not modify files in + the checkout. + + Read the high-signal entry points for the repository at `{{cwd}}`: + - AGENTS.md, CLAUDE.md, CONTRIBUTING.md at the repo root + - README.md + - Build manifest (package.json, Cargo.toml, go.mod, + pyproject.toml, etc.) for scripts, dependencies, and + language toolchain + - docs/ directory (design docs, architecture notes) + - Primary entry-point files (`src/cli.ts`, `main.rs`, + `main.go`, `src/index.*`, or what the build manifest + points at) + + Second, resolve the review scope from `range = "{{range}}"` inside + the cloned checkout: + - If `range` is `full`, the scope is the whole codebase. + Proceed through the review-dimension tasks with the whole + codebase in mind. + - If `range` is a git-style spec (`unstaged`, `staged`, + `last commit`, `HEAD~N..HEAD`, `main..branch`, or any + other phrase), translate it to a concrete git invocation + and identify the exact set of files and hunks that changed. + Run the appropriate command from `{{cwd}}`: + - `unstaged` -> `git diff` + - `staged` -> `git diff --cached` + - `last commit` -> `git show HEAD` + - a git range spec -> `git diff ` + Capture the list of touched files. For the rest of the + review, also read each touched file in full (not just the + diff) so you can judge the change in context. + + Third, state explicitly in Notes that this is a clone-based + direct/ad hoc review. There is no implementation run, no + `implementation_run_id`, no planning lineage, and no plan-coverage + pass in this assignment. + + Notes: a 5-10 line summary of what the project is, its major + modules, whether this is full-codebase or ranged, and -- if scoped -- + the concrete set of files and hunks under review. Include the + cloned repo URL, review root, requested target or remote default + branch, and checked-out commit SHA. This is context for the rest + of the review, not a finding section. + - review/architecture + - review/concurrency + - review/error-handling + - review/state-machine + - review/resources + - review/security + - review/types-schema + - review/simplification-and-duplication + - review/test-coverage + - review/docs-drift + - review/surface-completeness + - id: synthesis + title: Direct review synthesis + body: | + Now read back through the notes from the review-dimension tasks + (`review/architecture` through `review/surface-completeness`) and + build a single ranked list of the **top 10 highest-leverage findings** + from the clone-based direct review. Order by severity (CRITICAL first, + then HIGH, MEDIUM, LOW) and within a severity by impact. Each entry + must reference the original finding's file:line so the reader can + jump back. + + If you found fewer than 10 real issues, list the real ones and stop. + Padding the list with NITs is not a synthesis. + + Also include: + - One sentence on the project's overall health + - The single highest-leverage refactor (if any) that would make + several findings disappear at once + - A one-sentence ship / ship-with-changes / block recommendation. + Required for every review. For ranged reviews, this answers + "should this change land?" For full-codebase reviews, this + answers "would you ship this codebase as-is if a maintainer + asked you to tag a release right now?" Use exactly one of the + three outcomes. + - id: approval + title: Direct review ship / no-ship decision + body: | + This task is the gate. It is not a synthesis, a summary, or an + end-of-run checkbox -- it is a decision with a consequence. + `completed` means "I approve this cloned direct review target for + ship." `blocked` means "I cannot approve; halt cleanly for a delta + pass." Both are first-class outcomes; neither is a failure of the + review. The run's exit code carries this decision (0 on approved, + 2 on blocked), so scripts and callers will gate on it. + + **Lead with the blocked case.** If any of the following are true, + mark this task `blocked` with a short Notes block explaining which + condition held and stop: + + - Any HIGH or CRITICAL finding from the review-dimension tasks is + unresolved in the current diff or codebase. + - The `synthesis` recommendation is "block" or "ship with + changes" -- both mean more work is required. + - You were unable to trace a finding deeply enough to judge whether + it is real. Uncertainty blocks. + + **Completion criteria.** Only mark this task `completed` if all of + the following hold: + + - Every HIGH / CRITICAL finding is either resolved in the current + diff/codebase or explicitly declined by the caller upstream with a + written justification. + - The `synthesis` recommendation is "ship." + - You personally would stand behind this change or codebase landing + on the main branch as-is. If you would not, block. + + **Write-in requirement.** Whether you approve or block, paste a + one-paragraph decision record into this task's Notes block. Use these + formats verbatim so downstream scripts can grep them: + + Approval: + + APPROVED for ship. + Rationale: + Residual findings: + + Block: + + BLOCKED -- cannot approve. + Unresolved: + Path to approval: + + Do not delegate this task to a subagent. The approval decision must + live in the main reviewer's own context. +--- +You are reviewing the clone-based direct/ad hoc review target cloned from +`{{repo_url}}` at checked-out commit `{{commit_sha}}` with scope +`{{range}}`. Treat `{{cwd}}` as the root for everything you read. +The requested checkout target is `{{ref}}` when non-empty; otherwise +review the remote default branch selected by git. Do not modify any file +inside the checkout. Use the task CLI as the task interface for this run; +do not rely on workspace files. + +Work the tasks in order. Earlier tasks build context the later +ones depend on. The synthesis task at the end pulls from your +accumulated notes, so be specific in each task's notes block. + +A dimension with no real issues produces no findings in that +task. Say "No issues found in this dimension" and move on. +Padding the review to look thorough is worse than a short honest +review. + +**Delegating via subagents.** For a large review, you may +delegate independent review dimensions to subagents to parallelize, +for example running the concurrency review and the security review +as separate subagents while you continue other dimensions. Do not +delegate `orient`, `synthesis`, or `approval`; all three depend on +your own accumulated context and, in the case of `approval`, on a +judgment call that cannot be outsourced. + +When a subagent returns, fold its findings into the task's Notes +block in the same severity/format used by the rest of the review so +the synthesis pass sees a consistent shape. Delegation is optional; +for a small or ranged review, working the tasks yourself is usually +faster. + +**Re-review after agreed-upon fixes.** If this run is resumed with +a follow-up message asking you to re-check your prior findings, do +not re-walk all 14 tasks from scratch. Instead: + +1. Re-read your prior notes on `review/architecture` through + `review/surface-completeness`, plus `synthesis`. Those are your + original findings and synthesis. Also re-read the prior + `approval` decision record if it exists. +2. Inspect what has changed in the cloned repository since your prior + review. You are in a resumed session with full context of your + previous findings, so figure out what moved: new commits on the + branch, unstaged edits, or a widened scope. Do not assume the + original `range` spec is still stable. Use `git log`, + `git status`, `git diff`, and the commit history from your first + pass to orient yourself to the actual delta between the review + you already did and the current working tree. +3. For each prior finding, decide whether it is resolved, partially + resolved, not addressed, or explicitly declined by the caller. +4. Scan the new changes for any new issues introduced by the fixes + themselves. Hold these to the same severity bar as the original + review. +5. Write the delta as a fresh synthesis into `synthesis`'s Notes + block, replacing the prior synthesis. Include findings resolved, + findings still open, new findings introduced by the fixes, and a + one-sentence ship, ship-with-changes, or block recommendation. +6. Leave the Notes on `review/architecture` through + `review/surface-completeness` as the original audit trail and + put the delta in `synthesis` only. +7. Re-evaluate `approval` against the post-fix state. If every + HIGH/CRITICAL finding is now resolved or was explicitly declined + with justification and the synthesis recommendation is "ship", + mark `approval` `completed` with a fresh approval decision record. + Otherwise, mark it `blocked` again with an updated reason and + expect another delta pass. diff --git a/docs/agents-and-assignments.md b/docs/agents-and-assignments.md index 2b0c3bce..2beb55fd 100644 --- a/docs/agents-and-assignments.md +++ b/docs/agents-and-assignments.md @@ -462,6 +462,11 @@ Built-in hooks: - `git-worktree` runs in `prepare` and `beforeAttempt`. It ensures a git worktree, switches the run `cwd` to that path, and in `prepare` also projects `worktree_path` into runtime vars. +- `git-clone` runs in `prepare` only. It clones a remote or local Git URL + into a checkout, switches the run `cwd` to that checkout before the + manifest is first written, and projects clone metadata into runtime + vars. Resume and reset reuse the frozen cwd and vars; they do not + re-contact the remote. - `command` runs in every phase. `mode: status` treats exit code `0` as success and a non-zero exit code as block/reject. `mode: json` requires exit code `0` and parses a full hook result from stdout; @@ -472,6 +477,44 @@ Built-in hooks: `when.taskId` / `when.taskIds`, and set `requireAny: true` only when the task must refuse completion until at least one child run exists. +`git-clone` hook config uses snake_case fields only: + +```yaml +hooks: + prepare: + - builtin: git-clone + with: + repo_url: git@github.com:org/repo.git + ref: feature-branch + path: /tmp/task-runner-review-checkout + remote_name: origin + depth: 1 + collision: reuse +``` + +- `repo_url` is required and must be a non-empty string. +- `ref` is optional. Empty interpolated values are treated as omitted; when + omitted, git leaves the clone on the remote default branch. +- `path` is optional. When omitted, the checkout path defaults to + `${TASK_RUNNER_STATE_DIR}/checkouts/-`, where the + slug is derived from the repo URL and sanitized to one filesystem-safe + path segment. +- `remote_name` is optional and defaults to `origin`. +- `depth` is optional and must be a positive integer. +- `collision` is optional and must be `fail`, `reuse`, or `replace`. + Custom `path` values default to `fail`; managed checkouts under + `${TASK_RUNNER_STATE_DIR}/checkouts/` default to `reuse` so initialized + reconfigure runs can fetch and check out a new `ref` without colliding + with the existing checkout. + +If the checkout path already exists and is non-empty, `git-clone` follows +the configured collision policy before cloning. On success it emits +`repo_slug`, `checkout_path`, `commit_sha`, and `resolved_ref` when the +ref can be determined reliably. Managed checkout paths are removed when +their run is deleted. Do not put credentials in `repo_url`: HTTPS URLs +with embedded userinfo are rejected before the run manifest is written. +Use SSH agents or Git credential helpers instead. + ## Locked fields Both agents and assignments can declare `lockedFields`. The two sets are diff --git a/docs/cli.md b/docs/cli.md index 8ff59fb6..39ce4f9a 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -130,6 +130,21 @@ Assignment task refs follow the same explicit loader model: - bundled assignments that need shared review tasks use named refs such as `review/architecture` +For a clone-based direct review, let the `code-review-clone` assignment +clone the target and switch the run cwd during prepare: + +```bash +task-runner run \ + --agent code-reviewer \ + --assignment code-review-clone \ + --var repo_url=git@github.com:org/repo.git \ + --var ref=feature-branch \ + --var range=origin/main..HEAD +``` + +HTTPS URLs with embedded userinfo are rejected before the run manifest is +written; use SSH agents or Git credential helpers for private repos. + ## `init` Same inputs as `run` (except no `--detach`, `--max-retries`, diff --git a/docs/configuration.md b/docs/configuration.md index 9ddfed8a..f035cdad 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -39,8 +39,9 @@ The bundled repo also ships shared review task definitions under `tasks/review/`; bundled assignments reference those files with named refs such as `review/architecture`, resolved from `${TASK_RUNNER_CONFIG_DIR}/tasks`. If you copy the bundled -`code-review` assignments into another config directory, copy -`tasks/review/` with them so those named refs continue to resolve. +`code-review`, `code-review-direct`, or `code-review-clone` assignments +into another config directory, copy `tasks/review/` with them so those +named refs continue to resolve. ### State directory diff --git a/docs/daemon.md b/docs/daemon.md index 226bd220..a5e778e7 100644 --- a/docs/daemon.md +++ b/docs/daemon.md @@ -170,7 +170,7 @@ the WebSocket methods: "parentRunId": "abcd12", "runGroupId": "planning-wave", "backendSessionId": "session-123", - "cliVars": {}, + "webVars": {}, "overrides": {} } ``` @@ -179,6 +179,35 @@ Browser callers should send an explicit `callerCwd` on `POST /api/runs/init` and `POST /api/runs`. The daemon keeps `callerCwd` distinct from `overrides.cwd`; it is not a browser-only alias. +Clone-based direct reviews use the same `POST /api/runs` or +`POST /api/runs/init` shape with browser-sourced assignment vars in +`webVars`: + +```json +{ + "agent": "code-reviewer", + "assignment": "code-review-clone", + "callerCwd": "/tmp", + "webVars": { + "repo_url": "git@github.com:org/repo.git", + "ref": "feature-branch", + "range": "origin/main..HEAD" + }, + "overrides": {} +} +``` + +The caller does not supply `cwd` or a repo slug for this workflow. The +`git-clone` prepare hook clones the repo, switches the run cwd to the +checkout before manifest freeze, and persists the clone metadata in +runtime vars. Because arbitrary clone URLs cause network egress and disk +writes on the daemon host, expose the daemon only inside a trusted local +environment or protect remote access externally. The daemon has no built-in +token auth. HTTPS URLs with embedded userinfo are rejected before the run +manifest is written; use SSH agents or Git credential helpers instead. +Future deployments that expose the daemon beyond localhost may also want +repo URL allowlists. + Schedule bodies use the same flat input contract as the CLI: ```json diff --git a/docs/examples.md b/docs/examples.md index fde3e6d6..ed66d252 100644 --- a/docs/examples.md +++ b/docs/examples.md @@ -179,6 +179,25 @@ task, and no lineage attachment lookup. It reuses the same shared `review/...` dimension tasks as `code-review`, then produces a direct synthesis and approval decision. +### `code-review-clone` + +- Path: `assignments/code-review-clone/assignment.md` +- Vars: + - `repo_url` (string, required) - Git SSH/HTTP URL to clone for + review. HTTPS URLs with embedded userinfo are rejected. + - `ref` (string, optional) - branch, tag, or commit to check out before + review. + - `range` (string, optional, default `full`) - git range to review + inside the cloned checkout (`full`, `unstaged`, `staged`, + `last commit`, `HEAD~N..HEAD`, `main..`, etc.). + +Clone-based direct/user/Web UI code review for work that is not already +checked out on the local or daemon host. Its prepare hook clones +`repo_url`, checks out `ref` when supplied, switches the run cwd to the +checkout, and reuses the same shared `review/...` dimension tasks as +`code-review-direct`. It has no `implementation_run_id` and no +plan-coverage task. + ### `doc-review` - Path: `assignments/doc-review/assignment.md` @@ -195,7 +214,7 @@ subagents for parallelism. |-------|-------------------|-------| | `planner` | `plan-feature` | Produces an executable plan and a summary; uses nested `plan-review` and blocks for caller approval before delayed implementer creation. | | `implementer` | generated plan assignment | Created by `plan-feature` after approval; inspect `run brief` and then execute with `run --resume-run`. | -| `code-reviewer` | `plan-review`, `code-review`, or `code-review-direct` | Nested and direct review surfaces. | +| `code-reviewer` | `plan-review`, `code-review`, `code-review-direct`, or `code-review-clone` | Nested, direct, and clone-based direct review surfaces. | | `doc-reviewer` | `doc-review` | Review-only, writes no files. | | any | `repo-orientation` / `familiarize` | Quick or deep onboarding before other work. | | any | `test` | Smoke-check for installation or a new agent/backend combination. | @@ -216,10 +235,12 @@ subagents for parallelism. dependencies. - **Attachments as handoff** — planning artifacts attached to the planning run and later discovered via `attachment list --scope group`. -- **Multiple delta re-reviews** — `plan-review`, `code-review`, and - `code-review-direct` switch into delta mode on resume. +- **Multiple delta re-reviews** — `plan-review`, `code-review`, + `code-review-direct`, and `code-review-clone` switch into delta mode on + resume. - **Subagent delegation** — `plan-feature`, `code-review`, - `code-review-direct`, `doc-review`, and `familiarize` allow + `code-review-direct`, `code-review-clone`, `doc-review`, and + `familiarize` allow independent tasks to be parallelized while synthesis tasks stay in the main context. diff --git a/docs/variables.md b/docs/variables.md index 71f2fd11..613334d7 100644 --- a/docs/variables.md +++ b/docs/variables.md @@ -205,6 +205,20 @@ daemon, and web read surfaces redact env-backed values at projection time: Projected `RunDetail.runtimeVars` still redacts env-derived or inherited-env-derived values for humans. +Prepare hooks can add hook-owned runtime vars. The built-in `git-clone` +hook persists: + +| Key | Value | +|-----|-------| +| `repo_slug` | filesystem-safe slug derived from `repo_url` | +| `checkout_path` | absolute path to the cloned checkout; this also becomes the run `cwd` | +| `commit_sha` | checked-out `HEAD` commit SHA | +| `resolved_ref` | supplied `ref`, or the default branch/ref when it can be determined reliably | + +`code-review-clone` also persists the caller-supplied `repo_url` as a +normal runtime var. HTTPS URLs with embedded userinfo are rejected before +the manifest is written; use SSH agents or Git credential helpers instead. + ## Resume and variables Variables are resolved once at run creation and frozen. Resume rejects @@ -230,6 +244,17 @@ task-runner run \ --var range=unstaged ``` +For a direct review that should clone the repo first: + +```bash +task-runner run \ + --agent code-reviewer \ + --assignment code-review-clone \ + --var repo_url=git@github.com:org/repo.git \ + --var ref=feature-branch \ + --var range=origin/main..HEAD +``` + `--var` is repeatable. Values are split on the first `=`, so `--var message=key=value` yields `message=key=value`. diff --git a/packages/core/src/core/commands/service.ts b/packages/core/src/core/commands/service.ts index 79ee4bb3..59f5c35d 100644 --- a/packages/core/src/core/commands/service.ts +++ b/packages/core/src/core/commands/service.ts @@ -1,5 +1,5 @@ import { copyFileSync, existsSync, readFileSync, rmSync, statSync } from "node:fs"; -import { basename, dirname, join, resolve } from "node:path"; +import { basename, dirname, isAbsolute, join, relative, resolve } from "node:path"; import { type TaskState, type TaskStatus, @@ -18,7 +18,7 @@ import { loadAssignmentConfig, loadLauncherConfig, } from "../../config/loader.js"; -import { isPathArg } from "../../config/runtime-paths.js"; +import { isPathArg, resolveTaskRunnerStateDir } from "../../config/runtime-paths.js"; import type { AttachmentListEntry, AttachmentListOptions, @@ -1108,11 +1108,36 @@ export function unarchiveRun( return setRunArchived(target, false, auditOrigin, emitAuditEnvelope); } +function managedGitCloneCheckoutPath(manifest: RunManifest): string | null { + if (!manifest.resolvedHooks.some((hook) => hook.source.builtin === "git-clone")) { + return null; + } + const checkoutPath = manifest.runtimeVars.checkout_path; + if (typeof checkoutPath !== "string") { + return null; + } + const checkoutsRoot = resolve(resolveTaskRunnerStateDir(), "checkouts"); + const resolvedCheckoutPath = resolve(checkoutPath); + const relativeCheckoutPath = relative(checkoutsRoot, resolvedCheckoutPath); + if ( + relativeCheckoutPath.length === 0 || + relativeCheckoutPath.startsWith("..") || + isAbsolute(relativeCheckoutPath) + ) { + return null; + } + return resolvedCheckoutPath; +} + export function deleteRun(target: string): RunDeleteResult { const resolved = resolveRun(target); withTaskStateLock(resolved.workspaceDir, () => { resolved.manifest = resolveResumeTarget(resolved.workspaceDir).manifest; requireDeletableRun(resolved.manifest); + const checkoutPath = managedGitCloneCheckoutPath(resolved.manifest); + if (checkoutPath) { + rmSync(checkoutPath, { recursive: true, force: true }); + } rmSync(resolved.workspaceDir, { recursive: true, force: true }); }); return { runId: resolved.manifest.runId }; diff --git a/packages/core/src/core/hooks/builtin-git-clone.ts b/packages/core/src/core/hooks/builtin-git-clone.ts new file mode 100644 index 00000000..ee8f93b4 --- /dev/null +++ b/packages/core/src/core/hooks/builtin-git-clone.ts @@ -0,0 +1,398 @@ +import { execFileSync } from "node:child_process"; +import { existsSync, mkdirSync, readdirSync, rmSync, statSync } from "node:fs"; +import { dirname, isAbsolute, join, resolve } from "node:path"; +import { resolveTaskRunnerStateDir } from "../../config/runtime-paths.js"; +import { defineHook } from "../../hooks.js"; +import type { HookResult, PrepareHookContext } from "./types.js"; + +type GitCloneCollisionMode = "fail" | "reuse" | "replace"; + +interface GitCloneConfig { + repoUrl: string; + ref?: string; + path?: string; + remoteName: string; + depth?: number; + collision: GitCloneCollisionMode; +} + +interface CheckoutTarget { + path: string; +} + +const CONFIG_KEYS = new Set(["repo_url", "ref", "path", "remote_name", "depth", "collision"]); + +function gitEnv(): NodeJS.ProcessEnv { + return Object.fromEntries( + Object.entries(process.env).filter(([key]) => { + return !key.startsWith("GIT_"); + }), + ); +} + +function requiredString(record: Record, key: string): string { + const value = record[key]; + if (typeof value !== "string" || value.trim().length === 0) { + throw new Error(`git-clone config validation failed: \`${key}\` must be a non-empty string`); + } + return value.trim(); +} + +function rejectLeadingDash(value: string, key: string): void { + if (value.startsWith("-")) { + throw new Error(`git-clone config validation failed: \`${key}\` must not begin with '-'`); + } +} + +function validateRepoUrl(value: string): void { + rejectLeadingDash(value, "repo_url"); + try { + const parsed = new URL(value); + if ( + parsed.password || + ((parsed.protocol === "http:" || parsed.protocol === "https:") && parsed.username) + ) { + throw new Error( + "git-clone config validation failed: `repo_url` must not include embedded credentials", + ); + } + } catch (error) { + if (error instanceof TypeError) { + return; + } + throw error; + } +} + +function optionalNonEmptyString(record: Record, key: string): string | undefined { + if (!(key in record)) { + return undefined; + } + const value = record[key]; + if (typeof value !== "string") { + throw new Error(`git-clone config validation failed: \`${key}\` must be a string`); + } + const trimmed = value.trim(); + if (trimmed.length === 0) { + throw new Error( + `git-clone config validation failed: \`${key}\` must be non-empty when present`, + ); + } + return trimmed; +} + +function optionalRef(record: Record): string | undefined { + if (!("ref" in record)) { + return undefined; + } + const value = record.ref; + if (typeof value !== "string") { + throw new Error("git-clone config validation failed: `ref` must be a string"); + } + const trimmed = value.trim(); + if (trimmed.length === 0) { + return undefined; + } + rejectLeadingDash(trimmed, "ref"); + return trimmed; +} + +function optionalCollision(record: Record): GitCloneCollisionMode | undefined { + if (!("collision" in record)) { + return undefined; + } + const value = record.collision; + if (value !== "fail" && value !== "reuse" && value !== "replace") { + throw new Error( + "git-clone config validation failed: `collision` must be one of: fail, reuse, replace", + ); + } + return value; +} + +function gitCloneConfig(config: unknown): GitCloneConfig { + if (!config || typeof config !== "object" || Array.isArray(config)) { + throw new Error("git-clone config validation failed: hook requires an object config"); + } + const record = config as Record; + const unknownKeys = Object.keys(record).filter((key) => !CONFIG_KEYS.has(key)); + if (unknownKeys.length > 0) { + throw new Error( + `git-clone config validation failed: unknown field(s): ${unknownKeys.join(", ")}`, + ); + } + + const depth = record.depth; + if ( + depth !== undefined && + (typeof depth !== "number" || !Number.isInteger(depth) || depth <= 0) + ) { + throw new Error("git-clone config validation failed: `depth` must be a positive integer"); + } + + const repoUrl = requiredString(record, "repo_url"); + validateRepoUrl(repoUrl); + const path = optionalNonEmptyString(record, "path"); + const remoteName = optionalNonEmptyString(record, "remote_name") ?? "origin"; + rejectLeadingDash(remoteName, "remote_name"); + return { + repoUrl, + ref: optionalRef(record), + path, + remoteName, + depth: depth as number | undefined, + collision: optionalCollision(record) ?? (path ? "fail" : "reuse"), + }; +} + +function lastPathSegment(value: string): string { + const trimmed = value.trim().replace(/\/+$/, ""); + const scpLike = trimmed.match(/^[^/\s@]+@[^:\s]+:(.+)$/); + if (scpLike?.[1]) { + return scpLike[1].split("/").filter(Boolean).at(-1) ?? ""; + } + + try { + const parsed = new URL(trimmed); + const segment = parsed.pathname.split("/").filter(Boolean).at(-1) ?? ""; + try { + return decodeURIComponent(segment); + } catch { + return segment; + } + } catch { + return ( + trimmed + .split(/[/:\\]+/) + .filter(Boolean) + .at(-1) ?? "" + ); + } +} + +export function deriveRepoSlug(repoUrl: string): string { + const tail = lastPathSegment(repoUrl).replace(/\.git$/i, ""); + const slug = tail + .replace(/[^A-Za-z0-9._-]+/g, "-") + .replace(/^[.-]+/, "") + .replace(/[.-]+$/, ""); + if (!/[A-Za-z0-9_]/.test(slug)) { + throw new Error("git-clone invalid slug: could not derive a filesystem-safe repo slug"); + } + return slug; +} + +function resolveCheckoutTarget( + config: GitCloneConfig, + ctx: PrepareHookContext, + repoSlug: string, +): CheckoutTarget { + if (config.path) { + return { + path: isAbsolute(config.path) ? config.path : resolve(ctx.run.cwd, config.path), + }; + } + return { + path: join(resolveTaskRunnerStateDir(), "checkouts", `${repoSlug}-${ctx.run.runId}`), + }; +} + +function checkoutPathState(path: string): "missing" | "empty" | "occupied" { + if (!existsSync(path)) { + return "missing"; + } + const stat = statSync(path); + if (!stat.isDirectory()) { + throw new Error( + `git-clone path collision: checkout path ${path} exists and is not a directory`, + ); + } + return readdirSync(path).length === 0 ? "empty" : "occupied"; +} + +function redactGitOutput(value: string): string { + return value.replace(/((?:https?|ssh):\/\/)([^@\s/]+)@/gi, "$1@"); +} + +function safeErrorContext(error: unknown): string { + const err = error as NodeJS.ErrnoException & { + stderr?: string | Buffer; + stdout?: string | Buffer; + }; + const stderr = + typeof err.stderr === "string" + ? err.stderr + : Buffer.isBuffer(err.stderr) + ? err.stderr.toString("utf8") + : ""; + const stdout = + typeof err.stdout === "string" + ? err.stdout + : Buffer.isBuffer(err.stdout) + ? err.stdout.toString("utf8") + : ""; + const detail = (stderr.trim() || stdout.trim() || err.message || String(error)).trim(); + return redactGitOutput(detail); +} + +function git(args: string[], cwd: string, operation: string): string { + try { + return execFileSync("git", args, { + cwd, + encoding: "utf8", + env: gitEnv(), + stdio: ["ignore", "pipe", "pipe"], + }).trim(); + } catch (error) { + throw new Error(`git-clone ${operation} failed: ${safeErrorContext(error)}`); + } +} + +function cloneArgs(config: GitCloneConfig, checkoutPath: string): string[] { + const args = ["clone", "--origin", config.remoteName]; + if (config.depth !== undefined) { + args.push("--depth", String(config.depth)); + } + args.push("--", config.repoUrl, checkoutPath); + return args; +} + +function fetchArgs(config: GitCloneConfig, ref: string): string[] { + const args = ["fetch"]; + if (config.depth !== undefined) { + args.push("--depth", String(config.depth)); + } + args.push("--", config.remoteName, ref); + return args; +} + +function tagFetchArgs(config: GitCloneConfig, ref: string): string[] { + const args = ["fetch"]; + if (config.depth !== undefined) { + args.push("--depth", String(config.depth)); + } + args.push("--", config.remoteName, "tag", ref); + return args; +} + +function tryGit(args: string[], cwd: string): { ok: true; stdout: string } | { ok: false } { + try { + return { + ok: true, + stdout: execFileSync("git", args, { + cwd, + encoding: "utf8", + env: gitEnv(), + stdio: ["ignore", "pipe", "pipe"], + }).trim(), + }; + } catch { + return { ok: false }; + } +} + +function checkoutRef(config: GitCloneConfig, checkoutPath: string, ref: string): void { + const fetch = tryGit(fetchArgs(config, ref), checkoutPath); + if (fetch.ok) { + git(["checkout", "--detach", "FETCH_HEAD"], checkoutPath, "checkout"); + return; + } + + const tagFetch = tryGit(tagFetchArgs(config, ref), checkoutPath); + if (tagFetch.ok) { + git(["checkout", "--detach", "FETCH_HEAD"], checkoutPath, "checkout"); + return; + } + + git(["checkout", "--detach", "--", ref], checkoutPath, "checkout"); +} + +function resolvedDefaultRef(checkoutPath: string): string | undefined { + const symbolic = tryGit(["symbolic-ref", "--quiet", "--short", "HEAD"], checkoutPath); + if (symbolic.ok && symbolic.stdout.length > 0) { + return symbolic.stdout; + } + return undefined; +} + +function cloneRepository(config: GitCloneConfig, checkoutPath: string): string { + mkdirSync(dirname(checkoutPath), { recursive: true }); + try { + git(cloneArgs(config, checkoutPath), dirname(checkoutPath), "clone"); + if (config.ref) { + checkoutRef(config, checkoutPath, config.ref); + } + return git(["rev-parse", "HEAD"], checkoutPath, "rev-parse"); + } catch (error) { + rmSync(checkoutPath, { recursive: true, force: true }); + throw error; + } +} + +function existingRemoteUrl(config: GitCloneConfig, checkoutPath: string): string | undefined { + const remote = tryGit(["remote", "get-url", config.remoteName], checkoutPath); + return remote.ok ? remote.stdout : undefined; +} + +function reuseRepository(config: GitCloneConfig, checkoutPath: string): string { + const insideWorkTree = tryGit(["rev-parse", "--is-inside-work-tree"], checkoutPath); + const remoteUrl = existingRemoteUrl(config, checkoutPath); + if (!insideWorkTree.ok || remoteUrl !== config.repoUrl) { + throw new Error( + `git-clone path collision: checkout path ${checkoutPath} already exists and is not a reusable clone of ${config.repoUrl}`, + ); + } + if (config.ref) { + checkoutRef(config, checkoutPath, config.ref); + } + return git(["rev-parse", "HEAD"], checkoutPath, "rev-parse"); +} + +function prepareCheckoutPath(config: GitCloneConfig, target: CheckoutTarget): "clone" | "reuse" { + const state = checkoutPathState(target.path); + if (state === "missing" || state === "empty") { + return "clone"; + } + if (config.collision === "replace") { + rmSync(target.path, { recursive: true, force: true }); + return "clone"; + } + if (config.collision === "reuse") { + return "reuse"; + } + throw new Error( + `git-clone path collision: checkout path ${target.path} already exists and is non-empty`, + ); +} + +export default defineHook({ + name: "git-clone", + prepare(ctx: PrepareHookContext): HookResult { + const config = gitCloneConfig(ctx.config); + const repoSlug = deriveRepoSlug(config.repoUrl); + const target = resolveCheckoutTarget(config, ctx, repoSlug); + const checkoutPath = target.path; + const commitSha = + prepareCheckoutPath(config, target) === "reuse" + ? reuseRepository(config, checkoutPath) + : cloneRepository(config, checkoutPath); + const resolvedRef = config.ref ?? resolvedDefaultRef(checkoutPath); + const vars: Record = { + repo_slug: repoSlug, + checkout_path: checkoutPath, + commit_sha: commitSha, + }; + if (resolvedRef) { + vars.resolved_ref = resolvedRef; + } + return { + action: "continue", + mutate: { + run: { + cwd: checkoutPath, + }, + vars, + }, + }; + }, +}); diff --git a/packages/core/src/core/hooks/registry.ts b/packages/core/src/core/hooks/registry.ts index 50ecb946..5a248c28 100644 --- a/packages/core/src/core/hooks/registry.ts +++ b/packages/core/src/core/hooks/registry.ts @@ -1,4 +1,5 @@ import builtinCommandHook from "./builtin-command.js"; +import builtinGitCloneHook from "./builtin-git-clone.js"; import builtinGitWorktreeHook from "./builtin-git-worktree.js"; import builtinRequireChildrenSuccessHook from "./builtin-require-children-success.js"; import { HookConfigError } from "./errors.js"; @@ -7,6 +8,7 @@ import type { HookModule } from "./types.js"; const BUILTIN_HOOKS: Record = { command: builtinCommandHook, "require-children-success": builtinRequireChildrenSuccessHook, + "git-clone": builtinGitCloneHook, "git-worktree": builtinGitWorktreeHook, }; diff --git a/packages/core/src/core/run/reconfigure.ts b/packages/core/src/core/run/reconfigure.ts index 28db883d..a77b41fe 100644 --- a/packages/core/src/core/run/reconfigure.ts +++ b/packages/core/src/core/run/reconfigure.ts @@ -2,13 +2,20 @@ import { copyFileSync, rmSync } from "node:fs"; import { isDeepStrictEqual } from "node:util"; import { resolveBackend } from "../../backends/registry.js"; import { loadAgentConfig, loadAssignmentConfig } from "../../config/loader.js"; +import { + resolveTaskRunnerConfigDir, + resolveTaskRunnerStateDir, +} from "../../config/runtime-paths.js"; import type { ReconfigureRunPatch, RunDetail } from "../../contracts/runs.js"; import { toRunDetail } from "../../contracts/runs.js"; +import { resolveTaskRunnerCommand } from "../../task-runner-command.js"; import { cloneBackendSpecificConfig, cloneResolvedBackendArgs } from "../backends/types.js"; import { cloneResolvedLauncherConfig } from "../config/launchers.js"; import { loadedAgentFromManifest } from "../config/loaded.js"; import type { LoadedAgent, LoadedAssignment } from "../config/loaded.js"; import type { LockableField, VarDef } from "../config/schema.js"; +import { resolveAssignmentHooks } from "../hooks/loader.js"; +import type { ResolvedHookDescriptor } from "../hooks/types.js"; import { type ResolvedResumeTarget, ResumeError, @@ -136,6 +143,52 @@ function buildLoadedAssignment( }; } +function hookSourceKey(descriptor: ResolvedHookDescriptor): string { + const { source } = descriptor; + return `${source.builtin ?? ""}\0${source.name ?? ""}\0${source.path ?? ""}`; +} + +function buildReconfigureHookVars( + manifest: RunManifest, + cliVars: Record, +): Record { + return { + ...manifest.runtimeVars, + ...cliVars, + run_id: manifest.runId, + cwd: manifest.cwd, + config_dir: resolveTaskRunnerConfigDir(), + state_dir: resolveTaskRunnerStateDir(), + task_runner_cmd: resolveTaskRunnerCommand(), + ...(manifest.assignment ? { assignment_name: manifest.assignment.name } : {}), + }; +} + +function buildReconfigureResolvedHooks( + previous: RunManifest, + loadedAssignment: LoadedAssignment | undefined, + cliVars: Record, +): ResolvedHookDescriptor[] { + const next = resolveAssignmentHooks( + loadedAssignment, + buildReconfigureHookVars(previous, cliVars), + ); + return next.map((descriptor) => { + const previousDescriptor = previous.resolvedHooks.find( + (entry) => + entry.hookId === descriptor.hookId && hookSourceKey(entry) === hookSourceKey(descriptor), + ); + if (!previousDescriptor) { + return descriptor; + } + return { + ...descriptor, + source: { ...previousDescriptor.source }, + resolvedPath: previousDescriptor.resolvedPath, + }; + }); +} + function lockedFieldValue(manifest: RunManifest, field: LockableField): unknown { switch (field) { case "cwd": @@ -318,11 +371,7 @@ async function reconfigureResolvedRun( resume: resolved, initialize: true, stageInitialize: true, - resolvedHooksOverride: previous.resolvedHooks.map((descriptor) => ({ - ...descriptor, - source: { ...descriptor.source }, - when: descriptor.when ? { ...descriptor.when } : null, - })), + resolvedHooksOverride: buildReconfigureResolvedHooks(previous, loadedAssignment, cliVars), overrides: loadedAssignment === undefined && nextMessage !== null ? { message: nextMessage } : {}, }); diff --git a/test/command-services.test.mjs b/test/command-services.test.mjs index 361d3c87..cfb04920 100644 --- a/test/command-services.test.mjs +++ b/test/command-services.test.mjs @@ -698,6 +698,7 @@ test("command services: showDefinition resolves built-in code-review named task () => { const implementationReview = showDefinition("assignment", "code-review"); const directReview = showDefinition("assignment", "code-review-direct"); + const cloneReview = showDefinition("assignment", "code-review-clone"); assert.deepEqual( implementationReview.loaded.config.tasks.map((task) => task.id), @@ -707,6 +708,12 @@ test("command services: showDefinition resolves built-in code-review named task directReview.loaded.config.tasks.map((task) => task.id), ["orient", ...SHARED_REVIEW_TASK_IDS, "synthesis", "approval"], ); + assert.deepEqual( + cloneReview.loaded.config.tasks.map((task) => task.id), + ["orient", ...SHARED_REVIEW_TASK_IDS, "synthesis", "approval"], + ); + assert.deepEqual(Object.keys(cloneReview.loaded.config.vars), ["repo_url", "ref", "range"]); + assert.equal(cloneReview.loaded.config.hooks.prepare[0]?.builtin, "git-clone"); }, )); diff --git a/test/config-loader.test.mjs b/test/config-loader.test.mjs index 8ca24074..b69e510d 100644 --- a/test/config-loader.test.mjs +++ b/test/config-loader.test.mjs @@ -95,6 +95,9 @@ const BUILTIN_CODE_REVIEW_PATH = resolvePath( const BUILTIN_CODE_REVIEW_DIRECT_PATH = resolvePath( new URL("../assignments/code-review-direct/assignment.md", import.meta.url).pathname, ); +const BUILTIN_CODE_REVIEW_CLONE_PATH = resolvePath( + new URL("../assignments/code-review-clone/assignment.md", import.meta.url).pathname, +); const BUILTIN_IMPLEMENTER_AGENT_PATH = resolvePath( new URL("../agents/implementer/agent.md", import.meta.url).pathname, ); @@ -1509,6 +1512,53 @@ test("built-in code-review-direct assignment resolves shared review tasks withou assert.doesNotMatch(approval.body ?? "", /plan_coverage/); })); +test("built-in code-review-clone assignment resolves clone hook and shared review tasks", () => + withEnv({ TASK_RUNNER_CONFIG_DIR: REPO_ROOT }, () => { + const loaded = loadAssignmentConfig(BUILTIN_CODE_REVIEW_CLONE_PATH); + + assert.deepEqual( + loaded.config.tasks.map((task) => task.id), + ["orient", ...SHARED_REVIEW_TASK_IDS, "synthesis", "approval"], + ); + assert.deepEqual(Object.keys(loaded.config.vars), ["repo_url", "ref", "range"]); + assert.equal(loaded.config.vars.repo_url?.required, true); + assert.deepEqual(loaded.config.vars.repo_url?.sources, ["cli", "web"]); + assert.equal(loaded.config.vars.ref?.required, false); + assert.deepEqual(loaded.config.vars.ref?.sources, ["cli", "web"]); + assert.equal(loaded.config.vars.range?.required, false); + assert.equal(loaded.config.vars.range?.default, "full"); + assert.deepEqual(loaded.config.vars.range?.sources, ["cli", "web"]); + assert.deepEqual(loaded.config.hooks.prepare, [ + { + builtin: "git-clone", + with: { + repo_url: "{{repo_url}}", + ref: "{{ref}}", + }, + }, + ]); + assert.equal( + loaded.config.tasks.some((task) => task.id === "plan_coverage"), + false, + ); + assert.equal(loaded.config.vars.implementation_run_id, undefined); + + const orient = loaded.config.tasks.find((task) => task.id === "orient"); + const architecture = loaded.config.tasks.find((task) => task.id === "review/architecture"); + const approval = loaded.config.tasks.find((task) => task.id === "approval"); + + assert.ok(orient); + assert.ok(architecture); + assert.ok(approval); + assert.match(orient.body ?? "", /clone-based\s+direct\/ad hoc review/); + assert.match(orient.body ?? "", /{{repo_url}}/); + assert.match(orient.body ?? "", /{{commit_sha}}/); + assert.equal(architecture.title, "Architecture & module boundaries"); + assert.match(architecture.body ?? "", /Review the module layout/); + assert.match(approval.body ?? "", /BLOCKED -- cannot approve/); + assert.doesNotMatch(approval.body ?? "", /plan_coverage/); + })); + test("built-in plan-feature assignment uses cwd instead of repo_path for canonical repo context", () => { const loaded = loadAssignmentConfig(BUILTIN_PLAN_FEATURE_PATH); assert.equal(loaded.config.vars.repo_path, undefined); diff --git a/test/git-clone-hook.test.mjs b/test/git-clone-hook.test.mjs new file mode 100644 index 00000000..80e692a5 --- /dev/null +++ b/test/git-clone-hook.test.mjs @@ -0,0 +1,515 @@ +import { strict as assert } from "node:assert"; +import { execFileSync } from "node:child_process"; +import { + existsSync, + mkdirSync, + mkdtempSync, + readFileSync, + readdirSync, + rmSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { test } from "node:test"; +import { reconfigureRun } from "../packages/core/dist/app/service.js"; +import { loadAgentConfig, loadAssignmentConfig } from "../packages/core/dist/config/loader.js"; +import { + archiveRun, + deleteRun, + readyRun, + resetRun, +} from "../packages/core/dist/core/commands/service.js"; +import { deriveRepoSlug } from "../packages/core/dist/core/hooks/builtin-git-clone.js"; +import { resolveResumeTarget } from "../packages/core/dist/core/run/manifest.js"; +import { runAgent } from "../packages/core/dist/core/run/run-loop.js"; +import { setTaskStatusesForPrompt, withSharedRuntimeEnv } from "./helpers/runtime-paths.mjs"; + +const AGENT = `--- +schemaVersion: 1 +name: reviewer +backend: claude +--- +Review. +`; + +function tempDir() { + return mkdtempSync(join(tmpdir(), "task-runner-git-clone-")); +} + +function gitEnv() { + return Object.fromEntries( + Object.entries(process.env).filter(([key]) => { + return !key.startsWith("GIT_"); + }), + ); +} + +function git(args, cwd) { + return execFileSync("git", args, { cwd, encoding: "utf8", env: gitEnv() }).trim(); +} + +function writeAgent(baseDir) { + const agentDir = join(baseDir, "agents", "reviewer"); + mkdirSync(agentDir, { recursive: true }); + writeFileSync(join(agentDir, "agent.md"), AGENT); +} + +function writeAssignment(baseDir, body) { + const assignmentDir = join(baseDir, "assignments", "clone-review"); + mkdirSync(assignmentDir, { recursive: true }); + writeFileSync(join(assignmentDir, "assignment.md"), body); +} + +function cloneAssignment(extraWith = "") { + return `--- +schemaVersion: 1 +name: clone-review +vars: + repo_url: + type: string + required: true + sources: [cli, web] + ref: + type: string + required: false + sources: [cli, web] +hooks: + prepare: + - builtin: git-clone + with: + repo_url: "{{repo_url}}" + ref: "{{ref}}" +${extraWith} +tasks: + - id: t1 + title: Review +--- +Review cloned repo at {{cwd}}. +`; +} + +function initSourceRepo(baseDir) { + const repoDir = join(baseDir, "repo"); + const hooksDir = join(repoDir, ".githooks-disabled"); + mkdirSync(repoDir, { recursive: true }); + git(["init", "--initial-branch=main", repoDir], baseDir); + mkdirSync(hooksDir, { recursive: true }); + git(["config", "core.hooksPath", hooksDir], repoDir); + git(["config", "user.name", "Task Runner Tests"], repoDir); + git(["config", "user.email", "tests@example.com"], repoDir); + writeFileSync(join(repoDir, "README.md"), "main\n"); + git(["add", "README.md"], repoDir); + git(["commit", "-m", "main"], repoDir); + const mainSha = git(["rev-parse", "HEAD"], repoDir); + + git(["checkout", "-b", "feature"], repoDir); + writeFileSync(join(repoDir, "README.md"), "feature\n"); + git(["commit", "-am", "feature"], repoDir); + const featureSha = git(["rev-parse", "HEAD"], repoDir); + git(["tag", "feature-tag"], repoDir); + git(["checkout", "main"], repoDir); + + return { repoDir, mainSha, featureSha }; +} + +async function runClone(baseDir, options = {}) { + let backendInvoked = false; + const seen = {}; + return await withSharedRuntimeEnv(baseDir, async () => { + const loaded = loadAgentConfig("reviewer", baseDir); + const loadedAssignment = loadAssignmentConfig("clone-review", baseDir); + const outcome = await runAgent({ + loaded, + loadedAssignment, + cliVars: options.cliVars, + webVars: {}, + backend: { + id: "claude", + async invoke(ctx) { + backendInvoked = true; + seen.cwd = ctx.cwd; + setTaskStatusesForPrompt(ctx.prompt, { t1: "completed" }, baseDir); + return { + exitCode: 0, + signal: null, + timedOut: false, + sessionId: "clone-session", + transcript: "done", + rawStdout: "", + rawStderr: "", + }; + }, + }, + callerCwd: baseDir, + initialize: options.initialize ?? false, + }); + return { outcome, backendInvoked, seen }; + }); +} + +test("git-clone slug derivation handles supported URL shapes and rejects unusable slugs", () => { + assert.equal(deriveRepoSlug("git@github.com:kcosr/task-runner.git"), "task-runner"); + assert.equal(deriveRepoSlug("https://github.com/kcosr/task-runner.git"), "task-runner"); + assert.equal(deriveRepoSlug("ssh://git@host/org/my-repo.git"), "my-repo"); + assert.equal(deriveRepoSlug("https://github.com/org/my repo!.git"), "my-repo"); + assert.equal(deriveRepoSlug("file:///tmp/unsafe repo!.git"), "unsafe-repo"); + assert.throws(() => deriveRepoSlug("https://github.com/org/.git"), /git-clone invalid slug/); +}); + +test("git-clone prepare hook clones default ref, mutates cwd, and projects runtime vars", async () => { + const dir = tempDir(); + try { + writeAgent(dir); + writeAssignment(dir, cloneAssignment()); + const { repoDir, mainSha } = initSourceRepo(dir); + + const { outcome, backendInvoked, seen } = await runClone(dir, { + cliVars: { repo_url: repoDir }, + }); + + const expectedPath = join(dir, "checkouts", `repo-${outcome.runId}`); + assert.equal(backendInvoked, true); + assert.equal(seen.cwd, expectedPath); + assert.equal(outcome.manifest.cwd, expectedPath); + assert.equal(outcome.manifest.runtimeVars.repo_slug, "repo"); + assert.equal(outcome.manifest.runtimeVars.checkout_path, expectedPath); + assert.equal(outcome.manifest.runtimeVars.commit_sha, mainSha); + assert.equal(outcome.manifest.runtimeVars.resolved_ref, "main"); + assert.equal(readFileSync(join(expectedPath, "README.md"), "utf8"), "main\n"); + assert.equal(git(["rev-parse", "HEAD"], expectedPath), mainSha); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + +test("git-clone prepare hook checks out supplied refs and records the selected commit", async () => { + const dir = tempDir(); + try { + writeAgent(dir); + writeAssignment(dir, cloneAssignment(" remote_name: upstream\n")); + const { repoDir, featureSha } = initSourceRepo(dir); + + const { outcome } = await runClone(dir, { + cliVars: { repo_url: repoDir, ref: "feature" }, + }); + + const checkoutPath = outcome.manifest.runtimeVars.checkout_path; + assert.equal(outcome.manifest.runtimeVars.commit_sha, featureSha); + assert.equal(outcome.manifest.runtimeVars.resolved_ref, "feature"); + assert.equal(git(["rev-parse", "HEAD"], checkoutPath), featureSha); + assert.equal(git(["remote"], checkoutPath), "upstream"); + assert.equal(readFileSync(join(checkoutPath, "README.md"), "utf8"), "feature\n"); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + +test("git-clone prepare hook rejects option-like config values and credential-bearing HTTPS URLs", async () => { + for (const [extraWith, cliVars, pattern] of [ + ["", { repo_url: "--upload-pack=sh" }, /`repo_url` must not begin with '-'/], + ["", { repo_url: "https://token@example.com/org/repo.git" }, /embedded credentials/], + ["", { repo_url: "https://user:token@example.com/org/repo.git" }, /embedded credentials/], + [ + "", + { repo_url: "file:///tmp/repo.git", ref: "--config=core.sshCommand=sh" }, + /`ref` must not begin with '-'/, + ], + [ + " remote_name: --upload-pack=sh\n", + { repo_url: "file:///tmp/repo.git" }, + /`remote_name` must not begin with '-'/, + ], + ]) { + const dir = tempDir(); + try { + writeAgent(dir); + writeAssignment(dir, cloneAssignment(extraWith)); + let backendInvoked = false; + await assert.rejects( + () => + withSharedRuntimeEnv(dir, async () => { + const loaded = loadAgentConfig("reviewer", dir); + const loadedAssignment = loadAssignmentConfig("clone-review", dir); + await runAgent({ + loaded, + loadedAssignment, + cliVars, + webVars: {}, + backend: { + id: "claude", + async invoke() { + backendInvoked = true; + throw new Error("backend should not run"); + }, + }, + callerCwd: dir, + }); + }), + pattern, + ); + assert.equal(backendInvoked, false); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + } +}); + +test("git-clone prepare failures block before backend invocation", async () => { + const dir = tempDir(); + try { + writeAgent(dir); + const { repoDir } = initSourceRepo(dir); + const collisionDir = join(dir, "collision"); + mkdirSync(collisionDir, { recursive: true }); + writeFileSync(join(collisionDir, "file.txt"), "occupied\n"); + writeAssignment(dir, cloneAssignment(` path: ${JSON.stringify(collisionDir)}\n`)); + + let backendInvoked = false; + await assert.rejects( + () => + withSharedRuntimeEnv(dir, async () => { + const loaded = loadAgentConfig("reviewer", dir); + const loadedAssignment = loadAssignmentConfig("clone-review", dir); + await runAgent({ + loaded, + loadedAssignment, + cliVars: { repo_url: repoDir }, + webVars: {}, + backend: { + id: "claude", + async invoke() { + backendInvoked = true; + throw new Error("backend should not run"); + }, + }, + callerCwd: dir, + }); + }), + /git-clone path collision/, + ); + assert.equal(backendInvoked, false); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + +test("git-clone prepare hook reuses managed checkouts during initialized reconfigure", async () => { + const dir = tempDir(); + try { + writeAgent(dir); + writeAssignment(dir, cloneAssignment()); + const { repoDir, featureSha } = initSourceRepo(dir); + + const { outcome } = await runClone(dir, { + cliVars: { repo_url: repoDir, ref: "main" }, + initialize: true, + }); + const checkoutPath = outcome.manifest.runtimeVars.checkout_path; + assert.equal(readFileSync(join(checkoutPath, "README.md"), "utf8"), "main\n"); + + await withSharedRuntimeEnv(dir, () => + reconfigureRun(outcome.runId, { + vars: { ref: "feature" }, + }), + ); + const manifest = JSON.parse(readFileSync(join(outcome.workspaceDir, "run.json"), "utf8")); + + assert.equal(manifest.runtimeVars.checkout_path, checkoutPath); + assert.equal(manifest.runtimeVars.commit_sha, featureSha); + assert.equal(manifest.runtimeVars.resolved_ref, "feature"); + assert.equal(git(["rev-parse", "HEAD"], checkoutPath), featureSha); + assert.equal(readFileSync(join(checkoutPath, "README.md"), "utf8"), "feature\n"); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + +test("git-clone reset and ready-start reuse the frozen managed checkout", async () => { + const dir = tempDir(); + try { + writeAgent(dir); + writeAssignment(dir, cloneAssignment()); + const { repoDir, mainSha } = initSourceRepo(dir); + + const { outcome } = await runClone(dir, { + cliVars: { repo_url: repoDir }, + }); + const checkoutPath = outcome.manifest.runtimeVars.checkout_path; + + await withSharedRuntimeEnv(dir, () => { + resetRun(outcome.runId); + readyRun(outcome.runId); + }); + const target = withSharedRuntimeEnv(dir, () => resolveResumeTarget(outcome.runId, dir)); + let resumedCwd = null; + const resumed = await withSharedRuntimeEnv(dir, async () => { + const loaded = loadAgentConfig("reviewer", dir); + return await runAgent({ + loaded, + loadedAssignment: undefined, + cliVars: {}, + webVars: {}, + backend: { + id: "claude", + async invoke(ctx) { + resumedCwd = ctx.cwd; + setTaskStatusesForPrompt(ctx.prompt, { t1: "completed" }, dir); + return { + exitCode: 0, + signal: null, + timedOut: false, + sessionId: "clone-resumed-session", + transcript: "done", + rawStdout: "", + rawStderr: "", + }; + }, + }, + callerCwd: dir, + resume: target, + }); + }); + + assert.equal(resumedCwd, checkoutPath); + assert.equal(resumed.manifest.status, "success"); + assert.equal(git(["rev-parse", "HEAD"], checkoutPath), mainSha); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + +test("git-clone managed checkouts are removed when the run is deleted", async () => { + const dir = tempDir(); + try { + writeAgent(dir); + writeAssignment(dir, cloneAssignment()); + const { repoDir } = initSourceRepo(dir); + + const { outcome } = await runClone(dir, { + cliVars: { repo_url: repoDir }, + }); + const checkoutPath = outcome.manifest.runtimeVars.checkout_path; + assert.equal(existsSync(checkoutPath), true); + + await withSharedRuntimeEnv(dir, () => { + archiveRun(outcome.runId); + deleteRun(outcome.runId); + }); + + assert.equal(existsSync(checkoutPath), false); + assert.equal(existsSync(outcome.workspaceDir), false); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + +test("git-clone prepare hook rejects camelCase config aliases", async () => { + for (const alias of ["repoUrl", "remoteName"]) { + const dir = tempDir(); + try { + writeAgent(dir); + const { repoDir } = initSourceRepo(dir); + writeAssignment( + dir, + cloneAssignment( + ` ${alias}: ${JSON.stringify(alias === "repoUrl" ? repoDir : "upstream")}\n`, + ), + ); + let backendInvoked = false; + await assert.rejects( + () => + withSharedRuntimeEnv(dir, async () => { + const loaded = loadAgentConfig("reviewer", dir); + const loadedAssignment = loadAssignmentConfig("clone-review", dir); + await runAgent({ + loaded, + loadedAssignment, + cliVars: { repo_url: repoDir }, + webVars: {}, + backend: { + id: "claude", + async invoke() { + backendInvoked = true; + throw new Error("backend should not run"); + }, + }, + callerCwd: dir, + }); + }), + new RegExp(`unknown field\\(s\\): ${alias}`), + ); + assert.equal(backendInvoked, false); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + } +}); + +test("git-clone prepare fails clearly for invalid slugs and missing refs", async () => { + const invalidDir = tempDir(); + try { + writeAgent(invalidDir); + writeAssignment(invalidDir, cloneAssignment()); + let backendInvoked = false; + await assert.rejects( + () => + withSharedRuntimeEnv(invalidDir, async () => { + const loaded = loadAgentConfig("reviewer", invalidDir); + const loadedAssignment = loadAssignmentConfig("clone-review", invalidDir); + await runAgent({ + loaded, + loadedAssignment, + cliVars: { repo_url: "https://github.com/org/.git" }, + webVars: {}, + backend: { + id: "claude", + async invoke() { + backendInvoked = true; + throw new Error("backend should not run"); + }, + }, + callerCwd: invalidDir, + }); + }), + /git-clone invalid slug/, + ); + assert.equal(backendInvoked, false); + } finally { + rmSync(invalidDir, { recursive: true, force: true }); + } + + const missingRefDir = tempDir(); + try { + writeAgent(missingRefDir); + writeAssignment(missingRefDir, cloneAssignment()); + const { repoDir } = initSourceRepo(missingRefDir); + let backendInvoked = false; + await assert.rejects( + () => + withSharedRuntimeEnv(missingRefDir, async () => { + const loaded = loadAgentConfig("reviewer", missingRefDir); + const loadedAssignment = loadAssignmentConfig("clone-review", missingRefDir); + await runAgent({ + loaded, + loadedAssignment, + cliVars: { repo_url: repoDir, ref: "missing-ref" }, + webVars: {}, + backend: { + id: "claude", + async invoke() { + backendInvoked = true; + throw new Error("backend should not run"); + }, + }, + callerCwd: missingRefDir, + }); + }), + /git-clone checkout failed/, + ); + assert.equal(backendInvoked, false); + assert.deepEqual(readdirSync(join(missingRefDir, "checkouts")), []); + } finally { + rmSync(missingRefDir, { recursive: true, force: true }); + } +}); diff --git a/test/static-input-surface.test.mjs b/test/static-input-surface.test.mjs index 5728ad09..8920b74c 100644 --- a/test/static-input-surface.test.mjs +++ b/test/static-input-surface.test.mjs @@ -22,6 +22,10 @@ const CODE_REVIEW_DIRECT_ASSIGNMENT_PATH = new URL( "../assignments/code-review-direct/assignment.md", import.meta.url, ).pathname; +const CODE_REVIEW_CLONE_ASSIGNMENT_PATH = new URL( + "../assignments/code-review-clone/assignment.md", + import.meta.url, +).pathname; function writeAgent(baseDir, name, body) { const agentDir = join(baseDir, "agents", name); @@ -117,6 +121,10 @@ test("static input surface: review assignments expose the correct CLI/Web inputs loadedAgent, loadAssignmentConfig(CODE_REVIEW_DIRECT_ASSIGNMENT_PATH, REPO_ROOT), ); + const cloneReview = resolveStaticInputSurface( + loadedAgent, + loadAssignmentConfig(CODE_REVIEW_CLONE_ASSIGNMENT_PATH, REPO_ROOT), + ); assert.deepEqual( implementationReview.assignmentInputs.map((field) => field.key), @@ -136,6 +144,19 @@ test("static input surface: review assignments expose the correct CLI/Web inputs assert.ok( !directReview.assignmentInputs.some((field) => field.key === "implementation_run_id"), ); + + assert.deepEqual( + cloneReview.assignmentInputs.map((field) => field.key), + ["repo_url", "ref", "range"], + ); + assert.equal(fieldByKey(cloneReview.assignmentInputs, "repo_url").required, true); + assert.equal(fieldByKey(cloneReview.assignmentInputs, "repo_url").source, "available_override"); + assert.equal(fieldByKey(cloneReview.assignmentInputs, "repo_url").valueStatus, "unset"); + assert.notEqual(fieldByKey(cloneReview.assignmentInputs, "ref").required, true); + assert.equal(fieldByKey(cloneReview.assignmentInputs, "ref").source, "available_override"); + assert.equal(fieldByKey(cloneReview.assignmentInputs, "ref").valueStatus, "unset"); + assert.notEqual(fieldByKey(cloneReview.assignmentInputs, "range").required, true); + assert.equal(fieldByKey(cloneReview.assignmentInputs, "range").value, "full"); })); test("static input surface: launcher path and inline definitions preserve authored values", () =>