feat(lab): CL-08 bounded automation with trusted live-route dispatch - #1447
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCL-08 adds default-off automation for protocol and trusted live-route compatibility evidence. It adds policy validation, deterministic planning, bounded persistence, dispatch, scheduling, management APIs, CLI controls, trusted-route wiring, cancellation, recovery, budgets, cooldowns, and regression tests. ChangesCompatibility Lab automation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Server as startServer
participant Scheduler as startLabAutomationScheduler
participant Orchestrator as runLabAutomationTick
participant Dispatch as dispatchLabAutomationRun
participant Executor as createProductionLabRouteExecutor
participant Lifecycle as drainAndShutdown
Server->>Scheduler: start when persisted policy is enabled
Scheduler->>Orchestrator: run planned compatibility ticks
Orchestrator->>Dispatch: execute a claimed run
Dispatch->>Executor: run trusted live-route evidence collection
Lifecycle->>Scheduler: stop scheduler and request shutdown
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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. Comment |
|
✅ Deterministic PR hygiene checks passed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 31
🤖 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 `@devlog/_plan/260807_compatibility_lab/008_cl08_automation.md`:
- Line 443: Change the two “Attributable compatibility result” headings at the
affected locations from level-3 (`###`) to level-2 (`##`), preserving the
heading text and surrounding content to satisfy MD001.
- Around line 673-688: Standardize the budget reason code across the
observability list, Section 7, and the persisted contract used by
src/lab/automation/persistence.ts: choose one canonical value and replace the
other occurrences, ensuring callers can persist the listed reason without
triggering invalid_state.
In `@src/cli/lab.ts`:
- Around line 352-373: Update the `enable` case to preserve the stored
`liveRouteCompatibility` value from `loadLabAutomationPolicy(configDir)` when
neither `--protocol` nor `--live` is provided. Keep explicit flag behavior
unchanged, while avoiding replacement of the previously enabled live layer with
false.
- Around line 367-372: Update the CLI automation enable/disable output near
saveLabAutomationPolicy and buildLabAutomationStatus so it does not use
status.schedulerRunning, which reflects only the CLI module’s local timer.
Report the persisted scheduler intent from the saved policy/configuration
instead, or omit the scheduler-running field from CLI output while preserving
the existing daemon behavior.
In `@src/lab/automation/budgets.ts`:
- Around line 14-27: Prevent budget evidence from being evicted: update
evictOldestTerminal and trimTerminalRuns to receive now from enqueuePlannedRuns
and both trim call sites, and exclude terminal records whose numeric startedAt
remains within the current LAB_AUTOMATION_HARD_MAX.budgetWindowMs. Preserve
existing eviction rules for older records, and document trimTerminalRuns’ keepMs
dependency. Add a focused regression test near the existing budget tests that
fills maxPersistedRuns with recent completions, rolls the window, and verifies
runBudgetRemaining never exceeds the true unspent budget.
In `@src/lab/automation/cooldown.ts`:
- Around line 8-19: Update setCooldown to accept the scheduler-provided now
value, remove expired entries from cooldownUntilByKey before inserting the new
key, and enforce the persistence hard limit by refusing new scheduling when
active entries already reach that limit instead of returning invalid state.
Preserve existing active cooldown entries and use the established persistence
limit symbol.
In `@src/lab/automation/dispatch.ts`:
- Around line 121-124: Update the dispatch flow around persistConformanceResult
and rebuildLabProjection to apply the newly persisted observation incrementally
instead of replaying the full ledger after every run. Add or reuse an
incremental projection-update operation, and make rebuildLabProjection perform a
full replay only when the projection is missing or incompatible; apply this
behavior consistently in both dispatch branches.
In `@src/lab/automation/orchestrator.ts`:
- Around line 258-259: Add a concise comment immediately above the dispatch loop
controlled by maxDispatches in the orchestration flow, explicitly stating that
dispatchLabAutomationRun is awaited sequentially and maxConcurrentRuns
represents the number of sequential dispatches per tick here, not parallel
concurrency. Preserve the existing loop and await behavior unchanged.
- Around line 210-213: Update the cancellation branch in the run orchestration
flow around transitionRun so it calls setCooldown before returning the terminal
cancelled state, using the cancelled code and the same cooldown mechanism as the
other terminal paths. Inspect cooldownForFailure and ensure cancelled resolves
to a positive defined duration rather than the zero default.
- Around line 167-192: Replace the 30-second time-based eviction in
acquireStateLock with ownership-safe locking, preferably an OS lock tied to the
owning descriptor or equivalent fencing. Ensure a paused holder’s lock cannot be
removed or bypassed by another process, and release only the lock owned by the
current descriptor so the mutateLabAutomationState read-modify-write cycle
remains serialized.
In `@src/lab/automation/persistence.ts`:
- Around line 148-182: Update acquireStateLock to prevent stale-lock reclamation
based solely on mtime, using an ownership mechanism that cannot classify a
paused or suspended live owner as dead before deleting its lock. Ensure lock
release verifies that the lock is still owned by the acquiring process before
unlinking it, so a later owner’s lock cannot be removed.
- Around line 247-321: Extend the state-loading flow around normalizeRunRecord
to validate whole-state lifecycle invariants after records are normalized and
before returning state. Reject duplicate runId values, more than one queued or
running record per runKey, and any state-specific timestamp or terminal-field
combinations that violate the lifecycle contract. Ensure violations throw
LabAutomationError with invalid_state so corrupt persistence fails closed.
In `@src/lab/automation/planner.ts`:
- Around line 153-157: Update both planner sites in
src/lab/automation/planner.ts: lines 153-157 in the caseRecord flow and lines
230-234 in planLiveScenarios. Resolve suite-level and scenario-level maxAgeMs
independently, then pass those distinct values to effectiveMaxAgeMs before
freshnessReason; prioritize the planLiveScenarios change to prevent unnecessary
provider requests.
- Around line 199-229: Batch observation reads in the planner instead of calling
latestMatchingObservationCompletedAt inside the nested route/scenario loop. In
the route planning flow around planProtocolScenarios and the live scenario loop,
fetch one projection per (layer, subjectId), reduce results into a lookup keyed
by suite and scenario identity, and reuse it for freshness checks while
preserving deterministic, side-effect-free planning behavior.
- Around line 81-93: Update latestMatchingObservationCompletedAt to apply the
identity’s version and digest predicates plus the relevant exclusion predicate
within the database query before selecting a result. Return the newest usable
observation using completed_at/event_id descending order with LIMIT 1, and keep
this query’s limit independent of LAB_AUTOMATION_HARD_MAX.maxPersistedRuns.
In `@src/lab/automation/policy.ts`:
- Around line 86-100: Update the three concurrency validations in the policy
parser—maxConcurrentRuns, maxConcurrentLiveRuns, and
maxConcurrentRunsPerRoute—to require positive integers rather than non-negative
integers, using the existing positive-integer assertion if available. Leave
maxRunsPerHour and maxLiveRequestsPerHour unchanged so they continue accepting
zero.
In `@src/lab/automation/queue.ts`:
- Around line 164-175: Move the terminal-run retention duration into the
LAB_AUTOMATION_HARD_MAX definition in constants.ts, alongside blockedCooldownMs,
using a named retention setting. Update trimTerminalRuns to read that
centralized value instead of the hard-coded seven-day calculation, keeping
terminal records retained for at least the configured cooldown duration.
In `@src/lab/automation/runs-query.ts`:
- Around line 21-24: Update listLabAutomationRuns so a non-empty cursor absent
from state.runs returns the typed invalid_cursor error instead of resetting
start to zero; preserve normal pagination for known cursors. Map invalid_cursor
to HTTP 400 in the lab automation route handler, and add unknown and evicted
cursor coverage in the lab automation tests. If cursor recovery is needed,
encode and use the (createdAt, runId) sort boundary.
In `@src/lib/lab-live-route-production.ts`:
- Around line 56-58: Update the OAuth error handling around the catch block to
distinguish non-credential token-endpoint failures from missing or expired
credentials: classify network or other harness-side failures as TransportError
code "harness_failure", while preserving "auth_blocked" for credential-related
failures. Keep the thrown message generic and ensure it contains no error
details or token material; update only the relevant classification path in the
surrounding authentication flow.
- Around line 84-102: Move the input.initiatingRequest validation to the
beginning of the executor callback, before createCredentialLease and
buildLabProviderAuthHeaders. Preserve the existing TransportError behavior, and
only create the lease, auth headers, sender, and transport after the guard
succeeds.
- Around line 52-58: In the OAuth branch of the route handling flow, validate
routeContext.baseUrl with URL before calling getValidAccessTokenSnapshot, and
reject any protocol other than https: with the existing auth_blocked
TransportError path. Leave non-OAuth providers unchanged so http: remains
supported there.
In `@src/server/index.ts`:
- Around line 1656-1668: Update src/server/index.ts lines 1656-1668 to release
the dependencies established by startServer from its stop override at lines
1592-1613, preventing another server from redirecting the active scheduler;
update src/lab/automation/orchestrator.ts lines 335-341 so
resetLabAutomationSchedulerStateForTests clears both dispatchDeps and
schedulerTimer. Add the release function beside setLabAutomationDispatchDeps and
invoke it from both lifecycle paths.
In `@src/server/lifecycle.ts`:
- Around line 450-451: Update the shutdown flow containing
requestLabAutomationShutdown and stopLabAutomationScheduler to join in-flight
automation runs before returning. Expose an in-flight count or settle promise
from the automation orchestrator, then await it with the existing
drainAndShutdown deadline, preserving the current call order and bounded-timeout
behavior.
In `@src/server/management/lab-automation-routes.ts`:
- Around line 97-103: Guard the decodeURIComponent call in the
cancelLabAutomationRun route, using the cancelMatch/runId handling to catch
malformed percent-encoding and return a 400 automationErrorResponse for an
invalid run id. Preserve the existing cancellation and 404 behavior for
successfully decoded run IDs.
- Around line 146-180: Validate and normalize both policy and routes payloads
before persisting either update in the PUT handler. Restructure the flow around
loadLabAutomationPolicy, normalizeLabAutomationPolicyV1, and
saveLabAutomationRoutes so route validation runs first, then save the policy and
routes only after both inputs succeed; preserve applySchedulerPolicy after the
writes and keep invalid requests free of partial disk updates.
- Around line 126-136: Decouple the manual-run route’s submission response from
provider dispatch: update enqueueManualLabRun and its orchestration path so the
route receives and returns a queued record immediately, while a guaranteed
background worker performs dispatch. Ensure the background dispatch is started
reliably without awaiting it in the request handler, and preserve the existing
202 response shape for the queued manual run.
In `@tests/lab-automation-review-regressions.test.ts`:
- Around line 414-415: Update the runBudgetRemaining call in the
rollBudgetWindow test to pass the captured now value as its third argument,
ensuring the assertion evaluates the same clock instant used to create rolled.
In `@tests/lab-automation.test.ts`:
- Around line 352-361: Add HTTP-level regression tests in
tests/lab-automation.test.ts alongside the existing management tests for the
missing lab automation routes: verify cancel returns 200 and cancelled:true for
a queued run, returns 404 with code not_found for an unknown id, and handles a
malformed encoded id without leaking URIError; verify run-list limits 0 and 101
return 400 with code invalid_limit and valid pagination returns nextCursor;
verify POST /api/lab/automation/run rejects unknown body keys with 400 and code
invalid_body.
- Around line 337-350: Wrap the “task background execution stays disabled by
default” test body with the existing withHome helper, ensuring OPENCODEX_HOME is
set to the temporary test directory before calling planLabAutomationRuns.
Preserve the current policy, routes, state, and empty-plan assertion.
- Around line 553-571: Update the test “management PUT cannot inject trusted
executor authority” to dispatch a lab automation request after the PUT, rather
than only checking isTrustedLabRouteExecutor on locally created objects. Assert
the dispatch result is route_ineligible with liveRequest: false, matching the
existing contract test, and ensure no trusted executor is installed before
dispatch.
- Around line 356-357: Update the test around handleManagementAPI to explicitly
guard the nullable response before accessing status, asserting that res is
non-null and then checking for status 200. Preserve the existing request and
configuration setup while ensuring unhandled routes produce a clear assertion
failure instead of a TypeError.
🪄 Autofix
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 Plus
Run ID: 7a94e073-aba7-44c4-b0c9-013ec6502b02
📒 Files selected for processing (27)
devlog/_plan/260807_compatibility_lab/008_cl08_automation.mdsrc/cli/lab.tssrc/lab/automation/budgets.tssrc/lab/automation/constants.tssrc/lab/automation/cooldown.tssrc/lab/automation/dispatch.tssrc/lab/automation/index.tssrc/lab/automation/orchestrator.tssrc/lab/automation/persistence.tssrc/lab/automation/planner.tssrc/lab/automation/policy.tssrc/lab/automation/queue.tssrc/lab/automation/recovery.tssrc/lab/automation/route-context.tssrc/lab/automation/run-key.tssrc/lab/automation/runs-query.tssrc/lab/automation/types.tssrc/lab/index.tssrc/lab/live/executor.tssrc/lab/paths.tssrc/lib/lab-live-route-production.tssrc/server/index.tssrc/server/lifecycle.tssrc/server/management-api.tssrc/server/management/lab-automation-routes.tstests/lab-automation-review-regressions.test.tstests/lab-automation.test.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/lab/automation/persistence.ts (1)
348-356: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject
runningrecords withoutstartedAt.Lines 351-353 allow a
runningrecord with nostartedAt.isRollingBudgetEvidencethen ignores the run, while queue selection still counts it as running. A corrupted state file can therefore reduce dispatch capacity until recovery runs.Reject this state with
invalid_state.Proposed fix
- if (state === "running" && (completedAt !== undefined || terminalCode !== undefined)) { + if (state === "running" && (startedAt === undefined || completedAt !== undefined || terminalCode !== undefined)) { throw new LabAutomationError(`running run has terminal fields at ${index}`, "invalid_state"); }🤖 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 `@src/lab/automation/persistence.ts` around lines 348 - 356, Update the running-state validation near the existing lifecycle checks to throw LabAutomationError with code "invalid_state" when state is "running" and startedAt is undefined. Preserve the current validation for completedAt and terminalCode, and keep valid running records with startedAt unchanged.
🤖 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 `@src/lab/automation/cooldown.ts`:
- Around line 17-32: Update the scheduled-run claiming flow and setCooldown so
cooldown capacity is reserved before dispatch: account for active cooldowns plus
scheduled runs already in progress, and refuse claims that would exceed
LAB_AUTOMATION_HARD_MAX.maxPersistedRuns. Release reservations only after
successful terminal results, ensuring multiple failed scheduled runs retain
their cooldowns instead of being silently dropped; add coverage for 511 active
cooldowns and multiple failed scheduled runs.
In `@src/lab/automation/orchestrator.ts`:
- Around line 50-53: Update dispatchDepsFor so it returns only the dependencies
from dispatchDepsByConfigDir.get(configKey(configDir)); remove the fallback
lookup for the default key while preserving the empty-object fallback when no
scoped entry exists.
In `@src/lab/automation/persistence.ts`:
- Around line 186-198: Update the state-lock acquisition flow around
reclaimDeadStateLock to write PID/token metadata to a private temporary file
first, then atomically publish it at stateLockPath using create-if-absent
semantics, so readers only observe complete metadata and existing ownership
checks remain intact. Handle publication races and cleanup of temporary files
without reverting to mtime-based reclamation, and add a crash-injection test
covering termination after acquisition but before publication.
In `@src/lab/automation/planner.ts`:
- Around line 101-126: Bound freshness-index construction in the planner’s
pagination loop around queryLabObservations, preferably by querying only the
exact suite/scenario identities required by the current plan using an indexed
query; otherwise enforce documented retention and scan limits without
sacrificing freshness correctness. Update the relevant freshness lookup flow and
add a regression test with many irrelevant observations that verifies query work
remains bounded.
---
Duplicate comments:
In `@src/lab/automation/persistence.ts`:
- Around line 348-356: Update the running-state validation near the existing
lifecycle checks to throw LabAutomationError with code "invalid_state" when
state is "running" and startedAt is undefined. Preserve the current validation
for completedAt and terminalCode, and keep valid running records with startedAt
unchanged.
🪄 Autofix
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 Plus
Run ID: d7520ea5-89fe-4796-a168-7f6620a84330
📒 Files selected for processing (17)
devlog/_plan/260807_compatibility_lab/008_cl08_automation.mdsrc/cli/lab.tssrc/lab/automation/constants.tssrc/lab/automation/cooldown.tssrc/lab/automation/orchestrator.tssrc/lab/automation/persistence.tssrc/lab/automation/planner.tssrc/lab/automation/policy.tssrc/lab/automation/queue.tssrc/lab/automation/recovery.tssrc/lab/automation/runs-query.tssrc/lib/lab-live-route-production.tssrc/server/management/lab-automation-routes.tstests/lab-automation-coderabbit-regressions.test.tstests/lab-automation-management-http.test.tstests/lab-automation-review-regressions.test.tstests/lab-automation.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lab/automation/orchestrator.ts (1)
414-425: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftPersist cancellation requests for in-flight runs owned by another process.
If a persisted run is already
runningbut its controller belongs to another process,inFlightControllers.get(runId)returns no controller. The function then accepts onlyqueuedruns and returnsfalse. The owning process receives no abort signal, so its provider dispatch continues.Persist a cancellation request under the automation state lock. Make the owning dispatcher observe the request and finalize the run as cancelled. Scope the request by
configDirand run ID. Add a regression with a persisted running run and an empty local controller map.This finding is based on the PR objective for cross-process automation-state serialization.
🤖 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 `@src/lab/automation/orchestrator.ts` around lines 414 - 425, Update the cancellation flow around the inFlightControllers lookup and mutateLabAutomationState so running runs without a local controller persist a cancellation request keyed by configDir and runId under the state lock. Make the owning dispatcher observe that request and finalize the run as cancelled while preserving existing local-controller and queued-run behavior, and add a regression covering a persisted running run with an empty controller map.
🤖 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 `@src/lab/automation/orchestrator.ts`:
- Around line 233-239: Make cancellation backoff durable when the cooldown store
reaches capacity: update setCooldown and the queued and in-flight cancellation
paths around cancellationCooldownUntil so run.runKey is always persisted or
represented by a planner-honored cancellation tombstone until expiry. Add
regression coverage that fills the cooldown map to
LAB_AUTOMATION_HARD_MAX.maxPersistedRuns and verifies both cancellation paths
prevent the run from being re-admitted.
In `@tests/lab-automation-coderabbit-regressions.test.ts`:
- Around line 184-198: Extend the cancellation regression coverage beside the
existing queued test with a focused in-flight scenario that reaches finalizeRun
through an aborted controller. Wait for finalization after invoking
cancelLabAutomationRun, then assert the run is cancelled and
cooldownUntilByKey["key-1"] is at least before +
LAB_AUTOMATION_HARD_MAX.schedulerTickMs.
---
Outside diff comments:
In `@src/lab/automation/orchestrator.ts`:
- Around line 414-425: Update the cancellation flow around the
inFlightControllers lookup and mutateLabAutomationState so running runs without
a local controller persist a cancellation request keyed by configDir and runId
under the state lock. Make the owning dispatcher observe that request and
finalize the run as cancelled while preserving existing local-controller and
queued-run behavior, and add a regression covering a persisted running run with
an empty controller map.
🪄 Autofix
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 Plus
Run ID: c9c21a13-dc3e-4b36-804d-431b3b9ec450
📒 Files selected for processing (2)
src/lab/automation/orchestrator.tstests/lab-automation-coderabbit-regressions.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lab/automation/queue.ts (1)
29-66: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftKeep persisted state within
maxPersistedRuns.
isCancellationBackoffEvidenceprotects every cancelled scheduled run during backoff. If protected records exceedLAB_AUTOMATION_HARD_MAX.maxPersistedRuns,evictOldestTerminalreturnsfalseand Lines 203-205 return an oversized state.Distinct run keys can accumulate during route or manifest changes. State reads and writes then grow beyond the configured hard limit.
Store bounded cancellation suppression separately from terminal history, with expiry and a hard key limit. If that capacity is full, fail scheduled planning closed. Do not return a state that exceeds
maxPersistedRuns.Also applies to: 203-205
🤖 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 `@src/lab/automation/queue.ts` around lines 29 - 66, Bound cancellation suppression independently from persisted terminal history so protected cancelled scheduled runs cannot exceed maxPersistedRuns. Update isCancellationBackoffEvidence and the queue state read/write flow around evictOldestTerminal to use an expiring, hard-limited suppression store; when that capacity is exhausted, fail scheduled planning closed and ensure no returned or persisted state exceeds LAB_AUTOMATION_HARD_MAX.maxPersistedRuns.
🤖 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 `@src/lab/automation/queue.ts`:
- Around line 29-66: Bound cancellation suppression independently from persisted
terminal history so protected cancelled scheduled runs cannot exceed
maxPersistedRuns. Update isCancellationBackoffEvidence and the queue state
read/write flow around evictOldestTerminal to use an expiring, hard-limited
suppression store; when that capacity is exhausted, fail scheduled planning
closed and ensure no returned or persisted state exceeds
LAB_AUTOMATION_HARD_MAX.maxPersistedRuns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 44df6558-5dab-4d1a-b8e4-8eecab4eb089
📒 Files selected for processing (6)
src/lab/automation/planner.tssrc/lab/automation/queue.tssrc/lab/projection/schema.tssrc/lab/query/latest-observation.tstests/lab-automation-coderabbit-regressions.test.tstests/lab-automation-final-coderabbit-regressions.test.ts
flyingsquirrel0419
left a comment
flyingsquirrel0419
left a comment
There was a problem hiding this comment.
I am pausing maintainer approval at exact head 2583e1e72b8e20f023e8c7d48b688f46efbc7c76.
Current dev@849ab5e35fdacddb4d47416c00a315111c78f97e is not an ancestor of this head; the merge base is still da8ebd3135553c1d4dd85c1f258e998a5de14f28. For a 39-file / +6,091-line change that can acquire OAuth credentials and dispatch real provider traffic, I need the trust-boundary review and required CI to be against the current integration base, not the pre-rebase snapshot.
Please rebase onto current dev, keep the resulting diff focused, and let exact-head CI complete. git diff --check origin/dev...2583e1e72 also currently reports four trailing-whitespace lines at the top of devlog/_plan/260807_compatibility_lab/008_cl08_automation.md; please clean those during the rebase. I will resume the credential-destination, authority, persistence-lock, cancellation, and shutdown review on the rebased head. This is not a rejection of the CL-08 direction.
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
I am pausing maintainer approval at exact head 2583e1e72b8e20f023e8c7d48b688f46efbc7c76.
Current dev@849ab5e35fdacddb4d47416c00a315111c78f97e is not an ancestor of this head; the merge base is still da8ebd3135553c1d4dd85c1f258e998a5de14f28. For a 39-file / +6,091-line change that can acquire OAuth credentials and dispatch real provider traffic, I need the trust-boundary review and required CI to be against the current integration base, not the pre-rebase snapshot.
Please rebase onto current dev, keep the resulting diff focused, and let exact-head CI complete. git diff --check origin/dev...2583e1e72 also currently reports four trailing-whitespace lines at the top of devlog/_plan/260807_compatibility_lab/008_cl08_automation.md; please clean those during the rebase. I will resume the credential-destination, authority, persistence-lock, cancellation, and shutdown review on the rebased head. This is not a rejection of the CL-08 direction.
Record bounded orchestration scope, default-off policy, and implementation traceability for feat/cl-08-lab-automation.
Add default-off CL-08 policy, planner, queue, budgets, dispatch seam, scheduler lifecycle, and persistence without provider authority.
Issue production CL-03 executor at server startup, expose management API and CLI, and gate live evidence persistence on trusted_route authority.
Cover trusted dispatch, missing executor fail-closed, budget and shutdown behavior, and management API authority boundaries.
2583e1e to
e533234
Compare
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Requesting changes on exact head e5332346943526b7b833e3fb202aecd2af11425e. The rebase is correct and current dev@849ab5e35 is now an ancestor, but two lifecycle/commit blockers remain for credentialed background execution.
-
src/server/index.ts:1603-1625,1669-1681— the server stop lifecycle does not release the Lab scheduler or its credential executor.startServerregisterssetLabAutomationDispatchDepsand may start a timer for this config root, but theserver.stopoverride releases only the listener, overlay reconciler, background lifecycle, and native-main lifecycle. The keyed map prevents cross-root collisions, but it does not solve ownership for a stopped/restarted server using the same root: the old timer survives, the nextstartServeroverwrites that key with a newloadConfig/routeExecutor, and automation can keep issuing provider requests after the original listener has stopped. Add an owner-scoped registration receipt/release operation, invoke it from the stop override and failed-start path, and stop the keyed timer only when the stopping server still owns it. Add a same-root start/stop/restart regression proving that the stopped owner cannot dispatch or clear a successor. -
src/server/management/lab-automation-routes.ts:189-193— policy and route updates are not one commit. Validation happens before I/O, butsaveLabAutomationRoutescommits one file beforesaveLabAutomationPolicycommits the other. If the second write fails, the endpoint returns 500 while the new route set is already durable. With an existing enabled live policy, the still-running scheduler can then send credentialed probes to routes from an update the operator was told had failed. Persist one versioned combined configuration, or implement an ownership-safe transaction/rollback that leaves the scheduler fail-closed until both files share one committed generation. Add a fault-injection test that fails the second write and proves neither effective policy nor effective routes change and no dispatch occurs.
git diff --check origin/dev...e53323469 also still reports the four trailing-whitespace lines at devlog/_plan/260807_compatibility_lab/008_cl08_automation.md:5-8; please clean them. Exact-head Linux shards and gates are green, while macOS is still running. I am not treating CI as a substitute for these lifecycle guarantees.
3 participants
Summary
849ab5e35fdacddb4d47416c00a315111c78f97ee5332346943526b7b833e3fb202aecd2af11425edevvia feat(lab): CL-07 task effectiveness evidence #1438)Architecture
src/lab/automation/with hard ceilings, cross-process state serialization, and crash recovery.dispatch.ts) to CL-01 / CL-03 only; task-effectiveness background remains disabled./api/lab/automation) and CLI (ocx lab automation …,ocx lab run …).Default-off behavior
automation-policy.jsondefaults toenabled: falseand all layers off.Production trusted live dispatch
src/server/index.tscreatescreateProductionLabRouteExecutor({ configDir, loadConfig })at startup and passes it throughsetLabAutomationDispatchDeps.createHostIssuedLabRouteExecutor) and routes through CL-03 credential lease, pinned transport, and observation normalization (src/lib/lab-live-route-production.ts).route_ineligiblewith zero provider traffic.run_contract_changedwith no live request.executionAuthority === "trusted_route"; cancellation is checked again before persistence.Independent review hardening
The initial draft was independently reviewed before ready-for-review. The correction pass added regressions and fixes for:
automation-state.jsonwrite races;--protocolhandling and trusted live manual dispatch.Scheduled private-network probing still requires both explicit route enrollment in
automation-routes.jsonand the provider's existingallowPrivateNetwork: true; CL-03 destination/SSRF policy remains authoritative.Task-effectiveness background
taskEffectivenessBackgroundEnabledremains false; unattended CL-07 execution is not proven safe. Manual CL-07 paths unchanged.Persistence / security / privacy
Non-goals
Known limitation
live_transportharness feature only (CL08_TRUSTED_LIVE_HARNESS_FEATURES). Scenarios requiring inert tools, MCP stubs, or other harness features are not planned until a broader trusted contract is defined.Verification
Current rebased head
e5332346943526b7b833e3fb202aecd2af11425eondev849ab5e35fdacddb4d47416c00a315111c78f97e:dev.Focused CL-08 coverage includes
tests/lab-automation.test.ts,tests/lab-automation-review-regressions.test.ts, and the CodeRabbit regression suites.Checklist
Do not merge yet.
Summary by CodeRabbit