feat(lab): CL-04 CLI and management read surfaces - #1378
Conversation
📝 WalkthroughWalkthroughThe PR adds read-only Compatibility Lab projection queries, catalog discovery, management API routes, and ChangesCompatibility Lab read surfaces
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant LabCLI as ocx lab
participant ManagementAPI as management API
participant LabQuery as lab query layer
participant SQLite as read-only SQLite projection
Operator->>LabCLI: request projection inspection
LabCLI->>LabQuery: execute filtered query
Operator->>ManagementAPI: GET /api/lab request
ManagementAPI->>LabQuery: validate and execute route query
LabQuery->>SQLite: open and read projection
SQLite-->>LabQuery: return rows and metadata
LabQuery-->>LabCLI: return DTOs or typed errors
LabQuery-->>ManagementAPI: return DTOs or typed errors
LabCLI-->>Operator: print text or JSON
ManagementAPI-->>Operator: return JSON response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ 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. |
✅ READY
UI screenshot waived by the |
Wibias
added
the
gui-screenshot-waived
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
🤖 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/001_pr_stack_status.md`:
- Around line 191-207: Synchronize the earlier Authorization section with the
current CL-03 and CL-04 status recorded in the merge and start logs: mark CL-03
accepted/closed and CL-04 authorized/in progress, or explicitly label the older
statuses as historical with a date. Update only the relevant authorization
record.
In `@devlog/_plan/260807_compatibility_lab/004_cl04_read_surfaces.md`:
- Around line 37-45: Update the Validation (local) section to record the outcome
of every listed command, including 17/17 for the read-surface tests. Document
that tests/lab-evidence-ledger.test.ts completed 37/41 with four pre-existing
Windows SQLite EPERM flakes, and clearly distinguish this known exception from
successful validations.
In `@src/cli/codex-shim-autorestore.ts`:
- Line 20: Add a short explanatory comment immediately above the `if (command
=== "lab") return true;` condition, documenting that `ocx lab` is read-only and
must not trigger autorestore side effects. Preserve the existing behavior and
avoid changing neighboring command handling.
In `@src/cli/help.ts`:
- Around line 267-280: Use the “catalog” spelling consistently in the lab help
text and the empty-state message in the lab CLI flow, including the visible
`lab.details` catalog entry and the `src/cli/lab.ts` message currently using
“catalogue”.
In `@src/cli/lab.ts`:
- Around line 141-142: Define a shared assertRange helper in src/cli/lab.ts that
rejects cases where from is greater than to, then call it after parsing the
bounds in the verdicts, observations, and events subcommands. Reuse the helper
consistently while preserving the existing nonnegative bound validation.
- Around line 166-175: Update the single-resource JSON output in the “subject”
case to match the sibling “event” and “artifact” cases by passing only the
subject object to printData, removing the redundant subjectId envelope field
while preserving the existing subject lookup and human-readable title.
- Around line 136-156: Validate raw CLI enum options before constructing query
filters, covering the layer, verdict, and other enum-like options in the lab
read commands handled by the relevant command cases in lab.ts. Reuse the
existing enum definitions and invalid-input error/usage behavior so values such
as an unknown layer are rejected with the API-equivalent invalid_layer response,
rather than being passed to queryLabVerdicts or related query functions.
- Around line 261-264: Update the error handling around runCliAction in
src/cli/lab.ts so LabProjectionUnavailableError and
LabProjectionIncompatibleError remain distinct state errors instead of being
wrapped as CliUsageError. Introduce or reuse LabStateError, add the matching
branch in the runtime-api runCliAction mapping, and assign the established
non-usage exit code after checking existing conventions. Preserve usage output
and the usage exit code only for genuine command or syntax errors.
- Around line 122-135: Update handleLabCommand to remove global flags from argv
before destructuring the subcommand, so flag-only invocations such as --json
default to status. Ensure takeFlag and subsequent subcommand argument handling
operate on the flag-stripped arguments while preserving normal subcommand
behavior.
- Around line 206-216: Validate excludedRaw immediately after parsing in the lab
events command, rejecting any value other than "true" or "false" before calling
queryLabEvents. Preserve undefined only when --excluded was not provided, and
retain the existing boolean conversion for valid values.
- Around line 79-113: Extract the repeated pagination hint construction into a
shared helper and have verdictLines, subjectListLines, observationLines,
eventListLines, and artifactLines use it while preserving their existing
empty-result behavior. Update all five formatter signatures to use the
synchronous query result types directly, removing the no-op Awaited<> wrappers
and matching statusSummary and catalogLines.
In `@src/lab/query/connection.ts`:
- Around line 22-25: Move the `new Database(sqlitePath, { readonly: true })`
call into the existing `try` block in the connection logic, ensuring constructor
failures are caught and rethrown as `LabProjectionUnavailableError` without
exposing the SQLite path. Keep the existing missing-file handling and successful
database return behavior unchanged.
- Around line 74-77: Update countTable to validate table against an explicit
allowlist of the seven supported table identifiers before interpolating it into
the SQL query. Reject any value not on that allowlist, while preserving the
existing count query for valid names.
In `@src/lab/query/dto-map.ts`:
- Around line 205-213: Update mapValidatedEventToDto so the exclusionReason
assigned in the shared base object is passed through the same sanitizer used by
mapEventListRow and mapObservationRow, ensuring every event DTO branch inherits
sanitized text while preserving null handling.
In `@src/lab/query/queries.ts`:
- Around line 339-349: Remove the unused joinSql declaration and its
interpolation from the query in the surrounding query function, and change the
SELECT clause from SELECT DISTINCT to SELECT. Preserve the existing filters,
ordering, pagination, and parameter handling.
- Around line 375-383: Remove the unreachable fallback after
parseEventPayloadToDto in the event-mapping flow, returning the parser result
directly while preserving excluded and exclusionReason arguments. Then remove
the now-unused validateLabEvent and mapValidatedEventToDto imports from the
file.
In `@src/server/management/lab-routes.ts`:
- Around line 250-254: The three resource routes let malformed encoded path
segments escape their guarded handling. In src/server/management/lab-routes.ts
at lines 250-254, define decodePathSegment beside rejectUnsafeId to return null
for URIError, replace decodeURIComponent, and return the 404 not_found response
when decoding returns null; apply the same replacement and null response at
lines 323-327 and 360-364.
In `@tests/lab-read-surfaces.test.ts`:
- Around line 92-106: Strengthen the affected tests in
tests/lab-read-surfaces.test.ts:92-106 by asserting discovered scenarios are
non-empty in seedProjection before seeding; at 160-178, replace the
first.hasMore/nextCursor early return with assertions that both indicate another
page; at 249-253, assert payload_json on each verdictsBody.verdicts entry rather
than the envelope; and at 335-360, assert the update changes count is 1, require
artifacts.items to be non-empty before iterating, and query the corruption
surface so the inserted corruption row is actually validated.
- Around line 92-106: Add a precondition assertion in seedProjection immediately
after discoverScenarios so the helper fails clearly when no scenarios are found
for suiteId. Assert that scenarios is non-empty and include the suite identifier
in the failure message; preserve the existing slicing, persistence, and
projection rebuild flow.
- Around line 215-237: Extend the read-only coverage in the test "read calls do
not mutate ledger sqlite artifacts" by invoking queryLabSubjectById,
queryLabEventById, queryLabArtifactByDigest, and queryLabCatalogEntries
alongside the existing queries, using valid seeded-resource arguments. Keep the
existing byte-equality and mtime assertions unchanged so these single-resource
entry points are covered by the same non-mutation checks.
- Around line 298-321: Add output assertions to the lab command tests by
capturing console.log and verifying human status produces non-empty formatted
lines while --json produces JSON-shaped output. Extend argument coverage around
handleLabCommand to test the empty-argv default status behavior and the
["--json"] boundary case, asserting their expected exit codes and output or
error shape; keep these focused near the existing lab tests and exercise
statusSummary and the relevant *Lines formatters.
🪄 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: 1332924b-48aa-4519-8283-d836b455d6e6
📒 Files selected for processing (19)
devlog/_plan/260807_compatibility_lab/001_pr_stack_status.mddevlog/_plan/260807_compatibility_lab/004_cl04_read_surfaces.mdsrc/cli/codex-shim-autorestore.tssrc/cli/help.tssrc/cli/index.tssrc/cli/lab.tssrc/lab/index.tssrc/lab/query/catalog.tssrc/lab/query/connection.tssrc/lab/query/constants.tssrc/lab/query/cursor.tssrc/lab/query/dto-map.tssrc/lab/query/errors.tssrc/lab/query/index.tssrc/lab/query/queries.tssrc/lab/query/types.tssrc/server/management-api.tssrc/server/management/lab-routes.tstests/lab-read-surfaces.test.ts
| - ~~Independent acceptance review not performed~~ — reconciled at merge #1352 | ||
| - ~~Draft PR review findings not yet reconciled~~ — CodeRabbit/review findings addressed pre-merge | ||
| - Full local ledger suite may show pre-existing Windows SQLite `EPERM` flakes (`rebuild.ts` unchanged vs base) | ||
|
|
||
| ## CL-03 merge log (2026-08-09) | ||
|
|
||
| - **Merged to `dev`:** `68c71a4e9cdf882d812f09fd94783a28749db629` via upstream [#1352](https://github.com/lidge-jun/opencodex/pull/1352) | ||
| - **Final required CI:** green at merge (cross-platform) | ||
| - **CodeRabbit/review:** findings reconciled pre-merge | ||
| - **CL-03 state:** accepted/closed; CL-04 authorized from current `dev` | ||
|
|
||
| ## CL-04 start log (2026-08-09) | ||
|
|
||
| - **Starting `upstream/dev` SHA:** `68c71a4e9cdf882d812f09fd94783a28749db629` | ||
| - **Branch:** `feat/cl-04-lab-read-surfaces` | ||
| - **Scope:** read-only CLI (`ocx lab`), authenticated `GET /api/lab/*`, shared `src/lab/query/` layer | ||
| - **CL-05:** not started |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Synchronize the stale authorization record.
Lines 191-207 record CL-03 as accepted and CL-04 as authorized and in progress. The earlier ## Authorization section at Lines 160-163 still says that CL-03 is a draft and CL-04 is not started. Readers can apply the wrong acceptance gate.
Update the earlier section in this change, or mark it as historical with an explicit date.
Proposed correction
-- CL-03: **DRAFT PR OPEN** ... Not accepted.
-- CL-04: **NOT STARTED** ...
+- CL-03: **ACCEPTED/CLOSED** via `#1352`.
+- CL-04: **IMPLEMENTATION IN PROGRESS** from the accepted CL-03 merge.🤖 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 `@devlog/_plan/260807_compatibility_lab/001_pr_stack_status.md` around lines
191 - 207, Synchronize the earlier Authorization section with the current CL-03
and CL-04 status recorded in the merge and start logs: mark CL-03
accepted/closed and CL-04 authorized/in progress, or explicitly label the older
statuses as historical with a date. Update only the relevant authorization
record.
| ## Validation (local) | ||
|
|
||
| - `bun x tsc --noEmit` | ||
| - `bun test tests/lab-read-surfaces.test.ts` | ||
| - `bun test tests/lab-conformance-harness.test.ts` | ||
| - `bun test tests/lab-evidence-ledger.test.ts` | ||
| - `bun test tests/lab-live-probe.test.ts` | ||
| - `bun test tests/lab-live-sandbox.test.ts` | ||
| - `bun run privacy:scan` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Record validation outcomes and known failures.
This section lists validation commands without results. The stack record documents that tests/lab-evidence-ledger.test.ts completed with 37/41 tests and four pre-existing Windows SQLite EPERM flakes. Without that qualification, this record can be read as a clean validation result.
Record the result for each command, including 17/17 for the read-surface tests and the known evidence-ledger exception.
Proposed correction
-- - `bun test tests/lab-read-surfaces.test.ts`
-- - `bun test tests/lab-evidence-ledger.test.ts`
+- - `bun test tests/lab-read-surfaces.test.ts` — 17/17 passed
+- - `bun test tests/lab-evidence-ledger.test.ts` — 37/41 passed; 4 pre-existing Windows SQLite `EPERM` flakes in `wipeSqlite`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## Validation (local) | |
| - `bun x tsc --noEmit` | |
| - `bun test tests/lab-read-surfaces.test.ts` | |
| - `bun test tests/lab-conformance-harness.test.ts` | |
| - `bun test tests/lab-evidence-ledger.test.ts` | |
| - `bun test tests/lab-live-probe.test.ts` | |
| - `bun test tests/lab-live-sandbox.test.ts` | |
| - `bun run privacy:scan` | |
| ## Validation (local) | |
| - `bun x tsc --noEmit` | |
| - `bun test tests/lab-read-surfaces.test.ts` — 17/17 passed | |
| - `bun test tests/lab-conformance-harness.test.ts` | |
| - `bun test tests/lab-evidence-ledger.test.ts` — 37/41 passed; 4 pre-existing Windows SQLite `EPERM` flakes in `wipeSqlite` | |
| - `bun test tests/lab-live-probe.test.ts` | |
| - `bun test tests/lab-live-sandbox.test.ts` | |
| - `bun run privacy:scan` |
🤖 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 `@devlog/_plan/260807_compatibility_lab/004_cl04_read_surfaces.md` around lines
37 - 45, Update the Validation (local) section to record the outcome of every
listed command, including 17/17 for the read-surface tests. Document that
tests/lab-evidence-ledger.test.ts completed 37/41 with four pre-existing Windows
SQLite EPERM flakes, and clearly distinguish this known exception from
successful validations.
|
|
||
| export function skipsCodexShimAutoRestore(command: string | undefined, args: string[]): boolean { | ||
| if (command === "uninstall" || command === "remove") return true; | ||
| if (command === "lab") return true; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Record why lab is exempt; the reason differs from its neighbours.
Line 19 skips autorestore because uninstall and remove are destructive. Line 21 skips it because those codex-shim subcommands are the repair operation itself. Line 20 skips it for a third reason: ocx lab is read-only and must not trigger side effects.
A future reader cannot infer that from the code. The risk is that someone treats line 20 as a copy-paste error and deletes it, which silently restores the shim during a read-only inspection.
Add a short comment. The behavior itself is correct, and tests/lab-read-surfaces.test.ts line 295 covers it.
♻️ Proposed comment
+ // `lab` is read-only inspection; it must not trigger shim side effects.
if (command === "lab") return true;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (command === "lab") return true; | |
| // `lab` is read-only inspection; it must not trigger shim side effects. | |
| if (command === "lab") return true; |
🤖 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/cli/codex-shim-autorestore.ts` at line 20, Add a short explanatory
comment immediately above the `if (command === "lab") return true;` condition,
documenting that `ocx lab` is read-only and must not trigger autorestore side
effects. Preserve the existing behavior and avoid changing neighboring command
handling.
| lab: { | ||
| usage: "ocx lab <status|verdicts|subjects|subject|observations|events|event|artifacts|artifact|catalog> [options] [--json]", | ||
| summary: "Read-only Compatibility Lab projection inspection (local SQLite; no daemon).", | ||
| details: [ | ||
| "status Projection availability, schema versions, and row counts.", | ||
| "verdicts Paginated derived compatibility verdicts with filters.", | ||
| "subjects List subjects; subject <id> returns one typed subject.", | ||
| "observations Paginated observation rows from the projection.", | ||
| "events Event history; event <id> returns one safe typed event.", | ||
| "artifacts Artifact metadata only (no content download).", | ||
| "catalog Packaged protocol/live scenario catalogue metadata.", | ||
| "Reads never rebuild the projection, trigger probes, or require the proxy.", | ||
| ], | ||
| }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use one spelling of "catalog" in the user-facing text.
Line 268 names the subcommand catalog. Line 277 describes it as "catalogue". src/cli/lab.ts line 119 also prints "No catalogue scenarios" as the empty-state message.
The subcommand name is catalog, so the prose should match it. Mixed spelling in help text makes the feature harder to search for in the help output and in documentation.
✏️ Proposed fix
- "catalog Packaged protocol/live scenario catalogue metadata.",
+ "catalog Packaged protocol/live scenario catalog metadata.",Apply the same change in src/cli/lab.ts line 119:
- return lines.length > 0 ? lines : ["No catalogue scenarios"];
+ return lines.length > 0 ? lines : ["No catalog scenarios"];🤖 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/cli/help.ts` around lines 267 - 280, Use the “catalog” spelling
consistently in the lab help text and the empty-state message in the lab CLI
flow, including the visible `lab.details` catalog entry and the `src/cli/lab.ts`
message currently using “catalogue”.
| function verdictLines(page: Awaited<ReturnType<typeof queryLabVerdicts>>): string[] { | ||
| const lines = page.items.map((v) => | ||
| `${v.verdict} ${v.evidenceLayer} ${v.suiteId} subject=${v.subjectId} asOf=${v.asOf}`, | ||
| ); | ||
| if (page.hasMore) lines.push(`(more available; pass --cursor ${page.nextCursor ?? ""})`); | ||
| return lines.length > 0 ? lines : ["No verdicts"]; | ||
| } | ||
|
|
||
| function subjectListLines(page: Awaited<ReturnType<typeof queryLabSubjects>>): string[] { | ||
| const lines = page.items.map((s) => `${s.subjectId} (${s.subjectKind})`); | ||
| if (page.hasMore) lines.push(`(more available; pass --cursor ${page.nextCursor ?? ""})`); | ||
| return lines.length > 0 ? lines : ["No subjects"]; | ||
| } | ||
|
|
||
| function observationLines(page: Awaited<ReturnType<typeof queryLabObservations>>): string[] { | ||
| const lines = page.items.map((o) => | ||
| `${o.outcome} ${o.evidenceLayer} ${o.scenarioId} event=${o.eventId} completed=${o.completedAt}`, | ||
| ); | ||
| if (page.hasMore) lines.push(`(more available; pass --cursor ${page.nextCursor ?? ""})`); | ||
| return lines.length > 0 ? lines : ["No observations"]; | ||
| } | ||
|
|
||
| function eventListLines(page: Awaited<ReturnType<typeof queryLabEvents>>): string[] { | ||
| const lines = page.items.map((e) => | ||
| `${e.eventKind} ${e.eventId} recorded=${e.recordedAt}${e.excluded ? " excluded" : ""}`, | ||
| ); | ||
| if (page.hasMore) lines.push(`(more available; pass --cursor ${page.nextCursor ?? ""})`); | ||
| return lines.length > 0 ? lines : ["No events"]; | ||
| } | ||
|
|
||
| function artifactLines(page: Awaited<ReturnType<typeof queryLabArtifacts>>): string[] { | ||
| const lines = page.items.map((a) => `${a.status} ${a.digest} class=${a.artifactClass ?? "unknown"}`); | ||
| if (page.hasMore) lines.push(`(more available; pass --cursor ${page.nextCursor ?? ""})`); | ||
| return lines.length > 0 ? lines : ["No artifacts"]; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Extract the duplicated pagination hint into one helper.
Lines 83, 89, 97, 105, and 111 repeat the same (more available; pass --cursor ...) construction. Five copies must stay in sync if the hint text or the cursor flag name changes. Extract one helper and call it from each formatter.
Also note the Awaited<> wrappers on these five signatures are no-ops. The query functions are synchronous, as the call sites at lines 146-154 and the tests at tests/lab-read-surfaces.test.ts line 163 confirm. statusSummary and catalogLines already omit the wrapper. Dropping it makes the sync contract uniform.
♻️ Proposed refactor to deduplicate the hint
+function appendPaginationHint(lines: string[], page: { hasMore: boolean; nextCursor?: string | null }): void {
+ if (page.hasMore) lines.push(`(more available; pass --cursor ${page.nextCursor ?? ""})`);
+}
+
-function verdictLines(page: Awaited<ReturnType<typeof queryLabVerdicts>>): string[] {
+function verdictLines(page: ReturnType<typeof queryLabVerdicts>): string[] {
const lines = page.items.map((v) =>
`${v.verdict} ${v.evidenceLayer} ${v.suiteId} subject=${v.subjectId} asOf=${v.asOf}`,
);
- if (page.hasMore) lines.push(`(more available; pass --cursor ${page.nextCursor ?? ""})`);
+ appendPaginationHint(lines, page);
return lines.length > 0 ? lines : ["No verdicts"];
}Apply the same change to subjectListLines, observationLines, eventListLines, and artifactLines.
🤖 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/cli/lab.ts` around lines 79 - 113, Extract the repeated pagination hint
construction into a shared helper and have verdictLines, subjectListLines,
observationLines, eventListLines, and artifactLines use it while preserving
their existing empty-result behavior. Update all five formatter signatures to
use the synchronous query result types directly, removing the no-op Awaited<>
wrappers and matching statusSummary and catalogLines.
| const dto = parseEventPayloadToDto(row.payload_json, excluded, exclusionReason); | ||
| if (dto) return dto; | ||
| try { | ||
| const event = validateLabEvent(JSON.parse(row.payload_json)); | ||
| return mapValidatedEventToDto(event, excluded, exclusionReason); | ||
| } catch { | ||
| return null; | ||
| } | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
The fallback at lines 377-382 cannot ever succeed. Delete it.
Line 375 calls parseEventPayloadToDto. That function is defined in src/lab/query/dto-map.ts at lines 265-277 and performs exactly this sequence:
JSON.parse(payloadJson) → validateLabEvent(parsed) → mapValidatedEventToDto(...)
wrapped in try { ... } catch { return null; }.
Lines 377-382 repeat the identical three calls with the identical arguments inside an identical try/catch. So if line 375 returned null, the payload either failed JSON.parse or failed validateLabEvent. Re-running the same deterministic operations on the same string produces the same failure. The fallback returns null every time it is reached.
The code is therefore dead by outcome, and it hides that fact behind an apparent recovery path. A reader will assume the two branches differ.
Removing it also drops the now-unused validateLabEvent and mapValidatedEventToDto imports at lines 28 and 49 of this file.
♻️ Proposed refactor
const excluded = Number(row.excluded) === 1;
const exclusionReason = row.exclusion_reason ? String(row.exclusion_reason) : null;
- const dto = parseEventPayloadToDto(row.payload_json, excluded, exclusionReason);
- if (dto) return dto;
- try {
- const event = validateLabEvent(JSON.parse(row.payload_json));
- return mapValidatedEventToDto(event, excluded, exclusionReason);
- } catch {
- return null;
- }
+ return parseEventPayloadToDto(row.payload_json, excluded, exclusionReason);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const dto = parseEventPayloadToDto(row.payload_json, excluded, exclusionReason); | |
| if (dto) return dto; | |
| try { | |
| const event = validateLabEvent(JSON.parse(row.payload_json)); | |
| return mapValidatedEventToDto(event, excluded, exclusionReason); | |
| } catch { | |
| return null; | |
| } | |
| }); | |
| const excluded = Number(row.excluded) === 1; | |
| const exclusionReason = row.exclusion_reason ? String(row.exclusion_reason) : null; | |
| return parseEventPayloadToDto(row.payload_json, excluded, exclusionReason); | |
| }); |
🤖 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/query/queries.ts` around lines 375 - 383, Remove the unreachable
fallback after parseEventPayloadToDto in the event-mapping flow, returning the
parser result directly while preserving excluded and exclusionReason arguments.
Then remove the now-unused validateLabEvent and mapValidatedEventToDto imports
from the file.
| const subjectMatch = url.pathname.match(/^\/api\/lab\/subjects\/([^/]+)$/); | ||
| if (subjectMatch) { | ||
| const subjectId = decodeURIComponent(subjectMatch[1]!); | ||
| const unsafe = rejectUnsafeId(subjectId, ctx); | ||
| if (unsafe) return unsafe; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Three resource routes call decodeURIComponent outside their try block. Each site decodes the captured path segment before the guarded region begins, so a malformed percent sequence such as /api/lab/subjects/% throws URIError out of handleLabRoutes. handleManagementAPI in src/server/management-api.ts rethrows it at line 202, which turns a client-supplied one-character path into an unhandled 500 instead of the 404 that rejectUnsafeId is designed to return. Add one decodePathSegment helper beside rejectUnsafeId that returns null on URIError, then use it at all three sites.
src/server/management/lab-routes.ts#L250-L254: definedecodePathSegment, replace thedecodeURIComponentcall on line 252, and return the 404not_foundresponse when it yieldsnull.src/server/management/lab-routes.ts#L323-L327: replace thedecodeURIComponentcall on line 325 withdecodePathSegmentand return the 404not_foundresponse when it yieldsnull.src/server/management/lab-routes.ts#L360-L364: replace thedecodeURIComponentcall on line 362 withdecodePathSegmentand return the 404not_foundresponse when it yieldsnull.
📍 Affects 1 file
src/server/management/lab-routes.ts#L250-L254(this comment)src/server/management/lab-routes.ts#L323-L327src/server/management/lab-routes.ts#L360-L364
🤖 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/server/management/lab-routes.ts` around lines 250 - 254, The three
resource routes let malformed encoded path segments escape their guarded
handling. In src/server/management/lab-routes.ts at lines 250-254, define
decodePathSegment beside rejectUnsafeId to return null for URIError, replace
decodeURIComponent, and return the 404 not_found response when decoding returns
null; apply the same replacement and null response at lines 323-327 and 360-364.
| function seedProjection(home: string, suiteId = "responses-core") { | ||
| const authority = loadCaseAuthority(); | ||
| const scenarios = discoverScenarios(authority, [suiteId]); | ||
| let recordedAt = 1_700_000_000_000; | ||
| for (const caseRecord of scenarios.slice(0, 2)) { | ||
| const store = createArtifactStore(join(home, "lab", "artifacts")); | ||
| persistConformanceResult(syntheticPassResult(caseRecord), caseRecord, authority, { | ||
| configDir: home, | ||
| recordedAt: recordedAt++, | ||
| artifactStore: store, | ||
| }); | ||
| store.close(); | ||
| } | ||
| return rebuildLabProjection(home); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Four assertions in this suite can report success without verifying anything. The shared root cause is that the suite asserts on data without first proving that the data exists and that the assertion targets the right object. seedProjection is the common dependency: every affected test trusts it to produce rows, and none checks that it did. The result is a 17-test suite that can stay green through a fixture rename or a DTO regression.
tests/lab-read-surfaces.test.ts#L92-L106: assertscenarios.lengthis greater than 0 after line 94. IfdiscoverScenariosreturns empty, the loop never runs, the projection is empty, and the failure surfaces as six unrelated assertion errors at lines 150, 192, 197, 200, 206, and 252 instead of one clear message.tests/lab-read-surfaces.test.ts#L160-L178: replace theif (!first.nextCursor) return;early exit at line 165 withexpect(first.hasMore).toBe(true)andexpect(first.nextCursor).toBeTruthy(). The early exit skips lines 166-176, which hold the only coverage for cursor round-tripping and tampered-fingerprint rejection.tests/lab-read-surfaces.test.ts#L249-L253: move thepayload_jsoncheck at line 253 from the envelope onto each entry ofverdictsBody.verdicts. The envelope is{ verdicts, hasMore }and never carries that column, so the assertion passes unconditionally.tests/lab-read-surfaces.test.ts#L335-L360: assertchangesis 1 on the update at line 348 and assertartifacts.items.lengthis greater than 0 before the loop at line 354. Also query the corruption surface, because the corruption row inserted at lines 340-347 is never read and the"Bearer"check at line 357 inspects artifact DTOs for a string written only to that unread row.
📍 Affects 1 file
tests/lab-read-surfaces.test.ts#L92-L106(this comment)tests/lab-read-surfaces.test.ts#L160-L178tests/lab-read-surfaces.test.ts#L249-L253tests/lab-read-surfaces.test.ts#L335-L360
🤖 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 `@tests/lab-read-surfaces.test.ts` around lines 92 - 106, Strengthen the
affected tests in tests/lab-read-surfaces.test.ts:92-106 by asserting discovered
scenarios are non-empty in seedProjection before seeding; at 160-178, replace
the first.hasMore/nextCursor early return with assertions that both indicate
another page; at 249-253, assert payload_json on each verdictsBody.verdicts
entry rather than the envelope; and at 335-360, assert the update changes count
is 1, require artifacts.items to be non-empty before iterating, and query the
corruption surface so the inserted corruption row is actually validated.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Assert that the seed actually produced scenarios.
Line 94 calls discoverScenarios(authority, [suiteId]). Line 96 slices the first two results. If discovery returns an empty list, the loop body never runs and rebuildLabProjection builds an empty projection.
The failure then surfaces far from its cause. Line 150 fails with expect(status.verdictCount).toBeGreaterThan(0), and line 192, line 197, line 200, line 206, and line 252 fail the same way. A reader sees six unrelated assertion failures instead of "the fixture suite responses-core no longer exists".
Add one precondition assertion in the helper so a fixture rename fails once, with a clear message.
💚 Proposed precondition
const authority = loadCaseAuthority();
const scenarios = discoverScenarios(authority, [suiteId]);
+ expect(scenarios.length).toBeGreaterThan(0);
let recordedAt = 1_700_000_000_000;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function seedProjection(home: string, suiteId = "responses-core") { | |
| const authority = loadCaseAuthority(); | |
| const scenarios = discoverScenarios(authority, [suiteId]); | |
| let recordedAt = 1_700_000_000_000; | |
| for (const caseRecord of scenarios.slice(0, 2)) { | |
| const store = createArtifactStore(join(home, "lab", "artifacts")); | |
| persistConformanceResult(syntheticPassResult(caseRecord), caseRecord, authority, { | |
| configDir: home, | |
| recordedAt: recordedAt++, | |
| artifactStore: store, | |
| }); | |
| store.close(); | |
| } | |
| return rebuildLabProjection(home); | |
| } | |
| function seedProjection(home: string, suiteId = "responses-core") { | |
| const authority = loadCaseAuthority(); | |
| const scenarios = discoverScenarios(authority, [suiteId]); | |
| expect(scenarios.length).toBeGreaterThan(0); | |
| let recordedAt = 1_700_000_000_000; | |
| for (const caseRecord of scenarios.slice(0, 2)) { | |
| const store = createArtifactStore(join(home, "lab", "artifacts")); | |
| persistConformanceResult(syntheticPassResult(caseRecord), caseRecord, authority, { | |
| configDir: home, | |
| recordedAt: recordedAt++, | |
| artifactStore: store, | |
| }); | |
| store.close(); | |
| } | |
| return rebuildLabProjection(home); | |
| } |
🤖 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 `@tests/lab-read-surfaces.test.ts` around lines 92 - 106, Add a precondition
assertion in seedProjection immediately after discoverScenarios so the helper
fails clearly when no scenarios are found for suiteId. Assert that scenarios is
non-empty and include the suite identifier in the failure message; preserve the
existing slicing, persistence, and projection rebuild flow.
| test("read calls do not mutate ledger sqlite artifacts", () => { | ||
| withHome((home) => { | ||
| seedProjection(home); | ||
| const ledger = join(home, "lab", "compatibility.jsonl"); | ||
| const sqlite = join(home, "lab", "compatibility.sqlite"); | ||
| const ledgerBefore = readFileSync(ledger); | ||
| const sqliteBefore = readFileSync(sqlite); | ||
| const ledgerMtime = statSync(ledger).mtimeMs; | ||
| const sqliteMtime = statSync(sqlite).mtimeMs; | ||
|
|
||
| queryLabStatus(home); | ||
| queryLabVerdicts({}, undefined, undefined, home); | ||
| queryLabSubjects(undefined, undefined, undefined, home); | ||
| queryLabObservations({}, undefined, undefined, home); | ||
| queryLabEvents({}, undefined, undefined, home); | ||
| queryLabArtifacts({}, undefined, undefined, home); | ||
|
|
||
| expect(readFileSync(ledger).equals(ledgerBefore)).toBe(true); | ||
| expect(readFileSync(sqlite).equals(sqliteBefore)).toBe(true); | ||
| expect(statSync(ledger).mtimeMs).toBe(ledgerMtime); | ||
| expect(statSync(sqlite).mtimeMs).toBe(sqliteMtime); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Extend the non-mutation check to the single-resource lookups.
Lines 225-230 exercise the six list and status queries. The four single-resource queries are absent: queryLabSubjectById, queryLabEventById, queryLabArtifactByDigest, and queryLabCatalogEntries.
Those lookups are separate code paths that open their own connections. A future change that opens one of them in write mode would create or touch the SQLite file, and this test would not notice. The byte-equality guard at lines 232-233 is the strongest assertion in the file, so it should cover every read entry point the CLI and the API expose.
💚 Proposed additional calls
queryLabArtifacts({}, undefined, undefined, home);
+ const subjectsForRead = queryLabSubjects(undefined, undefined, undefined, home);
+ queryLabSubjectById(subjectsForRead.items[0]!.subjectId, home);
+ const eventsForRead = queryLabEvents({}, undefined, undefined, home);
+ queryLabEventById(eventsForRead.items[0]!.eventId, home);
+ const artifactsForRead = queryLabArtifacts({}, undefined, undefined, home);
+ queryLabArtifactByDigest(artifactsForRead.items[0]!.digest, home);
expect(readFileSync(ledger).equals(ledgerBefore)).toBe(true);🤖 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 `@tests/lab-read-surfaces.test.ts` around lines 215 - 237, Extend the read-only
coverage in the test "read calls do not mutate ledger sqlite artifacts" by
invoking queryLabSubjectById, queryLabEventById, queryLabArtifactByDigest, and
queryLabCatalogEntries alongside the existing queries, using valid
seeded-resource arguments. Keep the existing byte-equality and mtime assertions
unchanged so these single-resource entry points are covered by the same
non-mutation checks.
| test("lab status human and json without daemon", async () => { | ||
| await withHome(async (home) => { | ||
| seedProjection(home); | ||
| const code = await handleLabCommand(["status"], { configDir: home }); | ||
| expect(code).toBe(0); | ||
| const jsonCode = await handleLabCommand(["status", "--json"], { configDir: home }); | ||
| expect(jsonCode).toBe(0); | ||
| }); | ||
| }); | ||
|
|
||
| test("lab verdicts and invalid args", async () => { | ||
| await withHome(async (home) => { | ||
| seedProjection(home); | ||
| expect(await handleLabCommand(["verdicts", "--json", "--limit", "1"], { configDir: home })).toBe(0); | ||
| expect(await handleLabCommand(["unknown-sub"], { configDir: home })).toBe(2); | ||
| }); | ||
| }); | ||
|
|
||
| test("lab unavailable projection", async () => { | ||
| await withHome(async (home) => { | ||
| expect(await handleLabCommand(["status", "--json"], { configDir: home })).toBe(0); | ||
| expect(await handleLabCommand(["verdicts"], { configDir: home })).toBe(2); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
These CLI tests assert exit codes only; add output and argv coverage.
Lines 302, 304, 311, 312, 318, and 319 check the return value of handleLabCommand. No test inspects what the command printed.
Two gaps follow.
-
Formatting is untested. The test at line 298 is named "lab status human and json", but nothing distinguishes the two modes.
statusSummaryinsrc/cli/lab.tslines 63-77 and the five*Linesformatters at lines 79-113 have no coverage. A formatter could emit an empty array, or--jsoncould emit human text, and both calls would still return 0. -
Argument handling is untested at the boundaries. No case covers
handleLabCommand([]), which exercises the= "status"default atsrc/cli/lab.tsline 125. No case covershandleLabCommand(["--json"]), which currently fails withunknown lab subcommand: --jsonas I noted on that line.
Capture console.log and assert the shape.
💚 Proposed output assertions
test("lab status human and json without daemon", async () => {
await withHome(async (home) => {
seedProjection(home);
+ const lines: string[] = [];
+ const originalLog = console.log;
+ console.log = (...args: unknown[]) => { lines.push(args.join(" ")); };
+ try {
const code = await handleLabCommand(["status"], { configDir: home });
expect(code).toBe(0);
+ expect(lines.join("\n")).toContain("Lab projection: available");
+ lines.length = 0;
const jsonCode = await handleLabCommand(["status", "--json"], { configDir: home });
expect(jsonCode).toBe(0);
+ expect(JSON.parse(lines.join("")).projectionAvailable).toBe(true);
+ } finally {
+ console.log = originalLog;
+ }
});
});Add a case for the default subcommand:
+ test("bare lab defaults to status", async () => {
+ await withHome(async (home) => {
+ seedProjection(home);
+ expect(await handleLabCommand([], { configDir: home })).toBe(0);
+ });
+ });Based on path instructions: "A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem."
🤖 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 `@tests/lab-read-surfaces.test.ts` around lines 298 - 321, Add output
assertions to the lab command tests by capturing console.log and verifying human
status produces non-empty formatted lines while --json produces JSON-shaped
output. Extend argument coverage around handleLabCommand to test the empty-argv
default status behavior and the ["--json"] boundary case, asserting their
expected exit codes and output or error shape; keep these focused near the
existing lab tests and exercise statusSummary and the relevant *Lines
formatters.
Source: Path instructions
1 participant
Summary
src/lab/query/) over the CL-02 SQLite projection with cursor pagination, DTO sanitization, and stable catalog/status surfaces.ocx labCLI subcommands and management API routes for status, verdicts, subjects, observations, events, artifacts, and catalog reads.tests/lab-read-surfaces.test.tscovering query, management API, CLI, and privacy boundaries.Out of scope
Validation commands/results
bun x tsc --noEmit— passbun test tests/lab-read-surfaces.test.ts— 17 pass, 0 failbun test tests/lab-conformance-harness.test.ts— 17 pass, 0 failbun test tests/lab-evidence-ledger.test.ts— 37 pass, 4 fail (Windows SQLite file lock inwipeSqliteduring repeatedrebuildLabProjection; same failures on base68c71a4without CL-04)bun test tests/lab-live-probe.test.ts— 19 pass, 0 failbun test tests/lab-live-sandbox.test.ts— 17 pass, 0 failbun run privacy:scan— passSecurity notes
sanitizePublicText; corruption/artifact error fields are redacted in read surfaces.labis registered with codex shim autorestore skip to avoid side effects on read-only invocations.Summary by CodeRabbit
New Features
ocx labCLI.Bug Fixes
labcommand.Documentation