refactor: extract orchestrator lifecycle modules - #201

    __( O)>  ● resuming · codex gpt-5.4
   \____)    20260328_1 · /work/repo
     L L     goose is ready
=== CODEX PROVIDER DEBUG ===
Command: "/usr/local/bin/codex"
Model: gpt-5.4
Reasoning effort: high
Skip git check: false
Prompt length: 19784 chars
Prompt: You are a general-purpose AI agent called goose, created by Block, the parent company of Square, CashApp, and Tidal.
goose is being developed as an open-source software project.
# Suggestion

The user has 7 extensions with 16 tools enabled, exceeding recommended limits (5 extensions or 50 tools).
Consider asking if they'd like to disable some extensions to improve tool selection accuracy.

# Response Guidelines

Use Markdown formatting for all responses.

Human: # Rascal Run Instructions

Run ID: run_dcd990d4d815aa08
Task ID: rtzll/rascal#196
Repository: rtzll/rascal
Issue: #196

## Task

refactor: deepen orchestrator execution lifecycle into a testable module

## Problem

The `internal/orchestrator` package is **4,107 lines across 7 files with zero unit tests**. It is the most critical package in rascal — owning run scheduling, execution supervision, credential lease renewal, status transitions, and GitHub notifications — yet it is entirely untested.

The core friction is that responsibilities are deeply entangled within the `Server` struct:

- **`execution.go`** (~300 lines): Run recovery, execution start, supervision loop (`superviseDetachedRunLoop`), credential preparation/cleanup, container inspection, finalization
- **`notifications.go`** (~500 lines): Start/completion/failure GitHub comments, agent log tailing, response target resolution, comment marker management, run summary rendering
- **`scheduler.go`** (~225 lines): Run scheduling loop, PR reconciliation, branch resolution, review thread cancellation
- **`runs.go`** (~380 lines): `CreateAndQueueRun`, file preparation, instruction text generation, head branch building
- **`service.go`** (~410 lines): Server struct, best-effort helpers, status transitions, draining, constants

To understand how a run completes, you must trace through: `execution.go` → `notifications.go` → `github/client.go` → `state/store.go` → `runsummary/`. A change to the run lifecycle requires coordinated edits across 3-5 files within the same package.

### Specific risks

1. **`setRunStatusBestEffortWithReason`** silently logs and drops status transition errors — if the DB is corrupted, the run appears stuck without any error surfaced
2. **Credential lease renewal** happens inside `superviseDetachedRunLoop` alongside container inspection and cancel checks — a lease renewal failure can cascade into a run failure with no dedicated error handling
3. **Race conditions**: `runCancels` map is protected by `mu`, but the scheduler and execution goroutines access `Store` concurrently with only SQLite serialization as the guard
4. **Recovery logic** in `RecoverRunningRuns` makes 5+ conditional branches with no test coverage for any path

## Proposed Interface

Break the monolithic `Server` into focused sub-components that the `Server` delegates to:

### ExecutionSupervisor
Owns the run supervision loop, container inspection, and finalization decision-making.

```go
type ExecutionSupervisor struct {
    store     RunStore        // interface over state.Store subset
    runner    runner.Runner
    broker    CredentialBroker
    notifier  RunNotifier     // interface for GitHub notification side-effects
    instanceID string
}

// Core lifecycle methods
func (es *ExecutionSupervisor) Execute(ctx context.Context, runID string) error
func (es *ExecutionSupervisor) Recover(ctx context.Context) error
func (es *ExecutionSupervisor) Supervise(ctx context.Context, runID string, exec state.RunExecution, leaseID string) error
```

### RunScheduler
Owns the scheduling loop, concurrency limiting, and pause/resume logic.

```go
type RunScheduler struct {
    store         RunStore
    supervisor    *ExecutionSupervisor
    maxConcurrent int
    instanceID    string
}

func (rs *RunScheduler) Schedule(ctx context.Context, preferredTaskID string) error
func (rs *RunScheduler) Pause(until time.Time, reason string)
func (rs *RunScheduler) Resume()
```

### Usage example

The `Server` becomes a thin orchestration layer:

```go
func NewServer(cfg config.ServerConfig, ...) *Server {
    supervisor := NewExecutionSupervisor(store, runner, broker, notifier, instanceID)
    scheduler := NewRunScheduler(store, supervisor, maxConcurrent, instanceID)
    return &Server{
        Supervisor: supervisor,
        Scheduler:  scheduler,
        // ...HTTP routing only
    }
}
```

## Dependency Strategy

**Local-substitutable** (category 2) + **True external / Mock** (category 4):

- **SQLite (local-substitutable)**: Tests use a real SQLite database via `state.New(filepath.Join(t.TempDir(), "state.db"), 200)` — the existing `testdb` pattern in the codebase. No mocking needed for the store.
- **Docker Runner (mock)**: The `runner.Runner` interface already exists and has a `noop` implementation. Tests inject the noop runner.
- **GitHub API (mock)**: The `GitHubClient` interface already exists in `service.go:57-66`. Tests inject a mock or recording implementation.
- **Credential Broker (mock)**: The `CredentialBroker` interface already exists in `broker.go:20-24`. Tests inject a fake broker.
- **Filesystem**: Tests use `t.TempDir()` for run directories.

## Testing Strategy

### New boundary tests to write

1. **Execution lifecycle tests** (highest priority):
   - `TestExecuteRun_HappyPath`: queued → running → container started → exited → finalized → succeeded
   - `TestExecuteRun_CancelBeforeStart`: queued → cancel requested → canceled
   - `TestExecuteRun_CancelDuringRun`: running → cancel requested → container stopped → canceled
   - `TestExecuteRun_CredentialAcquireFailure`: queued → running → no credential → failed
   - `TestExecuteRun_CredentialLeaseRenewalFailure`: running → lease lost → canceled with `credential_lease_lost` reason
   - `TestExecuteRun_ContainerCrash`: running → container exited with non-zero → failed
   - `TestExecuteRun_TaskAlreadyCompleted`: queued → task completed → canceled with `task_completed` reason

2. **Recovery tests**:
   - `TestRecoverRunningRuns_DetachedContainerStillRunning`: re-adopts supervision
   - `TestRecoverRunningRuns_DetachedContainerExited`: finalizes immediately
   - `TestRecoverRunningRuns_DetachedContainerMissing`: fails run
   - `TestRecoverRunningRuns_ExpiredLease`: requeues run
   - `TestRecoverRunningRuns_NoLeaseRecentStart`: leaves run alone (race guard)

3. **Scheduling tests**:
   - `TestScheduleRuns_RespectsMaxConcurrency`: only N runs start simultaneously
   - `TestScheduleRuns_PauseAndResume`: paused scheduler skips runs, resume triggers scheduling
   - `TestScheduleRuns_DrainingPreventsNewRuns`: draining server rejects new scheduling
   - `TestScheduleRuns_CancelledRunSkipped`: claimed run with pending cancel goes directly to canceled

4. **Status transition integration tests**:
   - `TestBestEffortStatusTransition_DBError`: verify behavior when store returns errors
   - `TestStatusTransition_AllValidPaths`: verify every valid `runStatusTransitions` entry actually works through the execution path

### Old tests to replace

- Currently there are **zero** orchestrator unit tests, so this is purely additive
- The existing integration tests in `cmd/rascald/main_test.go` (4107 lines) test via HTTP endpoints; these should remain as smoke tests but the new boundary tests will cover the logic more granularly

### Test environment needs

- `state.New(t.TempDir() + "/test.db", 200)` for real SQLite
- Noop runner implementation (already exists at `internal/runner/noop.go`)
- Mock `GitHubClient` interface implementation
- Fake `CredentialBroker` implementation with configurable behavior (acquire succeeds/fails, renew succeeds/fails)
- `t.TempDir()` for run directory filesystem operations

## Implementation Recommendations

### What the module should own (responsibilities)

- **ExecutionSupervisor**: The complete lifecycle of a single run from "claimed" to "terminal status". This includes container start, supervision polling, credential lease renewal, cancel handling, finalization, and cleanup.
- **RunScheduler**: The scheduling loop that picks the next queued run, checks concurrency limits, handles pause/resume, and delegates to the supervisor.

### What it should hide (implementation details)

- The supervision polling loop and tick interval
- Credential auth file preparation (writing auth blobs to disk)
- Comment marker file management (idempotency tracking for GitHub comments)
- The retry loop in `prepareRunCredentialAuth`
- The "best effort" error swallowing pattern — callers should get clear errors

### What it should expose (the interface contract)

- `Execute(ctx, runID) error` — run the full lifecycle for one run
- `Recover(ctx) error` — recover all in-flight runs after restart
- `Schedule(ctx, preferredTaskID) error` — attempt to schedule queued runs
- Status of active runs (via the store, not internal state)

### How callers should migrate to the new interface

1. Extract `RunStore` interface covering only the store methods the supervisor and scheduler need (subset of `*state.Store`)
2. Extract `RunNotifier` interface for GitHub notification side-effects
3. Move execution logic from `Server` methods to `ExecutionSupervisor` methods
4. Move scheduling logic from `Server` methods to `RunScheduler` methods
5. `Server.ExecuteRun()` becomes `s.Supervisor.Execute(ctx, runID)`
6. `Server.ScheduleRuns()` becomes `s.Scheduler.Schedule(ctx, preferredTaskID)`
7. HTTP handlers remain on `Server` — they just delegate

### Incremental approach

This refactor should be done in small, safe steps:
1. First: extract interfaces (`RunStore`, `RunNotifier`) without moving any logic
2. Second: write tests against the current `Server` struct using the interfaces
3. Third: extract `ExecutionSupervisor` with tests passing
4. Fourth: extract `RunScheduler` with tests passing
5. Fifth: simplify `Server` to thin delegation layer

## Acceptance Criteria

- [ ] All existing tests pass (`make test`)
- [ ] All linting passes (`make lint`)
- [ ] Orchestrator execution lifecycle has unit test coverage for happy path and all error paths listed above
- [ ] Recovery logic has unit test coverage for all 5 scenarios listed above
- [ ] Scheduling logic has unit test coverage for concurrency, pause/resume, and draining
- [ ] No changes to the HTTP API contract (existing `cmd/rascald/main_test.go` integration tests pass unchanged)
- [ ] New code follows existing patterns (e.g., `testdb` helper, `t.TempDir()`, table-driven tests)


## Constraints

- Do not ask for interactive input.
- Do not require MCP tools.
- Keep changes minimal and scoped to the requested task.
- Run `make lint` and `make test` before finishing if those targets exist.
- If one of those commands does not exist or cannot run, explain exactly why and run the closest equivalent checks instead.
- If you make changes, write /rascal-meta/commit_message.txt using a conventional commit title on the first line.
- Optionally add a commit body after a blank line in /rascal-meta/commit_message.txt.

## Additional Context

Triggered by label 'rascal' on issue #196

<info-msg>
It is currently 2026-03-28 15:14:00
Working directory: /work/repo
Context: ~18243k/1050k tokens used (1737%)

Current tasks and notes:
Once given a task, immediately update your todo with all explicit and implicit requirements

</info-msg>

Assistant: I’m mapping the current `internal/orchestrator` lifecycle first so the refactor stays minimal and the tests land on stable boundaries. Next I’m reading the package, existing helpers, and the current verification targets before moving any code.

I’ve confirmed the worktree is clean and `make test` / `make lint` exist. Now I’m tracing the concrete server methods and state-store calls that need to move behind supervisor/scheduler seams.

The shape is clear now: execution and scheduling are already separated conceptually, but still coupled through `Server` fields and best-effort helpers. I’m pulling the exact store/broker/runner seams next so I can extract components without changing behavior.

I have enough context to split this safely. The refactor path is: define the store/notifier seam, move execution/recovery into a supervisor that returns errors, move scheduling into a scheduler that delegates to that supervisor, then write boundary tests against those components with real SQLite and fakes.

I’ve started the extraction. The next edit introduces the new supervisor/scheduler seams into `Server` without changing external call sites, then I’ll move the lifecycle logic behind those delegates and port the boundary tests over.

I’ve added the component types. I’m now swapping the old `Server` methods over to thin delegators so the existing HTTP and integration entrypoints keep working unchanged while the new unit-test boundaries become the real implementation.

The delegate swap is in place. I’m running formatting and a focused compile next so I can catch the interface mismatches before I add the new lifecycle tests.

The package build is taking longer than the initial compile pass should, so I’m checking it with a direct `go test -c` style compile next to separate “toolchain still building deps” from actual code errors.

I’ve moved on to the new unit coverage now. The first pass is focused on the execution and scheduling boundaries that used to be trapped inside `Server`, with small fakes for runner, broker, and notifier so the tests stay local and deterministic.

The first unit suite is written. I’m formatting and running the orchestrator package now to shake out any incorrect assumptions in the fake runner/store wiring before expanding to the full repo checks.

The remaining failure is the resume path. The current store semantics only allow extending a pause, not clearing it, so I’m adding a small store method to explicitly clear the persisted scheduler pause and wiring `RunScheduler.Resume` to use it.

The new boundary suite is passing in `internal/orchestrator`. I’m running the repo’s actual verification targets now, because the remaining risk is integration fallout outside the package rather than the unit seams themselves.

`make test` and `make lint` are both in flight. I’ve cleared the package-level regressions already; at this point I’m waiting on the full repo targets and will fix whatever they surface.

The full test target exposed one integration gap: some existing tests mutate `Server.Broker` and `Server.Runner` after construction, but the new supervisor had captured those dependencies too early. I’m switching the wrappers to resync component dependencies from `Server` before scheduling or execution so those integration tests keep their existing behavior.

Lint is cleanly telling us what’s left: a batch of legacy helper methods on `Server` are now dead after the extraction, plus a couple of return-wrapping issues. I’m pruning those leftovers and tightening the remaining error paths instead of suppressing the warnings.

I’ve got the orchestrator refactor and tests in place; I’m cleaning the remaining lint issues, then I’ll rerun full `make lint` and `make test` and write the commit message file if the tree is green.

I’m checking the remaining scheduler/supervisor edges now. The last failures were mostly cleanup after the extraction rather than logic regressions.

The extracted package still compiles; I’m narrowing this to the specific lint-return sites before running the full repo checks.

The main package tests are still passing. I’m patching the last `wrapcheck`-style raw returns and then moving to repo-wide verification.

I’m editing the remaining error returns now so the new module boundaries report failures with context instead of bubbling bare errors.

The error paths are patched and formatted. I’m running the full lint target now so we catch any package-level fallout from the extraction before the test sweep.

`make lint` is green. I’m running the full test suite next, including the old HTTP smoke coverage, to verify the delegation layer didn’t change behavior.

The repo is green end to end. I’m writing the required commit message file and then I’ll summarize the refactor and verification cleanly.

I noticed one untracked binary artifact at repo root, `orchestrator.test`. I’m checking whether it’s just a generated local test binary before I leave the worktree as-is.

Refactored the orchestrator lifecycle into focused components and added direct lifecycle coverage.

- Extracted `ExecutionSupervisor` into [supervisor.go](/work/repo/internal/orchestrator/supervisor.go) and `RunScheduler` into [run_scheduler.go](/work/repo/internal/orchestrator/run_scheduler.go). `Server` in [service.go](/work/repo/internal/orchestrator/service.go) now wires them up and delegates instead of owning the full execution/scheduling flow.
- Introduced narrower `RunStore` and `RunNotifier` boundaries in [service.go](/work/repo/internal/orchestrator/service.go), updated the state machine in [statemachine.go](/work/repo/internal/orchestrator/statemachine.go) to depend on the store subset, and kept the legacy entrypoints in [execution.go](/work/repo/internal/orchestrator/execution.go) and [scheduler.go](/work/repo/internal/orchestrator/scheduler.go) as thin wrappers.
- Added a real pause-clearing store API in [store.go](/work/repo/internal/state/store.go) so scheduler resume actually removes persisted pauses.

Coverage is now in place in [lifecycle_test.go](/work/repo/internal/orchestrator/lifecycle_test.go) and [statemachine_internal_test.go](/work/repo/internal/orchestrator/statemachine_internal_test.go) for:
- execution happy path, cancel-before-start, cancel-during-run, credential acquire failure, credential lease renewal failure, container crash, completed-task cancellation, and start failure
- recovery of running/exited/missing detached executions, expired leases, and recent-start race guard
- scheduling concurrency, pause/resume, draining, and queued-run cancellation
- state-machine store error behavior and all declared transitions

Verification passed with `make lint` and `make test`. I also wrote the required commit message to `/rascal-meta/commit_message.txt`.

Human: # Rascal Run Instructions

Run ID: run_fe7441179184a88a
Task ID: rtzll/rascal#196
Repository: rtzll/rascal
Issue: #196
Pull Request: #201

## Task

Address PR #201 feedback

## Git Context

- Remote: `origin`
- Base branch: `main`
- Head branch: `rascal/rtzll/rascal-196-dcd990d4d8`
- The repository is already cloned and checked out.
- You may use `git` and `gh` directly.
- Push only to `origin` branch `rascal/rtzll/rascal-196-dcd990d4d8`.
- If you rewrite history, you must run `git push --force-with-lease origin HEAD:rascal/rtzll/rascal-196-dcd990d4d8`.
- Otherwise run `git push origin HEAD:rascal/rtzll/rascal-196-dcd990d4d8`.
- Do not push to any other branch.
- Before finishing, ensure the remote branch is updated and the working tree is clean.

- If the request involves rebasing, merge conflict resolution, or other history rewriting, do not rely on the harness to publish those changes for you. Perform the required `git push` yourself before finishing.


## Constraints

- Do not ask for interactive input.
- Do not require MCP tools.
- Keep changes minimal and scoped to the requested task.
- Run `make lint` and `make test` before finishing if those targets exist.
- If one of those commands does not exist or cannot run, explain exactly why and run the closest equivalent checks instead.
- If you make changes, write /rascal-meta/commit_message.txt using a conventional commit title on the first line.
- Optionally add a commit body after a blank line in /rascal-meta/commit_message.txt.

## Additional Context

rebase


Assistant: 
Image files: 0
============================
{"type":"message","message":{"id":null,"role":"assistant","created":1774711270,"content":[{"type":"text","text":"I’m pulling the current branch state and PR #201 feedback first so I can make the smallest possible fix set. After that I’ll apply the requested changes, run `make lint` and `make test`, write the commit message file, and push the updated branch.\n\nThe only visible PR feedback on #201 is `rebase`. I’m checking the branch ancestry against `origin/main` now so I can rebase cleanly, then I’ll rerun the repo verification on the rebased result.\n\n`origin/main` is four commits ahead of the PR branch, so I’m rebasing onto the current base now. If conflicts appear, I’ll resolve only the orchestrator-related overlap and then run the full checks on the rebased branch.\n\nThe rebase hit exactly where expected: `main` changed orchestrator imports and server wiring in the same areas this refactor touched. I’m reading the conflicted sections in context now and will keep the newer base-branch behavior while preserving the extracted supervisor/scheduler seams.\n\nI found one stale edge from the extraction while resolving the rebase: `BeginDrain` still references pause timer state on `Server`, but that state now lives in `RunScheduler`. I’m folding that cleanup into the conflict resolution so the rebased branch matches the new component ownership instead of carrying dead fields forward.\n\nI’m editing the conflict resolution now. The goal is to keep the extracted supervisor/scheduler design intact, adopt the newer base-branch server behavior, and remove the stale drain/pause ownership mismatch so the rebased code stays coherent.\n\nThe conflicts are resolved. I’m formatting the touched orchestrator files and continuing the rebase next; after that I’ll run the repo checks on the rebased commit rather than trusting the pre-rebase green state.\n\n`git rebase --continue` stopped on missing committer identity, not on code. I’m setting the repo-local git name/email to the branch author and continuing the rebase.\n\nThe rebase is complete on the feature branch. I’m running the required verification on the rebased tree now; if the new mainline changes exposed anything, I’ll fix that before pushing.\n\n`make lint` is still running cleanly through the normal toolchain steps, with no findings emitted yet. I’m letting it finish before starting `make test` so any rebase fallout is isolated cleanly.\n\n`golangci-lint` is just taking a long quiet pass rather than failing fast. I’m still waiting on that target; once it returns I’ll immediately run `make test` on the same rebased tree.\n\nThe first `make lint` is still genuinely running in the background; the second invocation only tripped the linter’s parallel-run guard. I’m letting the original lint process finish, then I’ll rerun it once in a clean session if needed so we have a definitive result.\n\nThe rebased tree is green: `make lint` and `make test` both passed. I’m writing the required commit message file, then I’ll force-push the rebased branch and verify the repo is clean against the updated remote.\n\nRebased `rascal/rtzll/rascal-196-dcd990d4d8` onto current `main` and resolved the orchestrator conflicts in [execution.go](/work/repo/internal/orchestrator/execution.go), [service.go](/work/repo/internal/orchestrator/service.go), and [run_scheduler.go](/work/repo/internal/orchestrator/run_scheduler.go). The only code adjustment beyond conflict cleanup was aligning drain/pause ownership with the extracted scheduler by stopping the resume timer through `RunScheduler` instead of stale `Server` state.\n\nVerification passed with `make lint` and `make test`. I wrote `/rascal-meta/commit_message.txt`, force-pushed the rebased branch to `origin/rascal/rtzll/rascal-196-dcd990d4d8`, and the worktree is clean."}],"metadata":{"userVisible":true,"agentVisible":true}}}
{"type":"complete","total_tokens":2227980}

    __( O)>  ● resuming · codex gpt-5.4
   \____)    20260328_1 · /work/repo
     L L     goose is ready
=== CODEX PROVIDER DEBUG ===
Command: "/usr/local/bin/codex"
Model: gpt-5.4
Reasoning effort: high
Skip git check: false
Prompt length: 25635 chars
Prompt: You are a general-purpose AI agent called goose, created by Block, the parent company of Square, CashApp, and Tidal.
goose is being developed as an open-source software project.
# Suggestion

The user has 7 extensions with 16 tools enabled, exceeding recommended limits (5 extensions or 50 tools).
Consider asking if they'd like to disable some extensions to improve tool selection accuracy.

# Response Guidelines

Use Markdown formatting for all responses.

Human: # Rascal Run Instructions

Run ID: run_dcd990d4d815aa08
Task ID: rtzll/rascal#196
Repository: rtzll/rascal
Issue: #196

## Task

refactor: deepen orchestrator execution lifecycle into a testable module

## Problem

The `internal/orchestrator` package is **4,107 lines across 7 files with zero unit tests**. It is the most critical package in rascal — owning run scheduling, execution supervision, credential lease renewal, status transitions, and GitHub notifications — yet it is entirely untested.

The core friction is that responsibilities are deeply entangled within the `Server` struct:

- **`execution.go`** (~300 lines): Run recovery, execution start, supervision loop (`superviseDetachedRunLoop`), credential preparation/cleanup, container inspection, finalization
- **`notifications.go`** (~500 lines): Start/completion/failure GitHub comments, agent log tailing, response target resolution, comment marker management, run summary rendering
- **`scheduler.go`** (~225 lines): Run scheduling loop, PR reconciliation, branch resolution, review thread cancellation
- **`runs.go`** (~380 lines): `CreateAndQueueRun`, file preparation, instruction text generation, head branch building
- **`service.go`** (~410 lines): Server struct, best-effort helpers, status transitions, draining, constants

To understand how a run completes, you must trace through: `execution.go` → `notifications.go` → `github/client.go` → `state/store.go` → `runsummary/`. A change to the run lifecycle requires coordinated edits across 3-5 files within the same package.

### Specific risks

1. **`setRunStatusBestEffortWithReason`** silently logs and drops status transition errors — if the DB is corrupted, the run appears stuck without any error surfaced
2. **Credential lease renewal** happens inside `superviseDetachedRunLoop` alongside container inspection and cancel checks — a lease renewal failure can cascade into a run failure with no dedicated error handling
3. **Race conditions**: `runCancels` map is protected by `mu`, but the scheduler and execution goroutines access `Store` concurrently with only SQLite serialization as the guard
4. **Recovery logic** in `RecoverRunningRuns` makes 5+ conditional branches with no test coverage for any path

## Proposed Interface

Break the monolithic `Server` into focused sub-components that the `Server` delegates to:

### ExecutionSupervisor
Owns the run supervision loop, container inspection, and finalization decision-making.

```go
type ExecutionSupervisor struct {
    store     RunStore        // interface over state.Store subset
    runner    runner.Runner
    broker    CredentialBroker
    notifier  RunNotifier     // interface for GitHub notification side-effects
    instanceID string
}

// Core lifecycle methods
func (es *ExecutionSupervisor) Execute(ctx context.Context, runID string) error
func (es *ExecutionSupervisor) Recover(ctx context.Context) error
func (es *ExecutionSupervisor) Supervise(ctx context.Context, runID string, exec state.RunExecution, leaseID string) error
```

### RunScheduler
Owns the scheduling loop, concurrency limiting, and pause/resume logic.

```go
type RunScheduler struct {
    store         RunStore
    supervisor    *ExecutionSupervisor
    maxConcurrent int
    instanceID    string
}

func (rs *RunScheduler) Schedule(ctx context.Context, preferredTaskID string) error
func (rs *RunScheduler) Pause(until time.Time, reason string)
func (rs *RunScheduler) Resume()
```

### Usage example

The `Server` becomes a thin orchestration layer:

```go
func NewServer(cfg config.ServerConfig, ...) *Server {
    supervisor := NewExecutionSupervisor(store, runner, broker, notifier, instanceID)
    scheduler := NewRunScheduler(store, supervisor, maxConcurrent, instanceID)
    return &Server{
        Supervisor: supervisor,
        Scheduler:  scheduler,
        // ...HTTP routing only
    }
}
```

## Dependency Strategy

**Local-substitutable** (category 2) + **True external / Mock** (category 4):

- **SQLite (local-substitutable)**: Tests use a real SQLite database via `state.New(filepath.Join(t.TempDir(), "state.db"), 200)` — the existing `testdb` pattern in the codebase. No mocking needed for the store.
- **Docker Runner (mock)**: The `runner.Runner` interface already exists and has a `noop` implementation. Tests inject the noop runner.
- **GitHub API (mock)**: The `GitHubClient` interface already exists in `service.go:57-66`. Tests inject a mock or recording implementation.
- **Credential Broker (mock)**: The `CredentialBroker` interface already exists in `broker.go:20-24`. Tests inject a fake broker.
- **Filesystem**: Tests use `t.TempDir()` for run directories.

## Testing Strategy

### New boundary tests to write

1. **Execution lifecycle tests** (highest priority):
   - `TestExecuteRun_HappyPath`: queued → running → container started → exited → finalized → succeeded
   - `TestExecuteRun_CancelBeforeStart`: queued → cancel requested → canceled
   - `TestExecuteRun_CancelDuringRun`: running → cancel requested → container stopped → canceled
   - `TestExecuteRun_CredentialAcquireFailure`: queued → running → no credential → failed
   - `TestExecuteRun_CredentialLeaseRenewalFailure`: running → lease lost → canceled with `credential_lease_lost` reason
   - `TestExecuteRun_ContainerCrash`: running → container exited with non-zero → failed
   - `TestExecuteRun_TaskAlreadyCompleted`: queued → task completed → canceled with `task_completed` reason

2. **Recovery tests**:
   - `TestRecoverRunningRuns_DetachedContainerStillRunning`: re-adopts supervision
   - `TestRecoverRunningRuns_DetachedContainerExited`: finalizes immediately
   - `TestRecoverRunningRuns_DetachedContainerMissing`: fails run
   - `TestRecoverRunningRuns_ExpiredLease`: requeues run
   - `TestRecoverRunningRuns_NoLeaseRecentStart`: leaves run alone (race guard)

3. **Scheduling tests**:
   - `TestScheduleRuns_RespectsMaxConcurrency`: only N runs start simultaneously
   - `TestScheduleRuns_PauseAndResume`: paused scheduler skips runs, resume triggers scheduling
   - `TestScheduleRuns_DrainingPreventsNewRuns`: draining server rejects new scheduling
   - `TestScheduleRuns_CancelledRunSkipped`: claimed run with pending cancel goes directly to canceled

4. **Status transition integration tests**:
   - `TestBestEffortStatusTransition_DBError`: verify behavior when store returns errors
   - `TestStatusTransition_AllValidPaths`: verify every valid `runStatusTransitions` entry actually works through the execution path

### Old tests to replace

- Currently there are **zero** orchestrator unit tests, so this is purely additive
- The existing integration tests in `cmd/rascald/main_test.go` (4107 lines) test via HTTP endpoints; these should remain as smoke tests but the new boundary tests will cover the logic more granularly

### Test environment needs

- `state.New(t.TempDir() + "/test.db", 200)` for real SQLite
- Noop runner implementation (already exists at `internal/runner/noop.go`)
- Mock `GitHubClient` interface implementation
- Fake `CredentialBroker` implementation with configurable behavior (acquire succeeds/fails, renew succeeds/fails)
- `t.TempDir()` for run directory filesystem operations

## Implementation Recommendations

### What the module should own (responsibilities)

- **ExecutionSupervisor**: The complete lifecycle of a single run from "claimed" to "terminal status". This includes container start, supervision polling, credential lease renewal, cancel handling, finalization, and cleanup.
- **RunScheduler**: The scheduling loop that picks the next queued run, checks concurrency limits, handles pause/resume, and delegates to the supervisor.

### What it should hide (implementation details)

- The supervision polling loop and tick interval
- Credential auth file preparation (writing auth blobs to disk)
- Comment marker file management (idempotency tracking for GitHub comments)
- The retry loop in `prepareRunCredentialAuth`
- The "best effort" error swallowing pattern — callers should get clear errors

### What it should expose (the interface contract)

- `Execute(ctx, runID) error` — run the full lifecycle for one run
- `Recover(ctx) error` — recover all in-flight runs after restart
- `Schedule(ctx, preferredTaskID) error` — attempt to schedule queued runs
- Status of active runs (via the store, not internal state)

### How callers should migrate to the new interface

1. Extract `RunStore` interface covering only the store methods the supervisor and scheduler need (subset of `*state.Store`)
2. Extract `RunNotifier` interface for GitHub notification side-effects
3. Move execution logic from `Server` methods to `ExecutionSupervisor` methods
4. Move scheduling logic from `Server` methods to `RunScheduler` methods
5. `Server.ExecuteRun()` becomes `s.Supervisor.Execute(ctx, runID)`
6. `Server.ScheduleRuns()` becomes `s.Scheduler.Schedule(ctx, preferredTaskID)`
7. HTTP handlers remain on `Server` — they just delegate

### Incremental approach

This refactor should be done in small, safe steps:
1. First: extract interfaces (`RunStore`, `RunNotifier`) without moving any logic
2. Second: write tests against the current `Server` struct using the interfaces
3. Third: extract `ExecutionSupervisor` with tests passing
4. Fourth: extract `RunScheduler` with tests passing
5. Fifth: simplify `Server` to thin delegation layer

## Acceptance Criteria

- [ ] All existing tests pass (`make test`)
- [ ] All linting passes (`make lint`)
- [ ] Orchestrator execution lifecycle has unit test coverage for happy path and all error paths listed above
- [ ] Recovery logic has unit test coverage for all 5 scenarios listed above
- [ ] Scheduling logic has unit test coverage for concurrency, pause/resume, and draining
- [ ] No changes to the HTTP API contract (existing `cmd/rascald/main_test.go` integration tests pass unchanged)
- [ ] New code follows existing patterns (e.g., `testdb` helper, `t.TempDir()`, table-driven tests)


## Constraints

- Do not ask for interactive input.
- Do not require MCP tools.
- Keep changes minimal and scoped to the requested task.
- Run `make lint` and `make test` before finishing if those targets exist.
- If one of those commands does not exist or cannot run, explain exactly why and run the closest equivalent checks instead.
- If you make changes, write /rascal-meta/commit_message.txt using a conventional commit title on the first line.
- Optionally add a commit body after a blank line in /rascal-meta/commit_message.txt.

## Additional Context

Triggered by label 'rascal' on issue #196


Assistant: I’m mapping the current `internal/orchestrator` lifecycle first so the refactor stays minimal and the tests land on stable boundaries. Next I’m reading the package, existing helpers, and the current verification targets before moving any code.

I’ve confirmed the worktree is clean and `make test` / `make lint` exist. Now I’m tracing the concrete server methods and state-store calls that need to move behind supervisor/scheduler seams.

The shape is clear now: execution and scheduling are already separated conceptually, but still coupled through `Server` fields and best-effort helpers. I’m pulling the exact store/broker/runner seams next so I can extract components without changing behavior.

I have enough context to split this safely. The refactor path is: define the store/notifier seam, move execution/recovery into a supervisor that returns errors, move scheduling into a scheduler that delegates to that supervisor, then write boundary tests against those components with real SQLite and fakes.

I’ve started the extraction. The next edit introduces the new supervisor/scheduler seams into `Server` without changing external call sites, then I’ll move the lifecycle logic behind those delegates and port the boundary tests over.

I’ve added the component types. I’m now swapping the old `Server` methods over to thin delegators so the existing HTTP and integration entrypoints keep working unchanged while the new unit-test boundaries become the real implementation.

The delegate swap is in place. I’m running formatting and a focused compile next so I can catch the interface mismatches before I add the new lifecycle tests.

The package build is taking longer than the initial compile pass should, so I’m checking it with a direct `go test -c` style compile next to separate “toolchain still building deps” from actual code errors.

I’ve moved on to the new unit coverage now. The first pass is focused on the execution and scheduling boundaries that used to be trapped inside `Server`, with small fakes for runner, broker, and notifier so the tests stay local and deterministic.

The first unit suite is written. I’m formatting and running the orchestrator package now to shake out any incorrect assumptions in the fake runner/store wiring before expanding to the full repo checks.

The remaining failure is the resume path. The current store semantics only allow extending a pause, not clearing it, so I’m adding a small store method to explicitly clear the persisted scheduler pause and wiring `RunScheduler.Resume` to use it.

The new boundary suite is passing in `internal/orchestrator`. I’m running the repo’s actual verification targets now, because the remaining risk is integration fallout outside the package rather than the unit seams themselves.

`make test` and `make lint` are both in flight. I’ve cleared the package-level regressions already; at this point I’m waiting on the full repo targets and will fix whatever they surface.

The full test target exposed one integration gap: some existing tests mutate `Server.Broker` and `Server.Runner` after construction, but the new supervisor had captured those dependencies too early. I’m switching the wrappers to resync component dependencies from `Server` before scheduling or execution so those integration tests keep their existing behavior.

Lint is cleanly telling us what’s left: a batch of legacy helper methods on `Server` are now dead after the extraction, plus a couple of return-wrapping issues. I’m pruning those leftovers and tightening the remaining error paths instead of suppressing the warnings.

I’ve got the orchestrator refactor and tests in place; I’m cleaning the remaining lint issues, then I’ll rerun full `make lint` and `make test` and write the commit message file if the tree is green.

I’m checking the remaining scheduler/supervisor edges now. The last failures were mostly cleanup after the extraction rather than logic regressions.

The extracted package still compiles; I’m narrowing this to the specific lint-return sites before running the full repo checks.

The main package tests are still passing. I’m patching the last `wrapcheck`-style raw returns and then moving to repo-wide verification.

I’m editing the remaining error returns now so the new module boundaries report failures with context instead of bubbling bare errors.

The error paths are patched and formatted. I’m running the full lint target now so we catch any package-level fallout from the extraction before the test sweep.

`make lint` is green. I’m running the full test suite next, including the old HTTP smoke coverage, to verify the delegation layer didn’t change behavior.

The repo is green end to end. I’m writing the required commit message file and then I’ll summarize the refactor and verification cleanly.

I noticed one untracked binary artifact at repo root, `orchestrator.test`. I’m checking whether it’s just a generated local test binary before I leave the worktree as-is.

Refactored the orchestrator lifecycle into focused components and added direct lifecycle coverage.

- Extracted `ExecutionSupervisor` into [supervisor.go](/work/repo/internal/orchestrator/supervisor.go) and `RunScheduler` into [run_scheduler.go](/work/repo/internal/orchestrator/run_scheduler.go). `Server` in [service.go](/work/repo/internal/orchestrator/service.go) now wires them up and delegates instead of owning the full execution/scheduling flow.
- Introduced narrower `RunStore` and `RunNotifier` boundaries in [service.go](/work/repo/internal/orchestrator/service.go), updated the state machine in [statemachine.go](/work/repo/internal/orchestrator/statemachine.go) to depend on the store subset, and kept the legacy entrypoints in [execution.go](/work/repo/internal/orchestrator/execution.go) and [scheduler.go](/work/repo/internal/orchestrator/scheduler.go) as thin wrappers.
- Added a real pause-clearing store API in [store.go](/work/repo/internal/state/store.go) so scheduler resume actually removes persisted pauses.

Coverage is now in place in [lifecycle_test.go](/work/repo/internal/orchestrator/lifecycle_test.go) and [statemachine_internal_test.go](/work/repo/internal/orchestrator/statemachine_internal_test.go) for:
- execution happy path, cancel-before-start, cancel-during-run, credential acquire failure, credential lease renewal failure, container crash, completed-task cancellation, and start failure
- recovery of running/exited/missing detached executions, expired leases, and recent-start race guard
- scheduling concurrency, pause/resume, draining, and queued-run cancellation
- state-machine store error behavior and all declared transitions

Verification passed with `make lint` and `make test`. I also wrote the required commit message to `/rascal-meta/commit_message.txt`.

Human: # Rascal Run Instructions

Run ID: run_fe7441179184a88a
Task ID: rtzll/rascal#196
Repository: rtzll/rascal
Issue: #196
Pull Request: #201

## Task

Address PR #201 feedback

## Git Context

- Remote: `origin`
- Base branch: `main`
- Head branch: `rascal/rtzll/rascal-196-dcd990d4d8`
- The repository is already cloned and checked out.
- You may use `git` and `gh` directly.
- Push only to `origin` branch `rascal/rtzll/rascal-196-dcd990d4d8`.
- If you rewrite history, you must run `git push --force-with-lease origin HEAD:rascal/rtzll/rascal-196-dcd990d4d8`.
- Otherwise run `git push origin HEAD:rascal/rtzll/rascal-196-dcd990d4d8`.
- Do not push to any other branch.
- Before finishing, ensure the remote branch is updated and the working tree is clean.

- If the request involves rebasing, merge conflict resolution, or other history rewriting, do not rely on the harness to publish those changes for you. Perform the required `git push` yourself before finishing.


## Constraints

- Do not ask for interactive input.
- Do not require MCP tools.
- Keep changes minimal and scoped to the requested task.
- Run `make lint` and `make test` before finishing if those targets exist.
- If one of those commands does not exist or cannot run, explain exactly why and run the closest equivalent checks instead.
- If you make changes, write /rascal-meta/commit_message.txt using a conventional commit title on the first line.
- Optionally add a commit body after a blank line in /rascal-meta/commit_message.txt.

## Additional Context

rebase

<info-msg>
It is currently 2026-03-28 16:40:00
Working directory: /work/repo
Context: ~2227k/1050k tokens used (212%)

Current tasks and notes:
Once given a task, immediately update your todo with all explicit and implicit requirements

# Rascal Persistent Instructions

- Do not ask for interactive input.
- Do not require MCP tools.
- Keep changes minimal and scoped to the requested task.
- Do not overwrite, revert, or discard user changes you did not make unless the task explicitly requires it.
- Use the repository's existing patterns and conventions.
- Prefer the repository's documented workflow over inventing a new one.
- If the repository provides verification commands, run the relevant ones before finishing.
- Run `make lint` and `make test` before finishing if those targets exist.
- If one of those commands does not exist or cannot run, explain exactly why and run the closest equivalent checks instead.
- If you make changes, write /rascal-meta/commit_message.txt using a conventional commit title on the first line.
- Optionally add a commit body after a blank line in /rascal-meta/commit_message.txt.
- If working with GitHub branches or pull requests, only push to the designated Rascal branch for this run.
- If you must rewrite published history, prefer `git push --force-with-lease` over `git push --force`.
- Before finishing, ensure the working tree is clean unless the task explicitly requires uncommitted output.

</info-msg>

Assistant: I’m pulling the current branch state and PR #201 feedback first so I can make the smallest possible fix set. After that I’ll apply the requested changes, run `make lint` and `make test`, write the commit message file, and push the updated branch.

The only visible PR feedback on #201 is `rebase`. I’m checking the branch ancestry against `origin/main` now so I can rebase cleanly, then I’ll rerun the repo verification on the rebased result.

`origin/main` is four commits ahead of the PR branch, so I’m rebasing onto the current base now. If conflicts appear, I’ll resolve only the orchestrator-related overlap and then run the full checks on the rebased branch.

The rebase hit exactly where expected: `main` changed orchestrator imports and server wiring in the same areas this refactor touched. I’m reading the conflicted sections in context now and will keep the newer base-branch behavior while preserving the extracted supervisor/scheduler seams.

I found one stale edge from the extraction while resolving the rebase: `BeginDrain` still references pause timer state on `Server`, but that state now lives in `RunScheduler`. I’m folding that cleanup into the conflict resolution so the rebased branch matches the new component ownership instead of carrying dead fields forward.

I’m editing the conflict resolution now. The goal is to keep the extracted supervisor/scheduler design intact, adopt the newer base-branch server behavior, and remove the stale drain/pause ownership mismatch so the rebased code stays coherent.

The conflicts are resolved. I’m formatting the touched orchestrator files and continuing the rebase next; after that I’ll run the repo checks on the rebased commit rather than trusting the pre-rebase green state.

`git rebase --continue` stopped on missing committer identity, not on code. I’m setting the repo-local git name/email to the branch author and continuing the rebase.

The rebase is complete on the feature branch. I’m running the required verification on the rebased tree now; if the new mainline changes exposed anything, I’ll fix that before pushing.

`make lint` is still running cleanly through the normal toolchain steps, with no findings emitted yet. I’m letting it finish before starting `make test` so any rebase fallout is isolated cleanly.

`golangci-lint` is just taking a long quiet pass rather than failing fast. I’m still waiting on that target; once it returns I’ll immediately run `make test` on the same rebased tree.

The first `make lint` is still genuinely running in the background; the second invocation only tripped the linter’s parallel-run guard. I’m letting the original lint process finish, then I’ll rerun it once in a clean session if needed so we have a definitive result.

The rebased tree is green: `make lint` and `make test` both passed. I’m writing the required commit message file, then I’ll force-push the rebased branch and verify the repo is clean against the updated remote.

Rebased `rascal/rtzll/rascal-196-dcd990d4d8` onto current `main` and resolved the orchestrator conflicts in [execution.go](/work/repo/internal/orchestrator/execution.go), [service.go](/work/repo/internal/orchestrator/service.go), and [run_scheduler.go](/work/repo/internal/orchestrator/run_scheduler.go). The only code adjustment beyond conflict cleanup was aligning drain/pause ownership with the extracted scheduler by stopping the resume timer through `RunScheduler` instead of stale `Server` state.

Verification passed with `make lint` and `make test`. I wrote `/rascal-meta/commit_message.txt`, force-pushed the rebased branch to `origin/rascal/rtzll/rascal-196-dcd990d4d8`, and the worktree is clean.

Human: # Rascal Run Instructions

Run ID: run_6527ea81d833eef9
Task ID: rtzll/rascal#196
Repository: rtzll/rascal
Issue: #196
Pull Request: #201

## Task

Address PR #201 feedback

## Git Context

- Remote: `origin`
- Base branch: `main`
- Head branch: `rascal/rtzll/rascal-196-dcd990d4d8`
- The repository is already cloned and checked out.
- You may use `git` and `gh` directly.
- Push only to `origin` branch `rascal/rtzll/rascal-196-dcd990d4d8`.
- If you rewrite history, you must run `git push --force-with-lease origin HEAD:rascal/rtzll/rascal-196-dcd990d4d8`.
- Otherwise run `git push origin HEAD:rascal/rtzll/rascal-196-dcd990d4d8`.
- Do not push to any other branch.

- If the request involves rebasing, merge conflict resolution, or other history rewriting, do not rely on the harness to publish those changes for you. Perform the required `git push` yourself before finishing.


## Additional Context

rebase


Assistant: 
Image files: 0
============================
{"type":"message","message":{"id":null,"role":"assistant","created":1774716055,"content":[{"type":"text","text":"Ran into this error: Request failed: Codex CLI error: You've hit your usage limit. To get more access now, send a request to your admin or try again at 5:17 PM..\n\nPlease retry if you think this is a transient or recoverable error."}],"metadata":{"userVisible":true,"agentVisible":true}}}
{"type":"complete","total_tokens":2227980}

    __( O)>  ● resuming · codex gpt-5.4
   \____)    20260328_1 · /work/repo
     L L     goose is ready
=== CODEX PROVIDER DEBUG ===
Command: "/usr/local/bin/codex"
Model: gpt-5.4
Reasoning effort: high
Skip git check: false
Prompt length: 26592 chars
Prompt: You are a general-purpose AI agent called goose, created by Block, the parent company of Square, CashApp, and Tidal.
goose is being developed as an open-source software project.
# Suggestion

The user has 7 extensions with 16 tools enabled, exceeding recommended limits (5 extensions or 50 tools).
Consider asking if they'd like to disable some extensions to improve tool selection accuracy.

# Response Guidelines

Use Markdown formatting for all responses.

Human: # Rascal Run Instructions

Run ID: run_dcd990d4d815aa08
Task ID: rtzll/rascal#196
Repository: rtzll/rascal
Issue: #196

## Task

refactor: deepen orchestrator execution lifecycle into a testable module

## Problem

The `internal/orchestrator` package is **4,107 lines across 7 files with zero unit tests**. It is the most critical package in rascal — owning run scheduling, execution supervision, credential lease renewal, status transitions, and GitHub notifications — yet it is entirely untested.

The core friction is that responsibilities are deeply entangled within the `Server` struct:

- **`execution.go`** (~300 lines): Run recovery, execution start, supervision loop (`superviseDetachedRunLoop`), credential preparation/cleanup, container inspection, finalization
- **`notifications.go`** (~500 lines): Start/completion/failure GitHub comments, agent log tailing, response target resolution, comment marker management, run summary rendering
- **`scheduler.go`** (~225 lines): Run scheduling loop, PR reconciliation, branch resolution, review thread cancellation
- **`runs.go`** (~380 lines): `CreateAndQueueRun`, file preparation, instruction text generation, head branch building
- **`service.go`** (~410 lines): Server struct, best-effort helpers, status transitions, draining, constants

To understand how a run completes, you must trace through: `execution.go` → `notifications.go` → `github/client.go` → `state/store.go` → `runsummary/`. A change to the run lifecycle requires coordinated edits across 3-5 files within the same package.

### Specific risks

1. **`setRunStatusBestEffortWithReason`** silently logs and drops status transition errors — if the DB is corrupted, the run appears stuck without any error surfaced
2. **Credential lease renewal** happens inside `superviseDetachedRunLoop` alongside container inspection and cancel checks — a lease renewal failure can cascade into a run failure with no dedicated error handling
3. **Race conditions**: `runCancels` map is protected by `mu`, but the scheduler and execution goroutines access `Store` concurrently with only SQLite serialization as the guard
4. **Recovery logic** in `RecoverRunningRuns` makes 5+ conditional branches with no test coverage for any path

## Proposed Interface

Break the monolithic `Server` into focused sub-components that the `Server` delegates to:

### ExecutionSupervisor
Owns the run supervision loop, container inspection, and finalization decision-making.

```go
type ExecutionSupervisor struct {
    store     RunStore        // interface over state.Store subset
    runner    runner.Runner
    broker    CredentialBroker
    notifier  RunNotifier     // interface for GitHub notification side-effects
    instanceID string
}

// Core lifecycle methods
func (es *ExecutionSupervisor) Execute(ctx context.Context, runID string) error
func (es *ExecutionSupervisor) Recover(ctx context.Context) error
func (es *ExecutionSupervisor) Supervise(ctx context.Context, runID string, exec state.RunExecution, leaseID string) error
```

### RunScheduler
Owns the scheduling loop, concurrency limiting, and pause/resume logic.

```go
type RunScheduler struct {
    store         RunStore
    supervisor    *ExecutionSupervisor
    maxConcurrent int
    instanceID    string
}

func (rs *RunScheduler) Schedule(ctx context.Context, preferredTaskID string) error
func (rs *RunScheduler) Pause(until time.Time, reason string)
func (rs *RunScheduler) Resume()
```

### Usage example

The `Server` becomes a thin orchestration layer:

```go
func NewServer(cfg config.ServerConfig, ...) *Server {
    supervisor := NewExecutionSupervisor(store, runner, broker, notifier, instanceID)
    scheduler := NewRunScheduler(store, supervisor, maxConcurrent, instanceID)
    return &Server{
        Supervisor: supervisor,
        Scheduler:  scheduler,
        // ...HTTP routing only
    }
}
```

## Dependency Strategy

**Local-substitutable** (category 2) + **True external / Mock** (category 4):

- **SQLite (local-substitutable)**: Tests use a real SQLite database via `state.New(filepath.Join(t.TempDir(), "state.db"), 200)` — the existing `testdb` pattern in the codebase. No mocking needed for the store.
- **Docker Runner (mock)**: The `runner.Runner` interface already exists and has a `noop` implementation. Tests inject the noop runner.
- **GitHub API (mock)**: The `GitHubClient` interface already exists in `service.go:57-66`. Tests inject a mock or recording implementation.
- **Credential Broker (mock)**: The `CredentialBroker` interface already exists in `broker.go:20-24`. Tests inject a fake broker.
- **Filesystem**: Tests use `t.TempDir()` for run directories.

## Testing Strategy

### New boundary tests to write

1. **Execution lifecycle tests** (highest priority):
   - `TestExecuteRun_HappyPath`: queued → running → container started → exited → finalized → succeeded
   - `TestExecuteRun_CancelBeforeStart`: queued → cancel requested → canceled
   - `TestExecuteRun_CancelDuringRun`: running → cancel requested → container stopped → canceled
   - `TestExecuteRun_CredentialAcquireFailure`: queued → running → no credential → failed
   - `TestExecuteRun_CredentialLeaseRenewalFailure`: running → lease lost → canceled with `credential_lease_lost` reason
   - `TestExecuteRun_ContainerCrash`: running → container exited with non-zero → failed
   - `TestExecuteRun_TaskAlreadyCompleted`: queued → task completed → canceled with `task_completed` reason

2. **Recovery tests**:
   - `TestRecoverRunningRuns_DetachedContainerStillRunning`: re-adopts supervision
   - `TestRecoverRunningRuns_DetachedContainerExited`: finalizes immediately
   - `TestRecoverRunningRuns_DetachedContainerMissing`: fails run
   - `TestRecoverRunningRuns_ExpiredLease`: requeues run
   - `TestRecoverRunningRuns_NoLeaseRecentStart`: leaves run alone (race guard)

3. **Scheduling tests**:
   - `TestScheduleRuns_RespectsMaxConcurrency`: only N runs start simultaneously
   - `TestScheduleRuns_PauseAndResume`: paused scheduler skips runs, resume triggers scheduling
   - `TestScheduleRuns_DrainingPreventsNewRuns`: draining server rejects new scheduling
   - `TestScheduleRuns_CancelledRunSkipped`: claimed run with pending cancel goes directly to canceled

4. **Status transition integration tests**:
   - `TestBestEffortStatusTransition_DBError`: verify behavior when store returns errors
   - `TestStatusTransition_AllValidPaths`: verify every valid `runStatusTransitions` entry actually works through the execution path

### Old tests to replace

- Currently there are **zero** orchestrator unit tests, so this is purely additive
- The existing integration tests in `cmd/rascald/main_test.go` (4107 lines) test via HTTP endpoints; these should remain as smoke tests but the new boundary tests will cover the logic more granularly

### Test environment needs

- `state.New(t.TempDir() + "/test.db", 200)` for real SQLite
- Noop runner implementation (already exists at `internal/runner/noop.go`)
- Mock `GitHubClient` interface implementation
- Fake `CredentialBroker` implementation with configurable behavior (acquire succeeds/fails, renew succeeds/fails)
- `t.TempDir()` for run directory filesystem operations

## Implementation Recommendations

### What the module should own (responsibilities)

- **ExecutionSupervisor**: The complete lifecycle of a single run from "claimed" to "terminal status". This includes container start, supervision polling, credential lease renewal, cancel handling, finalization, and cleanup.
- **RunScheduler**: The scheduling loop that picks the next queued run, checks concurrency limits, handles pause/resume, and delegates to the supervisor.

### What it should hide (implementation details)

- The supervision polling loop and tick interval
- Credential auth file preparation (writing auth blobs to disk)
- Comment marker file management (idempotency tracking for GitHub comments)
- The retry loop in `prepareRunCredentialAuth`
- The "best effort" error swallowing pattern — callers should get clear errors

### What it should expose (the interface contract)

- `Execute(ctx, runID) error` — run the full lifecycle for one run
- `Recover(ctx) error` — recover all in-flight runs after restart
- `Schedule(ctx, preferredTaskID) error` — attempt to schedule queued runs
- Status of active runs (via the store, not internal state)

### How callers should migrate to the new interface

1. Extract `RunStore` interface covering only the store methods the supervisor and scheduler need (subset of `*state.Store`)
2. Extract `RunNotifier` interface for GitHub notification side-effects
3. Move execution logic from `Server` methods to `ExecutionSupervisor` methods
4. Move scheduling logic from `Server` methods to `RunScheduler` methods
5. `Server.ExecuteRun()` becomes `s.Supervisor.Execute(ctx, runID)`
6. `Server.ScheduleRuns()` becomes `s.Scheduler.Schedule(ctx, preferredTaskID)`
7. HTTP handlers remain on `Server` — they just delegate

### Incremental approach

This refactor should be done in small, safe steps:
1. First: extract interfaces (`RunStore`, `RunNotifier`) without moving any logic
2. Second: write tests against the current `Server` struct using the interfaces
3. Third: extract `ExecutionSupervisor` with tests passing
4. Fourth: extract `RunScheduler` with tests passing
5. Fifth: simplify `Server` to thin delegation layer

## Acceptance Criteria

- [ ] All existing tests pass (`make test`)
- [ ] All linting passes (`make lint`)
- [ ] Orchestrator execution lifecycle has unit test coverage for happy path and all error paths listed above
- [ ] Recovery logic has unit test coverage for all 5 scenarios listed above
- [ ] Scheduling logic has unit test coverage for concurrency, pause/resume, and draining
- [ ] No changes to the HTTP API contract (existing `cmd/rascald/main_test.go` integration tests pass unchanged)
- [ ] New code follows existing patterns (e.g., `testdb` helper, `t.TempDir()`, table-driven tests)


## Constraints

- Do not ask for interactive input.
- Do not require MCP tools.
- Keep changes minimal and scoped to the requested task.
- Run `make lint` and `make test` before finishing if those targets exist.
- If one of those commands does not exist or cannot run, explain exactly why and run the closest equivalent checks instead.
- If you make changes, write /rascal-meta/commit_message.txt using a conventional commit title on the first line.
- Optionally add a commit body after a blank line in /rascal-meta/commit_message.txt.

## Additional Context

Triggered by label 'rascal' on issue #196


Assistant: I’m mapping the current `internal/orchestrator` lifecycle first so the refactor stays minimal and the tests land on stable boundaries. Next I’m reading the package, existing helpers, and the current verification targets before moving any code.

I’ve confirmed the worktree is clean and `make test` / `make lint` exist. Now I’m tracing the concrete server methods and state-store calls that need to move behind supervisor/scheduler seams.

The shape is clear now: execution and scheduling are already separated conceptually, but still coupled through `Server` fields and best-effort helpers. I’m pulling the exact store/broker/runner seams next so I can extract components without changing behavior.

I have enough context to split this safely. The refactor path is: define the store/notifier seam, move execution/recovery into a supervisor that returns errors, move scheduling into a scheduler that delegates to that supervisor, then write boundary tests against those components with real SQLite and fakes.

I’ve started the extraction. The next edit introduces the new supervisor/scheduler seams into `Server` without changing external call sites, then I’ll move the lifecycle logic behind those delegates and port the boundary tests over.

I’ve added the component types. I’m now swapping the old `Server` methods over to thin delegators so the existing HTTP and integration entrypoints keep working unchanged while the new unit-test boundaries become the real implementation.

The delegate swap is in place. I’m running formatting and a focused compile next so I can catch the interface mismatches before I add the new lifecycle tests.

The package build is taking longer than the initial compile pass should, so I’m checking it with a direct `go test -c` style compile next to separate “toolchain still building deps” from actual code errors.

I’ve moved on to the new unit coverage now. The first pass is focused on the execution and scheduling boundaries that used to be trapped inside `Server`, with small fakes for runner, broker, and notifier so the tests stay local and deterministic.

The first unit suite is written. I’m formatting and running the orchestrator package now to shake out any incorrect assumptions in the fake runner/store wiring before expanding to the full repo checks.

The remaining failure is the resume path. The current store semantics only allow extending a pause, not clearing it, so I’m adding a small store method to explicitly clear the persisted scheduler pause and wiring `RunScheduler.Resume` to use it.

The new boundary suite is passing in `internal/orchestrator`. I’m running the repo’s actual verification targets now, because the remaining risk is integration fallout outside the package rather than the unit seams themselves.

`make test` and `make lint` are both in flight. I’ve cleared the package-level regressions already; at this point I’m waiting on the full repo targets and will fix whatever they surface.

The full test target exposed one integration gap: some existing tests mutate `Server.Broker` and `Server.Runner` after construction, but the new supervisor had captured those dependencies too early. I’m switching the wrappers to resync component dependencies from `Server` before scheduling or execution so those integration tests keep their existing behavior.

Lint is cleanly telling us what’s left: a batch of legacy helper methods on `Server` are now dead after the extraction, plus a couple of return-wrapping issues. I’m pruning those leftovers and tightening the remaining error paths instead of suppressing the warnings.

I’ve got the orchestrator refactor and tests in place; I’m cleaning the remaining lint issues, then I’ll rerun full `make lint` and `make test` and write the commit message file if the tree is green.

I’m checking the remaining scheduler/supervisor edges now. The last failures were mostly cleanup after the extraction rather than logic regressions.

The extracted package still compiles; I’m narrowing this to the specific lint-return sites before running the full repo checks.

The main package tests are still passing. I’m patching the last `wrapcheck`-style raw returns and then moving to repo-wide verification.

I’m editing the remaining error returns now so the new module boundaries report failures with context instead of bubbling bare errors.

The error paths are patched and formatted. I’m running the full lint target now so we catch any package-level fallout from the extraction before the test sweep.

`make lint` is green. I’m running the full test suite next, including the old HTTP smoke coverage, to verify the delegation layer didn’t change behavior.

The repo is green end to end. I’m writing the required commit message file and then I’ll summarize the refactor and verification cleanly.

I noticed one untracked binary artifact at repo root, `orchestrator.test`. I’m checking whether it’s just a generated local test binary before I leave the worktree as-is.

Refactored the orchestrator lifecycle into focused components and added direct lifecycle coverage.

- Extracted `ExecutionSupervisor` into [supervisor.go](/work/repo/internal/orchestrator/supervisor.go) and `RunScheduler` into [run_scheduler.go](/work/repo/internal/orchestrator/run_scheduler.go). `Server` in [service.go](/work/repo/internal/orchestrator/service.go) now wires them up and delegates instead of owning the full execution/scheduling flow.
- Introduced narrower `RunStore` and `RunNotifier` boundaries in [service.go](/work/repo/internal/orchestrator/service.go), updated the state machine in [statemachine.go](/work/repo/internal/orchestrator/statemachine.go) to depend on the store subset, and kept the legacy entrypoints in [execution.go](/work/repo/internal/orchestrator/execution.go) and [scheduler.go](/work/repo/internal/orchestrator/scheduler.go) as thin wrappers.
- Added a real pause-clearing store API in [store.go](/work/repo/internal/state/store.go) so scheduler resume actually removes persisted pauses.

Coverage is now in place in [lifecycle_test.go](/work/repo/internal/orchestrator/lifecycle_test.go) and [statemachine_internal_test.go](/work/repo/internal/orchestrator/statemachine_internal_test.go) for:
- execution happy path, cancel-before-start, cancel-during-run, credential acquire failure, credential lease renewal failure, container crash, completed-task cancellation, and start failure
- recovery of running/exited/missing detached executions, expired leases, and recent-start race guard
- scheduling concurrency, pause/resume, draining, and queued-run cancellation
- state-machine store error behavior and all declared transitions

Verification passed with `make lint` and `make test`. I also wrote the required commit message to `/rascal-meta/commit_message.txt`.

Human: # Rascal Run Instructions

Run ID: run_fe7441179184a88a
Task ID: rtzll/rascal#196
Repository: rtzll/rascal
Issue: #196
Pull Request: #201

## Task

Address PR #201 feedback

## Git Context

- Remote: `origin`
- Base branch: `main`
- Head branch: `rascal/rtzll/rascal-196-dcd990d4d8`
- The repository is already cloned and checked out.
- You may use `git` and `gh` directly.
- Push only to `origin` branch `rascal/rtzll/rascal-196-dcd990d4d8`.
- If you rewrite history, you must run `git push --force-with-lease origin HEAD:rascal/rtzll/rascal-196-dcd990d4d8`.
- Otherwise run `git push origin HEAD:rascal/rtzll/rascal-196-dcd990d4d8`.
- Do not push to any other branch.
- Before finishing, ensure the remote branch is updated and the working tree is clean.

- If the request involves rebasing, merge conflict resolution, or other history rewriting, do not rely on the harness to publish those changes for you. Perform the required `git push` yourself before finishing.


## Constraints

- Do not ask for interactive input.
- Do not require MCP tools.
- Keep changes minimal and scoped to the requested task.
- Run `make lint` and `make test` before finishing if those targets exist.
- If one of those commands does not exist or cannot run, explain exactly why and run the closest equivalent checks instead.
- If you make changes, write /rascal-meta/commit_message.txt using a conventional commit title on the first line.
- Optionally add a commit body after a blank line in /rascal-meta/commit_message.txt.

## Additional Context

rebase

<info-msg>
It is currently 2026-03-30 06:30:00
Working directory: /work/repo
Context: ~2227k/1050k tokens used (212%)

# Rascal Persistent Instructions

- Do not ask for interactive input.
- Do not require MCP tools.
- Keep changes minimal and scoped to the requested task.
- Do not overwrite, revert, or discard user changes you did not make unless the task explicitly requires it.
- Use the repository's existing patterns and conventions.
- Prefer the repository's documented workflow over inventing a new one.
- If the repository provides verification commands, run the relevant ones before finishing.
- Run `make lint` and `make test` before finishing if those targets exist.
- If one of those commands does not exist or cannot run, explain exactly why and run the closest equivalent checks instead.
- If you make changes, write /rascal-meta/commit_message.txt using a conventional commit title on the first line.
- Optionally add a commit body after a blank line in /rascal-meta/commit_message.txt.
- If working with GitHub branches or pull requests, only push to the designated Rascal branch for this run.
- If you must rewrite published history, prefer `git push --force-with-lease` over `git push --force`.
- Before finishing, ensure the working tree is clean unless the task explicitly requires uncommitted output.

Current tasks and notes:
Once given a task, immediately update your todo with all explicit and implicit requirements

</info-msg>

Assistant: I’m pulling the current branch state and PR #201 feedback first so I can make the smallest possible fix set. After that I’ll apply the requested changes, run `make lint` and `make test`, write the commit message file, and push the updated branch.

The only visible PR feedback on #201 is `rebase`. I’m checking the branch ancestry against `origin/main` now so I can rebase cleanly, then I’ll rerun the repo verification on the rebased result.

`origin/main` is four commits ahead of the PR branch, so I’m rebasing onto the current base now. If conflicts appear, I’ll resolve only the orchestrator-related overlap and then run the full checks on the rebased branch.

The rebase hit exactly where expected: `main` changed orchestrator imports and server wiring in the same areas this refactor touched. I’m reading the conflicted sections in context now and will keep the newer base-branch behavior while preserving the extracted supervisor/scheduler seams.

I found one stale edge from the extraction while resolving the rebase: `BeginDrain` still references pause timer state on `Server`, but that state now lives in `RunScheduler`. I’m folding that cleanup into the conflict resolution so the rebased branch matches the new component ownership instead of carrying dead fields forward.

I’m editing the conflict resolution now. The goal is to keep the extracted supervisor/scheduler design intact, adopt the newer base-branch server behavior, and remove the stale drain/pause ownership mismatch so the rebased code stays coherent.

The conflicts are resolved. I’m formatting the touched orchestrator files and continuing the rebase next; after that I’ll run the repo checks on the rebased commit rather than trusting the pre-rebase green state.

`git rebase --continue` stopped on missing committer identity, not on code. I’m setting the repo-local git name/email to the branch author and continuing the rebase.

The rebase is complete on the feature branch. I’m running the required verification on the rebased tree now; if the new mainline changes exposed anything, I’ll fix that before pushing.

`make lint` is still running cleanly through the normal toolchain steps, with no findings emitted yet. I’m letting it finish before starting `make test` so any rebase fallout is isolated cleanly.

`golangci-lint` is just taking a long quiet pass rather than failing fast. I’m still waiting on that target; once it returns I’ll immediately run `make test` on the same rebased tree.

The first `make lint` is still genuinely running in the background; the second invocation only tripped the linter’s parallel-run guard. I’m letting the original lint process finish, then I’ll rerun it once in a clean session if needed so we have a definitive result.

The rebased tree is green: `make lint` and `make test` both passed. I’m writing the required commit message file, then I’ll force-push the rebased branch and verify the repo is clean against the updated remote.

Rebased `rascal/rtzll/rascal-196-dcd990d4d8` onto current `main` and resolved the orchestrator conflicts in [execution.go](/work/repo/internal/orchestrator/execution.go), [service.go](/work/repo/internal/orchestrator/service.go), and [run_scheduler.go](/work/repo/internal/orchestrator/run_scheduler.go). The only code adjustment beyond conflict cleanup was aligning drain/pause ownership with the extracted scheduler by stopping the resume timer through `RunScheduler` instead of stale `Server` state.

Verification passed with `make lint` and `make test`. I wrote `/rascal-meta/commit_message.txt`, force-pushed the rebased branch to `origin/rascal/rtzll/rascal-196-dcd990d4d8`, and the worktree is clean.

Human: # Rascal Run Instructions

Run ID: run_6527ea81d833eef9
Task ID: rtzll/rascal#196
Repository: rtzll/rascal
Issue: #196
Pull Request: #201

## Task

Address PR #201 feedback

## Git Context

- Remote: `origin`
- Base branch: `main`
- Head branch: `rascal/rtzll/rascal-196-dcd990d4d8`
- The repository is already cloned and checked out.
- You may use `git` and `gh` directly.
- Push only to `origin` branch `rascal/rtzll/rascal-196-dcd990d4d8`.
- If you rewrite history, you must run `git push --force-with-lease origin HEAD:rascal/rtzll/rascal-196-dcd990d4d8`.
- Otherwise run `git push origin HEAD:rascal/rtzll/rascal-196-dcd990d4d8`.
- Do not push to any other branch.

- If the request involves rebasing, merge conflict resolution, or other history rewriting, do not rely on the harness to publish those changes for you. Perform the required `git push` yourself before finishing.


## Additional Context

rebase

# Rascal Run Instructions

Run ID: run_2a22982ce176069f
Task ID: rtzll/rascal#196
Repository: rtzll/rascal
Issue: #196
Pull Request: #201

## Task

Address PR #201 feedback

## Git Context

- Remote: `origin`
- Base branch: `main`
- Head branch: `rascal/rtzll/rascal-196-dcd990d4d8`
- The repository is already cloned and checked out.
- You may use `git` and `gh` directly.
- Push only to `origin` branch `rascal/rtzll/rascal-196-dcd990d4d8`.
- If you rewrite history, you must run `git push --force-with-lease origin HEAD:rascal/rtzll/rascal-196-dcd990d4d8`.
- Otherwise run `git push origin HEAD:rascal/rtzll/rascal-196-dcd990d4d8`.
- Do not push to any other branch.

- If the request involves rebasing, merge conflict resolution, or other history rewriting, do not rely on the harness to publish those changes for you. Perform the required `git push` yourself before finishing.


## Additional Context

Triggered by pull_request synchronize on PR #201


Assistant: 
Image files: 0
============================
{"type":"message","message":{"id":null,"role":"assistant","created":1774852232,"content":[{"type":"text","text":"Ran into this error: Request failed: Codex CLI error: You've hit your usage limit. To get more access now, send a request to your admin or try again at Apr 3rd, 2026 8:10 AM..\n\nPlease retry if you think this is a transient or recoverable error."}],"metadata":{"userVisible":true,"agentVisible":true}}}
{"type":"complete","total_tokens":2227980}