feat: agentic runs - #212

internal/core/run/parallel/orchestrator.go (1)

251-253: Avoid silently defaulting to context.Background() in Run.

This violates the coding guideline that forbids context.Background() outside main and focused tests. The fallback can mask caller mistakes and breaks explicit cancellation/deadline ownership at a runtime boundary. No callers currently pass nil context, so this is safe to change to return an error instead.

Suggested diff

 func (o *ExecutionOrchestrator) Run(ctx context.Context, plan ParallelPlan) (ParallelOutcome, error) {
-	if ctx == nil {
-		ctx = context.Background()
-	}
+	if ctx == nil {
+		return ParallelOutcome{}, errors.New("parallel execution: missing context")
+	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/run/parallel/orchestrator.go` around lines 251 - 253, The Run
function currently silently defaults to context.Background() when a nil context
is passed in, which violates the coding guideline forbidding
context.Background() outside main and focused tests. Instead of checking if ctx
is nil and defaulting to context.Background(), remove this fallback logic and
return an error when ctx is nil to ensure callers explicitly provide a context
with proper cancellation and deadline ownership.

Source: Coding guidelines

internal/core/run/ui/multi_remote_test.go (1)

881-935: ⚡ Quick win

Use the required t.Run("Should...") style for this new test case.

Please wrap this scenario in a t.Run("Should ...") block to match the test contract used across the repo.

As per coding guidelines, **/*_test.go: “MUST use t.Run("Should...") pattern for ALL test cases”.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/run/ui/multi_remote_test.go` around lines 881 - 935, The test
function TestMultiRunForwardsParallelEventsToActiveChild does not follow the
required t.Run("Should...") pattern specified in the repository's coding
guidelines. Wrap all the test logic currently in the function body inside a
t.Run call with a descriptive name following the "Should..." pattern, such as
t.Run("Should forward parallel events to active child", ...), while keeping the
test function signature and t.Parallel() call at the top level.

Source: Coding guidelines

internal/core/run/parallel/resolver.go (1)

128-130: ⚡ Quick win

Avoid context.Background() fallback in production resolver flow.

Using context.Background() here drops caller cancellation/deadline semantics. Prefer rejecting nil context explicitly so external calls (git/make/executor) remain cancellable only via caller-owned context.

Suggested fix

-	if ctx == nil {
-		ctx = context.Background()
-	}
+	if ctx == nil {
+		return ConflictResult{}, errors.New("conflict resolver: context is required")
+	}

As per coding guidelines, “Pass context.Context as the first argument to all functions crossing runtime boundaries; avoid context.Background() outside main and focused tests`.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/run/parallel/resolver.go` around lines 128 - 130, The nil
context fallback in the resolver flow is dropping caller cancellation and
deadline semantics by assigning context.Background(). Remove the conditional
check that assigns context.Background() when ctx is nil, and instead ensure the
context is always provided by the caller so that external calls to git, make, or
executor remain cancellable through the caller-owned context. This aligns with
the coding guideline to avoid context.Background() outside of main and focused
tests.

Source: Coding guidelines

internal/core/run/parallel/orchestrator_test.go (1)

23-618: 🏗️ Heavy lift

Use the required t.Run("Should...") pattern for added test cases.

This file adds many direct top-level cases without the repository’s required subtest naming/pattern. Please wrap scenarios with t.Run("Should ...") (table-driven where possible) to match test conventions.

As per coding guidelines, **/*_test.go: “MUST use t.Run("Should...") pattern for ALL test cases” and “Use table-driven tests with subtests (t.Run) as the default pattern”.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/run/parallel/orchestrator_test.go` around lines 23 - 618, The
test file contains multiple top-level test functions (such as
TestParallelFSMModelsRequiredHappyPathTransitions,
TestParallelExecutionOrchestratorBoundsWaveConcurrency,
TestParallelExecutionOrchestratorMergesWaveSeriallyInTaskOrder, and others) that
do not follow the repository's required testing pattern. Refactor these tests to
use the t.Run("Should...") pattern with descriptive subtest names that clearly
indicate what behavior is being tested. Where applicable, convert test logic
into table-driven test structures using t.Run() for each test case to improve
readability and maintainability while adhering to the coding guidelines that
require ALL test cases in *_test.go files to use the t.Run("Should...") pattern.

Source: Coding guidelines

internal/core/run/parallel/resolver_test.go (1)

18-217: ⚡ Quick win

Align resolver tests with required t.Run("Should...") naming/pattern.

Please wrap/rename these scenarios to the repo’s required subtest convention (including table rows), so all cases follow Should... semantics consistently.

As per coding guidelines, **/*_test.go: “MUST use t.Run("Should...") pattern for ALL test cases” and “Use table-driven tests with subtests (t.Run) as the default pattern”.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/run/parallel/resolver_test.go` around lines 18 - 217, Align all
test cases in the resolver_test.go file to follow the required
t.Run("Should...") naming pattern as per coding guidelines. For the single test
functions TestConflictResolverSystemPromptIncludesSkillAndConflictContext,
TestAgenticConflictResolutionBoundsAttemptsAtThree, and
TestAgenticConflictResolutionBuildsRuntimeConfigWithAgentSelection, wrap their
test logic inside a t.Run() call with a descriptive "Should..." name. For the
table-driven test
TestAgenticConflictResolutionDerivesResultFromStatusMarkersAndBuildGate, replace
the existing tt.name values in the test table with descriptive names following
the "Should..." pattern (for example, change "clean status and passing build
resolves" to "Should resolve when status is clean and build passes"). Ensure all
subtests consistently use the t.Run("Should...") convention throughout the file.

Source: Coding guidelines

internal/daemon/run_manager_test.go (1)

1749-1776: ⚡ Quick win

Use the required t.Run("Should...") test-case name.

Rename this new subtest to the repository’s required Should... pattern, e.g. Should merge daemon recovery overrides with project defaults.

As per coding guidelines, “MUST use t.Run("Should...") pattern for ALL test cases.”

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/daemon/run_manager_test.go` around lines 1749 - 1776, The test case
name in the t.Run() call does not follow the repository's required naming
convention. Rename the test case from "resolve recovery config" to follow the
"Should..." pattern (e.g., "Should merge daemon recovery overrides with project
defaults") as required by the coding guidelines for all test cases using
t.Run().

Source: Coding guidelines

internal/daemon/task_multi_test.go (1)

653-816: ⚡ Quick win

Wrap the new cases in t.Run("Should...") subtests.

These new test cases skip the required Should... subtest pattern; the invalid-launch-input checks would also read better as table-driven subtests.

As per coding guidelines, “MUST use t.Run("Should...") pattern for ALL test cases” and “Use table-driven tests with subtests (t.Run) as the default pattern.”

Also applies to: 2037-2315

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/daemon/task_multi_test.go` around lines 653 - 816, The test function
TestRunManagerStartTaskWorktreeChildRejectsInvalidLaunchInputs contains three
separate validation checks that are currently sequential if-statements but
should be refactored into table-driven subtests. Create a table with test case
structs containing fields for the test name (following "Should..." pattern), the
targetTaskNumber, taskMultiWorktreeBase configuration, allocator setup, and
expected error message substring. Then iterate through the table using t.Run
with the "Should..." naming pattern to execute each validation check as an
independent subtest, improving readability and following the required coding
guidelines.

Source: Coding guidelines

internal/core/api.go (1)

189-191: ⚡ Quick win

Wrap recovery validation errors with call-site context.

Returning the raw error loses where validation failed in the core config path.

💡 Suggested change

 if err := workspacecfg.ValidateAgentRecoveryConfig("core config", cfg.Recovery); err != nil {
-	return err
+	return fmt.Errorf("validate core recovery config: %w", err)
 }

As per coding guidelines, prefer explicit wrapped error returns using fmt.Errorf("context: %w", err).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/api.go` around lines 189 - 191, The error returned from the
workspacecfg.ValidateAgentRecoveryConfig call is being returned directly without
wrapping it with context information. Modify the error return statement to wrap
the error using fmt.Errorf with a descriptive message that indicates this is a
recovery config validation failure in the core config validation flow. This will
preserve the error chain and provide better debugging context about where the
validation failed.

Source: Coding guidelines

internal/cli/reviews_exec_daemon.go (1)

684-686: ⚡ Quick win

Add context when propagating recovery override parsing errors (both builders).

Both paths return bare errors, which makes it harder to identify whether the failure came from review-run or exec override assembly.

💡 Suggested change

 recovery, err := s.recoveryFlagOverrides(cmd)
 if err != nil {
-	return nil, err
+	return nil, fmt.Errorf("build review runtime recovery overrides: %w", err)
 }

 recovery, err := s.recoveryFlagOverrides(cmd)
 if err != nil {
-	return nil, err
+	return nil, fmt.Errorf("build exec runtime recovery overrides: %w", err)
 }

As per coding guidelines, prefer explicit wrapped error returns using fmt.Errorf("context: %w", err).

Also applies to: 765-767

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/reviews_exec_daemon.go` around lines 684 - 686, The error
handling at lines 684-686 and 765-767 returns bare errors without context,
making it unclear whether the failure came from review-run or exec override
assembly. Wrap both error returns using fmt.Errorf with appropriate context
messages that identify which builder or operation failed (e.g., "failed to parse
review-run override: %w" or "failed to parse exec override: %w"). Ensure each
wrapped error clearly indicates which path the error originated from to improve
debuggability.

Source: Coding guidelines

internal/core/run/executor/execution_acp_test.go (1)

328-591: 🏗️ Heavy lift

Refactor the newly added timeout scenarios into table-driven t.Run("Should...") subtests.

The new cases are valid, but they currently duplicate setup/execute/assert scaffolding and don’t follow the repository’s default subtest structure requirement.

💡 Suggested structure

-func TestExecuteJobWithTimeoutSetupInitDeadlineReturnsTypedTimeout(t *testing.T) { ... }
-func TestExecuteJobWithTimeoutSetupHangUsesInitTimeout(t *testing.T) { ... }
-func TestExecuteJobWithTimeoutSetupCancellationReturnsCanceled(t *testing.T) { ... }
+func TestExecuteJobWithTimeoutSetupPaths(t *testing.T) {
+	t.Parallel()
+
+	cases := []struct{
+		name string
+		// setup + expected assertions
+	}{
+		{name: "Should classify setup init-timeout as timeout"},
+		{name: "Should classify setup cancel as canceled"},
+		{name: "Should keep slow setup valid before activity watchdog timeout"},
+	}
+
+	for _, tc := range cases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			// arrange, execute, assert
+		})
+	}
+}

As per coding guidelines, **/*_test.go must use t.Run("Should...") and table-driven subtests as the default pattern.

Also applies to: 994-1082

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/run/executor/execution_acp_test.go` around lines 328 - 591, The
timeout-related test functions
TestJobRunnerRetriesSetupErrorAfterInitTimeoutThenSucceeds,
TestExecuteJobWithTimeoutClassifiesSetupErrorAfterInitTimeout,
TestExecuteJobWithTimeoutSlowSetupArmsActivityWatchdogAfterSessionEstablished,
TestExecuteJobWithTimeoutSetupInitDeadlineReturnsTypedTimeout, and
TestExecuteJobWithTimeoutSetupCancellationReturnsCanceled are written as
separate test functions with duplicated scaffolding instead of using the
repository's standard table-driven subtest pattern. Consolidate these tests by
creating a single parent test function that uses t.Run with subtests for each
scenario, extracting the common setup and execution logic into table entries
where each entry defines the specific client behavior, assertions, and expected
outcomes for that particular timeout scenario.

Source: Coding guidelines

internal/core/model/model_test.go (1)

545-576: ⚡ Quick win

Wrap this test in a t.Run("Should...") subtest.

TestRuntimeConfigClonePreservesTargetTaskNumber is a direct top-level case; wrap it in a Should... subtest to match the repository’s enforced test pattern.

Suggested patch

 func TestRuntimeConfigClonePreservesTargetTaskNumber(t *testing.T) {
 	t.Parallel()
 
-	targetTaskNumber := 3
-	cfg := &model.RuntimeConfig{
-		TargetTaskNumber: &targetTaskNumber,
-		AddDirs:          []string{"/workspace/docs"},
-	}
-
-	cloned := cfg.Clone()
-	if cloned == nil {
-		t.Fatal("Clone() = nil, want runtime config")
-	}
-	if cloned.TargetTaskNumber == nil {
-		t.Fatal("Clone().TargetTaskNumber = nil, want copied target")
-	}
-	if cloned.TargetTaskNumber == cfg.TargetTaskNumber {
-		t.Fatal("Clone().TargetTaskNumber aliases base pointer")
-	}
-	if *cloned.TargetTaskNumber != targetTaskNumber {
-		t.Fatalf("Clone().TargetTaskNumber = %d, want %d", *cloned.TargetTaskNumber, targetTaskNumber)
-	}
-
-	*cloned.TargetTaskNumber = 7
-	cloned.AddDirs[0] = "/workspace/changed"
-	if *cfg.TargetTaskNumber != targetTaskNumber {
-		t.Fatalf("base TargetTaskNumber changed to %d, want %d", *cfg.TargetTaskNumber, targetTaskNumber)
-	}
-	if cfg.AddDirs[0] != "/workspace/docs" {
-		t.Fatalf("base AddDirs changed to %#v", cfg.AddDirs)
-	}
+	t.Run("Should deep-copy TargetTaskNumber without aliasing", func(t *testing.T) {
+		t.Parallel()
+
+		targetTaskNumber := 3
+		cfg := &model.RuntimeConfig{
+			TargetTaskNumber: &targetTaskNumber,
+			AddDirs:          []string{"/workspace/docs"},
+		}
+
+		cloned := cfg.Clone()
+		if cloned == nil {
+			t.Fatal("Clone() = nil, want runtime config")
+		}
+		if cloned.TargetTaskNumber == nil {
+			t.Fatal("Clone().TargetTaskNumber = nil, want copied target")
+		}
+		if cloned.TargetTaskNumber == cfg.TargetTaskNumber {
+			t.Fatal("Clone().TargetTaskNumber aliases base pointer")
+		}
+		if *cloned.TargetTaskNumber != targetTaskNumber {
+			t.Fatalf("Clone().TargetTaskNumber = %d, want %d", *cloned.TargetTaskNumber, targetTaskNumber)
+		}
+
+		*cloned.TargetTaskNumber = 7
+		cloned.AddDirs[0] = "/workspace/changed"
+		if *cfg.TargetTaskNumber != targetTaskNumber {
+			t.Fatalf("base TargetTaskNumber changed to %d, want %d", *cfg.TargetTaskNumber, targetTaskNumber)
+		}
+		if cfg.AddDirs[0] != "/workspace/docs" {
+			t.Fatalf("base AddDirs changed to %#v", cfg.AddDirs)
+		}
+	})
 }

As per coding guidelines, **/*_test.go MUST use t.Run("Should...") for all test cases and use table-driven tests with subtests as the default pattern.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/model/model_test.go` around lines 545 - 576, Refactor the
TestRuntimeConfigClonePreservesTargetTaskNumber function to match the
repository's test pattern guidelines by wrapping the test logic in a t.Run()
call with a descriptive "Should..." subtest name. Keep the t.Parallel() call at
the function level, then wrap all the test assertions and logic within a t.Run()
that describes what the test validates (e.g., "Should preserve TargetTaskNumber
on clone" or similar). This ensures the test follows the enforced table-driven
test pattern with subtests used as the default pattern across all test files.

Source: Coding guidelines

internal/core/tasks/parser_test.go (1)

291-300: ⚡ Quick win

Rename subtests to the required Should... format.

The table-driven structure is good, but current case names do not follow the enforced t.Run("Should...") convention.

Suggested patch

 	tests := []struct {
 		name string
 		in   string
 		want string
 	}{
-		{name: "canonical filename", in: "task_042.md", want: "task_042"},
-		{name: "extensionless identity", in: "task_15", want: "task_15"},
-		{name: "nested path", in: "features/auth/task_07.md", want: "features/auth/task_07"},
-		{name: "backslash path", in: `features\auth\task_08.md`, want: "features/auth/task_08"},
-		{name: "empty input", in: "   ", want: ""},
+		{name: "Should parse canonical filename", in: "task_042.md", want: "task_042"},
+		{name: "Should keep extensionless identity", in: "task_15", want: "task_15"},
+		{name: "Should preserve nested path identity", in: "features/auth/task_07.md", want: "features/auth/task_07"},
+		{name: "Should normalize backslash path", in: `features\auth\task_08.md`, want: "features/auth/task_08"},
+		{name: "Should return empty for whitespace input", in: "   ", want: ""},
 	}

As per coding guidelines, **/*_test.go MUST use t.Run("Should...") for all test cases.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/tasks/parser_test.go` around lines 291 - 300, The test case
names in the table-driven test do not follow the required Should... naming
convention. Update each name field in the tests slice to start with "Should" and
provide descriptive context. For example, change "canonical filename" to "Should
extract canonical filename", "extensionless identity" to "Should handle
extensionless identity", "nested path" to "Should preserve nested path",
"backslash path" to "Should convert backslash path to forward slashes", and
"empty input" to "Should return empty string for whitespace input". Ensure all
test cases from the table starting at line 291 through the empty input case are
updated to comply with this naming convention.

Source: Coding guidelines

internal/core/run/exec/exec_test.go (1)

59-82: ⚡ Quick win

Align the new tests with the required subtest pattern.

Please wrap these new test cases in t.Run("Should ...") blocks (table-driven if adding more scenarios) to follow the repository test conventions.

As per coding guidelines: **/*_test.go requires “MUST use t.Run("Should...") pattern for ALL test cases” and “Use table-driven tests with subtests (t.Run) as the default pattern.”

Also applies to: 350-394

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/run/exec/exec_test.go` around lines 59 - 82, The test function
TestPersistedExecRunOmitsRuntimeOnlyRecoveryAttempt does not follow the
repository's required testing pattern of using t.Run("Should ...") for all test
cases. Refactor this test by wrapping all the test logic inside a t.Run() call
with a descriptive subtest name that starts with "Should" (for example,
describing that it should omit runtime-only recovery attempts), passing an
anonymous function that receives the testing.T parameter and contains all the
current test body code.

Source: Coding guidelines

internal/cli/workspace_config_test.go (1)

606-691: ⚡ Quick win

Use subtests for the new test cases.

These newly added cases are top-level tests; please wrap each case in t.Run("Should ...") (table-driven when practical) to match repo test conventions.

As per coding guidelines: **/*_test.go requires “MUST use t.Run("Should...") pattern for ALL test cases” and “Use table-driven tests with subtests (t.Run) as the default pattern.”

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/workspace_config_test.go` around lines 606 - 691, Refactor the
two test functions
TestApplyWorkspaceDefaultsMergesRecoveryAndPreservesExplicitRecoveryIDE and
TestApplyWorkspaceDefaultsNoRecoveryForcesDisabled to use subtests following the
repository's testing conventions. Wrap each test case in a t.Run() call with a
descriptive name following the "Should..." pattern. Either combine them into a
single parent test function with multiple subtests for different recovery
scenarios, or add t.Run() calls within each existing test function to break down
the test cases into logical subtests. Ensure all assertions and setup logic
remain intact while conforming to the table-driven test pattern with subtests.

Source: Coding guidelines

internal/core/run/executor/execution_acp_integration_test.go (1)

121-161: ⚡ Quick win

Wrap this new test case in a t.Run("Should ...") subtest.

The new test is added as a standalone top-level case; please move its assertions under a t.Run("Should ...") block to stay consistent with the repo test standard.

As per coding guidelines: **/*_test.go requires “MUST use t.Run("Should...") pattern for ALL test cases” and “Use table-driven tests with subtests (t.Run) as the default pattern.”

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/run/executor/execution_acp_integration_test.go` around lines
121 - 161, The test function
TestExecuteJobWithTimeoutACPSlowInitializeCompletesBeforeInitDeadline is defined
as a standalone top-level test instead of using the required t.Run("Should...")
subtest pattern. Convert this test by moving all of its logic (from the tmpDir
initialization through the final assertion checking for slow initialize output)
into a t.Run call with a descriptive "Should..." name, maintaining the same test
logic while conforming to the repository's testing standards for subtest
organization.

Source: Coding guidelines

internal/core/subprocess/transport_test.go (1)

117-146: ⚡ Quick win

Use t.Run("Should ...") wrappers for the newly added test cases.

Please restructure these new top-level tests into t.Run("Should ...") subtests (table-driven where it helps) to match the repository testing convention.

As per coding guidelines: **/*_test.go requires “MUST use t.Run("Should...") pattern for ALL test cases” and “Use table-driven tests with subtests (t.Run) as the default pattern.”

Also applies to: 296-347, 432-447

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/subprocess/transport_test.go` around lines 117 - 146, The test
functions TestTransportCloseClosesEndpointsOnce and
TestTransportCloseIgnoresNonClosableEndpoints are defined as standalone
top-level test functions instead of using the repository's required
t.Run("Should ...") subtest pattern. Refactor these tests by creating a parent
test function (e.g., TestTransportClose) and converting each test case into a
subtest using t.Run with a descriptive name starting with "Should" (e.g.,
t.Run("Should close endpoints only once", ...)). Move the test logic from each
standalone function into its corresponding t.Run callback function, preserving
all the assertions and setup code.

Source: Coding guidelines

internal/core/run/recovery/agentic_test.go (1)

23-174: ⚡ Quick win

Use the required t.Run("Should...") style for all test cases.

Several test cases are still expressed as plain top-level flows (and subtest names like "fixed"/"reject"), instead of the mandated Should... subtest pattern. Please wrap each scenario in t.Run("Should ...") and rename case labels accordingly for consistency with the suite conventions.

As per coding guidelines, **/*_test.go requires: MUST use t.Run("Should...") pattern for ALL test cases and Use table-driven tests with subtests (t.Run) as the default pattern.

Also applies to: 176-233, 235-319

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/run/recovery/agentic_test.go` around lines 23 - 174, The test
functions TestAgenticRemediationBuildsNonRecursiveRecoveryRunConfig and
TestAgenticRemediationSystemPromptContainsGuidanceAndFailureContext are written
as top-level test functions instead of using the required t.Run("Should...")
pattern. Refactor these tests to follow the suite conventions by replacing each
top-level test function with a parent test that uses t.Run() with descriptive
subtest names starting with "Should", such as "Should build non-recursive
recovery run config" and "Should contain guidance and failure context in system
prompt". Move the existing test logic into these t.Run() subtests while
maintaining the current assertions and test flow.

Source: Coding guidelines

internal/core/run/recovery/prompt.go (1)

111-138: 💤 Low value

Consider using io.SeekStart constant for clarity.

The magic number 0 in file.Seek(offset, 0) works but using the named constant improves readability:

-	if _, err := file.Seek(offset, 0); err != nil {
+	if _, err := file.Seek(offset, io.SeekStart); err != nil {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/run/recovery/prompt.go` around lines 111 - 138, The readLogTail
function uses a magic number 0 in the file.Seek call instead of the named
constant. Replace the hardcoded 0 with io.SeekStart in the file.Seek(offset, 0)
method call to improve code clarity and readability. This makes it immediately
obvious that you are seeking from the beginning of the file.

internal/core/run/recovery/orchestrator.go (2)

214-279: 💤 Low value

Unconventional triple return signature may confuse callers.

restartAfterFixedVerdict returns (RunOutcome, error, error) which is unusual in Go. Consider using a struct or named errors to clarify the distinction between "run error" and "orchestration error":

type restartResult struct {
    outcome      RunOutcome
    runErr       error  // error from the restarted run
    orchestrateErr error // error from orchestration (event emit, transition)
}

Alternatively, document the return semantics clearly if the current signature is preferred.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/run/recovery/orchestrator.go` around lines 214 - 279, The
restartAfterFixedVerdict function has a confusing triple return signature with
two error values that lack semantic clarity. Either refactor the function to
return a struct (such as restartResult with named fields like outcome, runErr,
and orchestrateErr) to clearly distinguish between the run error and
orchestration error, or add comprehensive documentation comments to the function
that explicitly explain what each return value represents and when it will be
non-nil. This will make the function's contract clearer to callers.

---

94-116: ⚡ Quick win

Defensive ctx == nil fallback creates context.Background() in library code.

Per coding guidelines, context.Background() should be avoided outside main and focused tests. If callers must provide a valid context, consider returning an error instead of silently substituting:

 func (o *RunRecoveryOrchestrator) Run(ctx context.Context, run PreparedRun) (RunOutcome, error) {
-	if ctx == nil {
-		ctx = context.Background()
-	}
+	if ctx == nil {
+		return RunOutcome{}, errors.New("run recovery: nil context")
+	}

However, if this is intentional for resilience in edge cases, document the rationale.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/run/recovery/orchestrator.go` around lines 94 - 116, In the Run
method of RunRecoveryOrchestrator, the defensive check that creates
context.Background() when ctx is nil violates library code guidelines. Either
remove the ctx == nil check and return an error if the caller fails to provide a
valid context, or if this defensive behavior is intentional for resilience
reasons, add a comment explaining the rationale for allowing a nil context
fallback in this library function.

Source: Coding guidelines

internal/core/run/internal/worktree/snapshot.go (1)

105-175: 💤 Low value

Minor inefficiency: head is trimmed twice.

bytes.TrimSpace(head) is called at line 138 to populate snapshot.head, but the same raw head slice is trimmed again at line 162 when computing the digest. Consider reusing the already-trimmed value:

 	h.Write([]byte{0})
-	h.Write(bytes.TrimSpace(head))
+	h.Write([]byte(snapshot.head))
 	h.Write([]byte{0})

This also ensures the digest is computed from the exact same bytes stored in the snapshot.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/run/internal/worktree/snapshot.go` around lines 105 - 175, The
Capture function trims the head bytes twice unnecessarily: once at the
assignment to snapshot.head and again when computing the digest hash. Store the
result of bytes.TrimSpace(head) in a variable once, use that trimmed value to
assign to snapshot.head (converting to string), and then reuse the same trimmed
bytes when calling h.Write for the digest computation instead of calling
bytes.TrimSpace(head) again.

internal/core/run/recovery/audit.go (1)

168-201: 💤 Low value

Avoid double call to snapshot.Porcelain() which copies the slice twice.

snapshot.Porcelain() returns a defensive copy each time it's called. Lines 190 and 192 both call it, creating two copies:

-	doc := auditSnapshotDocument{
+	porcelain := snapshot.Porcelain()
+	doc := auditSnapshotDocument{
 		SchemaVersion:          recoveryDiffAuditSchemaVersion,
 		Supported:              snapshot.IsSupported(),
 		GitAvailable:           snapshot.GitAvailable(),
 		IsGitRepo:              snapshot.IsGitRepo(),
 		HasCommits:             snapshot.HasCommits(),
 		Head:                   snapshot.Head(),
 		Branch:                 snapshot.Branch(),
 		Digest:                 snapshot.Digest(),
 		UnsupportedReason:      string(snapshot.UnsupportedReason()),
-		rawPorcelainForParsing: snapshot.Porcelain(),
+		rawPorcelainForParsing: porcelain,
 	}
-	if porcelain := snapshot.Porcelain(); len(porcelain) > 0 {
+	if len(porcelain) > 0 {
 		doc.RawPorcelainZBase64 = base64.StdEncoding.EncodeToString(porcelain)
 	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/run/recovery/audit.go` around lines 168 - 201, The function
captureAuditSnapshot calls snapshot.Porcelain() twice, creating two defensive
copies unnecessarily - once when assigning to rawPorcelainForParsing in the
auditSnapshotDocument struct initialization and again in the if condition check.
Refactor this by calling snapshot.Porcelain() once and storing the result in a
single variable, then use that variable for both the rawPorcelainForParsing
field assignment and the length check in the if statement.

internal/core/kernel/recovery_prepared_run.go (1)

11-15: ⚡ Quick win

Add compile-time assertions for recovery.PreparedRun implementers.

Adding interface assertions here prevents silent drift between these structs and the recovery seam during refactors.

Suggested refactor

 type kernelWorkflowPreparedRun struct {
 	ops        operations
 	runtimeCfg *model.RuntimeConfig
 	scope      model.RunScope
 }
+
+var _ recovery.PreparedRun = (*kernelWorkflowPreparedRun)(nil)
@@
 type kernelExecPreparedRun struct {
 	ops        operations
 	runtimeCfg *model.RuntimeConfig
 	scope      model.RunScope
 }
+
+var _ recovery.PreparedRun = (*kernelExecPreparedRun)(nil)

As per coding guidelines, "Use compile-time interface verification: var _ Interface = (*Type)(nil)."

Also applies to: 73-77

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/kernel/recovery_prepared_run.go` around lines 11 - 15, Add
compile-time interface assertions to verify that the kernelWorkflowPreparedRun
struct (defined at lines 11-15) and the other struct mentioned at lines 73-77
properly implement the recovery.PreparedRun interface. Use the compile-time
assertion pattern by adding a package-level variable declaration `var _
recovery.PreparedRun = (*kernelWorkflowPreparedRun)(nil)` after the struct
definition for kernelWorkflowPreparedRun, and similarly for the struct around
lines 73-77. This prevents silent drift during refactors by catching at
compile-time if either struct stops implementing the required interface.

Source: Coding guidelines

internal/core/run/recovery/reader_integration_test.go (1)

13-67: ⚡ Quick win

Wrap this scenario in t.Run("Should...") to satisfy test conventions.

This is a single-case test and can be wrapped directly in one Should... subtest for consistency with repo rules.

As per coding guidelines, "**/*_test.go: MUST use t.Run(\"Should...\") pattern for ALL test cases".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/run/recovery/reader_integration_test.go` around lines 13 - 67,
The test function TestReadRunOutcomeFromExecutorPersistedResult does not follow
the repository's test convention of using t.Run("Should...") pattern. Wrap all
the existing test logic (from t.Parallel() through the final assertion) inside a
t.Run call with a descriptive name that starts with "Should" to describe what
the test validates, ensuring the t.Parallel() call remains at the top level
before the t.Run invocation.

Source: Coding guidelines

internal/core/run/recovery/reader_test.go (1)

57-75: ⚡ Quick win

Rename nested subtests to the Should... format.

The inner subtests currently use "run status" / "job status"; switch to Should... names to keep the suite compliant and consistent.

As per coding guidelines, "**/*_test.go: MUST use t.Run(\"Should...\") pattern for ALL test cases".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/run/recovery/reader_test.go` around lines 57 - 75, The nested
subtests in the test file use generic names like "run status" and "job status"
instead of following the required "Should..." naming convention specified in the
coding guidelines. Update both test case name fields in the tests slice to use
descriptive "Should..." names (for example, changing "run status" to something
like "ShouldUpdateRunStatus" and "job status" to something like
"ShouldUpdateJobStatus") to ensure compliance with the test naming standards for
all test cases in *_test.go files.

Source: Coding guidelines

internal/core/run/recovery/orchestrator_test.go (1)

19-494: 🏗️ Heavy lift

Align new test cases with the required t.Run("Should...") pattern.

This file introduces many standalone top-level scenarios; please group them under t.Run("Should...") cases to match the repo’s enforced test structure.

As per coding guidelines, "**/*_test.go: MUST use t.Run(\"Should...\") pattern for ALL test cases".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/run/recovery/orchestrator_test.go` around lines 19 - 494, The
test file contains multiple standalone top-level test functions like
TestRecoveryFSMModelsRequiredTransitions,
TestRunRecoveryOrchestratorFixedVerdictRestartsFailedJobsAndRecovers, and others
that do not follow the required t.Run("Should...") pattern. Refactor each of
these standalone test functions by consolidating them into parent test functions
and using t.Run("Should...") subtests for each scenario. For example, move the
test logic from TestRunRecoveryOrchestratorBoundsAttempts into a parent test
function and wrap the existing t.Run blocks with descriptive "Should..." names,
and similarly convert other standalone functions into t.Run subtests within
appropriate parent test functions that describe the behavior being tested.

Source: Coding guidelines

internal/core/kernel/recovery_prepared_run_test.go (1)

27-456: 🏗️ Heavy lift

Convert standalone tests to t.Run("Should...") scenarios for policy compliance.

This file adds many independent top-level tests; please wrap/organize them under Should... subtests to match the enforced test contract.

As per coding guidelines, "**/*_test.go: MUST use t.Run(\"Should...\") pattern for ALL test cases".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/kernel/recovery_prepared_run_test.go` around lines 27 - 456,
The test functions in this file do not follow the required coding guideline of
using t.Run("Should...") pattern for test cases. Convert each standalone test
function (TestKernelWorkflowPreparedRunRestartFailedFiltersBySafeName,
TestRunStartRecoveryDisabledUsesOriginalPreparedPath,
TestRunStartRecoveryRecoversAndRestartsOnlyFailedJobs,
TestRunStartRecoveryIntegrationUsesDeterministicACPAndRealRestart,
TestRunStartRecoverySkipsCanceledAndRecoveryAttemptRuns,
TestRunStartRecoveryRejectFailsLoudlyAndRecordsOneAttempt, and
TestRunExecRecoveryRerunsSingleJob) to wrap their test logic inside t.Run()
calls with descriptive names using the "Should..." pattern (e.g., "Should filter
by safe name", "Should use original prepared path when recovery disabled"). For
tests that already use t.Run like
TestRunStartRecoverySkipsCanceledAndRecoveryAttemptRuns, update the subtest
names to follow the "Should..." convention instead of plain names like
"canceled" and "recovery attempt".

Source: Coding guidelines

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes