refactor(arch): executor RequestPreparer extraction + route-pref characterization test (Phase 4)

[matdev83]

Summary

• Completes the remaining Phase 4 tasks from the architecture review resolution plan:
  • Task 4.1 gap (route-preference characterization test): New test locks the planning-side effect of execctx.RouteCandidatePreferences at the Execute boundary — the 4th and final characterization gap. All 4 characterization test gaps are now filled.
  • Task 4.3 (RequestPreparer extraction): Extracts Execute phases 1-9 (validation, snapshot binding, secure-session begin-turn, tracing span, submit/A-leg, lifecycle start, exec views, secure turn, route pref clone — 62 lines) into executor_prepare_request.go as prepareRequest returning a preparedRequest struct + cleanup function. Execute now delegates preparation and reads as: prepareRequest -> parse selector -> plan route -> attempt loop (tryPlanOpenOnce) -> B-leg register -> retryRecvStream assembly -> interleaved wrappers.
  • Task 4.7 (budget tightening): executor.go budget tightened from 480 to 380 (current 355 lines, down from 416).
• Task 4.2 (field grouping): Attempted but reverted — Go struct embedding breaks struct literal initialization in 47 test files (promoted fields cannot be used in composite literals). Deferred to a future builder-pattern PR.
• Tasks 4.4-4.6 (further RoutePlanner/AttemptOpener/StreamAssembler extraction): The remaining Execute body is ~100 lines and already well-factored via tryPlanOpenOnce and helper methods. Further extraction is mechanical but lower-value; deferred.

Test plan

• go test -count=1 ./internal/core/runtime/ — all tests pass (including 4 characterization tests)
• make test-precommit-extra (executor matrix: precommit-tagged tests in internal/core/runtime/ + internal/qa/)
• make quality-checks (gofmt, tidy, build, vet, goroutine allowlist, regex hotpath, archtest incl. tightened executor.go budget 355 <= 380)
• make test-unit (zero failing packages)
• go run ./cmd/lipstd check-config — configuration valid
• lipstd serve smoke: GET /v1/responses -> 405 + X-Trace-Id — confirms the Execute path (prepareRequest -> routing -> attempt -> stream) works end-to-end
• Reviewer: confirm Execute signature stable; error wrapping preserved; no behavior change

[coderabbitai]

📝 Walkthrough

Summary by CodeRabbit

• Bug Fixes
  • Improved runtime execution flow for more reliable setup/teardown and more consistent tracing behavior.
  • Updated planning so route-candidate preferences are honored when selecting which backend to open.
  • Refreshed architecture “critical file” guardrails to better match the current runtime scope.
• Tests
  • Added coverage to confirm the planner chooses the preferred routing candidate and avoids opening non-preferred options.

Walkthrough

Refactors Executor.Execute to delegate request setup and teardown to a new prepareRequest flow, rewires routing and planning to use prep-scoped state, and adds a route-preference test while lowering the executor.go critical-file budget.

Changes

Executor request preparation extraction

Layer / File(s)	Summary
preparedRequest and prepareRequest/finalize internal/core/runtime/executor_prepare_request.go	Defines preparedRequest fields and implements prepareRequest validation, secure-session initialization, span start, A-leg scope start, cleanup closure, and finalize error recording.
Execute control flow and prep-scoped routing internal/core/runtime/executor.go	Rewrites Execute to call prepareRequest, defer finalize and cleanup, and source selector setup, routing state, attempt planning, B-leg registration, and retry stream construction from prep fields.
Route preference test and critical-file budget internal/core/runtime/executor_route_preference_test.go, internal/archtest/critical_files.go	Adds a test verifying route candidate preferences influence backend selection and updates the executor.go critical-file budget to 380.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

• matdev83/go-llm-interactive-proxy#108: Extends the same internal/archtest/* critical-file budget enforcement updated here.

🚥 Pre-merge checks | ✅ 8

✅ Passed checks (8 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the refactor, request-preparer extraction, route-preference test, and Phase 4 scope.
Description check	✅ Passed	The description is clearly about the executor refactor and added characterization test, so it is related to the changeset.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
No Secrets	✅ Passed	Changed files only add refactoring/test/budget text; no hardcoded credentials, keys, passwords, private keys, or sensitive URLs found.
Context Propagation	✅ Passed	PASS: prepareRequest threads the caller ctx through planning/opening, checks prep.ctx/aScope cancellation in the loop, and uses detached time-bounded cleanup; no new goroutines were added.
No Accidental Public Api Break	✅ Passed	No public API break: no pkg/** changes, exported signatures stayed the same, and the refactor only moved logic into internal helpers plus an internal archtest budget update.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with 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.

Inline comments:
In `@internal/core/runtime/executor_prepare_request.go`:
- Around line 78-82: Record prepareSubmitAndALeg failures on the request span
before ending it in executor_prepare_request.go. In prepareExecutorRequest, when
the prepareSubmitAndALeg call returns an error, make sure the prep.execSpan is
updated with RecordError and SetStatus (or routed through finalize(err)) before
execSpan.End so the lip.executor.execute telemetry still captures the failure.
Keep the existing prepareSubmitAndALeg and finalize(err) flow aligned so this
error path is not dropped.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

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

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: afd1db11-8b9d-4797-8422-96f17f83b69a

📥 Commits

Reviewing files that changed from the base of the PR and between 58595e6 and e0583be.

📒 Files selected for processing (4)

• internal/archtest/critical_files.go
• internal/core/runtime/executor.go
• internal/core/runtime/executor_prepare_request.go
• internal/core/runtime/executor_route_preference_test.go

📜 Review details

⏰ Context from checks skipped due to timeout. (1)

• GitHub Check: qa

🧰 Additional context used

📓 Path-based instructions (4)

**/*.go

📄 CodeRabbit inference engine (Custom checks)

**/*.go: For server, CLI, worker, or network Go code, ensure context.Context is propagated correctly, cancellation is respected, and new goroutines cannot leak indefinitely.
Do not make accidental public API breaks in Go code: under pkg/** or anywhere exported Go identifiers are changed, warn if the PR changes exported types, function signatures, error behavior, JSON fields, CLI flags, config keys, or documented behavior without clearly explaining the compatibility impact.

Files:

• internal/core/runtime/executor_route_preference_test.go
• internal/archtest/critical_files.go
• internal/core/runtime/executor_prepare_request.go
• internal/core/runtime/executor.go

---

⚙️ CodeRabbit configuration file

**/*.go: Review as production Go code. Prioritize correctness, race conditions, goroutine leaks, context cancellation, timeout handling, error wrapping, nil-pointer risks, resource cleanup, defer placement, API compatibility, interface design, dependency boundaries, and testability. Avoid generic style comments when gofmt/golangci-lint already covers the issue.

Files:

• internal/core/runtime/executor_route_preference_test.go
• internal/archtest/critical_files.go
• internal/core/runtime/executor_prepare_request.go
• internal/core/runtime/executor.go

**/*

📄 CodeRabbit inference engine (Custom checks)

Do not introduce hardcoded credentials, API keys, tokens, private keys, passwords, production secrets, or sensitive internal URLs.

Files:

• internal/core/runtime/executor_route_preference_test.go
• internal/archtest/critical_files.go
• internal/core/runtime/executor_prepare_request.go
• internal/core/runtime/executor.go

internal/**

⚙️ CodeRabbit configuration file

internal/**: Focus on package boundaries, hidden coupling, unexported API design, concurrency safety, deterministic behavior, and whether logic belongs in this internal package.

Files:

• internal/core/runtime/executor_route_preference_test.go
• internal/archtest/critical_files.go
• internal/core/runtime/executor_prepare_request.go
• internal/core/runtime/executor.go

**/*_test.go

⚙️ CodeRabbit configuration file

**/*_test.go: Review tests for meaningful assertions, table-driven coverage, race-prone tests, t.Parallel misuse, nondeterminism, leaked goroutines, real network or filesystem dependencies, fragile sleeps, and missing edge cases. Prefer testing observable behavior over implementation details.

Files:

• internal/core/runtime/executor_route_preference_test.go

🧠 Learnings (1)

📚 Learning: 2026-07-01T22:57:42.953Z

Learnt from: matdev83
Repo: matdev83/go-llm-interactive-proxy PR: 101
File: pkg/lipsdk/scope/context.go:0-0
Timestamp: 2026-07-01T22:57:42.953Z
Learning: In this repository, when defining unexported Go `context` key constants of type `ctxKey int` (e.g., `const ( kFoo ctxKey = iota + N )`), preserve the `iota + <offset>` pattern and keep the existing `<offset>` values rather than simplifying to plain `iota`. These per-package offsets are part of the repo-wide convention to avoid key collisions across packages, and each such constant set should include a short explanatory comment (for example: “offset avoids collision with other packages' context keys”).

Applied to files:

• internal/core/runtime/executor_route_preference_test.go
• internal/archtest/critical_files.go
• internal/core/runtime/executor_prepare_request.go
• internal/core/runtime/executor.go

🔇 Additional comments (6)

internal/core/runtime/executor_route_preference_test.go (1)

21-75: LGTM!

internal/archtest/critical_files.go (1)

39-49: LGTM!

internal/core/runtime/executor_prepare_request.go (1)

45-75: LGTM!

Also applies to: 84-118

internal/core/runtime/executor.go (3)

265-293: LGTM!

---

302-338: LGTM!

---

225-232: 🩺 Stability & Availability

cleanup() already sees streamReturned updates
prepareRequest returns &prep, so the cleanup closure and Execute share the same *preparedRequest instance; streamReturned is visible to cleanup().

			> Likely an incorrect or invalid review comment.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

internal/core/runtime/executor_route_preference_test.go (1)

61-68: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Add a timeout to the test context.

Both ex.Execute and lipapi.Collect run with context.Background()/an untimed derived context, with no deadline. If planning or streaming ever regresses to a hang, this test blocks indefinitely instead of failing fast in CI.

🕐 Suggested fix

-	ctx := execctx.WithRouteCandidatePreferences(context.Background(), []string{"preferred"})
+	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+	defer cancel()
+	ctx = execctx.WithRouteCandidatePreferences(ctx, []string{"preferred"})
 	stream, err := ex.Execute(ctx, call)
 	if err != nil {
 		t.Fatal(err)
 	}
-	if _, err := lipapi.Collect(context.Background(), stream); err != nil {
+	if _, err := lipapi.Collect(ctx, stream); err != nil {
 		t.Fatalf("Collect: %v", err)
 	}

As per path instructions, tests should be reviewed for nondeterminism and hang risk; "Review tests for meaningful assertions, table-driven coverage, race-prone tests, t.Parallel misuse, nondeterminism, leaked goroutines, real network or filesystem dependencies, fragile sleeps, and missing edge 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/runtime/executor_route_preference_test.go` around lines 61 -
68, Add a bounded timeout to the test in executor_route_preference_test by
creating a timed context before calling ex.Execute and reusing it for
lipapi.Collect instead of context.Background(); this keeps the test from hanging
indefinitely if planning or streaming stalls. Update the test body around
execctx.WithRouteCandidatePreferences, ex.Execute, and lipapi.Collect so both
operations share the same deadline-bearing context.

Source: Path instructions

🤖 Prompt for all review comments with 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.

Outside diff comments:
In `@internal/core/runtime/executor_route_preference_test.go`:
- Around line 61-68: Add a bounded timeout to the test in
executor_route_preference_test by creating a timed context before calling
ex.Execute and reusing it for lipapi.Collect instead of context.Background();
this keeps the test from hanging indefinitely if planning or streaming stalls.
Update the test body around execctx.WithRouteCandidatePreferences, ex.Execute,
and lipapi.Collect so both operations share the same deadline-bearing context.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 640ecff5-c3b3-40b0-a68e-f54e59dc9c74

📥 Commits

Reviewing files that changed from the base of the PR and between e0583be and 68e7a1b.

📒 Files selected for processing (2)

• internal/core/runtime/executor_prepare_request.go
• internal/core/runtime/executor_route_preference_test.go

📜 Review details

⏰ Context from checks skipped due to timeout. (1)

• GitHub Check: qa

🧰 Additional context used

📓 Path-based instructions (4)

**/*.go

📄 CodeRabbit inference engine (Custom checks)

**/*.go: For server, CLI, worker, or network Go code, ensure context.Context is propagated correctly, cancellation is respected, and new goroutines cannot leak indefinitely.
Do not make accidental public API breaks in Go code: under pkg/** or anywhere exported Go identifiers are changed, warn if the PR changes exported types, function signatures, error behavior, JSON fields, CLI flags, config keys, or documented behavior without clearly explaining the compatibility impact.

Files:

• internal/core/runtime/executor_route_preference_test.go
• internal/core/runtime/executor_prepare_request.go

---

⚙️ CodeRabbit configuration file

**/*.go: Review as production Go code. Prioritize correctness, race conditions, goroutine leaks, context cancellation, timeout handling, error wrapping, nil-pointer risks, resource cleanup, defer placement, API compatibility, interface design, dependency boundaries, and testability. Avoid generic style comments when gofmt/golangci-lint already covers the issue.

Files:

• internal/core/runtime/executor_route_preference_test.go
• internal/core/runtime/executor_prepare_request.go

**/*

📄 CodeRabbit inference engine (Custom checks)

Do not introduce hardcoded credentials, API keys, tokens, private keys, passwords, production secrets, or sensitive internal URLs.

Files:

• internal/core/runtime/executor_route_preference_test.go
• internal/core/runtime/executor_prepare_request.go

internal/**

⚙️ CodeRabbit configuration file

internal/**: Focus on package boundaries, hidden coupling, unexported API design, concurrency safety, deterministic behavior, and whether logic belongs in this internal package.

Files:

• internal/core/runtime/executor_route_preference_test.go
• internal/core/runtime/executor_prepare_request.go

**/*_test.go

⚙️ CodeRabbit configuration file

**/*_test.go: Review tests for meaningful assertions, table-driven coverage, race-prone tests, t.Parallel misuse, nondeterminism, leaked goroutines, real network or filesystem dependencies, fragile sleeps, and missing edge cases. Prefer testing observable behavior over implementation details.

Files:

• internal/core/runtime/executor_route_preference_test.go

🧠 Learnings (1)

📚 Learning: 2026-07-01T22:57:42.953Z

Learnt from: matdev83
Repo: matdev83/go-llm-interactive-proxy PR: 101
File: pkg/lipsdk/scope/context.go:0-0
Timestamp: 2026-07-01T22:57:42.953Z
Learning: In this repository, when defining unexported Go `context` key constants of type `ctxKey int` (e.g., `const ( kFoo ctxKey = iota + N )`), preserve the `iota + <offset>` pattern and keep the existing `<offset>` values rather than simplifying to plain `iota`. These per-package offsets are part of the repo-wide convention to avoid key collisions across packages, and each such constant set should include a short explanatory comment (for example: “offset avoids collision with other packages' context keys”).

Applied to files:

• internal/core/runtime/executor_route_preference_test.go
• internal/core/runtime/executor_prepare_request.go

🔇 Additional comments (3)

internal/core/runtime/executor_prepare_request.go (1)

80-84: LGTM!

internal/core/runtime/executor_route_preference_test.go (2)

21-59: LGTM!

---

69-75: LGTM!