feat(workflows): run Ralph stages from optional git worktree

[lavaman131]

Summary

Adds an optional `git_worktree_dir` input to the Ralph workflow so stages can run from a detached Git worktree instead of the caller's checkout. Pairs with side-effect-free workflow discovery, runtime empty-graph validation, and `ctx.cwd` exposure to replace the previous static source-inspection approach.

Key Changes

Ralph Workflow (`packages/workflows/builtin/ralph.ts`)

• New `git_worktree_dir` input (`string`, default `""`) — empty string preserves existing no-worktree behavior.
• Git repository requirement — when set, Ralph fails fast unless invoked from inside a Git repository.
• Path resolution — absolute paths used as-is; relative paths resolve from repository root.
• Worktree creation — missing paths created with `git worktree add --detach <base_branch>`.
• Worktree reuse — existing same-repository Git worktrees are reused as-is, including uncommitted state, enabling retry semantics without manual cleanup.
• No automatic cleanup — Ralph leaves `git_worktree_dir` intact after success or failure; callers handle cleanup.
• Fail-fast guards — rejects null-byte paths, missing `base_branch` for new worktrees, non-Git directories, and foreign-repository checkouts.
• Stable spec files — planner iterations revise one stable spec file rather than creating suffixed collision files.

Worktree Bindings (`packages/workflows/src/runs/shared/worktree.ts`)

• Adds `setupGitWorktree` / `GitWorktreeSetupOptions` / `GitWorktreeSetupResult` — the low-level primitive that resolves, creates, or reuses a git worktree and returns an effective `cwd`.
• Git env sanitization — scrubs Git-local env vars and disables hooks/fsmonitor before internal bookkeeping commands to avoid inheriting nested-Git state.
• Symlink-preserving path canonicalization — logical paths through symlinks are preserved when a grandparent is a symlink, avoiding broken paths inside Docker/container mounts.
• `worktree` and `gitWorktreeDir` are now mutually exclusive; combining them throws a clear error.

Executor (`packages/workflows/src/runs/foreground/executor.ts`)

• `ctx.cwd` is now exposed on `WorkflowRunContext`, explicitly set from the named-workflow runner's invocation directory.
• Input runtime defaults — `gitWorktreeDir`/`baseBranch` from workflow `inputBindings.worktree` propagate automatically to all stages as defaults.
• `stageOptionsWithGitWorktree` — resolves `gitWorktreeDir` into a concrete `cwd` at stage dispatch time, transparent to workflow authors.
• Runtime empty-graph validation — workflows that complete without creating any stages are recorded as failed with a clear error message (`assertWorkflowCreatedStage`).

Workflow Discovery (`packages/workflows/src/extension/discovery.ts`)

• Removes `Function.prototype.toString()` regex check for `.stage()` / `.task()` / `.chain()` / `.parallel()` — discovery is now fully side-effect-free (shape-only validation via `validateDefinitionShape`).
• `applyBatch` is now `async`; bundled startup seeding retains a synchronous `applyBatchShapeOnly` path.
• Runtime empty-graph validation in the executor is the authoritative guard replacing the removed static check.

Shared Prompts (`packages/workflows/builtin/shared-prompts.ts`)

• Adds `WORKER_PREFLIGHT_CONTRACT` instructing workers to infer project setup from repository evidence (manifests, lockfiles, CI configs) and run setup commands before implementation work.
• Injected into both Goal and Ralph worker prompts as a `<project_initialization_preflight>` block.

Deep-Research Workflow (`packages/workflows/builtin/deep-research-codebase.ts`)

• Artifact root and `countCodebaseLines` are now `cwd`-relative, enabling correct artifact placement when the workflow runs from a worktree.
• `displayPathFrom` helper renders paths relative to the invocation cwd for cleaner output.

Tests

• `test/integration/ralph-worktree.test.ts` (new, 12 tests) — real Git fixture coverage: relative/absolute paths, fail-fast outside Git, missing `base_branch`, non-Git directories, foreign checkouts/worktrees, same-repository worktree reuse, retry semantics, multi-iteration cwd propagation, and failure preservation.
• `test/unit/builtin-workflows.test.ts` — `git_worktree_dir` input contract, no-worktree cwd behavior, PR-stage guidance, removed cleanup markers.
• `test/unit/discovery.test.ts` / `test/unit/executor.test.ts` — side-effect-free structural discovery, non-execution of workflow bodies during discovery, runtime empty-graph validation.
• Parallelizable cwd-sensitive tests — Ralph and deep-research tests now pass explicit `cwd` seams instead of mutating `process.cwd()`.

Breaking Changes

None — `git_worktree_dir` defaults to `""` preserving existing behavior. The workflow SDK authoring API and compiled `WorkflowDefinition` shape are unchanged.

[claude]

Review: Optional Ralph worktree support

Thanks for the well-scoped change — the worktree plumbing is generally clean, the env-var scrubbing is the right call for hook contexts, and normalizeBranchInput + arg-array spawnSync (no shell) keep this safe from injection via base_branch or git_worktree_dir. A few things worth addressing:

Bugs / correctness

1. No worktree cleanup means re-runs fail. createGitWorktreeIfRequested calls git worktree add --detach <dir> <base_branch>, which fails if <dir> already exists. Nothing in the workflow ever calls git worktree remove, so:

   • Running Ralph twice with the same explicit git_worktree_dir will fail on the second run.
   • The fallback path is deterministic from repoRoot + baseBranch, so re-running without an explicit dir against the same base branch will also fail on the second run.
   • Each successful run leaks a .git/worktrees/<dir> entry and the on-disk checkout indefinitely.

   Consider either (a) removing the worktree at the end of .run in a finally, (b) reusing an existing worktree at the same path when it already points at the expected base branch, or (c) appending a run-id suffix to the default fallback path so collisions can't happen. Even just documenting that the caller is responsible for cleanup would help.

2. Silent fallback can mask real misuse. When a caller explicitly sets git_worktree_dir but git worktree add fails (parent dir unwritable, path already taken, base branch resolution failed, …), createGitWorktreeIfRequested silently swallows the failure and quietly switches to the deterministic tmpdir path. From the caller's POV, plan_path and stages now live somewhere other than what was requested with no signal. At a minimum, surface the requested-path failure (warning + reason) before falling back, or fail fast when the user gave an explicit path. The \0-rejection case is covered, but a git worktree add failure on an explicit path is the more likely real-world scenario.

3. Spec file in the worktree becomes an untracked file in the worktree's working tree. writeSpecFile(pathInWorkflowCwd(defaultSpecPath(prompt), workflowCwd), …) writes <worktree>/specs/<date>-<slug>.md. When the PR stage later runs git status --short from cwdOption, the spec is going to show up as an untracked file and could end up committed/PR'd by accident. Worth either writing the spec outside the worktree, gitignoring specs/ inside the worktree, or explicitly telling the PR stage to exclude it.

Goal worker contract

4. WORKER_RECEIPT_CONTRACT semantically mixes "how to do the work" with "how to report on it". The nine new bullets are all about performing preflight setup ("run or delegate the appropriate documented setup command", "you are responsible for initializing the checkout"). The contract is consumed as <worker_turn_contract>, so it does get applied to the worker turn, but the name and the rest of the bullets are framed as evidence-reporting rules. In ralph.ts you (correctly) put the same guidance under a dedicated project_initialization_preflight tag rather than mixing it into the report contract. Consider doing the same in Goal — split into WORKER_PREFLIGHT_CONTRACT (preflight) and WORKER_RECEIPT_CONTRACT (evidence framing), or at least rename the constant. The current arrangement makes the prompt structure inconsistent across the two workflows.

Test coverage gaps

5. The two new tests cover (a) relative path success and (b) explicit \0-rejection → fallback. Worth adding:

   • Absolute git_worktree_dir — the isAbsolute branch in resolveOptionalWorktreePath is currently uncovered.
   • Explicit path fails at git worktree add time — e.g., point at a path under a non-writable parent or an existing non-empty dir; verifies the silent-fallback path (point updates to readme and instructions #2 above) intentionally, or fails the test if you change to surface-and-throw.
   • Idempotent re-run — call d.run(ctx) twice with the same inputs; this is the test that would catch the cleanup gap from point add agent instructions #1.
   • No git_worktree_dir — assert that taskOptions[*].cwd is undefined so the default behavior is locked in (the existing tests don't exercise the new code path's undefined branch).

6. The new tests process.chdir(repo) but the describe("ralph") block lacks per-test cwd save/restore. Each test restores in its own finally, which is fine in isolation, but the deep-research-codebase describe block has explicit beforeEach/afterEach for exactly this; following the same pattern in describe("ralph") would prevent a future test from leaking cwd.

Minor / nits

7. GIT_LOCAL_ENV_VARS is a hand-maintained subset. Worth a one-line comment pointing at git's git(1) "ENVIRONMENT VARIABLES" section noting that this list intentionally scrubs worktree-scoping variables only (not GIT_AUTHOR_* etc., which the workflow probably wants to inherit). Future contributors will otherwise wonder why some vars are excluded.

8. runGit defaults cwd to process.cwd(); that's fine, but for the rev-parse --show-toplevel call inside defaultGitWorktreePath it means the fallback path is anchored to the caller's current dir. If a future caller invokes Ralph from a path outside the host repo this will throw. Worth a one-line doc on git_worktree_dir describing the assumption that the workflow is invoked from inside the host repo.

9. The goal.ts diff has substantial whitespace/reflow churn (looks like a different Prettier printWidth was applied) that's unrelated to the substantive worker-contract changes. The vast majority of the touched lines in goal.ts are formatting-only. It would help reviewers (and future `git blame` users) to split formatting and behavior changes into separate commits, or revert the unrelated reflows.

10. errorMessage is defined locally; if a similar helper already exists elsewhere in packages/workflows, consider hoisting a shared helper rather than duplicating.

What looks good

• gitProcessEnv scrubbing is the right fix for invocation from inside Git hooks — nice catch.
• safeWorktreePathSegment correctly slugifies branch names like origin/main → origin-main and caps length, so the fallback path stays sane.
• The deterministic hash suffix in defaultGitWorktreePath correctly distinguishes between different repo roots that share a branch name.
• All ctx.task / ctx.parallel sites are wired through ...cwdOption, and the test verifies that exhaustively. Good defensive coverage of the wiring.

Overall: solid foundation, but the cleanup/re-run gap and the silent-fallback behavior are worth fixing before merge — the rest can be follow-ups.

[claude]

Review

Thanks for this PR — the worktree feature is a nice addition for isolating Ralph runs, and the test coverage of the new code paths is genuinely solid (relative path, absolute path, occupied-path failure, re-run, and null-byte fallback all exercised against a real git repo). Findings grouped by priority.

High priority

1. Try-block body is not re-indented (packages/workflows/builtin/ralph.ts:569). The new try-block wraps roughly 750 lines but the body keeps its original 4-space indent instead of moving to 6. The whole run callback now reads like a syntactic accident rather than a structured try/finally. Either re-indent the body or extract the workflow body into a helper that the run callback calls inside try/finally — the latter would keep the diff readable in future reviews.

2. Worktree is force-removed unconditionally — work is lost if the PR push did not land. git worktree remove --force runs in the finally regardless of whether the orchestrator successfully pushed/created the PR. If the push fails (auth, network, no remote), the orchestrator commits live only on a detached HEAD inside the worktree, and cleanup discards them silently. At minimum document this guarantee (or lack of it); ideally skip removal when an error was thrown so a user can recover the work, mirroring the way you preserve runError already. The current logic almost does this — it only throws cleanup errors when there was no run error — but it still removes the worktree on the failure path.

3. goal.ts mixes a real change with around 80 lines of pure reformatting. The new WORKER_PREFLIGHT_CONTRACT and its project_initialization_preflight wiring is in scope for this PR, but the rest of the diff is whitespace/line-length churn that has nothing to do with worktrees. Per CLAUDE.md preference for focused changes, please split the formatting commits out — they make the worktree change harder to review and bloat blame. The PR description itself calls these "Formatting-only refactors (no logic changes) to comply with line-length lint rules" — that is a separate concern.

Medium priority

4. Orphan worktrees after SIGINT / crash. No signal handler, so a Ctrl-C between addGitWorktree and the finally leaves the worktree on disk; subsequent runs with the same git_worktree_dir then fail at "occupied path" until the user manually cleans up. Consider (a) git worktree add --force after detecting an existing managed worktree, (b) attempting git worktree remove first if the target path is registered, or (c) at least mentioning the recovery command in the error message thrown from createGitWorktreeIfRequested.

5. Cleanup errors are swallowed when a run error exists (ralph.ts:1322). When runError is defined, a failed git worktree remove is dropped on the floor. The user only learns about the run error; they get no signal that they now also have an orphan worktree. A console.warn on cleanup failure would help observability without changing the throw semantics.

6. Integration-style tests placed under test/unit/. The new tests shell out to real git init, git commit, and git worktree add — that is integration behavior. CLAUDE.md distinguishes test:unit from test:integration and the existing Ralph tests in this file are pure mock-context tests. Moving the five new tests into test/integration/ would keep test:unit fast and accurately scoped.

7. Detached-HEAD worktree contract is implicit. git worktree add --detach means the PR stage must git checkout -b branch (or git push origin HEAD:refs/heads/branch) inside the worktree before pushing. Today the PR-stage prompt does not explicitly mention this, and the worktree input description does not either. Worth either calling it out in the git_worktree_dir input description or in the PR-stage prompt so the orchestrator agent reliably handles it.

Low priority / nits

8. Concurrency: two parallel Ralph runs that share a git_worktree_dir (or fall back to the same SHA-keyed path because they share repoRoot + baseBranch) will collide. Worth a sentence in the input description; otherwise users will not know.

9. runError is only used as a boolean. The variable holds the thrown value but you only read runError === undefined. Simplify with a let runSucceeded = false flag set to true right before the return, or rely on the catch re-throw and a separate try around removeGitWorktree.

10. previousCwd = process.cwd() at builtin-workflows.test.ts:580 is a redundant initializer — beforeEach overwrites it before any test runs. let previousCwd: string reads cleaner.

11. safeWorktreePathSegment truncation interacts with defaultGitWorktreePath: the 12-hex SHA suffix preserves uniqueness even when names collapse, which is correct — but worth a one-line comment explaining that the 60-char clip is intentional and uniqueness rides on the digest, not the human-readable segments.

12. workflowCwdOption could be inlined. As a named helper reused 10+ times it is fine — just flagging that the abstraction is doing very little; a direct ternary at the spread site would be just as clear.

Things I liked

• Scrubbing GIT_* env vars (GIT_LOCAL_ENV_VARS) is the right call — invoking from inside a git hook or a nested git context is exactly the kind of thing that would otherwise produce confusing "fatal: not in a git directory" errors. Comprehensive list.
• SHA-256 keyed fallback path under TMPDIR/atomic-ralph-worktrees/ is deterministic and collision-resistant; users can find/clean orphans easily.
• The relative-path resolution via workflowStartCwd (rather than the in-flight process.cwd()) is the correct defensive move.
• Test that asserts plan_path lives in the host repo (not the worktree) catches a real subtle bug class — worktree-internal artifacts vanishing on cleanup.

Verification I did not do

• Did not run bun run test:unit or bun run lint locally — review is from the diff and code reading only.
• Did not verify whether prettier is wired into the lint step; if it is, the indentation issue (add agent instructions #1) would presumably already be flagging.

Overall: the worktree mechanics are well-thought-out, but the indentation issue, the silent-data-loss path, and the unrelated formatting churn in goal.ts should be addressed before merge.

[claude]

PR Review — feat(workflows): run Ralph stages from optional git worktree

Thanks for adding worktree isolation to Ralph — the env-var scrubbing, fallback-path handling, failure-preservation, and detached-HEAD PR guidance all show careful thought, and the test coverage (both unit assertions on input shape and integration tests that drive a real git repo) is genuinely good. A few items worth a second look:

Blocking / important

1. if (Date.now() < 0) static guard (packages/workflows/builtin/ralph.ts:1367-1369)

// Discovery validates workflow shape without executing helpers, so keep an
// unreachable direct stage call in this wrapper while runRalphWorkflow owns
// the real ctx.task()/ctx.parallel() orchestration.
if (Date.now() < 0) {
  await ralphCtx.task(""__discovery-stage-guard__"", { prompt: ""unused static guard"" });
}

This is a strong code smell. A future maintainer (or a lint rule) will almost certainly try to delete it as dead code, breaking workflow discovery in a way that's only surfaced at runtime. If discovery really requires a visible top-level ctx.task call, please either (a) fix the discovery walker to follow into the extracted helper, (b) expose a real, intentional API (e.g. a defineWorkflow.declareStages([...]) annotation), or (c) at minimum hoist this into a clearly named helper like __declareStagesForDiscovery__(ctx) with a comment that survives a refactor. The Date.now() < 0 trick is too clever-by-half.

2. Successful-run cleanup uses git worktree remove --force

removeGitWorktree runs with --force regardless of worktree state (ralph.ts:399-401). After a successful Ralph run the worktree typically does contain uncommitted work — that's the whole point — and the PR-stage prompt's contract is to push a branch first. If for any reason the PR stage decided not to push (e.g. ""review_loop did not approve, prefer reporting the remaining blockers over creating a PR"" per the pr_policy text at ralph.ts:1287-1296), runSucceeded is still true and the worktree gets blown away.

Consider:

• Gating cleanup on a stage-emitted ""work-pushed"" signal, or
• Detecting an untracked/dirty index before --force removal and preserving + warning in that case, or
• At minimum, expanding the git_worktree_dir input description to call out this destructive behavior so callers don't lean on the worktree for ad-hoc work they intend to keep.

The current behavior is also asymmetric with the failure path, which thoughtfully preserves the worktree and logs a recovery command.

Smaller issues

3. let for never-reassigned config (ralph.ts:588-661)

noAskQuestionToolSet, plannerModelConfig, orchestratorModelConfig, simplifierModelConfig, reviewerModelConfig, explorerModelConfig are all let but never reassigned. They're also re-declared per workflow invocation, which is wasted work for objects that don't depend on inputs. Either make them const inside the function or, better, hoist to module scope as const so the per-run allocation goes away.

4. JSON.stringify for shell quoting (ralph.ts:403-405)

return \`git -C \${JSON.stringify(repositoryCwd)} worktree remove --force \${JSON.stringify(worktreeDir)}\`;

This is clever but isn't actually shell quoting. It happens to work on POSIX for paths without $, backticks, single-quotes, or backslash sequences, but it'll mislead Windows users (cmd.exe doesn't interpret \\ as an escape inside double quotes the way JSON encoding implies). Since this is human-copy/paste recovery output, the practical risk is low, but a small shellQuotePosix(...) helper would be safer and clearer about intent.

5. Duplicated preflight contract text

WORKER_PREFLIGHT_CONTRACT in goal.ts:286-296 and the inline project_initialization_preflight section in ralph.ts:783-796 are essentially the same prose. If they're meant to drift independently, fine — but if they're meant to stay aligned, extract to a shared constant (e.g. in a small packages/workflows/builtin/_shared/preflight.ts) so a future tweak doesn't silently desync them.

6. Spec path intentionally outside the worktree (ralph.ts:752)

const specPath = await writeSpecFile(resolve(workflowStartCwd, defaultSpecPath(prompt)), planner.text);

This is correct (specs need to outlive the worktree cleanup), but it's a load-bearing design choice that's easy to ""fix"" by mistake. A one-line comment above this line — something like ""// Specs live in the host repo, not the worktree; the worktree is removed on success"" — would prevent regressions.

7. gitProcessEnv() recomputed per call (ralph.ts:320-326, 328-340)

Allocates a fresh process.env clone on every runGit call. Not a hot path, but trivially hoistable to a module-level constant given Node guarantees process.env is stable for the process lifetime in practice. Tiny nit.

8. repositoryCwd?: string weakly typed (ralph.ts:376-380)

GitWorktreeSetup lets cwd and repositoryCwd independently be undefined, but in practice they're set/unset together. A discriminated union ({ created: false } | { created: true; cwd: string; repositoryCwd: string }) would let removeGitWorktree(worktree.cwd, worktree.repositoryCwd) typecheck without the ?? workflowStartCwd fallback in the failure-warning branch.

9. Cleanup throw inside finally swallows the workflow result

In the success path, if removeGitWorktree fails (ralph.ts:1392-1395), the throw in the finally discards the return result from the try. That's defensible — the user needs to know about the orphaned worktree — but you lose the run's output. Consider attaching the result to the error (err.cause = { result } or a custom error class) so callers can still pick up artifacts like plan_path.

Test coverage

The integration test suite is thorough: relative path, absolute path, occupied-path fail-fast, idempotent re-run, \0-fallback, and failure-preservation. Two gaps worth thinking about:

• No test asserts the destructive---force behavior — e.g., a stage writes a file in the worktree, the run succeeds, the file is gone afterward. That would document the contract and prevent silent regressions.
• No test for the WORKER_PREFLIGHT_CONTRACT addition in goal.ts. Even a presence-check against the orchestrator prompt (similar to the existing ""detached HEAD"" assertion at test/unit/builtin-workflows.test.ts:1358-1362) would lock the wiring in.

Style / conventions

Matches the repo's ESM .js import convention, Bun-first test setup, and bun:test + node:assert/strict patterns. No any/unknown introduced. Lint-friendly line lengths. Good.

Overall this is a solid feature with one architectural smell (#1) and one foot-gun (#2) worth resolving before merge; the rest are polish.

[claude]

Code Review — Ralph git worktree support

Nice, well-structured change: detached-HEAD worktree lifecycle, env-var sanitization, real-git integration tests, and explicit user recovery commands when things go wrong. Most of the critical paths (path resolution, null-byte fallback, fail-fast on dirty target, preserve-on-failure) have direct integration coverage. A handful of concerns worth addressing before merge:

Findings

[P1] Successful workflow result is discarded if worktree cleanup fails
`packages/workflows/builtin/ralph.ts:1382-1398` — on a successful run the `finally` block calls `removeGitWorktree`, and if it returns non-zero it throws from `finally`. That throw replaces the already-returned `result`, so the caller sees a failure even though the planner/orchestrator/PR stage all succeeded. The user then has both a lost return value and an orphaned worktree to handle manually. The failed-run branch correctly uses `console.warn`; the cleanup-failure branch should do the same and emit the recovery command rather than throw — the workflow result is the load-bearing thing here.

[P2] `WORKER_PREFLIGHT_CONTRACT` duplicates `project_initialization_preflight` in `ralph.ts`
`packages/workflows/builtin/goal.ts:286-296` is character-for-character identical to the inline block at `packages/workflows/builtin/ralph.ts:786-794`. Two copies of a prompt contract guarantee they will drift once one is edited. Extract a single exported constant (e.g. in `packages/workflows/src/shared/` or alongside other shared prompt fragments) and import it from both call sites.

[P2] Missing CHANGELOG entry
Per `CLAUDE.md` ("New entries ALWAYS go under `## [Unreleased]`"), the new `git_worktree_dir` input and the Goal preflight contract are user-visible additions and belong under `packages/workflows/CHANGELOG.md` `## [Unreleased]` `### Added`. The section is currently empty.

[P2] `if (Date.now() < 0) { await ralphCtx.task(...) }` is fragile
`packages/workflows/builtin/ralph.ts:1364-1369` — the unreachable guard exists only to satisfy `workflowRunCreatesStage` in `packages/workflows/src/extension/discovery.ts:155-162`, which regex-scans `Function.prototype.toString(run)` for `\.\s*(?:stage|task|chain|parallel)\s*\(`. Two cleaner options:

1. Teach the discovery regex (or a sibling check) to accept calls inside named helpers — e.g. recognize a `runRalphWorkflow(ctx, …)` invocation or scan for `await ctx.task(` inside any module-local helper the run delegates to.
2. If you want to keep the wrapper-only approach, hoist the guard into a clearly named helper (e.g. `assertHasDiscoverableStageCalls(ctx)` whose body is unconditional dead code), so the why is obvious at the call site rather than buried behind `Date.now() < 0`.

Today's pattern works, but the next reader will treat it as a typo and try to delete it.

[P2] `runGit` uses `spawnSync` and blocks the event loop
`packages/workflows/builtin/ralph.ts:328-340` — every git invocation (root resolution, worktree add, worktree remove) is synchronous. `git worktree add --detach

` on a large repository can take seconds, during which the entire process (including the workflow TUI, intercom polling, other parallel agent work) is frozen. Since the surrounding code is already `async`, prefer `node:child_process`'s `execFile`/`spawn` via `util.promisify`, or Bun's `Bun.spawn`.

[P3] Recovery command JSON-quotes paths — not shell-safe
`packages/workflows/builtin/ralph.ts:403-405` — `formatWorktreeRecoveryCommand` uses `JSON.stringify` to quote paths. JSON quoting doesn't escape `$`, backticks, `!`, etc. For Ralph-generated `$TMPDIR/atomic-ralph-worktrees/...` paths this is fine, but a user-supplied `git_worktree_dir` containing `$HOME-something` or whitespace produces a recovery command that misbehaves when pasted into a shell. Consider a small `shellQuote` helper (POSIX single-quote, doubled `'\''` for embedded quotes).

[P3] Detached-HEAD PR handoff doesn't verify auth from the worktree
The `pr_policy` text instructs the agent to `git push origin HEAD:refs/heads/` from inside the worktree. Worktrees inherit the parent repo's remotes, but the PR agent's `gh auth status` check runs in the worktree cwd. Worth a quick sanity check that auth works correctly when invoked from a detached-HEAD worktree at a temp path — easy to integration-test by injecting a fake `gh` in `$PATH`.

[P3] Type cast `ctx as WorkflowRunContext` swallows API drift
`packages/workflows/builtin/ralph.ts:1357` — if `defineWorkflow`'s callback type changes upstream, this cast will silently accept an incompatible `ctx`. If `.input(...)` chaining can carry an inferred inputs generic, prefer that; otherwise narrow at the boundary with a runtime check rather than `as`.

[P3] Fallback-path collision risk is documented in the input but not in code
`packages/workflows/builtin/ralph.ts:363-374` — `defaultGitWorktreePath` is keyed by `sha256(repoRoot \0 baseBranch)`, so two concurrent runs against the same repo + base will collide. The input description warns about it; a one-line comment above the digest line ("concurrent runs against the same repo+base will collide; pass an explicit `git_worktree_dir` to disambiguate") would save the next debugger a `git worktree list` trip.

Test coverage

Solid. The integration tests genuinely exercise `git worktree add/remove` with real repos and verify both happy-path cleanup and failure preservation. Two small gaps worth filling:

• No test for the cleanup-failure-on-success branch (e.g., chmod the parent dir read-only after `runRalphWorkflow` resolves) — would have caught finding [P1].
• No test that asserts spec file path stays under `workflowStartCwd` for the absolute worktree case (the relative-path test asserts it; the absolute test only checks cwd plumbing).

Nits

• `RalphInputs` declares `readonly` fields but `defineWorkflow`'s default-coercion paths typically hand back a mutable object — fine, just cosmetic.
• The `// The readable segment is clipped; default path uniqueness comes from the digest.` comment lives in `safeWorktreePathSegment`, but the invariant matters at `defaultGitWorktreePath`; consider moving it.
• Integration test's `assertWorktreeRegistered(_repo, …)` accepts `_repo` but never uses it — drop the parameter.

Overall: ship-worthy after addressing the cleanup-vs-result issue [P1] and the duplicated prompt contract [P2]. The remaining items are quality-of-life.

[claude]

placeholder - real review below

[claude]

placeholder superseded by the review comment below

[claude]

Review — feat(workflows): run Ralph stages from optional git worktree

Overall this is a thoughtful change: clear scope (no behavior change when git_worktree_dir is empty), good lifecycle separation between successful runs (clean up) and failed runs (preserve + recovery command), and real-git integration coverage. The preflight contract extraction in goal.ts is also reasonable.

A few items below — most are minor; the dead-code block and the cleanup-failure semantics are the two I would want addressed before merge.

Findings

[P1] Unreachable Date.now() < 0 block in the workflow wrapper looks like dead code with a misleading comment

In packages/workflows/builtin/ralph.ts, inside the top-level .run handler, there is an always-false guard whose body calls ralphCtx.task("__discovery-stage-guard__", ...) and whose comment claims "Discovery validates workflow shape without executing helpers." Date.now() is always >= 0, so the body is unreachable. Two concerns: (1) if there is a real static-analysis pass that needs to see a ctx.task() call in the wrapper, the canonical fix is to expose a workflow-discovery hook (or call the helper at definition time), not to bury a guard behind an always-false runtime check; (2) if there is no such validator, this is just cruft and should be removed. CLAUDE.md is explicit about not adding code for scenarios that cannot happen. Please either link to the validator this is working around (and add a test that fails without it) or delete it.

[P1] Cleanup failure in the finally block clobbers a successful workflow result

When runSucceeded === true, the workflow has already produced its RalphWorkflowResult, but the throw new Error("Failed to remove git worktree at ...") inside the finally block replaces that return value with an error — the caller never sees plan_path, pr_report, or iterations_completed. For a successful run, I would expect cleanup failure to be treated the same way the failed-run branch is treated: console.warn with the exact git worktree remove --force recovery command, and still return the result. As-is the user loses a successful PR result because of an orphan checkout — the opposite of the recovery-friendliness the rest of the change aims for.

[P2] JSON.stringify for shell-quoting paths is not safe on Windows

formatWorktreeRecoveryCommand renders the recovery command via JSON.stringify on each path argument. JSON quoting and POSIX-shell double-quoting agree for plain ASCII paths, but diverge on backslashes. JSON.stringify renders the Windows path C:\Users\me as a JSON string with doubled backslashes; bash double-quoted strings preserve those doubled backslashes literally, so the recovery command pasted by a Windows user would be wrong. Since defaultGitWorktreePath is happy to produce paths under TMPDIR on any platform and the input path can be user-supplied, this matters. Consider a small shell-quoter, or at minimum a comment that the printed command assumes a POSIX shell.

[P2] Silent fallback when git_worktree_dir is null-byte / unusable can surprise users

createGitWorktreeIfRequested quietly substitutes the SHA-256-keyed TMPDIR/atomic-ralph-worktrees/... path when resolveOptionalWorktreePath returns undefined (null byte, etc.). The PR description documents this, but the user gets no indication that their requested path was discarded — they will wonder why the worktree shows up somewhere else. A console.warn ("ignored unusable git_worktree_dir; falling back to ...") would be cheap and preserve current behavior. Alternatively, throw — a null byte in a path almost certainly indicates a caller bug, not an intentional fallback request.

[P3] WORKER_PREFLIGHT_CONTRACT duplicates the project_initialization_preflight block kept inline in ralph.ts

The new constant in goal.ts and the orchestrator project_initialization_preflight block in runRalphWorkflow are the same nine bullets. If they are meant to stay in lockstep, extract a shared constant (for example in a shared/contracts.ts under packages/workflows); if they are allowed to diverge, please add a short comment in each location explaining the duplication.

[P3] Two adjacent git rev-parse --show-toplevel calls

requireGitRepositoryForWorktree and defaultGitWorktreePath each spawnSync rev-parse --show-toplevel separately on the fallback path. They could share a single call — one extra subprocess per Ralph invocation is harmless, but the duplication makes the control flow slightly harder to follow.

[P3] ctx as WorkflowRunContext<RalphInputs> cast looks avoidable

At the top of the .run handler, ctx is cast to WorkflowRunContext<RalphInputs>. If defineWorkflow("ralph").input(...) already threads input types into ctx.inputs, the cast suggests either a typing gap in the builder or that RalphInputs should be declared on the builder so the cast becomes unnecessary. Worth a look — not a blocker.

Tests

Integration coverage is solid (real git repo fixtures, relative + absolute paths, happy-path cleanup, fail-fast on bad path, rerun, fallback, preservation-on-failure). A couple of gaps worth considering:

• Concurrent-run protection is called out in the PR description but not tested. A single-process test that pre-creates a git worktree add at the target path, then runs Ralph and expects the documented "orphaned Ralph worktree" recovery message, would lock that contract in.
• Cleanup-failure case (related to the [P1] above): no test exercises what happens when git worktree remove --force fails after a successful run.
• The unit/builtin-workflows.test.ts beforeEach/afterEach now process.chdir globally. Bun runs test files in parallel but tests within a file sequentially, so this is currently safe — worth a one-line comment so a future contributor does not introduce a parallel describe and quietly cause flakes.

Smaller nits

• safeWorktreePathSegment strips leading/trailing dashes but happily passes .. through. It is only used as a filename segment (never as a path component), so this is safe today, but if it ever gets joined with join(..., segment, ...) the assumption breaks. A one-line comment noting "filename-segment only" would help.
• The spec file is intentionally written to workflowStartCwd (host repo) rather than the worktree, and the orchestrator reads it via reads: [specPath, ...]. Worth confirming the SDK reads mechanism is happy with absolute paths that live outside the stage cwd; the tests assert plan_path starts with the host repo but do not assert the orchestrator could actually read it.

— Reviewed against CLAUDE.md (Bun-only, no build step for workflows, raw .ts, ESM .js import specifiers — all respected).

[claude]

PR Review: feat(workflows): run Ralph stages from optional git worktree

Nice scope — small surface, real isolation benefit, and the helper extraction + tests are solid. A few correctness and policy notes below.

High-priority issues

1. Dead fallback branch in createGitWorktreeIfRequested (packages/workflows/builtin/ralph.ts:419)

After the if (!trimmed) return { created: false } guard, value is guaranteed non-empty. resolveOptionalWorktreePath only returns undefined when !trimmed (throws on null byte, otherwise returns a resolved path). So requestedWorktreeDir is always defined, the if (requestedWorktreeDir !== undefined) branch is always taken, and the entire fallback block (lines 443–454, plus defaultGitWorktreePath itself) is unreachable.

Either:

• Remove the dead fallback (and the defaultGitWorktreePath helper) — simplest, matches the actual behavior.
• Or make it reachable, e.g. treat git_worktree_dir as a sentinel like \"auto\" / \"default\" that selects the SHA-256-keyed temp path. The PR description's "null-byte or unusable paths fall back to a deterministic SHA-256-keyed path" suggests this was the intent, but today null-byte paths throw before reaching the fallback.

No integration test exercises the fallback either, which is consistent with it being unreachable.

2. Force-removing the worktree on "success" can silently destroy work when the PR stage fails to push (packages/workflows/builtin/ralph.ts:1400-1420)

runRalphWorkflow returns normally even when the pull-request stage reports it could not create/push a PR (no credentials, no remote, etc.) — the stage produces a Markdown report, it doesn't throw. So runSucceeded becomes true and the finally block calls removeGitWorktree(..., --force), blowing away all the implemented changes.

This is the opposite of the PR's stated guarantee ("failed runs preserve it for recovery"). The user only learns after the fact, because the cleanup path doesn't warn on success.

Suggested fix: have the PR stage return a structured signal (e.g. via a pr_pushed boolean in the result), and skip force-cleanup when no branch was pushed. At minimum, log the worktree path on successful cleanup so the loss is visible.

3. Missing CHANGELOG entry

CLAUDE.md requires packages/*/CHANGELOG.md updates under ## [Unreleased]. packages/workflows/CHANGELOG.md should get an ### Added entry for git_worktree_dir and a ### Changed entry for the worker preflight contract.

Medium-priority

4. workflowCtx adapter is a no-op wrapper (packages/workflows/builtin/ralph.ts:1383-1387)

const workflowCtx: WorkflowRunContext<RalphInputs> = {
  ...ralphCtx,
  task: (name, options) => ralphCtx.task(name, options),
  parallel: (steps, options) => ralphCtx.parallel(steps, options),
};

The comment says "Discovery validates workflow shape without executing helpers, so this live adapter keeps direct stage primitive calls visible while the helper owns the real orchestration body." If that's an actual constraint of the workflow discovery system, please leave a code reference to the validator that needs this — otherwise this looks like dead indirection and should be removed (just pass ralphCtx directly).

5. Recovery error message can mislead on concurrent runs (packages/workflows/builtin/ralph.ts:435-438, 449-453)

The thrown error suggests "If this path is an orphaned Ralph worktree from an interrupted run, recover or remove it." When two Ralph runs target the same path concurrently (which the PR description warns against), the second run's failure message will tell the user to remove the first run's live worktree. Consider softening to "another Ralph run, or an orphaned worktree from an interrupted run."

Minor / nits

6. assertEveryRalphStageCwd is duplicated between test/unit/builtin-workflows.test.ts:1289 and test/integration/ralph-worktree.test.ts:124. Worth factoring into a shared test helper.

7. Pre-existing but in your diff: model config let bindings — plannerModelConfig, orchestratorModelConfig, etc. (packages/workflows/builtin/ralph.ts:616-675) are never reassigned. Since the helper was extracted in this PR, would be a clean moment to flip them to const.

8. Spec path writes to host repo while orchestrator runs in worktree cwd (packages/workflows/builtin/ralph.ts:766). This is intentional per the PR description, but the orchestrator's spec_file prompt section doesn't mention that the path lives outside the worktree. A subagent that does cat specs/... (relative) inside the worktree will not find it. Worth one extra sentence in the prompt clarifying the spec is absolute / in the host repo.

Things that are good

• Env scrubbing list (GIT_LOCAL_ENV_VARS) is thorough and avoids touching identity vars.
• Worktree path sanitization + SHA-256 digest is sensible.
• quoteRecoveryCommandArgument handles Windows vs POSIX correctly.
• Integration tests cover the meaningful state transitions: relative/absolute paths, no-repo, occupied dir, re-run after cleanup, null-byte, cleanup failure, and failure preservation.
• The duplicated WORKER_PREFLIGHT_CONTRACT in goal.ts and ralph.ts is explicitly called out in comments — appreciated, though I'd still lift it to a shared constant later.

[claude]

Code review

Solid PR overall — clean separation between worktree lifecycle and the workflow body, comprehensive integration tests, and the no-worktree path is preserved as a true no-op. A few things worth flagging:

Correctness / bugs

1. prReportAllowsWorktreeCleanup is a substring match against a model-generated report. (ralph.ts:407-409) The PR-stage prompt instructs the model to emit exactly one of two markers, but both strings appear in the prompt itself (lines 1287, 1302). If the model echoes the prompt, includes both markers in different sections (e.g. an explanation), or quotes the rule verbatim, safe-to-remove will be detected and the worktree will be removed even when the model chose preserve. Suggest matching the last cleanup-signal line, or requiring the safe marker to appear without the preserve marker — i.e. report.includes(SAFE) && !report.includes(PRESERVE).

2. Path-resolution semantics don't match the input description. The input doc says Relative paths resolve from that repository cwd (ralph.ts:1350), but resolveWorktreePath resolves relative to workflowStartCwd (captured process.cwd() at invocation), not the git repo root. If a user invokes Ralph from a subdirectory of a repo, a relative git_worktree_dir will resolve from that subdirectory, not the toplevel. Either document this precisely ("from the directory Ralph was invoked from") or resolve relative paths against the repo root returned by requireGitRepositoryForWorktree (which currently discards its return value at line 419).

3. Windows recovery-command quoting is incorrect for cmd.exe. quoteRecoveryCommandArgument (ralph.ts:395-398) uses single-quoted strings on win32 with '' escaping — that's PowerShell syntax. cmd.exe doesn't recognize single-quoted strings at all; the printed recovery command will fail to parse if a user pastes it into a cmd prompt. Either use double quotes with appropriate escaping or document that the recovery command is PowerShell.

Code quality

4. WORKER_PREFLIGHT_CONTRACT is duplicated verbatim across goal.ts and ralph.ts. The inline comment at ralph.ts:778 acknowledges the duplication, but the two copies will drift. Suggest exporting the constant from goal.ts (or a shared module) and importing it in ralph.ts. The "stage-specific orchestration context" justification is about how the contract is wrapped, not what it says.

5. worktree.repositoryCwd ?? workflowStartCwd is dead defensiveness. (ralph.ts:1383) createGitWorktreeIfRequested always sets repositoryCwd to the cwd arg it receives, which is always workflowStartCwd. The fallback never fires. Either drop the field or actually capture the repo toplevel (per updates to readme and instructions #2).

6. requireGitRepositoryForWorktree computes the repo root and throws it away. Used only for its side effect. If you don't end up resolving relative paths against it (updates to readme and instructions #2), at least drop the return value or rename it (e.g. assertInsideGitRepository).

7. The workflowCtx wrapper at ralph.ts:1365-1369 is a static-analysis workaround. The comment is helpful, but this is fragile — if discovery's guard changes, the workaround silently breaks. Worth a follow-up to expose a discovery-friendly way for workflows to factor out their body, since this pattern will likely recur for other workflows that want try/finally around the run.

Performance

8. runGit uses spawnSync, blocking the event loop during worktree create/remove. The synchronous calls are only at run start and run end, so the impact is small, but node:child_process has a promisified execFile that would be a one-line change and more idiomatic in an otherwise async path.

Test coverage

The new integration suite is genuinely good — covers cleanup, preservation, fail-fast, recovery guidance, and repeat-runs. A few gaps:

• No test for a non-existent base_branch — git worktree add --detach <dir> nonexistent will fail; the error path through gitFailureMessage isn't exercised.
• No multi-iteration test (max_loops > 1) verifying cwd propagation persists across iterations.
• No test for git_worktree_dir pointing at a non-empty existing directory (distinct from the "occupied worktree" case).
• The cleanup tests verify console.warn was called but don't verify the function still returns the result successfully — assert.equal(typeof result[\"plan_path\"], \"string\") is a weak check; consider asserting the full RalphWorkflowResult shape so a regression that throws inside the finally is caught.

Security

Generally fine — null bytes are rejected, base_branch is regex-validated for git-ref-safe characters before reaching spawnSync (no shell), and GIT_* env scrubbing is thorough. No injection vectors I can see.

Nit

• The git_worktree_dir input description is informative but quite long (4 sentences). Consider trimming to one sentence and moving the operational detail to a comment in the workflow file or to docs.

Nothing blocking — the substring-match issue (#1) and the Windows quoting (#3) are the most concrete defects worth addressing before merge.

[claude]

Claude Code Review

Solid, well-scoped change — the worktree lifecycle is preserve-by-default on every failure mode I can see, the recovery commands are surfaced in both errors and warnings, and the integration test suite is genuinely thorough (8 tests covering relative/absolute paths, non-repo, occupied path, null bytes, missing base branch, multi-iteration, cleanup-failure, and preserve-on-throw). The WORKER_PREFLIGHT_CONTRACT extraction is a clean dedupe across Goal and Ralph. Below are issues to consider before merging.

Bugs / correctness

1. PR description ↔ implementation mismatch on relative-path resolution.
The PR description says: "Path resolution — relative git_worktree_dir values resolve from process.cwd()", but createGitWorktreeIfRequested resolves from the repo root, not the process cwd:

const repoRoot = requireGitRepositoryForWorktree(cwd);
const requestedWorktreeDir = resolveWorktreePath(trimmed, repoRoot);

The input description string in the workflow definition correctly says "repo-root-relative path" and the integration test (creates a relative git_worktree_dir from repo root...) intentionally chdirs to a subdir and asserts the worktree lands under repo/, not subdir/. So the code and tests are consistent — only the PR description is wrong. Just fix the description; the repo-root semantic is the better one (predictable regardless of where the user invokes Ralph from).

2. --force removal can silently lose work if the LLM emits the wrong marker.
removeGitWorktree runs git worktree remove --force. The entire safety story rests on the LLM emitting Worktree cleanup: preserve when there are unpushed/uncommitted changes. If the model misjudges (e.g. "I created the branch" → safe marker, but the git push actually failed with a non-zero exit it didn't notice), the worktree — including any uncommitted state and the unpushed branch — disappears. Consider a belt-and-suspenders check: even after seeing the safe marker, run git -C <worktree> status --porcelain and refuse to force-remove when it's non-empty, warning with the preserve message instead. Cheap to add, removes a whole class of "the LLM was confidently wrong" failures.

3. Stale origin/main produces a stale worktree.
The worktree is created from comparisonBaseBranch (default origin/main) without a preceding fetch. If the user hasn't fetched recently, the worktree starts from a stale commit and reviewers compare against that same stale ref — so the staleness is internally consistent but invisible to the user. Either:

• run git fetch for origin/* refs before worktree add, or
• warn when base_branch looks like a remote-tracking ref (origin/…) and the local ref is older than N hours, or
• at minimum, document this in the input description.

4. prReportAllowsWorktreeCleanup is line-exact, which is fail-safe but worth confirming the LLM cooperates.
Filtering for line === MARKER after trim means a bullet-prefixed marker (- Worktree cleanup: safe-to-remove), a bolded marker (**Worktree cleanup: safe-to-remove**), or any wrapping markdown produces the preserve path. The test preserves worktree when cleanup markers appear only in prose covers the wrap-in-sentence case. This is intentionally conservative and I like it — just flagging that real-world models love to bold things, so users may see "unexpected" preserves. The PR-stage prompt does say "include exactly one cleanup signal line" which should help; consider also adding "do not bold, quote, prefix, or otherwise decorate this line" to remove the most common LLM tic.

Code smell — the workflowCtx "live adapter"

const workflowCtx: WorkflowRunContext<RalphInputs> = {
  ...ralphCtx,
  task: (name, options) => ralphCtx.task(name, options),
  parallel: (steps, options) => ralphCtx.parallel(steps, options),
};

The comment explains this exists to satisfy workflowRunCreatesStage in packages/workflows/src/extension/discovery.ts:155, which does Function.prototype.toString.call(run) and regex-matches \\.\\s*(?:stage|task|chain|parallel)\\s*\\( to confirm the .run() body is non-empty. Since the orchestration moved into runRalphWorkflow, the .run() body would otherwise contain zero stage primitive calls — so these wrappers exist purely to put .task( and .parallel( literals in the .run() source.

This works, but it's quietly fragile:

• A future contributor cleaning up "dead" wrappers will silently de-discover Ralph.
• The two wrappers also shadow chain and stage from the spread — those still satisfy the regex because they came from ...ralphCtx, but only because spread reflection works that way at runtime; the static check sees only the .run() source text.

Two cheaper fixes than what's here:

• A single inline statement like void (ctx.task as unknown); void (ctx.parallel as unknown); would be uglier but more clearly load-bearing.
• Or update workflowRunCreatesStage to recursively check helpers reachable from .run() via a module-level whitelist, then drop this wrapper entirely.

At minimum: add a unit test that asserts Function.prototype.toString.call(ralph.run) matches the discovery regex. That converts the silent-break risk into a CI failure.

Performance

WORKER_PREFLIGHT_CONTRACT is ~10 sentences added to every worker turn in Goal.
In Ralph it runs once per orchestrator stage per iteration — fine. In Goal it runs every worker turn (potentially many short turns), each carrying the full preflight block as static prefix. The contract is genuinely useful in a fresh worktree but is dead weight on turn 5 of an existing-checkout run. Consider:

• Goal currently doesn't know whether it's in a fresh worktree; if it could be told (or could cheaply infer from node_modules etc. once on turn 1), the contract could be conditional or summarized to a single line after turn 1.
• Or accept the cost — it's small relative to the tool output volume — and just be aware of the per-turn surcharge.

Test coverage

Good: 8 integration tests cover the lifecycle reasonably well, the cleanup-failure-still-returns-result case is tested, and the preserve-on-throw case is tested. The unit test verifying cwd is propagated to every stage (assertEveryRalphStageCwd) is a nice invariant.

Gaps:

• process.chdir across files. The unit test comment says "cwd-sensitive mock tests run serially in this file" — correct for in-file, but bun:test can run across files concurrently. Since process.chdir is process-global, an unrelated test file running concurrently and not chdir'ing back can break these. Either add a guard at suite start (if (process.env.BUN_TEST_CONCURRENT) test.skip(...)) or document the constraint somewhere the next contributor will see it. The integration test has the same pattern.
• No test for concurrent Ralph runs hitting the same git_worktree_dir. The error path is implemented ("another active Ralph run") but only tested via a pre-existing worktree, which is the orphan case, not the genuine race. A test that overlaps two d.run(ctx) calls with the same path would lock in the fail-fast.
• No test that git_worktree_dir containing .. traversal is accepted/rejected as intended. It currently resolves to whatever the user wrote. Worth at least one test asserting the intended behavior (probably: accept it, since this is workflow config, not untrusted input).
• No assertion that the spec file written to workflowStartCwd/specs/ is readable from inside the worktree. The orchestrator prompt warns it's a host-repo absolute path, but no test verifies the file actually exists at that path after writeSpecFile resolves it against workflowStartCwd.

Security

• runGit uses spawnSync with an args array — no shell, no injection risk. ✓
• gitProcessEnv scrubs 15 GIT_* env vars (PR description says 12 — minor, just an off-by-three in the description). ✓
• quoteRecoveryCommandArgument correctly escapes single quotes via '\\'' on POSIX and doubles double quotes on Windows. The recovery commands are user-facing strings, not executed by Ralph, so the only risk is a copy-paste hazard — the escaping handles the obvious cases. ✓
• Null-byte rejection in resolveWorktreePath is a good defensive check. ✓

Style / repo conventions

• Bun-only commands respected (no npm/node invocations). ✓
• .js import specifiers preserved (./shared-prompts.js, ../src/index.js). ✓
• No build artifacts introduced. ✓
• Changelog updated under [Unreleased] with both Added and Changed sections. ✓
• readonly used consistently on type fields. ✓

Summary

Ship-ready with caveats. The two things I'd actually block on:

1. Fix the PR description (or the implementation) for relative-path resolution — they disagree.
2. Add a git status --porcelain belt-and-suspenders check before worktree remove --force to make the marker-based cleanup robust against LLM misreporting.

Everything else is nice-to-have polish.

---

Reviewed by Claude (claude-opus-4-7).

[claude]

Review — Ralph git worktree support

Thanks for this — the worktree isolation, the evidence-based PR cleanup signal, and the broad integration test matrix are all great. A few comments below, mostly around robustness and minor polish.

Correctness / behavior

• gitProcessEnv may strip CI-injected git config (GIT_CONFIG_COUNT / GIT_CONFIG_PARAMETERS). GitHub Actions (and similar) inject HTTP credentials and other config through these. The internal git commands here are local-only (rev-parse, worktree add/remove), so stripping them is fine for these calls — but the env scrubbing is only applied to internal runGit, not to the worker stages. That's the right scope, but it's worth a one-line comment in gitProcessEnv clarifying that scrubbing is intentionally limited to Ralph's bookkeeping commands so workers retain whatever credential helper the host has wired up.

• Cleanup signal is purely trust-based. prReportAllowsWorktreeCleanup decides whether to delete the worktree based on what the PR-stage LLM says it did, not on what it actually pushed. A model that hallucinates Worktree cleanup: safe-to-remove will cause Ralph to silently destroy the worktree (and any unpushed local commits). Consider adding a defensive check before removal — e.g., run git -C <worktree> status --porcelain and git -C <worktree> log @{u}.. --oneline 2>/dev/null and refuse cleanup if there are uncommitted or unpushed commits. Cheap insurance against a confused agent.

• Spec path is rooted at workflowStartCwd, not the repo root. When invoked from a subdir of the repo, the spec lands at <subdir>/specs/<slug>.md rather than <repoRoot>/specs/<slug>.md. The integration test creates a relative git_worktree_dir from repo root... actually exercises this path (planPath under subdir/specs), confirming the behavior is intentional. Worth a comment near the resolve(workflowStartCwd, defaultSpecPath(prompt)) call so a future reader doesn't "fix" it. (Or anchor it at worktree.repositoryRoot when a worktree was created — arguably more predictable for users invoking from arbitrary cwds.)

• workflowCtx identity adapter. The wrapping task: (name, options) => ralphCtx.task(name, options) exists solely to keep the discovery static analyzer happy. The comment explains why, which is great. Consider adding a // keep in sync with workflowRunCreatesStage in packages/workflows/src/extension/discovery.ts cross-reference so a future refactor of the discovery walker doesn't silently regress this.

• RalphInputs cast. const ralphCtx = ctx as WorkflowRunContext<RalphInputs>; works but is a type assertion at the public boundary of the workflow .run. If defineWorkflow supports a generic Inputs parameter (e.g., defineWorkflow<RalphInputs>("ralph") or input-builder inference), prefer that to get the typing without the cast. If it doesn't, the cast is fine.

Minor

• The input description (Optional Git worktree path; when set, Ralph must start inside a Git repo and runs stages from a detached worktree created at this repo-root-relative path.) says "repo-root-relative path" — but resolveWorktreePath also accepts absolute paths (covered by the creates an absolute git_worktree_dir... test). Update the description to acknowledge both, e.g. "absolute or repo-root-relative".

• PR description says "12 related vars" but GIT_LOCAL_ENV_VARS lists 15 — small docs nit.

• quoteRecoveryCommandArgument uses cmd.exe-style double-quote escaping on win32, but if the user is on Windows with PowerShell or git-bash the recovery command won't paste cleanly. Acceptable since affected users can adjust by hand, but a one-line note in the warning message ("Windows: shown for cmd.exe; adjust quoting if using PowerShell") would help.

• addGitWorktree returns status: 1 with a synthetic stderr when the parent mkdir throws. createGitWorktreeIfRequested only surfaces it via gitFailureMessage, so readers chasing a git-prefixed error message will be confused when it's actually a filesystem error. Consider prefixing the synthetic stderr with mkdir: (or distinguishing them in the result type) so the error is self-locating.

Tests

• Test coverage is strong (14 integration tests across happy paths, fail-fast, marker parsing, multi-iteration cwd propagation, cleanup-failure warnings). Nice.

• test/integration/ralph-worktree.test.ts re-implements MockCalls, makeMockCtx, promptText, makeTaskResult that already exist in test/unit/builtin-workflows.test.ts. Extracting these into a test/_helpers/workflow-mock.ts (or similar) would prevent drift if either test file evolves.

• Consider a test where the PR-stage worker emits the cleanup marker but there are real uncommitted changes left in the worktree — to lock in whatever protection you decide to add (if any) per the cleanup-trust point above.

• Missing: a test for the case where git rev-parse --show-toplevel from the start cwd returns a path the user didn't expect (e.g., a submodule). Not blocking, but worth pinning down which root wins.

Style / conventions

• New TS code looks clean, follows the no-build .ts + .js import convention (CLAUDE.md), uses bun:test, no any/unknown. Good.
• The shared-prompts extraction is a nice simplification — adding the preflight contract to Goal is a meaningful win independent of the worktree feature.
• runRalphWorkflow extraction makes the worktree lifecycle wrapper much easier to reason about than threading try/finally through the inline body.

Overall: solid, well-tested change. The two items I'd most want addressed before merge are (1) a defensive check for uncommitted/unpushed commits before cleanup, and (2) updating the git_worktree_dir input description to mention absolute paths.

[claude]

Code Review — feat(workflows): run Ralph stages from optional git worktree

Solid foundation for running Ralph in an isolated checkout; the cwd plumbing through every stage looks clean and the gitProcessEnv scrubbing is a nice touch. A few things stood out, the biggest one being that the PR description doesn't match what the code actually does.

1. PR description is significantly out of sync with the code (please update)

The body advertises behavior that doesn't exist in the diff:

• "Worktree lifecycle — removed on clean exit when the PR stage emits Worktree cleanup: safe-to-remove; preserved when the PR stage emits Worktree cleanup: preserve, the run fails, or cleanup itself fails" — there is no marker-based cleanup. The PR-stage prompt at packages/workflows/builtin/ralph.ts:1289 says the opposite: "Ralph does not remove git_worktree_dir automatically." The unit test at test/unit/builtin-workflows.test.ts:1350 even asserts that the strings Worktree cleanup: safe-to-remove and Worktree cleanup: preserve are absent from the prompt.
• "Concurrent-run safety — a second Ralph run targeting the same worktree path fails fast" — createGitWorktreeIfRequested (ralph.ts:416) explicitly reuses an existing worktree as-is, and the test \"can re-run with the same git_worktree_dir without cleanup\" (ralph-worktree.test.ts:295) locks that behavior in.
• "Scrubs 16 GIT_* environment variables" — GIT_LOCAL_ENV_VARS (ralph.ts:302) lists 15.
• "test/integration/ralph-worktree.test.ts (new, 14 tests)" — file has 10 tests.
• "PR stage emits an evidence-driven cleanup signal" — the PR-stage prompt has no such instruction.

The CHANGELOG entry ("leaving worktrees in place for retries") is accurate; please rewrite the PR description to match.

2. No isolation between concurrent Ralph runs sharing a worktree

Because an existing path that is a git worktree is silently reused, two concurrent Ralph invocations with the same git_worktree_dir will overwrite each other's edits, run git status/git diff against the same dirty tree, and create non-deterministic results. The PR body claims fail-fast behavior here that doesn't exist. Options:

• Add a .ralph.lock (PID + ISO timestamp) at the worktree root, write on entry, refuse to enter if it exists and the PID is alive, remove on exit — and document this.
• Or, if concurrent reuse is intentional, surface that clearly in the input description and remove the claim from the PR body.

3. Reused worktrees carry stale state forward

When the worktree already exists, createGitWorktreeIfRequested returns it without inspecting its HEAD, dirty state, or whether HEAD diverged from base_branch. A user retrying after a failed run gets the previous run's half-applied edits as the starting state — reviewers will then compare "current working tree" against base_branch and flag the prior leftovers as new findings. At minimum, log the worktree HEAD so the user knows what they're starting from; ideally offer an opt-in refresh (git -C <wt> checkout --detach <base_branch> when the directory is clean).

4. test/integration/ralph-worktree.test.ts is detected as a binary file

The test at line 375 (git_worktree_dir: \"invalid path\") contains a literal NUL byte in the source between invalid and path (verified with od -c). Consequences:

• file(1) reports data, not text.
• grep silently skips the file by default — grep test test/integration/ralph-worktree.test.ts returns 0 matches without an error. Easy to miss in CI logs and grep-based tooling.
• Some editors / review tools fall back to binary mode.

Use the JS escape instead: git_worktree_dir: \"invalid\\u0000path\". The runtime value is identical; the source stays text.

5. Smaller items

• TOCTOU: between pathExists(requestedWorktreeDir) and addGitWorktree(...) in createGitWorktreeIfRequested, another process could create the directory and turn the friendly "already exists but is not a Git worktree" error into a confusing raw git error. Low priority — interactive flow — but worth a comment.
• spawnSync in an async codebase: runGit blocks the event loop during worktree creation. promisify(execFile) would fit the rest of the file better.
• Windows recovery quoting: quoteRecoveryCommandArgument uses \"\" to escape \", which is PowerShell-correct but cmd.exe uses \\\". Since this string is only echoed for the user, it's cosmetic, but a brief comment about which shell the recovery command targets would help.
• Stale guidance when no worktree is used: the PR-stage prompt unconditionally says "Ralph-created worktrees are detached HEAD checkouts. If you are preparing a PR from a detached HEAD, create and push a branch from the current HEAD…" (ralph.ts:1288). When git_worktree_dir is empty (the default) this is misleading — the model will be told to handle detached HEAD even though it's on the user's actual branch. Consider gating that paragraph on workflowCwd !== undefined.
• Preflight injection sites: WORKER_PREFLIGHT_CONTRACT only flows into the orchestrator (Ralph) and work-turn (Goal). That's a deliberate choice (the orchestrator gates implementation), but a one-line comment in runRalphWorkflow explaining why the planner / simplifier / reviewers don't get the preflight would prevent future churn. The contract's phrasing ("Before normal implementation delegation") fits the orchestrator well but reads awkwardly inside the Goal worker, which is the implementer rather than a delegator — minor wording polish if you do another pass.
• Spec path location: writeSpecFile(resolve(workflowStartCwd, defaultSpecPath(prompt)), ...) (ralph.ts:757) puts the spec under <invoking-cwd>/specs/, not under the repo root. Running Ralph from a subdir scatters specs across subdirectories. The test on line 199 of the integration file pins this behavior, so it's intentional, but worth a one-line comment so future readers don't "fix" it.

Positives

• The cwd propagation pattern (...cwdOption spread into every ctx.task / ctx.parallel call) is clean and the integration tests verify it on every stage — that's the right shape.
• GIT_LOCAL_ENV_VARS scrubbing is correctly scoped to Ralph's internal git invocations and not applied to worker stages (which keep credential env). The comment at ralph.ts:300 makes that explicit.
• Extracting WORKER_PREFLIGHT_CONTRACT into shared-prompts.ts is the right unit; reusable across Goal and Ralph without circular imports.
• The null-byte path rejection and helpful recovery command in the "already in use" error message are nice user-facing touches.
• normalizeBranchInput's reject-then-fall-back-to-default pattern keeps a malformed base_branch from getting interpolated into shell-y reviewer prompts (covered by the existing goal sanitization tests).

Happy to look at a follow-up commit that updates the description and addresses the concurrent-reuse / stale-state behavior either by implementing locking or by making the documented contract match the code.

[claude]

Code Review — PR #1068 (Ralph git worktree support)

Nice piece of work: scope is well-contained, the git_worktree_dir="" default keeps the existing path unchanged, the env-scrubbing is thoughtful, and integration coverage is thorough. A few concrete things worth addressing before merge.

Correctness / bugs

1. existingPathIsGitWorktree doesn't verify the worktree belongs to the invoking repo.
packages/workflows/builtin/ralph.ts:392 calls git -C <path> rev-parse --is-inside-work-tree, which returns true for any git work tree — including an unrelated repository, or even the host repo's own root if the user passes that path. If git_worktree_dir accidentally points at an unrelated checkout, Ralph silently runs every stage in that other repository. Suggest cross-checking that git -C <worktreeDir> rev-parse --git-common-dir resolves under the same repoRoot returned by requireGitRepositoryForWorktree, and rejecting otherwise.

2. PR-stage prompt assumes a reused worktree is detached HEAD.
packages/workflows/builtin/ralph.ts:1288 reads "Ralph-created worktrees are detached HEAD checkouts." That's true for newly created worktrees, but the reuse branch (existingPathIsGitWorktree) skips --detach and may pick up an existing worktree with a checked-out branch. The branch-from-HEAD guidance is still safe (the LLM is told "if you are preparing a PR from a detached HEAD…"), but the leading sentence over-claims and could confuse the reviewer model into unnecessary branch creation. Consider softening to "Ralph-created worktrees default to detached HEAD" or describing the conditional explicitly.

3. gitFailureMessage emits "git exited with status null" on signal termination.
packages/workflows/builtin/ralph.ts:350 — when result.status === null (signal-killed) and there is no stdout/stderr, the message reads literally "git exited with status null". Worth special-casing null (e.g. mention result.signal) for better diagnostics. Low impact, trivial fix.

Race / footgun

4. TOCTOU between pathExists and addGitWorktree.
packages/workflows/builtin/ralph.ts:427 — two concurrent Ralph runs against the same git_worktree_dir can both see the path as missing, then one wins git worktree add and the other fails. The failure message already hints at this case with the recovery command, so this is documented rather than buggy. However the "directory exists but not a worktree" branch (ralph.ts:431) doesn't include the same recovery hint — adding it would help users disambiguate "stale Ralph state" from "I pointed at the wrong directory."

Style / nits

5. Worker stages inherit the un-scrubbed Git env.
The comment at ralph.ts:301-302 correctly chooses to preserve credentials in worker stages, but the same inherited env also includes GIT_DIR/GIT_WORK_TREE from the invoking shell, which can pin worker git calls to the host checkout regardless of stage `cwd`. Probably out of scope for this PR, but worth a follow-up note.

6. PR description references a `test/unit/discovery.test.ts` update that isn't in the changed-files list.
Stale wording in the description — worth tidying before merge so the description matches the diff.

7. Missing trailing comma.
`ralph.ts:1351` (description string in the `git_worktree_dir` input) lacks the trailing comma the surrounding inputs use. Cosmetic.

8. `as WorkflowRunContext` cast in the run wrapper.
`ralph.ts:1354`, `1366-1370` — the cast and the `task`/`parallel` re-export exist solely to satisfy the static `workflowRunCreatesStage` regex guard. The inline comment explains why nicely; consider extracting into a tiny named helper (e.g. `wrapForDiscovery(ralphCtx)`) so future maintainers see it isn't gratuitous indirection.

Tests

Coverage is solid: relative/absolute path, fail-fast outside repo, missing base branch, non-empty non-git dir, null-byte rejection, multi-iteration cwd propagation, failed-run persistence, reuse with uncommitted state, retry without cleanup. Things that aren't covered and would harden the contract:

• A `git_worktree_dir` pointing at an unrelated git repo — currently passes silently (see finding add agent instructions #1).
• A `git_worktree_dir` equal to the repo root — same as above; current check accepts it.
• Symlinked worktree paths — `realpathSync.native` is used in the test comparison helper but not in the production resolver, so a symlinked worktree may double-register.

Nice-to-haves; the existing tests cover the headline contract well.

Security

No new shell-injection surface — `spawnSync(["worktree","add",...])` passes args as an array, so user-supplied paths aren't shell-interpreted. The null-byte rejection in `resolveWorktreePath` is a nice guard. No new credentials or secrets touched.

Performance

`spawnSync` blocks the event loop, but only for one-shot bookkeeping calls (`rev-parse --show-toplevel`, `worktree add`, `rev-parse --is-inside-work-tree`). Acceptable.

CLAUDE.md compliance

• Bun-first: ✅ tests use `bun:test` + `node:assert/strict`.
• ESM `.js` import specifiers: ✅
• No build step added to `packages/workflows`: ✅
• CHANGELOG under `[Unreleased]` with `### Added` / `### Changed`: ✅
• Avoids `any`/`unknown`: mostly ✅; one `as` cast at the workflow boundary, called out above.

Summary

Solid PR overall. The biggest item worth addressing before merge is #1 (worktree-vs-repo identity check); the rest is quality-of-life polish.

[claude]

Code Review — feat(workflows): run Ralph stages from optional git worktree

Overall this is a well-scoped, well-tested addition. The lock-file design, env-scrubbing, recovery-command formatting, and integration coverage (null bytes, stale PID, absolute/relative paths, multi-iteration cwd) are all good. A few things worth flagging:

Correctness

1. Reviewers aren't told .ralph.lock is internal state — they will see it in git status --short and may flag it (P1).

.ralph.lock only appears in the orchestrator prompt (packages/workflows/builtin/ralph.ts:943) and the pull-request stage prompt (ralph.ts:1403). The reviewer prompt is explicit that reviewers should inspect untracked files via git status --short (ralph.ts:1217, 1387). Since the lock is untracked, it will show as ?? for every iteration's reviewers, and bug_selection_guidelines invites them to flag patch-introduced clutter. Two failure modes:

• A reviewer raises a P2/P3 finding about a stray lockfile, the planner addresses it next iteration → wasted work and a confused review loop.
• Worse, a reviewer keeps stop_review_loop=false because of this "finding," and the loop never converges.

Suggest adding a short internal_run_state block to the reviewer prompt (same wording as the orchestrator's L943) so reviewers treat .ralph.lock as exempt.

2. acquireRalphWorktreeLock recurses on EEXIST→ENOENT race; unbounded under pathological flapping (P2).

ralph.ts:482 calls acquireRalphWorktreeLock(worktreeDir) recursively if the file vanishes between the wx write and the read. Realistically the recursion will resolve on the next attempt, but if another process is concurrently creating/removing the lock (e.g., a buggy supervisor or a wedged Ralph), this could stack-overflow before reporting a useful error. A bounded for (let attempt = 0; attempt < N; attempt++) loop with a clear "give up" error after a few tries would be more defensive and easier to read.

3. PID liveness has cross-host / PID-reuse caveats that aren't surfaced (P2).

processIdIsAlive (ralph.ts:421) uses process.kill(pid, 0). Two cases the current design silently mishandles:

• Network filesystem / containers: if the lock was created by PID 1234 on host A and a different process on host B holds PID 1234, the lock is reported as active (false-positive lockout).
• PID reuse on the same host: if PID 1234 has been recycled to an unrelated process, same false-positive.

Recording host identity (os.hostname()) and/or the process boot time in the lock file would let stale-lock detection be much more accurate. At minimum, consider mentioning this in the git_worktree_dir input description so users on NFS-style setups know to use distinct paths.

Smaller issues

4. existingPathIsGitWorktree accepts the main repo too (P3).

The check (ralph.ts:400) is rev-parse --is-inside-work-tree, which is true for the primary checkout as well as linked worktrees. If a user accidentally points git_worktree_dir at the main repo root, Ralph will silently "reuse" it and write .ralph.lock at the top of their working checkout. Consider verifying via git rev-parse --git-common-dir differing from --git-dir, or at least logging the resolved worktree so the user notices.

5. PR description claims test/unit/discovery.test.ts was updated; the diff doesn't change it (P3 — doc).

The summary lists test/unit/discovery.test.ts — updated discovery expectations for the new shared prompt module, but git diff shows no change there. Since bundled discovery is driven by packages/workflows/builtin/index.ts (not a directory scan), shared-prompts.ts is invisible to discovery and no update was needed — the description is just stale. Worth correcting before merge so future readers aren't hunting for a phantom test change.

6. Lock release relies on byte-exact content equality (P3).

releaseRalphWorktreeLock (ralph.ts:504) refuses to remove the lock unless its content matches lock.content exactly. This is a sensible fence, but the warning ("Preserving Ralph worktree lock because it no longer belongs to this run") is a console.warn that won't surface to the user in most TUI/agent run modes. Consider plumbing this through the workflow logger so an unexpected leftover lock isn't silently left for the next run to trip over.

Things I checked and liked

• gitProcessEnv() only scrubs Git-local vars and only on Ralph's internal calls — worker stages still inherit CI/credential setup. The comment at ralph.ts:300-302 explains the intent clearly.
• normalizeBranchInput blocks --prefixed branch names from reaching git worktree add as a flag (combined with spawnSync array args, no command-injection risk).
• The workflowCtx adapter at ralph.ts:1480-1484 explicitly preserves the discovery static-guard pattern (.task(, .parallel( literally in the run body) — the inline comment makes the dependency on discovery.ts's regex obvious, which will save a future contributor a debugging session.
• Integration tests cover the right shapes: null-byte path, stale PID via real spawnSync exit, idempotent re-run, multi-iteration cwd, and failure-leaves-worktree.

Style nits (not blocking)

• addGitWorktree returns a GitCommandResult even from the mkdir failure path (ralph.ts:391); the synthetic status: 1 works but mixes concerns. A small WorktreeCreateError type might read more cleanly, though it's hardly worth churn.
• acquireRalphWorktreeLock could short-circuit the read on the rare ENOENT path with a single tail-recursive loop and an attempts++ ceiling — see point 2.

Nice work overall — the lock semantics and recovery-command output are the kind of thing that's easy to underspecify, and the test coverage on the lock paths is solid.

[claude]

Review — feat(workflows): run Ralph stages from optional git worktree

Solid PR overall. The design is small, defaults to a no-op, fails fast on bad inputs, and is backed by a real-git integration suite. Comments below from highest to lowest concern.

Code quality / correctness

1. Discovery static-guard coupling is fragile. The workflowCtx adapter at packages/workflows/builtin/ralph.ts:1366-1370 exists only so workflowRunCreatesStage's regex (/\.\s*(?:stage|task|chain|parallel)\s*\(/ in packages/workflows/src/extension/discovery.ts:161) finds literal ralphCtx.task( / ralphCtx.parallel( strings in the .run source. The comment explains the intent, but a future refactor (e.g., destructuring task from ralphCtx) would silently de-register Ralph from the bundled manifest. Worth either (a) a lightweight regression test that asserts ralph shows up in discoverStartupWorkflowsSync's sources (the test at test/unit/discovery.test.ts:61 already iterates four bundled names, so this is partially covered), or (b) a tiny unit test on workflowRunCreatesStage against the actual ralph .run.toString().
2. existingPathIsGitWorktree does not verify the path belongs to repoRoot. runGit([\"-C\", worktreeDir, \"rev-parse\", \"--is-inside-work-tree\"]) returns true for any git repo — including one rooted in a different repository or even the same path being a separate clone. If a user re-uses a stale git_worktree_dir that now points at a different repo, Ralph silently runs workers there and base_branch (anchored to repoRoot) becomes meaningless. Consider also checking git -C <worktreeDir> rev-parse --git-common-dir against repoRoot's common dir before accepting reuse, or at least document that ownership is the user's responsibility.
3. Concurrent worktree creation is racy. Two Ralph runs targeting the same missing path will both call git worktree add --detach <path> and one will fail mid-flight. The PR description acknowledges no locking, but the create path is the most surprising case — the existing-path branch is documented as user-managed sharing, whereas a creation race feels like a bug to a user. Even a brief retry-on-race or a mkdir-based sentinel would be friendlier; at minimum, the error message produced by addGitWorktree already hints at concurrency, which is good.
4. workflowStartCwd = process.cwd() is captured per .run call. This is fine in production but bun:test runs separate test files in parallel by default, and process.cwd() is process-global. The new unit tests in test/unit/builtin-workflows.test.ts:1275-1287 and integration tests in test/integration/ralph-worktree.test.ts:100-112 both process.chdir, and the author has helpfully noted the unit tests must run serially. The integration suite, however, uses 10 sibling tests each chdir-ing into a per-test tempdir without serialization — if Bun ever runs describe blocks concurrently within a file, these will flake. Adding a top-level // bun-test: --concurrency=1 style note, or capturing process.cwd() once per beforeEach into a closure (already done), is at least worth a comment.
5. writeSpecFile retry loop is unbounded (packages/workflows/builtin/ralph.ts:278-291). With flag: \"wx\" and suffix += 1 it'll spin forever on a persistent non-EEXIST error masked as EEXIST. The current behavior is fine in practice but a hard cap (e.g., 1000) would be a cheap safety net.

Tests

6. No coverage for WORKER_PREFLIGHT_CONTRACT injection. This PR's second big change — injecting <project_initialization_preflight> into Goal and Ralph worker prompts (packages/workflows/builtin/goal.ts:1042-1046, packages/workflows/builtin/ralph.ts:789-792) — has no test. A regression that removes either injection would not be caught. Two-line additions to existing tests would close this:
   • builtin-workflows.test.ts "persists a goal ledger…" → assert.match(ctx.calls.prompts[\"work-turn-1\"]?.[0] ?? \"\", /<project_initialization_preflight>/).
   • "leaves stage cwd unset when git_worktree_dir is not provided" → check that ctx.calls.prompts[\"orchestrator-1\"]?.[0] contains <project_initialization_preflight>.
7. PR description claims test/unit/discovery.test.ts was updated; the diff doesn't touch it. Not a real bug (shared-prompts.ts isn't re-exported from builtin/index.ts, so the discovery 4-builtin count is still correct), but the description should be corrected so reviewers don't go looking for it.
8. No unit tests for resolveWorktreePath / createGitWorktreeIfRequested / normalizeBranchInput. Integration tests cover the happy paths well, but every edge case currently requires a full git init + chdir + mock-ctx setup. A small pure-function unit suite (null-byte handling, absolute vs relative resolution, branch regex edge cases) would tighten the feedback loop and protect the regex in normalizeBranchInput from accidental loosening.

Security

9. No shell injection surface — all spawnSync(\"git\", args) calls pass arrays, never strings; base_branch is constrained by the strict regex in normalizeBranchInput at ralph.ts:243-247; git_worktree_dir is passed as a positional arg, not interpolated. Good.
10. quoteRecoveryCommandArgument (ralph.ts:407-410) is only used in error messages, not actually executed. The cmd.exe-style \"\" escaping is correct for that audience; non-issue.

Performance

11. spawnSync blocks the event loop in runGit. Fine for the handful of bookkeeping calls Ralph makes, but if existingPathIsGitWorktree ever gets called inside a loop it would matter. Currently single-shot, so non-issue.

Style / project conventions

12. Imports look correct for the no-build Bun/TS setup — .js specifiers preserved (./shared-prompts.js), no dist/, no compile step added. ✅
13. as const on GIT_LOCAL_ENV_VARS is the right call, readonly typing is consistent with the rest of the file. ✅
14. CHANGELOG entries are correctly placed under [Unreleased] with appropriate subsections. ✅

Smaller nits

• addGitWorktree's thrown error suggests a worktree remove --force recovery command, but the create just failed, meaning git either never registered the worktree (so remove no-ops) or registered it in a half-baked state. The wording is good, but worth a quick proofread to ensure the suggested command is always the right next step.
• formatWorktreeRecoveryCommand could live next to addGitWorktree's callsite since it's only used there.
• The unit-test comment at test/unit/builtin-workflows.test.ts:1274 ("these cwd-sensitive mock tests run serially in this file") would be even better as a reference to whatever Bun config flag actually enforces serialization — otherwise future contributors may not realize the constraint.

Overall

LGTM with the test-coverage gap on WORKER_PREFLIGHT_CONTRACT as the only thing I'd want to see addressed before merge — everything else is suggestion-tier. Nice work on the integration suite, especially the multi-iteration cwd propagation test.

[claude]

Claude Code Review — PR #1068

Thanks for this — the worktree feature is well-scoped and the integration test coverage is excellent. Notes below, organized by severity.

---

Finding 1 — Empty-graph validation is a behavior change for callers of run() directly (severity: medium)

The PR description says "Breaking Changes: None," but moving the empty-graph check from discovery to executor.run() is a runtime-observable change for callers who construct a workflow programmatically and invoke run(def, ...) without going through discovery. Previously these would return status: "completed"; now they return status: "failed". The test diffs in test/unit/depth-enforcement.test.ts, test/integration/runtime-tunables.test.ts, test/unit/executor-phase-c.test.ts, etc. are themselves evidence of this — multiple tests that previously did async () => ({ ok: true }) now have to add a stage to keep passing.

If @bastani/workflows has external consumers that drive executor.run() directly (tests, ad-hoc scripts, or SDK uses), they may now hit the new error message. Consider:

• mentioning this in the CHANGELOG under Changed rather than only as a discovery simplification, OR
• listing it under Breaking Changes in packages/workflows/CHANGELOG.md to give downstream a heads-up.

The user-facing experience also changes: previously a no-stage workflow was filtered at discovery (never showed up). Now it appears in the catalogue and only fails when the user picks it. That trade-off is worth being explicit about in CHANGELOG.

---

Finding 2 — Partial-creation retry path produces an unhelpful error (severity: low)

In createGitWorktreeIfRequested (packages/workflows/builtin/ralph.ts:416-444), if git worktree add partially succeeds — creates the directory but fails before registering the worktree — the next Ralph run will hit the "git_worktree_dir already exists but is not a Git worktree" branch.

That error message has no recovery hint, unlike the initial failure path which references formatWorktreeRecoveryCommand. Consider extending it with the same git -C <repo> worktree remove --force <path> suggestion (or the equivalent rm -rf for a stranded non-worktree dir), since this is precisely the orphaned-state case the recovery hint exists for.

---

Finding 3 — existingPathIsGitWorktree uses process.cwd() instead of the worktree path (severity: nit)

packages/workflows/builtin/ralph.ts:392-395 invokes runGit(["-C", worktreeDir, "rev-parse", "--is-inside-work-tree"]). The -C flag makes this correct, but runGit defaults the spawn cwd to process.cwd(). Passing worktreeDir as the second runGit arg would be clearer and remove a small dependency on process.cwd() for an operation that conceptually targets worktreeDir. Same pattern applies to other runGit callers — consider always passing cwd explicitly to avoid surprises if process.cwd() ever drifts mid-run.

---

Finding 4 — Worktree creation race between concurrent Ralph runs (severity: low)

The PR description acknowledges shared worktrees by design, but the creation race is a different case: two Ralph runs that both see pathExists() === false will both call git worktree add; one wins and the other fails with the existing fail-fast error. That is the documented behavior, but it might surprise users who started two Ralphs in parallel expecting them to share state. The error message handles this well (it points at "another active Ralph run is using this path"). No code change needed — just confirming this is the intended UX.

---

Finding 5 — Windows quote-escaping in quoteRecoveryCommandArgument is non-portable (severity: nit)

packages/workflows/builtin/ralph.ts:407-410 uses "" inside double-quotes on win32 — a cmd.exe convention. PowerShell expects backtick-quoted or \". Since this output is just a copy-pasteable hint in an error message (not executed), the impact is small. Worth a one-line comment so future readers understand the constraint, or detect shell and emit the right escaping.

---

Finding 6 — Subtle cwd redundancy in parallel call (severity: nit)

In the parallel discovery block (around lines 1450-1452 of ralph.ts), cwdOption is spread both onto each step and onto the parallel options. That is belt-and-suspenders, which is fine, but worth a one-line comment explaining whether step-level cwd or parallel-level cwd is authoritative when both are set — readers (and reviewers like me) will wonder.

---

Security

• spawnSync is invoked with array args throughout, so the base_branch and git_worktree_dir values never reach a shell — good.
• normalizeBranchInput rejects unsafe-looking refs and falls back to origin/main before reaching git worktree add. Good defense-in-depth, though spawnSync already neutralizes injection.
• The null-byte rejection in resolveWorktreePath is solid.
• The Git env scrubbing list (15 vars) looks comprehensive. Worth a one-line code comment noting why worker stages keep the inherited env while Ralph bookkeeping commands scrub it — your existing comment hits this, just confirming the asymmetry is deliberate.
• Path traversal via .. in git_worktree_dir is permitted (worktree may be created outside the repo root). Acceptable since the user explicitly supplied the path, but the input description could call out "may resolve outside the repo root" for transparency.

---

Test coverage

Excellent — the 10 integration tests cover most realistic states. A few gaps to consider, none blocking:

• Concurrent Ralph runs racing to create the same worktree (would need a fork/spawn harness; skip if too costly).
• Reusing a worktree whose .git file references a missing main repository (rare, but a real failure mode after a user removes the parent repo).
• Permission-denied on mkdir for the worktree parent — the code path returns status: 1 with a synthesized stderr, but no test exercises it.
• Symlinked worktree path (the integration test uses realpathSync for comparison so this is partially covered, but no explicit symlink fixture).

---

Smaller things

• The hand-written cast ctx as WorkflowRunContext<RalphInputs> in the .run() body could be avoided if defineWorkflow().input(...) propagated a typed ctx.inputs. That is outside the scope of this PR — flagging only because the cast is the friction this PR exposes.
• WORKER_PREFLIGHT_CONTRACT lives in shared-prompts.ts and is currently imported in two places (Ralph orchestrator + Goal worker). That is the right shape; consider adding a brief module-level docstring describing the contract audience so future workers know to use it.
• In packages/workflows/builtin/ralph.ts:757, the comment "Keep generated specs under the directory where Ralph was invoked, not in the worktree" is helpful — same kind of comment would be useful on workflowStartCwd itself in the run handler so readers do not wonder why it is captured.

---

Summary

The core design (git_worktree_dir reuse-as-is + no-cleanup + fail-fast guards) is sound and the implementation is careful with Git env hygiene and shell-safe arg passing. The biggest item is being explicit about the empty-graph validation move in CHANGELOG, since it is user-observable behavior for run() callers. Everything else is polish.

Nice work.

— Generated with Claude Code (Opus 4.7)

[claude]

Code Review — feat(workflows): run Ralph stages from optional git worktree

Thanks for the thorough write-up; the design is clearly thought-through and the test coverage (especially the new test/integration/ralph-worktree.test.ts matrix) is solid. A few substantive findings and several smaller notes below.

Bugs / correctness

1. existingPathGitWorktreeStatus treats any directory inside the same repo as a reusable "worktree." (packages/workflows/builtin/ralph.ts:418-433)

The check uses git rev-parse --is-inside-work-tree plus a common-dir match. That's true for repoRoot, any subdirectory of repoRoot, and any sibling linked worktree — they all share the same --git-common-dir. So:

• git_worktree_dir: \".\" → "same-repository" → Ralph silently runs stages with cwd set to the invoking checkout, which is not an isolated worktree at all.
• git_worktree_dir: \"packages/coding-agent\" (or any tracked subfolder) → same: Ralph reuses the live checkout in-place. Commits/branches affect the user's main working tree.

This is the opposite of the documented "isolated detached worktree" contract and could silently mutate the user's main checkout. Consider validating against git worktree list --porcelain (or comparing canonical paths to the toplevel of each listed worktree) so only true linked worktrees are accepted as reuse candidates, and rejecting the invoking repo root / a subdir with a precise error. At minimum, an integration test for the git_worktree_dir: \".\" case would have caught this.

2. Implementation-notes temp directory is never cleaned up. (packages/workflows/builtin/ralph.ts:492-509)

createImplementationNotesFile mkdtemps under os.tmpdir() and there's no removal path on success or failure. Every Ralph invocation leaks one directory (1 file + dir). Workflow has bounded loops (max_loops) and worktrees are intentionally left in place — but the notes file location is opaque to the user (only reachable via result.implementation_notes_path), so they have no obvious cue to clean it up. Two reasonable options:

• Move the notes file under the worktree/repo so its lifecycle matches the worktree the PR description already commits to leaving in place; or
• Document the path explicitly in the pr_report output and accept the leak.

The current state isn't great because the notes file is what carries authoritative implementation state, but it lives somewhere the user doesn't think to look or clean.

3. Path canonicalization in comparableGitPath falls back inconsistently. (packages/workflows/builtin/ralph.ts:395-404)

If realpathSync.native throws on one side (e.g. perms, ENOENT during a race) and succeeds on the other, you'll compare a realpathed path against a non-realpathed path and incorrectly return foreign-repository. Low likelihood, but easy to harden: fall back consistently on both sides, or treat the realpath error as a "cannot verify" case and reject rather than misclassify.

Design / API

4. Sharing worktrees across concurrent Ralph runs is left to the user. The PR description acknowledges this explicitly, but the prompt-level guidance to the agent (the "no automatic cleanup; retries are safe" framing) plus the fact that uncommitted state is preserved as-is means two concurrent runs targeting the same git_worktree_dir will produce nondeterministic interleaving of git operations, index lock contention, and partial commits. If you don't want to add a lockfile (fair given the "intentional sharing" goal), I'd suggest the git_worktree_dir input description explicitly call out "intended for one Ralph run at a time" so users don't infer from "retries are safe" that "parallel is safe."

5. PR-stage prompt names the branch <branch>. (packages/workflows/builtin/ralph.ts:1330)

git push origin HEAD:refs/heads/<branch> is handed to the LLM as a literal placeholder. Most models will substitute something, but it's nondeterministic. Consider deriving a default branch name (e.g. ralph/<spec-slug>-<date> like defaultSpecPath does) and putting the concrete suggestion in the prompt — then the user gets reproducible branch names across retries.

Discovery / executor changes

6. Runtime empty-graph validation looks correct (packages/workflows/src/runs/foreground/executor.ts:885-892, exercised by test/unit/executor.test.ts:120 and :2425). Moving away from Function.prototype.toString() regex matching is the right call — it was always going to miss helpers/indirection — and the runtime check has the right "fail with a clear message" shape. Persistence behavior is covered.

One minor: the error message lists ctx.stage(), ctx.task(), ctx.chain(), ctx.parallel() — worth confirming this matches the currently supported author-facing primitives. Today it does, but if you add new primitives that count as "stage creation," the message will drift.

Tests

7. Good coverage for: fail-fast outside repo, foreign-repo, foreign-worktree, missing base branch, non-empty non-git dir, null-byte path, retry-after-success, multi-iteration cwd propagation, and failure-preserves-worktree. Suggested additions to catch finding #1:

• git_worktree_dir: \".\" from repo root should be rejected (or accepted with an explicit "not a worktree" error).
• git_worktree_dir: \"<subdir-of-repo>\" should be rejected.
• A bare repo / .git directory as git_worktree_dir should be rejected.

8. test/integration/ralph-worktree.test.ts:189 addDetachedWorktree uses stdio: \"ignore\" everywhere — fine, but if git worktree add ever fails in CI (older git, sandbox restrictions) the failure will be invisible. Capture stderr or use stdio: [\"ignore\", \"ignore\", \"inherit\"] for diagnostics-on-failure.

Code quality / style (small)

• packages/workflows/builtin/ralph.ts:154-221 PLANNER_RFC_TEMPLATE embeds !\`git config user.name\` — relying on the agent's slash-command shell-execution syntax inside a prompt template. If a different agent surface ever runs this prompt, that becomes literal text. Worth a comment explaining the intent so it's not silently broken when someone tries Ralph on a non-Atomic/Pi runtime.
• packages/workflows/builtin/ralph.ts:647-706 model names (openai/gpt-5.5, openai/gpt-5.4-mini, etc.) read like placeholders — pre-existing to this PR, but worth flagging if these aren't real catalog entries the model-validation layer recognizes.
• WORKER_PREFLIGHT_CONTRACT (packages/workflows/builtin/shared-prompts.ts) is good; both Ralph (ralph.ts:832-834) and Goal (goal.ts:1042-1044) wire it up consistently.

Security

No injection surface: every git invocation is spawnSync with arg arrays, baseBranch runs through normalizeBranchInput's allowlist regex, paths are absolute after resolveWorktreePath, and \\0 is rejected. GIT_LOCAL_ENV_VARS scrubbing is a nice defensive touch. Looks clean.

Summary

The headline issue is #1 (subdir-of-repo silently counted as a reusable worktree). That's worth fixing before merge, ideally with a precise git worktree list check and a test. Everything else is incremental polish.

[claude]

Review — feat(workflows): run Ralph stages from optional git worktree

Thanks for the careful design write-up. Overall this is a well-scoped change with strong test coverage — real git fixtures exercising relative/absolute paths, foreign repos, retries, and worktree reuse are all valuable. A few findings, ordered by severity.

Design / behavior

[P2] PR-stage prompt mentions worktrees unconditionally. packages/workflows/builtin/ralph.ts:1334-1335 adds:

Ralph-created worktrees are detached HEAD checkouts. If you are preparing a PR from a detached HEAD, create and push a branch from the current HEAD…
Ralph does not remove git_worktree_dir automatically. Leave the worktree intact for retries or user recovery.

…to every Ralph run, including the default no-worktree case. For users who didn't pass git_worktree_dir, this is irrelevant noise that could nudge the PR agent into branch gymnastics when HEAD is already on a branch. Worth gating on workflowCwd !== undefined and threading a flag through RalphWorkflowOptions. Two related sub-points:

• The "Ralph-created worktrees are detached HEAD" framing is also inaccurate for the reuse path — an existing same-repository worktree may already be on a branch, not detached.
• The unit test test/unit/builtin-workflows.test.ts ("pull-request stage documents detached HEAD branch handoff…") explicitly asserts this guidance is present even with no git_worktree_dir, so the test would need updating if you gate it.

[P2] Discovery validation change shifts the failure surface. Previously, a workflow whose source didn't textually call .stage()/.task()/.chain()/.parallel() was rejected at discovery time with an INVALID_DEFINITION warning surfaced at session start (see test/unit/extension.test.ts before this PR). Now the workflow registers cleanly and only fails when invoked, with the runtime error appearing inside a run record rather than as a startup warning. That's the intended trade-off, but users authoring third-party workflows lose the early signal — consider whether discovery should still emit a non-fatal warning when the source clearly contains no stage primitive, leaving runtime failure as the hard guard.

[P3] WORKER_PREFLIGHT_CONTRACT is a meaningful Goal-worker behavior change with thin CHANGELOG coverage. The shared contract tells workers they are "responsible for initializing the checkout when setup commands are documented" — i.e., Goal workers will now run bun install/equivalent on partially-initialized checkouts. The CHANGELOG entry ("Split worker project-initialization preflight guidance from Goal receipt/reporting instructions") undersells this; readers won't realize Goal worker behavior changed. Consider an explicit "Changed: Goal workers now run documented setup commands when repository evidence shows missing initialization."

Code quality

[P3] existingPathGitWorktreeStatus collapses "git metadata read failed" into "not a worktree". In ralph.ts:418-433, if gitCommonDir(repoRoot) or gitCommonDir(worktreeDir) returns undefined (e.g., a malformed repo, transient git failure), we report not-git-worktree, which surfaces as "exists but is not a Git worktree". For a path that is in fact a worktree but where git failed, the error message will be confusing. Rare, but a distinct "could not read git metadata" diagnostic would help users recover.

[P3] gitProcessEnv() rebuilds the scrubbed env on every git invocation. The set of scrubbed vars is constant for a Ralph run; consider hoisting the cleaned env once. Trivial perf, but it also clarifies intent (the env is fixed for the lifetime of the helper, not per-call).

[P3] git_worktree_dir input description is missing the trailing period. Cosmetic — ralph.ts:1397.

Things done well

• Path safety: spawnSync array form + null-byte rejection + cross-platform recovery-command quoting (quoteRecoveryCommandArgument) is the right shape for user-controlled paths used in both git args and shell hints.
• comparableGitPath correctly uses realpathSync.native with case-folding on Windows, so symlinked worktree paths compare correctly.
• Foreign-repo detection via git common-dir comparison is the right primitive — handles linked worktrees correctly without false negatives.
• Spec artifacts (writeSpecFile(resolve(workflowStartCwd, …))) deliberately stay in the host invocation directory so they survive worktree churn. Nice call.
• Runtime empty-graph assertion lives in the right place — inside the try/catch so it routes through the standard failure persistence path (assertWorkflowCreatedStage at executor.ts:2485).
• Integration tests use real git, cover both error paths and the reuse-with-uncommitted-state path, and the failure-preservation test ensures the worktree isn't auto-cleaned. Strong coverage.

Security / perf

No concerns. Inputs aren't fed to a shell, env scrubbing is conservative (only Git's local-scoping vars; credentials and PATH are preserved), and the few extra spawnSync git calls only run at Ralph startup.

[claude]

Code Review (atomic-monorepo conventions)

Solid PR overall — the git_worktree_dir design is well-thought-out (fail-fast, no auto-cleanup, Claude-Code-style sharing semantics), the cwd plumbing is consistent, the move to runtime empty-graph validation is a real improvement over the brittle Function.prototype.toString() regex, and the integration test suite covers an impressive matrix of fixtures (relative/absolute paths, foreign repos, reuse, recovery, multi-iteration). Findings below.

Bugs / correctness

• packages/workflows/builtin/ralph.ts:418-433 — existingPathGitWorktreeStatus misclassifies real worktrees as "not-git-worktree" when git-common-dir lookups fail. If rev-parse --is-inside-work-tree returns "true" but either gitCommonDir call returns undefined (transient git failure, permission glitch, corrupted gitdir pointer), the function falls through to "not-git-worktree". The caller then throws git_worktree_dir already exists but is not a Git worktree, which is misleading and could send users down a wrong recovery path. Either propagate a distinct "unknown" status with a more honest error ("could not determine whether belongs to this repository"), or refuse to make a same-vs-foreign judgement and tell the user to investigate manually.

• packages/workflows/builtin/ralph.ts:454-490 — race window between pathExists and git worktree add. Two concurrent Ralph invocations targeting the same missing git_worktree_dir will both observe pathExists === false and both try git worktree add; the loser gets the Failed to create git worktree error path rather than falling into the same-repository reuse branch. The recovery message is helpful, but if "two same-name invocations resume/share" is meant to apply to the create race as well, consider catching the already exists stderr from git worktree add and re-running the same-repository status check before erroring. If a small race window where the loser must retry is intentional, one line of comment is enough.

• packages/workflows/builtin/ralph.ts:331-343 — runGit blocks the event loop via spawnSync. Most calls are short (rev-parse), but git worktree add on a large repo can take seconds and stalls every other workflow stage in the same process. Since createGitWorktreeIfRequested is already async, switching to child_process.spawn / Bun.spawn would be a small change and avoid head-of-line blocking on detached background runs. Low impact in practice — flag-only.

• packages/workflows/builtin/ralph.ts:354-360 — resolveWorktreePath only rejects \0, not ... A relative git_worktree_dir like ../../somewhere-outside-repo will resolve and then git worktree add will run against that path (since repoRoot is used as the base for relative resolution). git will refuse most pathological locations on its own, but the description's "relative paths resolve from the repo root" reads as containment to me. If repo containment is the intent, add an explicit check; if escape is acceptable, document it.

Style / conventions (per CLAUDE.md)

• Comment policy. CLAUDE.md is explicit: "default to writing no comments" and "don't explain WHAT the code does." A few new comments restate code rather than the why — ralph.ts:301-302 ("Scrub only Git's local repository/worktree-scoping environment…"), ralph.ts:321-323 ("Only Ralph's internal bookkeeping…"), ralph.ts:468-471 ("Match Claude Code's named-worktree model…"). The Claude-Code-named-worktree rationale is worth keeping (it's a real "why"); the env-scrub ones largely paraphrase the variable list above them.

• Test seam naming. runRalphWorkflowFromCwd is exported solely so tests can inject workflowStartCwd instead of mutating process.cwd() — a good call. The name reads like a public API though; an @internal JSDoc tag (or a _ prefix) would prevent future callers from treating it as a stable surface.

• workflowCwdOption helper at ralph.ts:492-494 + ...cwdOption spreads at ~10 call sites is workable but noisy. An alternative would be one mergeCwd(options, cwd) helper at each task site — equivalent code, less spread soup. Pure preference; current code is correct.

Test coverage

• Integration coverage is excellent — fail-fast paths, reuse, recovery, multi-iteration cwd propagation, and post-failure persistence are all exercised against real git fixtures.
• No test for the gitCommonDir-failure misclassification path above. A fixture where is-inside-work-tree succeeds but git-common-dir fails (e.g. by deleting .git/HEAD after worktree creation, or revoking read on the gitdir) would lock in whichever behavior you intend.
• No test for the concurrent-creation race. Two runRalphWorkflowFromCwd invocations racing on the same missing path. Even if the design says "the loser must retry," a test pins the boundary.
• test/unit/executor.test.ts:120-130 — good empty-graph coverage. Worth also confirming classifyWorkflowFailure returns a sensible kind/resumable for the empty-graph error so downstream telemetry/UX is consistent with other validation failures.

Performance / Security

• git worktree add --detach <path> <base_branch> runs via spawnSync argv (no shell) and base_branch is normalizeBranchInput-sanitized. Worktree path is user-supplied but not shell-interpolated. No injection vector.
• GIT_LOCAL_ENV_VARS scrubbing is conservative (only repo-scoping vars; auth/SSH/committer identity preserved for worker stages).
• formatWorktreeRecoveryCommand correctly quotes both paths for the host shell.
• Per-iteration git overhead is bounded (~4 small calls), and the early return at ralph.ts:460 short-circuits the no-worktree case.

Process

• CHANGELOG entry is in the right place under [Unreleased] with correctly-named Added/Changed subsections — matches the repo convention.

🤖 Generated with Claude Code

[claude]

Code Review — feat(workflows): Ralph git worktree support

Reviewed the Ralph worktree integration, shared preflight prompt, and the discovery/runtime validation refactor. Overall: focused, well-tested PR with clean fail-fast semantics, sensible "Claude Code-style" no-lock reuse model, and good defense against shell injection via spawnSync array form + normalizeBranchInput sanitization. Test coverage of the new worktree behavior is genuinely thorough (12 integration tests covering reuse, foreign-repo rejection, base-branch failure, multi-iteration cwd propagation, and failure-time preservation).

A few findings worth considering before merge:

Bugs / Correctness

[P2] displayPathFrom returns empty string when path equals cwd — packages/workflows/builtin/deep-research-codebase.ts:300-306. When the artifact path is identical to workflowStartCwd, relative(cwd, path) returns "" and the early-return branch falls into displayPath(\"\") → \"\". Several call sites embed this into prompts (e.g. Read the scout artifact before making evidence claims: ${displayWorkflowPath(scoutPath)}). If this ever fires, the LLM prompt becomes ambiguous. Probably never triggered in practice (artifacts are always under artifactRoot), but the guard is cheap — return "." or the absolute path when the relative result is empty.

[P2] validateWorkflowCreatesStage fail-open swallows arbitrary errors — packages/workflows/src/extension/discovery.ts:236-248. The catch block returns null (valid) for both the sentinel and any other exception, with a comment explaining this is intentional fail-open behavior. The runtime empty-graph guard backstops correctness, but the UX is asymmetric: a workflow whose run() throws synchronously before any ctx.stage/task/chain/parallel call passes startup validation, then fails at runtime with a generic "completed without creating any workflow stages" error (which is misleading — it didn't complete, it threw). Worth distinguishing the cases at least in the discovery diagnostic comment, or surfacing a softer "workflow run threw during validation probe; runtime will verify" warning.

[P3] existingPathGitWorktreeStatus collapses gitCommonDir failure into not-git-worktree — packages/workflows/builtin/ralph.ts:418-433. If git rev-parse --git-common-dir succeeds for the requested worktree but fails for repoRoot (rare: corrupted index, transient FS issue), the function reports not-git-worktree and Ralph errors with "already exists but is not a Git worktree" — a confusing message when the path is a git worktree but the invoking repo's .git couldn't be probed. Consider distinguishing "could not classify" from "definitely not a worktree" so the error message points at the real failure.

Robustness

[P3] writeSpecFile unbounded EEXIST retry loop — packages/workflows/builtin/ralph.ts:276-292. The for (let suffix = 0; ; suffix += 1) loop only exits on successful write or non-EEXIST error. On a pathological filesystem (e.g. read-only mount where every write fails as EEXIST due to some quirky overlay), this would spin forever. A bounded retry (say, 1000) with a clear error would fail fast instead.

[P3] resolveWorktreePath null-byte message wording — packages/workflows/builtin/ralph.ts:357. The check is on the entire string but the message says "path segment". Minor — "...contains an unusable null byte; provide a valid path..." would read more naturally.

Design observations (not blockers)

• The "Claude Code-style sharing" model (no app-level lock, concurrent Ralph runs against the same git_worktree_dir race) is documented in the PR description and Ralph's input schema, but only a code comment captures it in createGitWorktreeIfRequested. Worth adding a short note to the user-facing description that two concurrent runs will see each other's intermediate state.
• gitProcessEnv() scrubs 15 GIT_* vars for Ralph's bookkeeping commands but worker stages keep the inherited env (per the comment). This is the right trade-off for credential passthrough, but if the inherited env contains e.g. GIT_DIR pointing to a different repo, workers could behave oddly. Comment captures the intent — fine as documented.
• workflowCwdOption is a tiny helper that wraps an optional cwd into a spread-friendly object. Used heavily throughout runRalphWorkflow. Fine, but if the workflow grows another conditional option, a generic definedOption helper would scale better.

Test coverage

Coverage of the new worktree paths is good:

• Happy paths: relative-from-repo-root, absolute, reuse-with-uncommitted-state, multi-iteration cwd propagation.
• Fail-fast: outside git repo, missing base branch, non-empty non-git dir, foreign-repo worktree, foreign repo at path, null-byte path, planner failure preserves worktree.

Gaps worth filling:

• No coverage for git_worktree_dir == workflowStartCwd (e.g., user passes .). Should resolve to the repo root, classify as same-repository, and reuse — but untested. The integration test fixture infrastructure already supports this.
• No coverage for the runtime empty-graph guard in runRalphWorkflowFromCwd path — the executor test test/unit/executor.test.ts:120 covers assertWorkflowCreatedStage in isolation, but no test verifies a Ralph-shaped workflow that exits early without a stage gets the failed status. Low priority since the executor test owns this.

Style / conventions

The PR adheres to the repo's Bun + raw-TS conventions, uses node: prefixed imports, and keeps the new helpers reasonably small. The reviewer-failure JSON synthesis (reviewerErrorResult) mirrors the goal-runner pattern — good consistency.

Nice work on the artifact-vs-worktree path split (specs stay under invocation cwd, stages run from worktree cwd) — that's exactly the right division for retry/inspection ergonomics.

🤖 Generated with Claude Code

[claude]

PR Review — feat(workflows): run Ralph stages from optional git worktree

Reviewed against CLAUDE.md conventions (Bun, raw-TS workflows, no build step, strict types). Overall the worktree contract is well-thought-out and the integration test suite is thorough. A few items below worth a second look before merging.

Likely-genuine issues

1. Discovery validation probe now executes user/project workflow run bodies on startup.
packages/workflows/src/extension/discovery.ts:228-272 replaces the old regex check with a real call to def.run(probeCtx) that throws on the first stage primitive. Any work the run function does before its first ctx.stage/task/chain/parallel call now runs at every Atomic startup for project-local, user-global, settings-, and package-discovered workflows. Bundled workflows are correctly exempted via applyBatchShapeOnly, but our own builtins illustrate the pattern that's now risky for user code:

• ralph.ts calls await createImplementationNotesFile(prompt) (mkdtemp + writeFile) before the first ctx.task (packages/workflows/builtin/ralph.ts:660).
• goal.ts calls await createGoalLedger(...) (mkdtemp + writeFile) before the first ctx.task (packages/workflows/builtin/goal.ts:993).
• deep-research-codebase.ts calls countCodebaseLines(workflowStartCwd) (spawns git ls-files + wc -l) and createArtifactRoot(...) before the first stage primitive.

The PR description treats this as an internal cleanup, but it's a real behavior change for downstream workflow authors: any user-authored workflow that follows this very natural pattern will leak temp dirs, fire spawns, and possibly hit the network on every CLI startup. Two suggestions:

• Call this out explicitly in the CHANGELOG (Changed) so authors know to defer side effects until after the first ctx.task/stage.
• Consider extending applyBatchShapeOnly to all sources (or making the probe opt-in), since the runtime empty-graph check in executor.ts:887-890 already catches the no-stage case at execution time. The probe duplicates that guarantee at the cost of running user code at discovery.

2. Probe rejects workflows that conditionally create stages based on input.
Because validationInputsFor synthesizes ""/0/false/first-choice values when no default is set (discovery.ts:203-215), a workflow like:

.run(async (ctx) => {
  if (ctx.inputs.feature_enabled) {
    await ctx.task("foo", { ... });
  }
})

will be rejected with INVALID_DEFINITION ("run must create at least one workflow stage"), even though it's perfectly valid when the toggle is true. The old regex check was permissive here. Worth either widening the probe (count any reachable markStageObserved in any branch) or downgrading "no stage observed under default inputs" from error → warn.

3. displayPaths is dead code after the refactor.
packages/workflows/builtin/deep-research-codebase.ts:309-311 — every call site was migrated to displayWorkflowPaths. The function (and the only-internally-used displayPath if the refactor went further) is now unreferenced. Repo CLAUDE.md calls out noUnusedLocals as a convention; remove or it'll just be confusing for future readers.

Smaller items

4. Probe sentinel can be swallowed by user try/catch.
WORKFLOW_STAGE_OBSERVED is a Symbol, so a workflow that wraps await ctx.task(...) in try { ... } catch (err) { logger.warn(err); } will swallow the sentinel and continue executing arbitrary code at discovery time (with stageObserved already set). The probe handles this gracefully (post-loop check returns success), but the workflow's full body runs at startup. Low risk in practice; could be hardened by re-throwing if err === WORKFLOW_STAGE_OBSERVED is detected via a finally pattern or by replacing primitives with no-ops that record observation in closed-over state without throwing.

5. Misleading error when git_worktree_dir is an existing regular file.
existingPathGitWorktreeStatus (ralph.ts:432-457) only branches on git status. If the user passes a path that is a file (not a directory), git rev-parse --is-inside-work-tree will fail with ENOTDIR, producing "git_worktree_dir already exists but is not a Git worktree" instead of the truer "not a directory". Cheap fix: stat() after pathExists() and distinguish isDirectory() explicitly.

6. PR-stage prompt assumes detached HEAD.
ralph.ts:1361 tells the worker: "Ralph-created worktrees are detached HEAD checkouts." But a reused existing worktree (the same-repository branch on ralph.ts:491-497) may already be on a branch. The instruction is still safe — it conditionalizes on "If you are preparing a PR from a detached HEAD" — but the lead-in sentence could be tightened to "Ralph-created worktrees are detached; reused worktrees may already be on a branch — check git symbolic-ref -q HEAD first."

7. Bundled-only shape validation means user workflows now pay an async-discovery cost.
applyBatch is now async and serial (discovery.ts:316-365). With N user workflows, startup walks them sequentially, each potentially doing I/O up to the first stage primitive (see #1). For large numbers of project/user workflows this could noticeably slow Atomic boot. Worth measuring or parallelizing if it becomes a problem.

Test coverage — strong

test/integration/ralph-worktree.test.ts is excellent: relative + absolute paths, fail-fast for outside-of-repo / foreign-repo / non-empty-non-git / unusable / missing-base-branch, reuse with uncommitted state, multi-iteration cwd propagation, failure-state preservation. Two small additions would round it out:

• Symlinked git_worktree_dir (path resolution + canonicalization correctness).
• An existing reused worktree that's currently on a branch (verify branch state is preserved through the run).

Nits

• ralph.ts:519-521 workflowCwdOption is a fine helper but could be one line at call sites (workflowCwd ? { cwd: workflowCwd } : {}).
• validationInputValue (discovery.ts:203-215) handles type: "text" | "string" but the discriminated union is TextInputSchema with type: "text" | "string" — the switch is correct, just worth a comment noting both alias the same schema.

Summary

The Ralph worktree feature itself is well-designed, well-tested, and ships with helpful error messages and recovery commands. The two items I'd most want addressed before merge are (1) the silent side-effect change for non-bundled workflow discovery and (2) the conditional-stage false-negative — both are correctness/UX regressions vs. the previous behavior for user workflow authors. (3) dead code is a quick cleanup.

[claude]

Review for PR #1068 — feat(workflows): run Ralph stages from optional git worktree

Thanks for the thorough write-up and tests — the contract is well-documented and the worktree state machine (missing / same-repo / foreign-repo / not-git / unclassified) reads cleanly. A few observations, ranked roughly by impact:

---

Bugs / correctness concerns

1. cwd seam in runWorkflow is not propagated into the workflow body, making the refactored workflow-runner.test.ts test misleading.

In packages/workflows/src/runs/shared/workflow-runner.ts:209, options.cwd is only forwarded to discoverWorkflows, never to the executor's run() or into the workflow's ctx. The executor itself never threads cwd into the workflow definition's run(ctx) call. The new test at test/unit/workflow-runner.test.ts:177-196 removes process.chdir(dir) in favor of { cwd: dir, ... }, but for the named-workflow path dir ends up effectively unused — runDeepResearchCodebaseWorkflow(ctx) defaults workflowStartCwd to process.cwd(), so the deep-research artifacts (research/<date>-...md, research/.deep-research-...) will be written under the test runner's process.cwd() (typically the repo root), not dir.

This is both a test-pollution risk (real files dropped under the repo during tests) and a regression of the test's original intent. Two options:

• Plumb a workflow-level cwd seam: pass options.cwd into run() and expose it on the WorkflowRunContext (ctx.cwd), then update bundled workflows to read it instead of process.cwd(). This is the principled fix.
• Short term, at minimum, assert that artifacts land under dir so the test fails fast if the seam regresses (right now there's no dir-based assertion and the variable is essentially dead).

---

2. Discovery's stage-validation probe will emit VALIDATION_PROBE_FAILED for any user workflow that input-validates before its first stage primitive.

validateWorkflowCreatesStage (discovery.ts:228) materializes inputs from schema defaults (e.g. "" for text), then runs the workflow body. If a workflow does the very-reasonable pattern of throw new Error("foo requires an X input.") before calling ctx.stage/task/chain/parallel (this is exactly what builtin/goal.ts:984-987 does for objective), the probe takes the validationSuccess(warning) branch and a warning is logged every time that user workflow is loaded.

Bundled workflows dodge this because discoverBundledManifest uses applyBatchShapeOnly. But any custom workflow authored in ~/.atomic/agent/workflows/ or .atomic/workflows/ with the same defensive-input-validation pattern will spam this warning. Two safer options:

• Treat a missing-required-input throw from the probe as success, not a warning (catch and inspect the message — or, cleaner, populate required inputs with a recognizable sentinel and suppress warnings when the throw stack mentions the sentinel).
• Only flag the warning when the throw isn't an input-validation throw — e.g. probe twice: once with defaults, once with all-required-inputs-filled sentinels; only warn if both throw.

At minimum, please document the expected DX in the warning message ("populate inputs defaults to silence this warning").

---

3. validationInputValue uses inputs.default ?? "" for text (discovery.ts:203-215), meaning text schemas with no default get "". For required inputs with no default (like prompt on ralph), the synthesized value is "", leading users into the trap above. Consider materializing required-with-no-default inputs as something distinct (e.g. an opaque marker) so the workflow can distinguish a probe call from a real one.

---

Smaller issues

4. Windows quoting in quoteRecoveryCommandArgument (ralph.ts:449-452) uses doubled "" quoting (cmd.exe style). That's correct for cmd but wrong for PowerShell, where backtick escaping or '…' is the idiom. Since the string is documentation only (printed in the error, not executed), this is low impact — but a comment noting "cmd.exe quoting; users on PowerShell may need to adjust" would be honest.

5. runGit uses spawnSync (ralph.ts:311). Synchronous spawn is fine for the bookkeeping classification flow you've built, but it does block the event loop while git runs. For git rev-parse --git-common-dir and friends this is microseconds; for git worktree add on a large repo, it could be a noticeable hiccup if Ralph is running alongside other async work. Not a blocker, but worth noting if the workflow grows.

6. Regex normalizeBranchInput is duplicated between ralph.ts:244 and goal.ts:344. Same character-class, same negative lookaheads. Since both files import from each other's neighborhood and you already have shared-prompts.ts for shared concerns, a shared-git.ts (or similar) would be a natural home. Not a blocker for this PR.

7. existingPathGitWorktreeStatus runs git rev-parse --is-inside-work-tree inside worktreeDir itself (ralph.ts:413). If a malicious actor controls the worktree path, they could plant a .git/config with core.fsmonitor or core.hooksPath set to an executable they own — which git happily executes during rev-parse. The Git env scrubbing in gitProcessEnv (ralph.ts:301-309) doesn't defend against repo-local config. For Ralph this is probably fine because git_worktree_dir is user-supplied and the same user controls the host, but if Ralph is ever invoked in a context where the worktree path comes from untrusted input (e.g. a CI config), this is the standard safe.directory / GIT_CONFIG_GLOBAL=/dev/null territory. Consider documenting this trust boundary, or add -c safe.directory=* and / or GIT_CONFIG_GLOBAL scrubbing to the env.

8. comparableGitPath falls back to resolve(path) when realpathSync.native throws (ralph.ts:386-392). If the worktree dir is a symlink under a symlinked parent, this could yield a false foreign-repository classification (real vs unresolved paths compared). The current code only catches errors, but doesn't note that this is best-effort. A short comment explaining the silent fallback would help future-you.

9. Tests rely on git config commit.gpgsign=false only on the initial commit (test/integration/ralph-worktree.test.ts:141). Subsequent commands like git worktree add don't sign, so this is fine — just noting that if any future test commits in a worktree, it'll need the same flag.

---

Things I like

• The state machine for existing-path classification — including the unclassified branch with a discrete reason — is exactly right. It surfaces the real failure ("Ralph could not verify…") rather than misclassifying as foreign.
• The CHANGELOG entries land in the right [Unreleased] subsections and the attribution style matches CLAUDE.md.
• 12 real-git integration tests with proper mkdtempSync setup/cleanup. Coverage spans both creation and reuse paths, and addDetachedWorktree is a nice helper.
• Symbol-based short-circuit in validateWorkflowCreatesStage (throw WORKFLOW_STAGE_OBSERVED) survives an inner try/catch because stageObserved is flagged before the throw. Clean.
• Worktree path resolution: absolute used as-is, relative resolved from repo root (not cwd). Matches the described contract and matches the test assertions.
• The "share existing worktrees as-is, no app-level lock" design is well-justified in the PR description and the comments at ralph.ts:471-475.

---

Recommendation

The Ralph worktree feature itself is well-built. The two things I'd want addressed before merge:

1. The cwd plumbing question in runWorkflow — even just an explicit acknowledgment in a code comment that options.cwd only affects discovery, with the test asserting against dir if the intent is to verify isolation.
2. A decision on the VALIDATION_PROBE_FAILED warning UX for input-validating user workflows.

The rest are nice-to-haves. Happy to dig deeper if any of these need clarification.

🤖 Generated with Claude Code

[claude]

Review

Solid, well-scoped PR. The worktree integration is implemented as a tight, fail-fast surface around Ralph; the discovery refactor replaces a brittle regex with a real observation probe; and test coverage (especially the 12 real-Git fixture integration tests) is excellent. A handful of concerns worth considering before merge.

Code quality & correctness

comparableGitPath symlink fallback is asymmetric (packages/workflows/builtin/ralph.ts:383-392)
When realpathSync.native(path) throws, the catch falls back to resolve(path), which does not resolve symlinks. If the repo's common dir is reachable via one path and the worktree's is the symlinked equivalent — and one of the two realpathSync calls happens to fail (permissions, transient ENOENT) — you'd get an asymmetric comparison and a false foreign-repository rejection. Two options: either fail loudly when realpath fails (it's signaling something), or apply the same resolve fallback consistently and document that fact. As written, the failure mode is silent and produces a misleading error message to the user.

TOCTOU between pathExists and addGitWorktree (packages/workflows/builtin/ralph.ts:469-496)
A concurrent run could create the directory between the pathExists check and addGitWorktree. The error message already hints at this and asks the user to rerun, which is fine given the intentional sharing model — but the wording "If another process just created this same-repository worktree, rerun Ralph to resume it" buries the actual remediation. Consider auto-retrying the classification path once on git worktree add failure when the path now exists, so an interleaved-create case heals itself instead of demanding a user rerun.

quoteRecoveryCommandArgument Windows quoting is CMD-only (packages/workflows/builtin/ralph.ts:449-452)
The Windows branch doubles \" to \"\", which is cmd.exe quoting. PowerShell (the default modern Windows shell) uses backtick or single-quote escaping; running the printed recovery command in PowerShell with a path containing a literal \" will misparse. Since this is only a printed hint, it's not blocking — but worth either noting in the message which shell it assumes or using a form both shells will accept (most simply: avoid double-quoting paths that don't actually contain spaces).

Inherited process.env in runGit (packages/workflows/builtin/ralph.ts:301-323)
The scrub list is targeted and well-commented. One thing to verify: if a host process sets GIT_TERMINAL_PROMPT=0 or auth-related vars, those flow through. That's intentional for credential handling, but the scrub also leaves GIT_TRACE* and GIT_PAGER in place — fine, but if a CI host has GIT_PAGER=less exported and stdin is closed, git rev-parse should still be fine since it doesn't paginate. Just confirm no env-driven path could cause runGit to block.

Discovery validation probe

Real def.run(ctx) execution at discovery time is a behavior change (packages/workflows/src/extension/discovery.ts:228-273)
This is much better than the previous regex check, but it does mean every author's run function will be partially executed at startup with synthesized inputs. The probe inputs include a __atomic_workflow_validation_probe_input__ sentinel for required text/string fields — but if a workflow author validates input shape after an await readFile(...) or makes a network call before the first ctx.stage()/ctx.task(), that side effect now happens at every discovery. The probe handles thrown errors gracefully via the VALIDATION_PROBE_FAILED warning, but side-effectful imports are silent. Worth adding a brief note in the workflow authoring docs that run should not perform side effects before the first stage primitive.

validationInputValue for select with empty choices (packages/workflows/src/extension/discovery.ts:212-214)
Falls back to \"\", which may not be a member of the schema's choices. If a workflow has if (!choices.includes(value)) throw, it'll get a probe warning. Probably acceptable, but a no-choices select is unusual enough that an explicit check upstream might catch authoring bugs earlier.

Tests

Coverage is genuinely strong here. Two minor nits:

• test/integration/ralph-worktree.test.ts:269-288 ("reuses an existing git worktree in its current state") sets base_branch: \"missing-branch\" to prove the reuse path skips branch validation, but the test name doesn't telegraph that. A comment explaining "the base branch is intentionally bogus to prove reuse short-circuits creation" would help future readers.
• test/integration/ralph-worktree.test.ts:355-371 ("fails fast when base_branch does not exist") relies on the \"missing-branch\" literal but normalizeBranchInput would also reject some malformed branch names by silently rewriting to origin/main. Worth one assertion confirming the failure path is the worktree-creation failure, not the silent normalization fallback.

Nits

• packages/workflows/builtin/ralph.ts:642 — workflowSpecPath is computed once with new Date() captured inside defaultSpecPath. If Ralph runs across a midnight UTC boundary, later iterations still revise the original file (which is what you want — good), but consider locking the date at the top of runRalphWorkflowFromCwd and threading it through, so the value is obvious from the call site.
• packages/workflows/builtin/ralph.ts:1437 — git_worktree_dir description is dense; consider splitting the description into a short headline and a longer description if the SDK supports it, since the help text will be shown on workflow-list output.
• packages/workflows/CHANGELOG.md — "Avoided blank deep-research display paths" and the matching displayPathFrom change look correct; a one-line test fixture asserting displayPathFrom(cwd, cwd) === \".\" would lock that in.

Summary

LGTM with the caveats above. The comparableGitPath symlink fallback is the most worth-fixing item; the rest are observations rather than blockers.

[claude]

Review of #1068 — Ralph git worktree + discovery probe refactor

Thanks for the well-documented PR. Test coverage is excellent — 12 new integration tests plus thorough unit coverage of the discovery probe semantics. I focused on the moving parts that have non-obvious runtime implications. Findings below, prioritized P1–P3 per the repo's reviewer norms.

[P1] Discovery now executes user workflow bodies at startup

validateWorkflowCreatesStage in packages/workflows/src/extension/discovery.ts:208 invokes def.run(ctx) against every discovered non-bundled workflow during async discovery (applyBatch). That's a substantive behavioral change vs. the previous Function.prototype.toString() regex guard, which never executed the body. The probe aborts on stage()/task() via a thrown WORKFLOW_STAGE_OBSERVED Symbol, but any side effects performed before the first stage primitive now run at startup with sentinel inputs (e.g. __atomic_workflow_validation_probe_input__). For example, a user workflow with await mkdir(...), await fetch(...), or await createImplementationNotesFile(prompt) at the top of run() will execute those calls every time discovery runs.

Notes:

• Bundled workflows are unaffected because discoverBundledManifest() calls applyBatchShapeOnly (good). The risk is entirely on user/settings-*/user-global/package workflows.
• The VALIDATION_PROBE_FAILED warning-but-register behavior is the right escape hatch for throws, but a workflow whose preflight succeeds (e.g. writes a temp file) leaves footprints.
• This should be called out explicitly in the changelog and/or the validateWorkflowCreatesStage JSDoc so authors know "don't put side effects before your first stage primitive."

Suggestions: (a) document the contract loudly above validateWorkflowCreatesStage; (b) consider gating the probe behind an opt-out env var or schema flag for workflows that need to declare "I have setup before my first stage"; (c) at minimum, add a test that exercises a probe-side-effect workflow so the behavior is pinned.

[P2] writeSpecFile silently overwrites pre-existing spec files

packages/workflows/builtin/ralph.ts:267 removed the flag: "wx" + suffixed-collision path and now writes with default truncating semantics:

async function writeSpecFile(path: string, content: string): Promise<string> {
  await mkdir(dirname(path), { recursive: true });
  await writeFile(path, content.endsWith("\n") ? content : `${content}\n`, { encoding: "utf8" });
  return path;
}

The unit test revises the original Ralph spec file across planner iterations (test/unit/builtin-workflows.test.ts:1333) confirms this is intentional and asserts the pre-existing file is replaced. Fine within a single workflow run, but two real-world hazards:

1. Two Ralph runs on the same project, same date, same prompt slug → second run silently clobbers the first run's RFC.
2. A user who manually authored or edited specs/YYYY-MM-DD-<slug>.md between runs will lose that work without warning.

This is a behavioral change from the previous suffixed-collision flow, and the PR description lists "Breaking Changes: None" — I'd argue this qualifies as user-visible breakage. Either document it as a breaking change, or detect & preserve non-Ralph content (e.g. read the file first, only overwrite if it looks like a prior Ralph spec), or include a timestamp/runId in the path so unrelated runs don't collide.

[P2] chain/parallel probe handlers don't short-circuit like stage/task

In validateWorkflowCreatesStage (discovery.ts:215):

stage: () => markStageObserved(),   // throws
task:  () => markStageObserved(),   // throws
chain: (steps) => {
  if (steps.length > 0) markStageObserved();  // throws only when steps non-empty
  return Promise.resolve([]);                  // continues otherwise
},
parallel: (steps) => {
  if (steps.length > 0) markStageObserved();
  return Promise.resolve([]);
},

Two inconsistencies:

• chain([])/parallel([]) neither set stageObserved nor throw, so the probe keeps executing the workflow body past those calls — more discovery-time side-effect surface area (compounds with the P1 finding).
• For non-empty cases, markStageObserved() always throws, so return Promise.resolve([]) is unreachable dead code that misleads readers.

Aligning the four handlers to a single semantic (e.g. "any invocation, including with empty steps, marks observed and throws") would also let an await ctx.chain([...]) short-circuit identically to await ctx.task(...).

[P2] runRalphWorkflowFromCwd is now an exported public symbol

packages/workflows/builtin/ralph.ts exports both runRalphWorkflowFromCwd and the inner runRalphWorkflow. Tests import the former as an entry point. If that's deliberately a public API for programmatic Ralph invocation, it should appear in the workflows SDK surface and CHANGELOG ### Added. If not, mark it @internal (or move to an internal/ subpath) so it isn't part of the package's effective contract — once a test imports it, consumers will too.

[P3] Detached HEAD reliance is prompt-only

The PR stage tells the worker: "if you're preparing a PR from a detached HEAD, create and push a branch from the current HEAD." That's the only protection against a worker forgetting to create a branch, and PRs from detached HEAD have surprising failure modes (the pushed commit may be unreachable from any local ref after the workflow exits if the worker pushes by SHA). Consider creating a deterministic branch (e.g. ralph/<runId>) inside the worktree after git worktree add --detach instead of relying on the worker to remember.

[P3] comparableGitPath Win32 lowercase heuristic

comparableGitPath (ralph.ts:386) always lowercases the canonical path on win32. NTFS is usually case-insensitive but can be configured per-directory to be case-sensitive (e.g. with fsutil file setCaseSensitiveInfo). On such volumes the lowercase comparison could incorrectly classify two distinct paths as the same repository. Worth a one-line comment noting this is a heuristic.

[P3] Bundled vs. discovered probe asymmetry

discoverBundledManifest() uses applyBatchShapeOnly while everything else goes through the probing applyBatch. Asymmetry is intentional (synchronous startup), but it means a regression like an empty-graph bug in a builtin like goal.ts would not be caught at discovery — only by the runtime assertWorkflowCreatedStage guard. Acceptable, but worth a brief comment in discoverBundledManifest.

Nits

• EMPTY_WORKFLOW_GRAPH_ERROR_MESSAGE in executor.ts:889 is captured into the test regex; that's fine but a constant exported from a shared location would let other tests reuse it instead of inlining the regex (test/unit/executor.test.ts:130 and elsewhere).
• WORKFLOW_STAGE_VALIDATION_ERROR in discovery.ts duplicates message content with EMPTY_WORKFLOW_GRAPH_ERROR_MESSAGE in executor.ts — minor, but if you ever rephrase one, the other will drift.
• validationInputValue for select falls back to "" when schema.choices[0] is undefined, even though "" is generally not a valid choice. If choices is non-empty (typical), unreachable; still, returning schema.choices[0] directly without ?? "" keeps types tighter.

Test coverage

test/integration/ralph-worktree.test.ts is thorough — happy to see real Git fixtures rather than mocks, the failure-preservation test, and explicit absolute vs relative path coverage. The decision to remove process.chdir and pass explicit cwd seams (per CLAUDE.md guidance on parallelizable tests) is exactly right.

Overall this is a well-scoped change with strong test backing. The P1 discovery-side-effect surface is my main concern — once that's either documented or constrained, the rest are small refinements.

🤖 Generated with Claude Code

[claude]

Code Review — PR #1068 (feat(workflows): run Ralph stages from optional git worktree)

Solid, well-scoped PR. The Ralph worktree work is internally consistent, well-tested, and the supporting changes (discovery refactor, ctx.cwd, shared preflight prompt) are reasonable. Most comments below are about edge-cases and a couple of concrete bugs worth fixing before merge.

Bugs / correctness

1. existingPathGitWorktreeStatus cannot distinguish a worktree root from a subdirectory of one — packages/workflows/builtin/ralph.ts:419-460

If git_worktree_dir points at an existing subdirectory of the invoking repository (e.g. git_worktree_dir=./src or ./worktrees after a previous run created the parent but not the leaf), the current classifier returns same-repository and Ralph happily uses it as the worker cwd. That's because:

• git rev-parse --is-inside-work-tree returns true for any directory inside a working tree, not just worktree roots.
• git rev-parse --git-common-dir returns the shared common dir for the main worktree and every linked worktree of the same repo, so any subdirectory of the main checkout looks identical to a real worktree root by this check.

Net effect: a user who points Ralph at myrepo/wt thinking they're isolating writes can end up with stage cwd = myrepo/wt (a regular subdir of myrepo), and worker edits land inside the main checkout. This silently defeats the headline isolation benefit.

Suggested fix: compare against git -C <path> rev-parse --show-toplevel (or --git-dir and require it to look like .git/worktrees/<name> for linked worktrees, or be <path>/.git for the main worktree) and only accept the path when it is actually a worktree root for this repo. The integration test in test/integration/ralph-worktree.test.ts doesn't currently exercise this case — worth adding.

2. runGit cwd default falls back to process.cwd() instead of the caller's workflowStartCwd — packages/workflows/builtin/ralph.ts:311

runRalphWorkflowFromCwd carefully threads workflowStartCwd everywhere it matters, but runGit(args, cwd = process.cwd()) uses the process cwd when a call site forgets to pass one. Today every caller passes one explicitly, so this is latent; but it's the same process.cwd() ambient-state hazard the rest of this PR worked hard to eliminate. Either remove the default and force callers to pass cwd explicitly, or default to undefined and let spawnSync decide (so misuse becomes obvious).

3. Misleading error label when mkdir for the worktree parent fails — packages/workflows/builtin/ralph.ts:362-369, 510-517

When mkdir(dirname(worktreeDir)) fails, addGitWorktree synthesises a GitCommandResult with stderr: \"mkdir: failed ...\", and the caller wraps it as "Failed to create git worktree at requested git_worktree_dir ... Git reported: mkdir: failed ...". The "Git reported" framing is wrong here — this is a filesystem failure before git ever ran. Minor, but the error class is one of the harder ones to triage if mistyped. Consider raising the mkdir failure separately.

4. writeSpecFile silently overwrites an unrelated pre-existing spec at the same path — packages/workflows/builtin/ralph.ts:266-272, 665

The old code suffixed -2, -3, … to avoid colliding with a user's existing markdown. The new behavior — "one stable original spec file under specs/ … revise that same spec" — is explicitly intended for retries, but it also clobbers any unrelated file at specs/<date>-<slug>.md with no warning. The unit test at test/unit/builtin-workflows.test.ts:1353-1386 actually demonstrates this data-loss path ("pre-existing spec" → planner-2's output). At a minimum, a brief note in the input description would help; ideally Ralph could detect a pre-existing file whose content doesn't match a prior Ralph spec and either warn or fall back to a suffixed path.

Design / minor concerns

5. pathExists follows symlinks — packages/workflows/builtin/ralph.ts:462-470

stat() (not lstat()) means a dangling symlink at requestedWorktreeDir is treated as non-existent, then git worktree add fails with a confusing error. Cheap fix is to use lstat, or document that symlinks aren't supported as the worktree path.

6. Discovery loses an early-validation signal — packages/workflows/src/extension/discovery.ts:152-181

Removing the source-level regex check is reasonable (the PR description spells out the side-effect concern, and assertWorkflowCreatedStage is a strict runtime backstop). But the failure mode shifts from "workflow rejected at discovery with code INVALID_DEFINITION" to "workflow runs, completes normally with {}, then fails at the post-body assertion". For user-authored workflows this lengthens the debugging loop. Consider surfacing this as its own diagnostic kind/error code (EMPTY_WORKFLOW_GRAPH instead of the generic failure path) so the UX is at least as helpful as the regex check was.

7. ctx.cwd is optional on the public interface, but the executor always populates it — packages/workflows/src/shared/types.ts:508-509, executor.ts:1831

The executor sets cwd: opts.cwd ?? process.cwd() unconditionally, so workflows can rely on it being defined in practice. Marking it readonly cwd?: string forces every consumer to handle the undefined case — the deep-research and open-claude-design builtins both default it back to process.cwd(), which means the same fallback is encoded in three places. Worth either making cwd: string on WorkflowRunContext (eliminating the redundant defaults) or documenting why it must remain optional.

8. gitProcessEnv env-scrub list is a static frozen tuple — ralph.ts:283-309

The list of 15 vars matches the Git docs nicely, but if Git ever adds another local-scoping env (or someone uses GIT_AUTH_HEADER-style vars carelessly), this drifts silently. A short comment naming the source ("see git help environment, scope: local repo/worktree/index") would help future maintenance. Not blocking.

Testing

Test coverage is genuinely good — the new test/integration/ralph-worktree.test.ts exercises the important fail-fast paths, retries, reuse, and the multi-iteration cwd propagation. Notable gaps:

• No test for the subdir-of-repo classification bug above (issue add agent instructions #1). Add a case where git_worktree_dir is an existing non-worktree subdirectory of the invoking repo and assert it is rejected or treated distinctly.
• No test for the concurrent-run sharing semantics the PR description calls out as intentional. Worth at least one assertion that two overlapping runRalphWorkflowFromCwd calls against the same git_worktree_dir don't deadlock and don't add their own lock — to lock in the documented Claude Code-style behavior against regressions.
• The unit test that overwrites a pre-existing user spec (builtin-workflows.test.ts:1353) currently rewards the data-loss behavior. If you adopt a warn-or-suffix policy for issue Flora131/feat/add skills #4, this test should be updated to match.

Style / conventions

• assertWorkflowCreatedStage is fine, but the message "Workflow run completed without creating any workflow stages…" could mention ctx.stage/ctx.task by their exact module identity once instead of enumerating four primitives in a single sentence. Minor.
• Per CLAUDE.md, this repo strongly prefers no comments that restate the obvious. A handful of the new comments in ralph.ts (e.g. // Match Claude Code's named-worktree model… is good; some of the per-step explanations duplicate the function name). Not blocking.

Overall: lots of careful work, especially around env scrubbing, the unclassified/foreign/same-repo state machine, and threading cwd through the executor. Fixing issues #1 and #2 before merging would be my main ask; the rest are nice-to-haves.

[claude]

Review: Optional Ralph worktree + workflow cwd plumbing

Solid follow-up on the earlier review pass. The same-repository worktree classification, stable spec file across iterations, and fail-fast guards directly address the cleanup/silent-fallback concerns from the prior round. Arg-array spawnSync plus normalizeBranchInput keep base_branch / git_worktree_dir injection-safe, and scrubbing the 15 GIT_* env vars is the right call for hook contexts where Ralph would otherwise inherit a nested-Git scope. Below are the things still worth thinking about.

Bugs / correctness

1. Stale worktree state on resume — no git fetch / branch sync. createGitWorktreeIfRequested returns an existing same-repository worktree "as-is" (packages/workflows/builtin/ralph.ts:494). When a user re-runs Ralph against the same git_worktree_dir, the reviewer is told to diff against comparisonBaseBranch (default origin/main), but that ref in the worktree's view may now be days behind the actual remote. The reviewer can then mark a stale patch "correct" against an out-of-date baseline. The shared-as-is model is reasonable, but it would be worth either (a) running git fetch on reuse, (b) noting this in the input description, or (c) refusing reuse when the worktree HEAD is far from the resolved base_branch.

2. Worktree creation has a TOCTOU window. createGitWorktreeIfRequested checks pathExists then calls addGitWorktree. Two concurrent Ralph invocations against the same brand-new git_worktree_dir will both pass the existence check and only one git worktree add will succeed; the loser surfaces a low-level git error. The error message does guide users to "rerun Ralph to resume it," which is graceful, but consider re-running existingPathGitWorktreeStatus on add failure so the message is consistent with the same-repo reuse case rather than handing off raw git stderr.

3. PR stage runs from inside the worktree but the spec lives outside it. workflowSpecPath = resolve(workflowStartCwd, defaultSpecPath(prompt)) (packages/workflows/builtin/ralph.ts:665) is intentional, but the pull-request stage runs with cwdOption (the worktree) and inspects git diff from there. Files written to workflowStartCwd/specs/... will not appear in that diff, so the PR will not include the planner RFC. The prompt mentions "Planner spec path: ..." implicitly, but it would be worth either telling the PR stage to copy/stage the spec into the worktree, or stating in the input description that the RFC is a host-side artifact deliberately excluded from the PR.

4. assertWorkflowCreatedStage only fires on the success path. executor.ts:2488 asserts after def.run(ctx) resolves. If def.run throws (e.g., Ralph's createGitWorktreeIfRequested fail-fast paths), the run is finalized as failed with the thrown error — which is fine — but a workflow that throws before ever calling ctx.stage/ctx.task skips the empty-graph diagnostic entirely. Probably intentional (a thrown error is already a clear signal), but worth confirming the diagnostic isn't expected to fire in that path.

5. writeSpecFile overwrites unconditionally. The new behavior is "revise one stable spec file across planner iterations" (good change). But the path is resolve(workflowStartCwd, defaultSpecPath(prompt)) which is date+slug based — two distinct Ralph runs against the same prompt on the same day will silently overwrite each other's spec file if they share workflowStartCwd. The shared-worktree model is consciously lock-free, so this matches the design, but I would call it out in the git_worktree_dir description: "spec files are also shared across runs on the same day with the same prompt; for isolated specs use a distinct invocation directory."

Code quality / maintainability

6. workflowCwdOption adds noise without clarity. The helper at ralph.ts:522-524 produces { cwd?: string } from a possibly-undefined input, but the spread pattern ...cwdOption appears at ~12 call sites — direct cwd: workflowCwd would be just as legal in WorkflowTaskOptions (it accepts string | undefined). Worth simplifying unless the workflow runner specifically rejects an explicit cwd: undefined.

7. existingPathGitWorktreeStatus makes 3 git subprocess calls + 2 realpathSync.native per check. Fine for a one-shot startup check, but if you ever broaden worktree validation to per-iteration, you'll want to cache the repo-side common-dir lookup. Not blocking.

8. quoteRecoveryCommandArgument Windows quoting targets cmd.exe. replace(/"/g, "\"\"") is the cmd.exe convention; PowerShell users would need a different escape. The recovery hint is best-effort, so this is a nit, but it could be worth saying "(run in cmd.exe / a POSIX shell)" inline.

9. Spread-cast at the entrypoint. runRalphWorkflowFromCwd(ctx as WorkflowRunContext<RalphInputs>, ctx.cwd) (ralph.ts:1462) erases the inputs typing at the boundary. With RalphInputs as Partial<...> you could narrow via runtime checks instead of a cast — minor, but as here masks future field-name drift.

Security / safety

10. gitProcessEnv() clones process.env for every runGit(). This is correct (avoids mutating the shared env), but if any hook writes to process.env between calls the snapshot semantics change. Consider caching the scrubbed env once per runRalphWorkflow invocation.

11. Worker stages run inside the worktree but keep host credentials. Documented as intentional ("worker stages keep the host/CI credential configuration they inherit"), and that's the right call — but the pull-request stage operates from inside the detached worktree against host credentials, and the prompt explicitly tells the agent to run gh auth status --show-token-scopes. Anything caller-uncontrolled in a resumed worktree could now reach those scopes via the model. Not a regression from this PR, but worth re-reading once in the new "worktree may contain caller-uncontrolled state on resume" context.

Test coverage

12. Strong integration coverage with real git. 12 fixture-based tests covering relative/absolute paths, outside-repo, foreign repo checkout, non-empty non-git dir, null-byte input, reuse, missing base branch, multi-iteration cwd propagation, and failure preservation — a noticeable improvement over the Function.prototype.toString() regex tests they replaced.

13. No test for concurrent same-path runs. The TOCTOU window above isn't exercised. A simple test launching two runRalphWorkflowFromCwd calls in parallel against the same not-yet-existing git_worktree_dir would document the expected behavior (one wins, the other surfaces the recovery hint, neither corrupts state).

14. WORKER_PREFLIGHT_CONTRACT placement isn't asserted. shared-prompts.ts is now load-bearing for both Goal and Ralph orchestrator prompts. A small unit test verifying both workflows actually include the contract block would protect against future prompt refactors silently dropping it.

Nits

• The CHANGELOG entry reads "regex-based workflow discovery stage validation"; the old check was Function.prototype.toString() regex inspection of the run body, not a workflow-stage discovery regex. Tightening the wording would reduce reader confusion.
• addGitWorktree returns mkdir failures with status: 1 and a mkdir:-prefixed stderr, which gitFailureMessage then surfaces as if it were a git error. Minor cosmetic; either label the wrapper explicitly or fold the prefix into the outer error message.
• reviewerErrorResult.name === "reviewer-error" (ralph.ts:594-597) is synthetic and never collides with real stage names, but using a name pattern outside the reviewer-a / reviewer-b namespace (e.g., reviewer-error-\${iteration}) would be grep-friendlier in logs.

Overall the worktree plumbing reads cleanly, the safety guards are well-thought-out, and the test bed is genuinely good. The biggest items I'd consider before merging are #1 (stale baseline on resume) and #3 (spec outside worktree → not in PR diff), since both affect end-user output.

[claude]

PR Review: feat(workflows): run Ralph stages from optional git worktree

Overall a well-scoped change with strong test coverage (12 new integration tests with a real Git fixture, plus expanded unit coverage for the SDK seams). The fail-fast guards (null-byte rejection, foreign-repo detection, missing base branch) are appropriately defensive, and the move to shape-only discovery with runtime empty-graph validation is a nice tightening of the contract. A few notes below.

Correctness

• PR description mentions Git env sanitization that scrubs ~15 Git-local env vars; the implementation does not. runGit (packages/workflows/src/runs/shared/worktree.ts:103-122) only adds GIT_OPTIONAL_LOCKS: \"0\" to process.env. If a parent process exports GIT_DIR, GIT_WORK_TREE, GIT_INDEX_FILE, GIT_OBJECT_DIRECTORY, GIT_NAMESPACE, etc. (e.g. running inside a pre-commit hook, a wrapper script, or a CI runner that pre-configures Git), the internal bookkeeping commands like git rev-parse --show-toplevel, git rev-parse --git-common-dir, and especially git worktree add --detach will inherit them and target the wrong repository. Either the description is overpromising, or there's a missing scrub step before each spawnSync. If the intent is to scrub, a quick allowlist filter on env (drop keys starting with GIT_, preserving GIT_TERMINAL_PROMPT=0 and GIT_OPTIONAL_LOCKS=0) inside runGit would close this gap.

• runGit uses a fixed timeout: 5000 for every git invocation including worktree add (worktree.ts:114). Read-only rev-parse commands are fine, but git worktree add --detach <path> <ref> checks out the full tree and on a large repo or slow CI disk can absolutely exceed 5 s. A failure here would surface as git exited with status null from gitFailureMessage rather than a clear timeout message. Consider splitting into a fast budget for read-only inspection and a longer one (or unbounded) for worktree add.

• base_branch defaults to \"origin/main\" in Ralph (ralph.ts:1167-1171), but setupGitWorktree only falls back to \"HEAD\" when baseBranch is empty (worktree.ts:300). On a local-only repo with no origin remote (which is more common than you'd think for one-off ralph invocations on scratch repos), creation will fail with Failed to create git worktree at requested gitWorktreeDir … from origin/main. The integration test (test/integration/ralph-worktree.test.ts:308) sidesteps this by passing base_branch: \"main\". Either weaken the default to \"HEAD\" when origin/main cannot be resolved, or call this out in the input description so users know to override.

Performance

• stageOptionsWithGitWorktree is called once per stage AND once per task (executor.ts:1897 and 2443). Each call runs setupGitWorktree, which invokes git rev-parse --show-toplevel plus, for an existing worktree, git rev-parse --show-toplevel and git rev-parse --git-common-dir twice each (once for the invoking repo, once for the worktree), plus 4× realpathSync.native. Ralph fans out ~5 stages × up to 10 iterations = ~50 stage creations, so a single Ralph run can incur 150-300 extra git invocations and ~200 syscalls just for re-validating the same worktree. Worth memoizing the resolved worktree per workflow run (e.g. cache keyed by (workflowCwd, gitWorktreeDir, baseBranch) on the run context), especially since the inputs are immutable for the duration of the run.

Minor / nits

• normalizeBranchInput silently swallows invalid input (ralph.ts:236-247). If a user typos base_branch: \"--main\" or \"main..\", the regex rejects it and falls back to \"origin/main\". The reviewer prompt then quietly compares against the wrong baseline. Throwing (or at minimum logging a warning into the workflow result) would help users debug.

• defaultSpecPath derives the filename from date + slug(prompt) (ralph.ts:259-262). Two Ralph runs on the same day with the same prompt overwrite each other's RFC under specs/. The PR description correctly flags "stable spec files" as intentional within a run — but cross-run collision deserves a note in the workflow doc, or a per-run suffix when existsSync(path) && !sameWorkflowRun.

• createImplementationNotesFile (ralph.ts:272-289) writes to a fresh mkdtemp directory under the OS temp dir on every run and never cleans up. Over time this leaves behind one atomic-ralph-notes-* directory per Ralph invocation. Low-impact since the notes file is small, but worth a try { rm(notesDir, { recursive: true }) } catch in a finally once the PR-stage report has captured the contents — or at least documenting that this is intentional for retry recovery.

• open-claude-design.ts and deep-research-codebase.ts now read ctx.cwd but ExtensionRuntime.runOptions (packages/workflows/src/extension/runtime.ts:139-164) doesn't forward an explicit cwd to the executor — only workflow-runner.ts does. Through the dispatcher path, ctx.cwd falls back to process.cwd(). That's likely correct for in-process pi invocations, but the inconsistency between the two entry points is worth a comment or, ideally, threading cwd through runtime.ts for symmetry.

Tests

The new integration tests (test/integration/ralph-worktree.test.ts) are well-structured — real Git fixtures, both relative and absolute path coverage, fail-fast paths for every guard, retry semantics, multi-iteration cwd propagation. The unit-test mock context (makeMockCtx) thoughtfully replicates the executor's worktree-resolution behavior so the SDK contract is exercised independent of the foreground runner. One small gap: no test for the env-leakage scenario above (e.g. setting GIT_DIR before invoking Ralph and asserting it doesn't redirect the internal git commands). Worth adding if the env scrub is implemented.

Security

Path handling is solid: null-byte rejection, foreign-repo detection via git-common-dir comparison, traversal rejection in resolveGitWorktreePath, and no shell interpolation (all spawns use argv arrays). --detach avoids branch-name collisions. No issues spotted here.

---

LGTM in spirit. The env-sanitization gap is the only one I'd consider blocking unless the PR description is updated to drop that claim; the others are polish or follow-up.

[claude]

Code Review: feat(workflows): run Ralph stages from optional git worktree

Reviewed the implementation, executor wiring, worktree primitive, builder API, and the new test surface. Overall this is well-scoped, defensively coded, and very well tested — the integration suite exercises a broad set of failure modes against real Git fixtures, and the input-binding builder API is a clean way to thread reusable worktree defaults without polluting every stage call. The PR description matches the implementation, breaking-change story holds (default \"\" preserves current behavior), and the pull-request stage prompt correctly warns about detached HEAD handoff and "no automatic cleanup".

A few items below worth addressing before/after merge.

Bugs / correctness

1. setupGitWorktree re-runs on every nested stage dispatch — measurable cost for Ralph.
   In packages/workflows/src/runs/foreground/executor.ts:

   • ctx.task (line 2443) resolves the worktree once, producing options with cwd: <worktreeCwd> and gitWorktreeDir: undefined, then strips gitWorktreeDir via taskStageOptions.
   • It then calls ctx.stage (line 1897), which re-runs stageOptionsWithInputDefaults(...). Because withoutUndefinedProperties in stageOptionsWithInputDefaults drops the stripped gitWorktreeDir: undefined, the default from inputRuntimeDefaults is re-applied, and stageOptionsWithGitWorktree runs setupGitWorktree again.
   • Each setupGitWorktree call issues ~4 git subprocesses (rev-parse --show-toplevel ×2, rev-parse --git-common-dir ×2) plus realpath syscalls, all synchronously via spawnSync.
   • For Ralph (≈8 stages × up to 10 iterations × 2 resolutions = ~160 redundant setupGitWorktree calls and 600+ git subprocesses just for path validation), this blocks the event loop and adds real wall-clock latency on slow filesystems / large repos.

   Suggested fix: either cache the resolved { worktreeRoot, cwd, repositoryRoot } per (workflowCwd, gitWorktreeDir, baseBranch) for the lifetime of the run, or set a sentinel on the resolved options so the second pass short-circuits.

2. runGit hardcoded 5000 ms timeout. packages/workflows/src/runs/shared/worktree.ts:114 sets timeout: 5000 for every Git invocation. On cold filesystem caches, busy CI runners, or large mono-repos, git worktree add (which does a checkout) can plausibly exceed 5s. When it does, the error surfaces as a generic ETIMEDOUT instead of a Git failure message. Consider increasing this for the worktree-add path specifically, or making it configurable.

3. workspaceCwdForGitWorktreeRoot never verifies the cwd exists inside the new worktree. If the user invokes Ralph from repo/packages/api and creates a new worktree from a base_branch that does not contain packages/api (e.g., the directory was added on a different branch), the executor returns cwd: <worktreeRoot>/packages/api and the first stage's chdir/git call will fail with a confusing ENOENT. A quick existsSync check (or mkdir -p if you want to permit it) with a clear error would be friendlier.

4. Spec files now silently overwrite across concurrent runs. writeSpecFile (ralph.ts:264-270) no longer falls back to suffixed paths on EEXIST. This is intentional for the new "stable spec across iterations" model — but two Ralph runs with the same prompt on the same date now stomp each other without warning. Worth a one-line note in the workflow docs, or fall back to a suffix only when the existing file's content was not produced by the current run.

Code quality / API

5. applyBatch in discovery.ts is async but has no await. Lines 213-254: the function is purely synchronous (no I/O after the imports complete), and applyBatchShapeOnly at line 257 is the identical body but synchronous. The async wrapper only adds a microtask per call site and obscures the equivalence. Either consolidate to a single sync helper or leave a comment explaining why two near-duplicates exist.

6. stageOptionsWithGitWorktree writes gitWorktreeDir: undefined, baseBranch: undefined rather than deleting the keys (executor.ts:765). This means downstream code that uses \"gitWorktreeDir\" in options will see true. Right now taskStageOptions and the ctx.stage/ctx.task paths handle this fine, but it's a footgun for future consumers.

7. Cast in Ralph's .run handler. ralph.ts:1180: const workflowCtx = ctx as WorkflowRunContext<RalphInputs>; works because RalphInputs widens Record<string, unknown>, but the cast is silent — if a future input is removed, the cast hides the type error. Consider parameterising defineWorkflow (defineWorkflow<RalphInputs>(\"ralph\")) so the input schema and the run signature stay in sync.

Tests

8. Integration tests re-implement the executor's worktree wiring. test/integration/ralph-worktree.test.ts mocks ctx.task/ctx.parallel and calls setupGitWorktree directly from withRalphWorktreeDefaults. This validates setupGitWorktree and Ralph's prompt layer, but it does not exercise the production stageOptionsWithGitWorktree → ctx.task → ctx.stage plumbing in executor.ts. Worth one end-to-end test that runs the real executor (with a stub agent session) so the input-binding → executor → setup chain has direct coverage. This would also catch the double-resolution issue in add agent instructions #1.

9. No test asserts spec path stability when reusing the same worktree across runs. The "can re-run with the same git_worktree_dir without cleanup" integration test confirms the worktree is reused, and the unit test "revises the original Ralph spec file across planner iterations" confirms in-run stability, but neither asserts that two separate Ralph runs with the same prompt write to the same spec path (relevant to bug Flora131/feat/add skills #4).

Nits

• ralph.ts:1114 — "Ralph-created worktrees are detached HEAD checkouts." Strictly true only when Ralph created the worktree; reused worktrees may already be on a branch. Consider "may be detached HEAD checkouts" or split the guidance.
• WORKER_PREFLIGHT_CONTRACT is now used by both Goal and Ralph — nice consolidation. The Goal workflow wraps it in <project_initialization_preflight> tags, the Ralph orchestrator uses [\"project_initialization_preflight\", WORKER_PREFLIGHT_CONTRACT] in taggedPrompt. Equivalent rendering, just worth noting for future readers.
• CHANGELOG entry is appropriately scoped under [Unreleased].

Nice work overall — the worktree primitive itself is the highlight, with good symlink handling, clear error messaging, and careful Git env scrubbing. The performance issue in #1 is the only finding I'd block on for production use of Ralph with reusable worktrees.