feat: add provider-neutral ACP speed control - #267

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

internal/api/httpapi/openapi_contract_test.go (2)

541-558: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Use slices.Sort for consistency with the new slices import.

♻️ Proposed tweak

-	sort.Strings(values)
+	slices.Sort(values)

Verify sort is still needed elsewhere in the file before dropping the import.

🤖 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/api/httpapi/openapi_contract_test.go` around lines 541 - 558, Update
schemaEnumStrings to sort values with slices.Sort instead of sort.Strings,
reusing the file’s existing slices import; then remove the sort import if no
other references remain in the file.

---

350-373: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Exact-string matching against generated TypeScript is brittle.

These fragments depend on the generator's exact spacing, member ordering, and union ordering; a harmless regeneration (or generator upgrade) breaks the suite even though the contract is unchanged. The spec-level test above already pins the canonical literals — consider asserting only that each Speed* type is a union of the expected members (e.g. regex per type, order-insensitive) and that the fields are optional/required. Also wrap this in a Should … subtest per the test conventions.

🤖 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/api/httpapi/openapi_contract_test.go` around lines 350 - 373, Update
TestGeneratedOpenAPIContractKeepsSpeedFieldsOptionalAndLiteral to avoid exact
generated TypeScript fragment matching: use a Should subtest and assertions that
each Speed* type contains the expected union members order-independently, while
separately verifying speed and speed_resolution are optional and
requested/status are required. Preserve the existing canonical literals and
remove dependence on generator spacing, ordering, or formatting.

Source: Path instructions

internal/core/workspace/config_test.go (2)

1841-1844: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Asserting the exact TOML decoder message couples the suite to the library's wording.

"decode workspace config: strict mode: fields in the document are missing in the target struct" will break on any go-toml message change. Asserting the wrapper prefix plus a strings.Contains on "strict mode" keeps the intent without the coupling.

🤖 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/workspace/config_test.go` around lines 1841 - 1844, Update the
error assertion in the strict-mode workspace configuration test to verify the
stable “decode workspace config:” wrapper prefix and use strings.Contains to
check for “strict mode” rather than comparing the TOML decoder’s full message.
Preserve the existing failure reporting and ensure the test imports strings if
needed.

---

1661-1699: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

New tests across the speed cohort skip the mandated Should … subtest naming. One shared root cause: cases are either named descriptively without the Should prefix or run directly in the parent test body. As per path instructions, "MUST use t.Run("Should...") pattern for ALL test cases".

• internal/core/workspace/config_test.go#L1661-L1699: rename the table-case names ("normal under defaults", etc.) to Should … phrasing; do the same for the precedence table at L2325-L2390.
• internal/api/contract/contract_integration_test.go#L165-L167: rename to "Should keep run list and detail shape compatible".
• internal/api/httpapi/openapi_contract_test.go#L329-L347: move the SpeedResolution/RunJobSummary assertions into Should … subtests, and wrap TestGeneratedOpenAPIContractKeepsSpeedFieldsOptionalAndLiteral (L350-L373) in one as well.
• internal/api/client/client_contract_test.go#L501-L530: wrap the historical-snapshot body in a Should decode historical snapshots without speed fields subtest.

🤖 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/workspace/config_test.go` around lines 1661 - 1699, Update the
speed-cohort tests to follow the mandated Should-subtest convention: in
internal/core/workspace/config_test.go ranges 1661-1699 and 2325-2390, rename
every table case with Should-prefixed phrasing; in
internal/api/contract/contract_integration_test.go range 165-167, rename the
case to “Should keep run list and detail shape compatible”; in
internal/api/httpapi/openapi_contract_test.go range 329-347, move the
SpeedResolution and RunJobSummary assertions into Should subtests, and wrap
TestGeneratedOpenAPIContractKeepsSpeedFieldsOptionalAndLiteral in a Should
subtest; in internal/api/client/client_contract_test.go range 501-530, wrap the
historical-snapshot assertions in a “Should decode historical snapshots without
speed fields” subtest.

Source: Path instructions

internal/api/contract/contract_integration_test.go (1)

199-205: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Negative substring assertion is broad but acceptable; consider asserting on the decoded run summary instead.

strings.Contains(body, "\"speed\"") guards shape stability, yet it would also fail if any unrelated nested payload legitimately gains a speed key later. Decoding into a map[string]any and checking the run objects keeps the intent narrower.

🤖 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/api/contract/contract_integration_test.go` around lines 199 - 205,
Refine the response-shape assertion in the integration test around the existing
response body check: decode the response JSON into a map or equivalent
structure, inspect only the run summary objects, and assert that those objects
do not contain the “speed” field. Preserve the current failure context using
testCase.path and the response details.

internal/core/agent/client_test.go (2)

273-344: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

New speed tests do not follow the Should … subtest convention. The shared root cause is that the tests added in this PR register table cases (or omit subtests entirely) without the repository-mandated Should … naming, which makes failures harder to scan against the rest of the suites.

• internal/core/agent/client_test.go#L273-L344: rename the TestClientCreateSessionSpeedOutcomes case names ("fast applied after write", "unsupported absent", …) to "Should …" phrasing; apply the same to TestClientAtomicSessionSetupOrdersModelSpeedAndPrompt at L162-L176.
• internal/cli/daemon_commands_test.go#L3594-L3613: rename the three table entries ("omitted speed leaves the payload empty", "explicit normal remains present", "explicit fast remains present") to "Should …" phrasing.
• internal/core/run/executor/execution_acp_test.go#L33-L51: wrap the three assertions of TestHookRuntimeConfigPreservesSpeed in t.Run("Should …", …) subtests.

As per path instructions, "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/agent/client_test.go` around lines 273 - 344, Rename all
table-driven test case names in internal/core/agent/client_test.go (lines
273-344, including TestClientCreateSessionSpeedOutcomes and
TestClientAtomicSessionSetupOrdersModelSpeedAndPrompt) to use “Should …”
phrasing. Rename the three table entries in internal/cli/daemon_commands_test.go
(lines 3594-3613) similarly. In internal/core/run/executor/execution_acp_test.go
(lines 33-51), wrap each assertion in TestHookRuntimeConfigPreservesSpeed with a
t.Run subtest named “Should …”.

Source: Path instructions

---

3062-3097: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Capability-claim assertion partially duplicates the DeepEqual check.

The reflect.DeepEqual comparison against the expected ClientCapabilities already rejects any extra claim; the subsequent substring scan over the marshaled JSON ("session", "configOptions", "boolean", "_meta") is a weaker, string-based restatement that will silently stop protecting anything if field names change. Consider dropping it, or keeping only the JSON check with a comment explaining the wire-shape invariant it guards.

🤖 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/agent/client_test.go` around lines 3062 - 3097, Remove the
redundant marshaling and substring checks from helperAgent.Initialize after the
reflect.DeepEqual comparison, including the associated error handling and
RequestError path. Keep the exact ClientCapabilities comparison as the sole
assertion unless a focused JSON check is required to enforce a distinct
wire-format invariant.

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

719-737: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Remove the unused conflict-session helpers.

conflictAtomicClient, newConflictAtomicSession, installConflictRuntimeProbeStub, and swapConflictAtomicClient are only definitions in internal/core/run/parallel/resolver_test.go; no tested path installs or uses them, so they should be removed.

🤖 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 719 - 737, Remove
the unused test-only helpers conflictAtomicClient, newConflictAtomicSession,
installConflictRuntimeProbeStub, and swapConflictAtomicClient from
resolver_test.go. Delete their associated fields and methods as well, without
changing any active resolver tests or production behavior.

internal/cli/root_command_execution_test.go (1)

425-480: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

New speed tests don't use the required t.Run("Should ...") subtest pattern.

Path instructions for **/*_test.go state tests MUST use t.Run("Should...") pattern for ALL test cases. All seven newly-introduced test functions below use descriptive-but-non-"Should..." subtest names, or skip subtests entirely, deviating from this rule (even though it mirrors much of this codebase's pre-existing local convention).

• internal/cli/root_command_execution_test.go#L425-L480: wrap TestExecCommandRejectsConflictingExplicitResumeSpeedBeforeSubmission's body in t.Run("Should reject a conflicting explicit --speed during resume", func(t *testing.T) {...}).
• internal/cli/root_test.go#L648-L733: rename each t.Run(tc.path, ...) subtest to a "Should ..." description (e.g., "Should document speed contract in " + tc.path).
• internal/cli/root_test.go#L1014-L1040: rename t.Run(tc.name, ...) to "Should register --speed for " + tc.name.
• internal/cli/root_test.go#L1042-L1077: rename t.Run(string(tc.kind), ...) to a "Should ..." description per command kind.
• internal/cli/root_test.go#L1079-L1105: rename t.Run(tc.name, ...) to "Should reject " + tc.name.
• internal/cli/root_test.go#L1107-L1152: rename t.Run(tc.name, ...) to a "Should ..." description per case.
• internal/core/agents/mcpserver/engine_test.go#L169-L258: wrap TestRunAgentInheritsSpeedAcrossIndependentChildExecutions's body in t.Run("Should keep child runtime speed independent across executions", func(t *testing.T) {...}).

🤖 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/root_command_execution_test.go` around lines 425 - 480, Update
all seven listed test sites to use subtest names beginning with “Should”: wrap
TestExecCommandRejectsConflictingExplicitResumeSpeedBeforeSubmission and
TestRunAgentInheritsSpeedAcrossIndependentChildExecutions in the specified
Should-named t.Run blocks; rename the table-driven subtests in
internal/cli/root_test.go at ranges 648-733, 1014-1040, 1042-1077, 1079-1105,
and 1107-1152 to descriptive Should-prefixed names while preserving their
existing test logic.

Source: Path instructions

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

888-924: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Attempt gating is skipped when Resolution is nil.

A stale jobSpeedMsg carrying only Requested (no resolution) bypasses the attempt < job.speedResolutionAttempt guard and can overwrite job.requestedSpeed for a newer attempt. Consider applying the same gate before the v.Requested assignment.

♻️ Proposed adjustment

 	attempt := v.Attempt
+	if attempt <= 0 {
+		attempt = job.attempt
+	}
+	if attempt > 0 && attempt < job.speedResolutionAttempt {
+		return
+	}
 	if v.Resolution != nil {
-		if attempt <= 0 {
-			attempt = job.attempt
-		}
-		if attempt > 0 && attempt < job.speedResolutionAttempt {
-			return
-		}
-	}
+		job.speedResolutionAttempt = attempt
+	}

🤖 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/update.go` around lines 888 - 924, Update
uiModel.handleJobSpeed to apply attempt gating whenever v.Attempt identifies an
older attempt, including messages with a nil Resolution, before assigning
v.Requested to job.requestedSpeed. Preserve the existing fallback to job.attempt
and ensure stale requested-only messages return without mutating job state.

pkg/compozy/events/kinds/speed.go (1)

1-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider colocating a canonical validator with the type.

Speed is validated in at least the CLI/workspace-config layers; exposing func (s Speed) Valid() bool (or ParseSpeed(string) (Speed, error)) here would keep the accepted set in one place as more surfaces (HTTP, UDS, SDK) start accepting speed input.

🤖 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 `@pkg/compozy/events/kinds/speed.go` around lines 1 - 44, Add a canonical
validation API alongside the Speed type, preferably Speed.Valid() or ParseSpeed,
that accepts only the defined SpeedNormal and SpeedFast values. Update existing
CLI and workspace-config validation to reuse this API so all input surfaces
share one accepted-value set.

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

205-205: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Prefer an explicit conditional over the map[bool]string{...}[cond] trick.

map[bool]string{true: "sess-speed"}[test.resume] allocates a map to express an if/else and reads poorly next to the surrounding literal fields.

♻️ Suggested rewrite

-				ExpectedLoadSessionID:    map[bool]string{true: "sess-speed"}[test.resume],
+				ExpectedLoadSessionID:    expectedLoadSessionID(test.resume),

func expectedLoadSessionID(resume bool) string {
	if resume {
		return "sess-speed"
	}
	return ""
}

🤖 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` at line 205,
Replace the map-based ExpectedLoadSessionID expression in the test case literal
with an explicit conditional helper or equivalent if/else logic: return
"sess-speed" when test.resume is true and an empty string otherwise. Keep the
surrounding expected literal fields unchanged.

internal/core/agent/speed_config.go (1)

14-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Keep the iota block for shapes only.

speedConfigCategoryModelConfig is a different type and sits inside the speedConfigShape iota block; a future shape constant inserted above it would silently shift values. Declaring the category in its own const block removes that footgun.

♻️ Suggested split

 const (
 	speedConfigShapeBoolean speedConfigShape = iota + 1
 	speedConfigShapeSelect
-	speedConfigCategoryModelConfig = acp.SessionConfigOptionCategory("model_config")
 )
+
+const speedConfigCategoryModelConfig = acp.SessionConfigOptionCategory("model_config")

🤖 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/agent/speed_config.go` around lines 14 - 18, Move
speedConfigCategoryModelConfig out of the iota-based const block into its own
const declaration, leaving only speedConfigShapeBoolean and
speedConfigShapeSelect in the speedConfigShape block so future shape constants
cannot alter the category value.

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

302-315: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicate zero-value-to-nil helper.

optionalSpeedResolution here duplicates optionalExecSpeedResolution in internal/core/run/exec/exec.go. Will address via consolidated comment.

🤖 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/lifecycle.go` around lines 302 - 315, Remove the
duplicate optionalSpeedResolution helper from the lifecycle code and reuse the
existing optionalExecSpeedResolution helper from the exec package, updating
optionalRejectedSpeedResolution accordingly while preserving its rejected-status
filtering and zero-value-to-nil behavior.

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

1017-1023: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicate zero-value-to-nil helper.

optionalExecSpeedResolution is a byte-for-byte duplicate of optionalSpeedResolution in internal/core/run/executor/lifecycle.go. Given how many layers in this PR need this exact conversion, consolidating into a shared exported helper (e.g., on kinds.SpeedResolution) would avoid drift.

🤖 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.go` around lines 1017 - 1023, Replace the
duplicate optionalExecSpeedResolution helper with a shared exported
zero-value-to-nil conversion helper associated with kinds.SpeedResolution, then
update both exec.go and lifecycle.go callers to use it and remove the local
helpers. Preserve the existing behavior of returning nil for an empty resolution
and a copied pointer for non-zero values.

internal/core/run/internal/acpshared/command_io.go (2)

214-226: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Remove the always-empty option_shape log field.

"option_shape", "" never carries a value, so it only adds noise to structured logs. Either drop it or populate it from the resolution source.

♻️ Proposed cleanup

 	logger.Info(
 		"ACP session speed resolved",
 		"speed_requested", resolution.Requested,
 		"speed_status", resolution.Status,
 		"speed_reason", resolution.Reason,
-		"option_shape", "",
 		"resumed", job != nil && strings.TrimSpace(job.ResumeSession) != "",
 	)

🤖 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/acpshared/command_io.go` around lines 214 - 226,
Remove the always-empty "option_shape" field from the structured logging
arguments in logSpeedResolution, leaving the existing speed resolution and
resumed-session fields unchanged.

---

236-257: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Collapse the duplicated write/error-wrap branches.

♻️ Proposed simplification

-	if resolution.Reason == "" {
-		if _, err := fmt.Fprintf(dst, "Speed %s: %s\n", resolution.Requested, resolution.Status); err != nil {
-			return fmt.Errorf("write speed resolution status: %w", err)
-		}
-		return nil
-	}
-	if _, err := fmt.Fprintf(
-		dst,
-		"Speed %s: %s (%s)\n",
-		resolution.Requested,
-		resolution.Status,
-		resolution.Reason,
-	); err != nil {
-		return fmt.Errorf("write speed resolution status: %w", err)
-	}
-	return nil
+	line := fmt.Sprintf("Speed %s: %s", resolution.Requested, resolution.Status)
+	if resolution.Reason != "" {
+		line += fmt.Sprintf(" (%s)", resolution.Reason)
+	}
+	if _, err := fmt.Fprintln(dst, line); err != nil {
+		return fmt.Errorf("write speed resolution status: %w", err)
+	}
+	return 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/internal/acpshared/command_io.go` around lines 236 - 257,
Collapse the duplicated fmt.Fprintf and error-wrapping branches in
WriteSpeedResolutionStatus by constructing the output format or message based on
whether resolution.Reason is empty, then performing one write and one “write
speed resolution status” error-wrap path. Preserve the current output for both
reason-present and reason-absent cases.

pkg/compozy/runs/run.go (1)

447-452: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Document the index reconciliation rule.

runJobIndex silently prefers Summary.Index only when job.Index == 0, which is indistinguishable from a legitimate job at index 0 whose summary index is also 0. It works for the current payloads (state index omitted, summary index present), but a one-line comment stating that the state index is authoritative and the summary index is the fallback for payloads that omit it would prevent future misreadings.

🤖 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 `@pkg/compozy/runs/run.go` around lines 447 - 452, Document the reconciliation
rule directly above or within runJobIndex: job.Index is authoritative when
present, while the nonzero Summary.Index is used only as the fallback when the
state index is omitted. Keep the existing behavior unchanged.

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

91-150: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Wrap the scenario in a Should … subtest.

This new top-level test asserts a single behavior but doesn't use the repo's t.Run("Should …") convention that the rest of the suite follows (e.g. the helper-based scenarios below). t.Setenv correctly rules out t.Parallel, so only the naming needs adjusting.

As per coding guidelines: "Go tests must use Should … subtest names".

🤖 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 91 - 150, The
TestParallelExecutionOrchestratorConflictSpeedIntegration test should run its
existing scenario inside a t.Run subtest named with the repository’s “Should …”
convention. Preserve the setup, assertions, and environment handling unchanged,
and do not add t.Parallel.

Source: Coding guidelines

sdk/extension/types.go (1)

523-571: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff

sdk/extension/types.go is well past the 500-line cap and keeps growing.

This change extends an already oversized contracts file (RuntimeConfig alone ends at Line 571). Not introduced by this PR, but a good moment to split the session/request contracts from the runtime-config contracts into separate files.

As per coding guidelines: "Production Go files must have one responsibility and remain at or below 500 lines; split contracts, registries, implementations, and cross-cutting helpers into separate files."

🤖 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 `@sdk/extension/types.go` around lines 523 - 571, Split the oversized contracts
file so session/request contract definitions are separate from
runtime-configuration contracts, keeping each production Go file at or below 500
lines. Move RuntimeConfig and its related runtime symbols into the dedicated
runtime-config file while preserving package visibility and existing references;
leave unrelated contracts in their appropriate file.

Source: Coding guidelines

internal/daemon/run_snapshot_test.go (1)

526-559: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Split the two invariants into separate tests.

This test covers both snapshot builder historical-absence projection and cloneRunJobSummary deep-copy independence; a failure in one obscures the other. Splitting (and using t.Run("Should …") subtests) keeps ownership clear.

As per coding guidelines: "Go tests must use Should … subtest names".

🤖 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_snapshot_test.go` around lines 526 - 559, Split
TestRunSnapshotBuilderPreservesHistoricalSpeedAbsenceAndClonesResolution into
separate tests: one covering historical speed absence from
newRunSnapshotBuilder/applyEvent and assertRunSnapshotSpeed, and another
covering cloneRunJobSummary deep-copy independence. Use t.Run subtests with
names beginning “Should …” so each invariant reports failures independently and
follows the repository’s Go test naming guideline.

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