-
Notifications
You must be signed in to change notification settings - Fork 746
refactor(core): keep Compatibility Lab off the request path of users who never opted in #1681
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
+3,052
−145
Merged
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
37084c2
docs(devlog): Lab/core decoupling roadmap + owner-only core CODEOWNERS
lidge-jun 9d979f6
docs(devlog): audit rounds 2-3 — remove the activation window entirely
lidge-jun 199c19f
docs(devlog): audit round 3 close-out — GO-WITH-FIXES, no High blockers
lidge-jun 8f6908b
refactor(server): break the Lab import cycle at the shutdown path
lidge-jun 00a345b
fix(lab): register scheduler teardown where the scheduler is started
lidge-jun 72aa7fb
test(lab): cover the reviewer-reproduced shutdown cases at outcome level
lidge-jun db315a9
test(lab): pin the two-key scheduler hook interaction
lidge-jun 8babf7d
refactor(responses): move Lab route linkage off the per-request path
lidge-jun 6f0315b
docs(devlog): record WP1 verification evidence and the full-suite bas…
lidge-jun 6832333
refactor(routing): complete the Lab/core boundary with a provider slot
lidge-jun 7fb5793
test(boundary): close a real hole in the guard and pin it against attack
lidge-jun 2e2cb00
fix(lab): an invalid automation config must not take startup down
lidge-jun 41061b2
fix(lab): distinguish a busy state lock from an invalid automation co…
lidge-jun 0f94c68
docs(agents): record the optional-subsystem boundary invariant
lidge-jun c33a507
docs(devlog): record the sharded verification for phase 3
lidge-jun 716e91a
docs(devlog): record PR #1681 and the CI result
lidge-jun File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,138 @@ | ||
| # 000 — Plan: enforce the Compatibility Lab / proxy-core boundary | ||
|
|
||
| Unit: `260814_lab_core_decoupling` | ||
| Baseline: `dev` @ `ff674b8b7fd7905078f42b0f258d447cc785e8c2` | ||
| Owner directive: 2026-08-14. Feature work on the CL line is frozen; CL-10 PRs | ||
| [#1628](https://github.com/lidge-jun/opencodex/pull/1628) and | ||
| [#1510](https://github.com/lidge-jun/opencodex/pull/1510) are closed pending this boundary. | ||
|
|
||
| ## The defect | ||
|
|
||
| opencodex is a provider proxy. A user who configures one provider and one model — no | ||
| routing profile, no Compatibility Lab, no evidence collection — currently executes Lab | ||
| code on every request and loads the entire Lab module graph at startup. | ||
|
|
||
| This is not a performance regression. Measured RSS on a live proxy is ~48 MB and the | ||
| per-request Lab call is pure computation over the config object plus one 32-byte salt | ||
| read. The defect is **architectural**: an optional subsystem sits on the mandatory path, | ||
| with no configuration surface to decline it. Every contributor reading `responses/core.ts` | ||
| must now reason about Lab, and every user ships it. | ||
|
|
||
| ## Verified coupling (read, not assumed) | ||
|
|
||
| | # | Location | Runs when | Gate today | | ||
| |---|---|---|---| | ||
| | 1 | `src/server/responses/core.ts:36` → call at `1997-2009` | **every request attempt**, streaming and non-streaming, once per attempt; once per combo child | none | | ||
| | 2 | `src/router.ts:34` → call at `524` | `policy/` routes only | runtime-gated, import unconditional | | ||
| | 3 | `src/server/index.ts:48-53` → block at `1738-1750` | every server start | `enabled:false` skips the scheduler only | | ||
| | 4 | `src/server/lifecycle.ts:10` → calls at `455-456` | every shutdown | none | | ||
| | 5 | `src/usage/log.ts:50` `labRouteSubjectId`, validator at `214`, persisted at `297` | every attempt log write | none | | ||
| | 6 | `src/server/management/lab-routes.ts`, `lab-automation-routes.ts` | management API mount | none | | ||
|
|
||
| ### The import cycle is the real finding | ||
|
|
||
| `assemble.ts` does not reach Lab through anything resembling compatibility logic. It | ||
| reaches Lab through the **quota chain, and back out through `lifecycle.ts`**: | ||
|
|
||
| ``` | ||
| routing/compatibility/assemble.ts:7 → routing/quota.ts:21 | ||
| → providers/quota.ts:6 → codex/auth-api.ts | ||
| → codex/native-main-admission.ts:2 → server/lifecycle.ts:10 | ||
| → lab/automation/orchestrator.ts | ||
| ``` | ||
|
|
||
| `src/lib/lab-live-route-production.ts:11` reaches the identical cycle through | ||
| `oauth/index.ts:28 → oauth/health.ts:2 → oauth/anthropic-routing.ts:19 → providers/quota.ts`. | ||
|
|
||
| Both entry points therefore pull in **~69 `src/lab/` runtime files**. Coupling point 4 — | ||
| one line in `lifecycle.ts` — is what closes the loop. Cutting it is the highest-leverage | ||
| single edit in this unit, and it is why phase ordering below starts there rather than at | ||
| the most visible symptom. | ||
|
|
||
| ### What the per-request hook actually feeds | ||
|
|
||
| `labRouteSubjectId` is not dead weight. It is written to `usage.jsonl` and read by | ||
| `src/lab/query/passive-production.ts:82`, surfaced through | ||
| `GET /api/lab/production-signals` (`lab-routes.ts:198`), `ocx lab production-signals` | ||
| (`cli/lab.ts:241`), and the Compatibility Matrix GUI | ||
| (`compatibility-matrix-api.ts:238`, `CompatibilityMatrix.tsx:194`). | ||
|
|
||
| So deletion is not free: it retires a shipped, user-visible read surface. Gating keeps | ||
| the feature for installs that opted in. **This unit gates; it does not delete.** Scope | ||
| boundary for anyone extending this work: removing CL-09 entirely is a separate decision | ||
| with its own user-facing deprecation, not a side effect of a boundary fix. | ||
|
|
||
| ### The routing constraint that shapes everything | ||
|
|
||
| `routeModelInternal` (`src/router.ts:504`) is **synchronous**, and so are its public | ||
| wrappers `routeModel` (`654`) and `routeConcreteModel` (`684`). A dynamic `import()` | ||
| inside it is impossible without making the chain async, which would touch ~11 production | ||
| call expressions, ~272 test call sites, and — the actual blocker — the synchronous | ||
| subagent-fallback API in `src/codex/subagent-model-fallback.ts` (`tryRouteFallbackModel:63` | ||
| feeding `isNativeModelQuotaExhausted:200`, `isModelHealthBlocked:216`, | ||
| `isSubagentModelUnavailable:234`, `selectAvailableSubagentModel:269`, | ||
| `noteSubagentModelFailure:298`, `applySubagentModelFallback:510`). | ||
|
|
||
| Making routing async to remove a Lab import would be a far larger and riskier change than | ||
| the problem justifies. **Routing stays synchronous.** The boundary is drawn with a | ||
| provider-registration seam instead. | ||
|
|
||
| ## Design: registration, not dynamic import | ||
|
|
||
| The core already owns the exact pattern needed — `src/lib/server-resource-ownership.ts`, | ||
| `registerCurrentServerResourceCleanup` — and Lab already consumes it at | ||
| `lab/automation/orchestrator.ts:104`. This unit generalizes that idea rather than | ||
| inventing a mechanism: | ||
|
|
||
| - Core declares a **slot** (a nullable function reference) for each optional Lab capability. | ||
| - Core calls the slot when populated, and skips when null. No `import` of Lab anywhere. | ||
| - Lab **registers into** the slot during an explicit activation step. | ||
| - Activation runs only when the install actually has a routing profile / enabled automation. | ||
|
|
||
| A profile-less install therefore never activates, never registers, never loads Lab — | ||
| and every core call site is a null check on the mandatory path. | ||
|
|
||
| ## Phase map (dependency-ordered) | ||
|
|
||
| Ordered so each phase consumes the previous phase's verified output. Not effort-ordered. | ||
|
|
||
| | Phase | Doc | Delivers | Depends on | | ||
| |---|---|---|---| | ||
| | 1 | [`010`](./010_lifecycle_shutdown_registry.md) | Break the import cycle at `lifecycle.ts` via a shutdown-hook registry | — | | ||
| | 2 | [`020`](./020_request_path_gate.md) | Remove Lab from the per-request path; register the passive linker | 1 | | ||
| | 3 | [`030`](./030_router_and_startup_activation.md) | Policy-evidence slot + lazy Lab activation at startup | 1, 2 | | ||
| | 4 | [`040`](./040_boundary_guard_test.md) | Executable boundary guard so the property cannot silently regress | 1–3 | | ||
| | 5 | [`050`](./050_governance_and_release.md) | CODEOWNERS/branch protection, verification on `lidge`, PR, release | 1–4 | | ||
|
|
||
| Phase 1 first because it closes the cycle that makes phases 2 and 3 leaky: while | ||
| `lifecycle.ts` statically imports the orchestrator, any module reaching `lifecycle.ts` | ||
| still drags Lab in regardless of what phases 2–3 do. | ||
|
|
||
| ## Scope | ||
|
|
||
| **IN:** `src/server/lifecycle.ts`, `src/server/responses/core.ts`, `src/router.ts`, | ||
| `src/server/index.ts`, `src/routing/compatibility/*`, a new core-owned optional-capability | ||
| module, `tests/` regressions, `.github/CODEOWNERS`, branch protection, this devlog unit. | ||
|
|
||
| **OUT:** deleting `src/lab`, removing routing-profile or Compatibility Matrix features, | ||
| retiring CL-09 as a product surface, provider/adapter changes, GUI redesign, and every | ||
| open PR from other contributors. | ||
|
|
||
| ## Accept criteria | ||
|
|
||
| 1. `rg` over `src/router.ts`, `src/server/index.ts`, `src/server/responses/core.ts`, | ||
| `src/server/lifecycle.ts` returns **no** static `lab/` or `routing/compatibility/` import. | ||
| 2. A request served from a config with zero routing profiles executes no Lab code — | ||
| proven by an executable test, not by inspection. | ||
| 3. Routing-profile installs keep candidate compatibility evidence and CL-09 passive | ||
| signals working. | ||
| 4. `bun x tsc --noEmit` exits 0. | ||
| 5. Full suite green on the remote Linux runner `lidge`. | ||
| 6. Core-file changes require owner approval going forward. | ||
|
|
||
| ## Verification | ||
|
|
||
| Local: focused `bun test` per phase, then `bun x tsc --noEmit`. | ||
| Remote: full suite on `lidge` (`~/.bun/bin/bun`, Bun 1.3.14, Ubuntu). The local pre-push | ||
| hook (`.git/hooks/pre-push` → `bun run prepush`) is bypassed with `--no-verify` because | ||
| the authoritative suite run happens on `lidge`. | ||
176 changes: 176 additions & 0 deletions
176
devlog/_plan/260814_lab_core_decoupling/010_lifecycle_shutdown_registry.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,176 @@ | ||
| # 010 — Phase 1: break the import cycle at `lifecycle.ts` | ||
|
|
||
| Unit: `260814_lab_core_decoupling`. Depends on: nothing. Blocks: phases 2–4. | ||
|
|
||
| ## Why this is first | ||
|
|
||
| `src/server/lifecycle.ts:10` is the edge that closes the cycle documented in | ||
| [`000_plan.md`](./000_plan.md). While it stands, any module that transitively reaches | ||
| `lifecycle.ts` — including `routing/compatibility/assemble.ts` by way of the quota chain — | ||
| drags ~69 `src/lab/` files into the graph no matter what phases 2 and 3 do to their own | ||
| imports. Removing this one import is the highest-leverage edit in the unit. | ||
|
|
||
| A dynamic `await import()` here would compile (the function is already async) but is the | ||
| wrong instrument: it would still load Lab during the shutdown of a process that never | ||
| activated Lab, and it would do so inside a deadline-bounded drain | ||
| (`drainAndShutdown` computes `deadline` at line 414 and reaches Lab cleanup at 455). | ||
| Loading a 69-file graph at that point is exactly when it is least affordable. | ||
|
|
||
| ## Design | ||
|
|
||
| A core-owned shutdown-hook registry. Core never names Lab; Lab registers itself when it | ||
| activates. This mirrors `src/lib/server-resource-ownership.ts`, which Lab already uses at | ||
| `lab/automation/orchestrator.ts:106`. | ||
|
|
||
| ## NEW: `src/lib/optional-shutdown-hooks.ts` | ||
|
|
||
| ```ts | ||
| /** | ||
| * Core-owned registry for optional-subsystem shutdown work. | ||
| * | ||
| * The proxy core must not import optional subsystems (Compatibility Lab and anything | ||
| * added later) merely to be able to stop them. A subsystem registers its teardown when | ||
| * it activates; a process that never activates it registers nothing, and shutdown does | ||
| * no work and loads no module. | ||
| * | ||
| * Hooks are synchronous and best-effort by contract: shutdown runs under an absolute | ||
| * deadline, so a hook that throws must not prevent its siblings or `server.stop` from | ||
| * running. | ||
| */ | ||
|
|
||
| type ShutdownHook = () => void; | ||
|
|
||
| const hooks = new Map<string, ShutdownHook>(); | ||
|
|
||
| /** | ||
| * Register (or replace) the teardown for one optional subsystem. | ||
| * Keyed so repeated activation of the same subsystem cannot accumulate duplicates. | ||
| * Returns a detach function for owner-scoped release. | ||
| */ | ||
| export function registerOptionalShutdownHook(key: string, hook: ShutdownHook): () => void { | ||
| hooks.set(key, hook); | ||
| return () => { | ||
| if (hooks.get(key) === hook) hooks.delete(key); | ||
| }; | ||
| } | ||
|
|
||
| /** Run every registered teardown. Never throws. */ | ||
| export function runOptionalShutdownHooks(): void { | ||
| for (const [key, hook] of [...hooks]) { | ||
| try { | ||
| hook(); | ||
| } catch (err) { | ||
| console.warn( | ||
| `[shutdown] optional subsystem "${key}" teardown failed:`, | ||
| err instanceof Error ? err.message : err, | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** Test-only reset so isolated lifecycle tests do not inherit registrations. */ | ||
| export function resetOptionalShutdownHooksForTests(): void { | ||
| hooks.clear(); | ||
| } | ||
| ``` | ||
|
|
||
| ## MODIFY: `src/server/lifecycle.ts` | ||
|
|
||
| Line 10 — remove the Lab import, add the registry import: | ||
|
|
||
| ```diff | ||
| -import { stopLabAutomationScheduler, requestLabAutomationShutdown } from "../lab/automation/orchestrator"; | ||
| +import { runOptionalShutdownHooks } from "../lib/optional-shutdown-hooks"; | ||
| ``` | ||
|
|
||
| Lines 454-456 inside `drainAndShutdown` — replace the two direct calls: | ||
|
|
||
| ```diff | ||
| stopStorageCleanupScheduler(); | ||
| - requestLabAutomationShutdown(); | ||
| - stopLabAutomationScheduler(); | ||
| + // Optional subsystems (Compatibility Lab, and anything added later) tear themselves | ||
| + // down through hooks they registered at activation. A process that never activated | ||
| + // one runs nothing here and never loads its module graph. | ||
| + runOptionalShutdownHooks(); | ||
| stopStateStoreSweeper(); | ||
| ``` | ||
|
|
||
| ## MODIFY: `src/lab/automation/orchestrator.ts` | ||
|
|
||
| `setLabAutomationDispatchDeps` (line 79) already owns activation-scoped lifetime and | ||
| already registers a server-resource cleanup at line 106. Register the shutdown hook in the | ||
| same place, so activation and teardown registration cannot drift apart. | ||
|
|
||
| Add to imports: | ||
|
|
||
| ```diff | ||
| import { registerCurrentServerResourceCleanup } from "../../lib/server-resource-ownership"; | ||
| +import { registerOptionalShutdownHook } from "../../lib/optional-shutdown-hooks"; | ||
| ``` | ||
|
|
||
| Inside `setLabAutomationDispatchDeps`, extend the existing lease wiring: | ||
|
|
||
| ```diff | ||
| let released = false; | ||
| let detachServerCleanup = () => {}; | ||
| + let detachShutdownHook = () => {}; | ||
| const release = () => { | ||
| if (released) return; | ||
| released = true; | ||
| detachServerCleanup(); | ||
| + detachShutdownHook(); | ||
| const current = dispatchDepsByConfigDir.get(key); | ||
| if (current?.token !== token) return; | ||
| dispatchDepsByConfigDir.delete(key); | ||
| const scheduler = schedulerTimers.get(key); | ||
| if (scheduler?.ownerToken === token) { | ||
| clearInterval(scheduler.timer); | ||
| schedulerTimers.delete(key); | ||
| } | ||
| }; | ||
| detachServerCleanup = registerCurrentServerResourceCleanup(release); | ||
| + // Shutdown teardown is registered here, at activation, so lifecycle.ts never has to | ||
| + // import Lab to be able to stop it. | ||
| + detachShutdownHook = registerOptionalShutdownHook(`lab-automation:${key}`, () => { | ||
| + requestLabAutomationShutdown(); | ||
| + stopLabAutomationScheduler(deps.configDir); | ||
| + }); | ||
| return release; | ||
| } | ||
| ``` | ||
|
|
||
| Both functions are already defined in this module, so no new import is needed for them. | ||
|
|
||
| ## Behavioral equivalence | ||
|
|
||
| | Before | After | | ||
| |---|---| | ||
| | `requestLabAutomationShutdown()` on every shutdown | runs only if Lab automation was activated | | ||
| | `stopLabAutomationScheduler()` process-wide (no arg) | scoped to the activated `configDir` | | ||
| | Lab loaded on every shutdown | Lab loaded only if already activated | | ||
|
|
||
| The scoping change is a deliberate correction, not a regression: the previous call passed | ||
| no `configDir` and therefore keyed on the default, while `setLabAutomationDispatchDeps` | ||
| is explicitly per-`configDir`. Multi-config test processes were the case where these | ||
| disagreed. | ||
|
|
||
| ## Tests | ||
|
|
||
| NEW `tests/optional-shutdown-hooks.test.ts`: | ||
|
|
||
| 1. `runOptionalShutdownHooks()` with nothing registered is a no-op and does not throw. | ||
| 2. A registered hook runs exactly once per invocation. | ||
| 3. Re-registering the same key replaces rather than accumulates. | ||
| 4. A throwing hook does not prevent a sibling hook from running. | ||
| 5. The detach function removes the hook; a stale detach after replacement is inert. | ||
|
|
||
| MODIFY existing lab automation lifecycle tests: assert the scheduler is stopped after | ||
| `drainAndShutdown` when automation was activated — the outcome, not the direct call. | ||
|
|
||
| ## Accept criteria | ||
|
|
||
| - `rg -n "lab/" src/server/lifecycle.ts` returns nothing. | ||
| - Activated automation is still stopped by `drainAndShutdown`. | ||
| - Shutdown for a never-activated process performs no Lab work. | ||
| - `bun x tsc --noEmit` exits 0. |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align all boundary and ownership artifacts with the final three-file contract. The final design excludes
src/server/index.tsfrom the protected core because it is the composition root with synchronous gated activation, but the plan, guard documentation, verification evidence, andCODEOWNERSstill either treat it as protected or include it without explaining the exception. Update these artifacts consistently: mark the original rule as superseded, scope the guard claims to the three protected modules, and either remove/src/server/index.tsfromCODEOWNERSor document its intentional separate approval requirement.📍 Affects 3 files
devlog/_plan/260814_lab_core_decoupling/000_plan.md#L11-L13(this comment)devlog/_plan/260814_lab_core_decoupling/040_boundary_guard_test.md#L22-L27.github/CODEOWNERS#L35-L46devlog/_plan/260814_lab_core_decoupling/000_plan.md#L87-L93🤖 Prompt for AI Agents