feat(lab): CL-04 CLI and management read surfaces - #1378
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 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🧪 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 Hygiene✅ Deterministic PR hygiene checks passed. |
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
| 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”.
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