From 099f70ddc094ba0d400d6a472147cb205b63dde1 Mon Sep 17 00:00:00 2001 From: Task Runner Tests Date: Mon, 27 Apr 2026 16:49:15 -0500 Subject: [PATCH 1/3] Add clone-based code review workflow --- CHANGELOG.md | 2 + assignments/code-review-clone/assignment.md | 260 ++++++++++++++ docs/agents-and-assignments.md | 35 ++ docs/cli.md | 15 + docs/configuration.md | 5 +- docs/daemon.md | 31 +- docs/examples.md | 29 +- docs/variables.md | 25 ++ .../core/src/core/hooks/builtin-git-clone.ts | 294 +++++++++++++++ packages/core/src/core/hooks/registry.ts | 2 + test/command-services.test.mjs | 7 + test/config-loader.test.mjs | 50 +++ test/git-clone-hook.test.mjs | 338 ++++++++++++++++++ test/static-input-surface.test.mjs | 21 ++ 14 files changed, 1107 insertions(+), 7 deletions(-) create mode 100644 assignments/code-review-clone/assignment.md create mode 100644 packages/core/src/core/hooks/builtin-git-clone.ts create mode 100644 test/git-clone-hook.test.mjs diff --git a/CHANGELOG.md b/CHANGELOG.md index 7fe7dc22..c0a42898 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 diff --git a/assignments/code-review-clone/assignment.md b/assignments/code-review-clone/assignment.md new file mode 100644 index 00000000..260fd50e --- /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. Do not embed credentials. + 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..e8b0411e 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,36 @@ 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 +``` + +- `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. + +If the checkout path already exists and is non-empty, `git-clone` fails +before cloning. On success it emits `repo_slug`, `checkout_path`, +`commit_sha`, and `resolved_ref` when the ref can be determined reliably. +Do not put credentials in `repo_url`: runtime vars are persisted in +`run.json`. 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..9c415da2 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 +``` + +Do not embed credentials in `repo_url`; runtime vars are persisted in the +run manifest. 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..c261e44d 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. Do not embed credentials in `repo_url`; it is persisted in +runtime vars. 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..9c8a3786 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. Do not embed credentials. + - `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..ca8594ec 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. Do not include credentials in that URL; 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/hooks/builtin-git-clone.ts b/packages/core/src/core/hooks/builtin-git-clone.ts new file mode 100644 index 00000000..6d4e8cf6 --- /dev/null +++ b/packages/core/src/core/hooks/builtin-git-clone.ts @@ -0,0 +1,294 @@ +import { execFileSync } from "node:child_process"; +import { existsSync, mkdirSync, readdirSync, 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"; + +interface GitCloneConfig { + repoUrl: string; + ref?: string; + path?: string; + remoteName: string; + depth?: number; +} + +const CONFIG_KEYS = new Set(["repo_url", "ref", "path", "remote_name", "depth"]); + +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 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(); + return trimmed.length === 0 ? undefined : trimmed; +} + +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"); + } + + return { + repoUrl: requiredString(record, "repo_url"), + ref: optionalRef(record), + path: optionalNonEmptyString(record, "path"), + remoteName: optionalNonEmptyString(record, "remote_name") ?? "origin", + depth: depth as number | undefined, + }; +} + +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 resolveCheckoutPath(config: GitCloneConfig, ctx: PrepareHookContext, repoSlug: string) { + if (config.path) { + return isAbsolute(config.path) ? config.path : resolve(ctx.run.cwd, config.path); + } + return join(resolveTaskRunnerStateDir(), "checkouts", `${repoSlug}-${ctx.run.runId}`); +} + +function assertCheckoutPathAvailable(path: string): void { + if (!existsSync(path)) { + return; + } + const stat = statSync(path); + if (!stat.isDirectory()) { + throw new Error( + `git-clone path collision: checkout path ${path} exists and is not a directory`, + ); + } + if (readdirSync(path).length > 0) { + throw new Error( + `git-clone path collision: checkout path ${path} already exists and is non-empty`, + ); + } +} + +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 }); + git(cloneArgs(config, checkoutPath), dirname(checkoutPath), "clone"); + if (config.ref) { + checkoutRef(config, checkoutPath, config.ref); + } + return git(["rev-parse", "HEAD"], checkoutPath, "rev-parse"); +} + +export default defineHook({ + name: "git-clone", + prepare(ctx: PrepareHookContext): HookResult { + const config = gitCloneConfig(ctx.config); + const repoSlug = deriveRepoSlug(config.repoUrl); + const checkoutPath = resolveCheckoutPath(config, ctx, repoSlug); + assertCheckoutPathAvailable(checkoutPath); + const commitSha = 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/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..55abe57e --- /dev/null +++ b/test/git-clone-hook.test.mjs @@ -0,0 +1,338 @@ +import { strict as assert } from "node:assert"; +import { execFileSync } from "node:child_process"; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { test } from "node:test"; +import { loadAgentConfig, loadAssignmentConfig } from "../packages/core/dist/config/loader.js"; +import { deriveRepoSlug } from "../packages/core/dist/core/hooks/builtin-git-clone.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, + }); + 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 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 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.equal(existsSync(join(missingRefDir, "checkouts")), true); + } 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", () => From 1c0c91b9d035a08ecf518c88c75091746f5c9ed4 Mon Sep 17 00:00:00 2001 From: Task Runner Tests Date: Mon, 27 Apr 2026 17:35:53 -0500 Subject: [PATCH 2/3] Harden clone review workflow --- CHANGELOG.md | 4 + README.md | 5 +- assignments/code-review-clone/assignment.md | 2 +- docs/agents-and-assignments.md | 20 +- docs/cli.md | 4 +- docs/daemon.md | 8 +- docs/examples.md | 2 +- docs/variables.md | 4 +- packages/core/src/core/commands/service.ts | 29 ++- .../core/src/core/hooks/builtin-git-clone.ts | 152 ++++++++++++--- packages/core/src/core/run/reconfigure.ts | 59 +++++- test/git-clone-hook.test.mjs | 181 +++++++++++++++++- 12 files changed, 419 insertions(+), 51 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c0a42898..edef3315 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -331,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/assignments/code-review-clone/assignment.md b/assignments/code-review-clone/assignment.md index 260fd50e..63276d79 100644 --- a/assignments/code-review-clone/assignment.md +++ b/assignments/code-review-clone/assignment.md @@ -6,7 +6,7 @@ vars: type: string required: true sources: [cli, web] - description: Git SSH/HTTP URL to clone for review. Do not embed credentials. + description: Git SSH/HTTP URL to clone for review. HTTPS URLs with embedded userinfo are rejected. ref: type: string required: false diff --git a/docs/agents-and-assignments.md b/docs/agents-and-assignments.md index e8b0411e..2beb55fd 100644 --- a/docs/agents-and-assignments.md +++ b/docs/agents-and-assignments.md @@ -489,6 +489,7 @@ hooks: path: /tmp/task-runner-review-checkout remote_name: origin depth: 1 + collision: reuse ``` - `repo_url` is required and must be a non-empty string. @@ -500,12 +501,19 @@ hooks: path segment. - `remote_name` is optional and defaults to `origin`. - `depth` is optional and must be a positive integer. - -If the checkout path already exists and is non-empty, `git-clone` fails -before cloning. On success it emits `repo_slug`, `checkout_path`, -`commit_sha`, and `resolved_ref` when the ref can be determined reliably. -Do not put credentials in `repo_url`: runtime vars are persisted in -`run.json`. Use SSH agents or Git credential helpers instead. +- `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 diff --git a/docs/cli.md b/docs/cli.md index 9c415da2..39ce4f9a 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -142,8 +142,8 @@ task-runner run \ --var range=origin/main..HEAD ``` -Do not embed credentials in `repo_url`; runtime vars are persisted in the -run manifest. Use SSH agents or Git credential helpers for private repos. +HTTPS URLs with embedded userinfo are rejected before the run manifest is +written; use SSH agents or Git credential helpers for private repos. ## `init` diff --git a/docs/daemon.md b/docs/daemon.md index c261e44d..a5e778e7 100644 --- a/docs/daemon.md +++ b/docs/daemon.md @@ -203,10 +203,10 @@ 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. Do not embed credentials in `repo_url`; it is persisted in -runtime vars. Use SSH agents or Git credential helpers instead. Future -deployments that expose the daemon beyond localhost may also want repo URL -allowlists. +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: diff --git a/docs/examples.md b/docs/examples.md index 9c8a3786..ed66d252 100644 --- a/docs/examples.md +++ b/docs/examples.md @@ -184,7 +184,7 @@ synthesis and approval decision. - Path: `assignments/code-review-clone/assignment.md` - Vars: - `repo_url` (string, required) - Git SSH/HTTP URL to clone for - review. Do not embed credentials. + 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 diff --git a/docs/variables.md b/docs/variables.md index ca8594ec..613334d7 100644 --- a/docs/variables.md +++ b/docs/variables.md @@ -216,8 +216,8 @@ hook persists: | `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. Do not include credentials in that URL; use SSH agents -or Git credential helpers instead. +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 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 index 6d4e8cf6..ee8f93b4 100644 --- a/packages/core/src/core/hooks/builtin-git-clone.ts +++ b/packages/core/src/core/hooks/builtin-git-clone.ts @@ -1,19 +1,26 @@ import { execFileSync } from "node:child_process"; -import { existsSync, mkdirSync, readdirSync, statSync } from "node:fs"; +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"]); +const CONFIG_KEYS = new Set(["repo_url", "ref", "path", "remote_name", "depth", "collision"]); function gitEnv(): NodeJS.ProcessEnv { return Object.fromEntries( @@ -31,6 +38,32 @@ function requiredString(record: Record, key: string): 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; @@ -57,7 +90,24 @@ function optionalRef(record: Record): string | undefined { throw new Error("git-clone config validation failed: `ref` must be a string"); } const trimmed = value.trim(); - return trimmed.length === 0 ? undefined : trimmed; + 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 { @@ -80,12 +130,18 @@ function gitCloneConfig(config: unknown): GitCloneConfig { 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: requiredString(record, "repo_url"), + repoUrl, ref: optionalRef(record), - path: optionalNonEmptyString(record, "path"), - remoteName: optionalNonEmptyString(record, "remote_name") ?? "origin", + path, + remoteName, depth: depth as number | undefined, + collision: optionalCollision(record) ?? (path ? "fail" : "reuse"), }; } @@ -126,16 +182,24 @@ export function deriveRepoSlug(repoUrl: string): string { return slug; } -function resolveCheckoutPath(config: GitCloneConfig, ctx: PrepareHookContext, repoSlug: string) { +function resolveCheckoutTarget( + config: GitCloneConfig, + ctx: PrepareHookContext, + repoSlug: string, +): CheckoutTarget { if (config.path) { - return isAbsolute(config.path) ? config.path : resolve(ctx.run.cwd, config.path); + return { + path: isAbsolute(config.path) ? config.path : resolve(ctx.run.cwd, config.path), + }; } - return join(resolveTaskRunnerStateDir(), "checkouts", `${repoSlug}-${ctx.run.runId}`); + return { + path: join(resolveTaskRunnerStateDir(), "checkouts", `${repoSlug}-${ctx.run.runId}`), + }; } -function assertCheckoutPathAvailable(path: string): void { +function checkoutPathState(path: string): "missing" | "empty" | "occupied" { if (!existsSync(path)) { - return; + return "missing"; } const stat = statSync(path); if (!stat.isDirectory()) { @@ -143,11 +207,7 @@ function assertCheckoutPathAvailable(path: string): void { `git-clone path collision: checkout path ${path} exists and is not a directory`, ); } - if (readdirSync(path).length > 0) { - throw new Error( - `git-clone path collision: checkout path ${path} already exists and is non-empty`, - ); - } + return readdirSync(path).length === 0 ? "empty" : "occupied"; } function redactGitOutput(value: string): string { @@ -193,7 +253,7 @@ function cloneArgs(config: GitCloneConfig, checkoutPath: string): string[] { if (config.depth !== undefined) { args.push("--depth", String(config.depth)); } - args.push(config.repoUrl, checkoutPath); + args.push("--", config.repoUrl, checkoutPath); return args; } @@ -202,7 +262,7 @@ function fetchArgs(config: GitCloneConfig, ref: string): string[] { if (config.depth !== undefined) { args.push("--depth", String(config.depth)); } - args.push(config.remoteName, ref); + args.push("--", config.remoteName, ref); return args; } @@ -211,7 +271,7 @@ function tagFetchArgs(config: GitCloneConfig, ref: string): string[] { if (config.depth !== undefined) { args.push("--depth", String(config.depth)); } - args.push(config.remoteName, "tag", ref); + args.push("--", config.remoteName, "tag", ref); return args; } @@ -244,7 +304,7 @@ function checkoutRef(config: GitCloneConfig, checkoutPath: string, ref: string): return; } - git(["checkout", "--detach", ref], checkoutPath, "checkout"); + git(["checkout", "--detach", "--", ref], checkoutPath, "checkout"); } function resolvedDefaultRef(checkoutPath: string): string | undefined { @@ -257,21 +317,65 @@ function resolvedDefaultRef(checkoutPath: string): string | undefined { function cloneRepository(config: GitCloneConfig, checkoutPath: string): string { mkdirSync(dirname(checkoutPath), { recursive: true }); - git(cloneArgs(config, checkoutPath), dirname(checkoutPath), "clone"); + 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 checkoutPath = resolveCheckoutPath(config, ctx, repoSlug); - assertCheckoutPathAvailable(checkoutPath); - const commitSha = cloneRepository(config, checkoutPath); + 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, 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/git-clone-hook.test.mjs b/test/git-clone-hook.test.mjs index 55abe57e..80e692a5 100644 --- a/test/git-clone-hook.test.mjs +++ b/test/git-clone-hook.test.mjs @@ -1,11 +1,27 @@ import { strict as assert } from "node:assert"; import { execFileSync } from "node:child_process"; -import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +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"; @@ -126,6 +142,7 @@ async function runClone(baseDir, options = {}) { }, }, callerCwd: baseDir, + initialize: options.initialize ?? false, }); return { outcome, backendInvoked, seen }; }); @@ -188,6 +205,56 @@ test("git-clone prepare hook checks out supplied refs and records the selected c } }); +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 { @@ -227,6 +294,116 @@ test("git-clone prepare failures block before backend invocation", async () => { } }); +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(); @@ -331,7 +508,7 @@ test("git-clone prepare fails clearly for invalid slugs and missing refs", async /git-clone checkout failed/, ); assert.equal(backendInvoked, false); - assert.equal(existsSync(join(missingRefDir, "checkouts")), true); + assert.deepEqual(readdirSync(join(missingRefDir, "checkouts")), []); } finally { rmSync(missingRefDir, { recursive: true, force: true }); } From 02659ab469f00dd53c33fef7adda709b0b1ecd50 Mon Sep 17 00:00:00 2001 From: Task Runner Tests Date: Mon, 27 Apr 2026 21:35:33 -0500 Subject: [PATCH 3/3] Stabilize live attempt resume selection --- apps/web/src/app.test.tsx | 182 +++++++++++++++++- apps/web/src/components/run-detail-drawer.tsx | 59 +++++- apps/web/src/lib/run-timeline.ts | 16 +- 3 files changed, 244 insertions(+), 13 deletions(-) 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(); } },