Repository navigation
feat: declare the management route surface and add ocx capabilities - #2778
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughThe CLI adds ChangesCapability discovery
CLI and proxy version alignment
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant CapabilityRegistry
participant ManagementRouteRegistry
User->>CLI: run ocx capabilities
CLI->>CapabilityRegistry: select and filter capabilities
CapabilityRegistry-->>CLI: return capability metadata
CLI->>ManagementRouteRegistry: resolve route declarations
ManagementRouteRegistry-->>CLI: return method and path data
CLI-->>User: print JSON or human-readable capability listing
sequenceDiagram
participant CLIStatus
participant ProxyLiveness
participant RunningProxy
CLIStatus->>ProxyLiveness: findLiveProxy()
ProxyLiveness->>RunningProxy: GET /healthz
RunningProxy-->>ProxyLiveness: return pid and version
ProxyLiveness-->>CLIStatus: return LiveProxy with version
CLIStatus->>CLIStatus: computeVersionSkew(cliVersion, proxyVersion)
CLIStatus-->>RunningProxy: report warning when versions differ
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 104a6043ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * Deliberately incomplete at this phase: it covers the read/write surface the CLI | ||
| * already reaches, and later phases add entries as they add verbs. The parity test | ||
| * measures routes against capabilities plus declared exemptions, so a missing entry | ||
| * shows up as an unexplained route rather than being quietly tolerated. |
There was a problem hiding this comment.
The table intentionally stops after five entries even though the command describes itself as enumerating every CLI capability. Existing operations such as ocx account pause, ocx provider add, and all other mutating commands are therefore absent, making --mutating-only return an empty list and causing --route to report that their routes have no CLI verb. Populate the existing surface or avoid exposing this as the authoritative machine-readable index until it is complete.
Useful? React with 👍 / 👎.
| command: ["provider", "list"], | ||
| summary: "Configured providers with connectivity and selected models.", | ||
| routes: [{ method: "GET", path: "/api/providers" }], | ||
| flags: [{ name: "--json", value: "boolean", summary: "Emit the provider list as JSON." }], |
There was a problem hiding this comment.
ocx provider list reads configuration directly through loadConfig() in src/cli/provider.ts and never requests GET /api/providers, so this entry makes ocx capabilities --route /api/providers falsely claim that the command drives that management route. Declare no route for the current implementation, as was done for status, or change the command to use the advertised endpoint.
Useful? React with 👍 / 👎.
| const json = takeFlag(args, "--json"); | ||
| const mutatingOnly = takeFlag(args, "--mutating-only"); | ||
| const route = takeValueFlag(args, "--route"); | ||
|
|
||
| if (route !== undefined && route.length === 0) { |
There was a problem hiding this comment.
When invoked as ocx capabilities --route, takeValueFlag removes the flag and returns undefined, so the later empty-string check is unreachable and the command prints the complete capability table with exit code 0. Distinguish a missing value from an absent flag and return the documented usage error instead.
Useful? React with 👍 / 👎.
| return { | ||
| cliVersion, | ||
| proxyVersion: proxy, | ||
| skewed: true, | ||
| warning: `CLI ${cliVersion} does not match the running proxy ${proxy} — this ocx on PATH is stale. ` | ||
| + "Its help and features describe a different build. Reinstall, or run the proxy's own binary.", |
There was a problem hiding this comment.
Every unequal version is labeled as a stale CLI, including the common case where the CLI has just been upgraded but the still-running proxy is older. For example, CLI 2.36.1 against proxy 2.35.0 incorrectly tells the operator that the CLI on PATH is stale and should be reinstalled. Either compare versions before assigning direction or use a neutral mismatch warning.
Useful? React with 👍 / 👎.
| const declaredForModule = MANAGEMENT_ROUTES.filter(r => r.module === mod); | ||
| const nonLiteral = declaredForModule.filter(r => r.mechanism); | ||
| // A non-literal route can ALSO be scannable (a negated guard is both), so count | ||
| // the union rather than adding two overlapping sets. | ||
| const expected = new Set<string>(pairs); | ||
| for (const r of nonLiteral) expected.add(key(r.method, r.path)); |
There was a problem hiding this comment.
The reconciliation cannot detect an omitted non-literal route because nonLiteral is derived from MANAGEMENT_ROUTES itself and then copied into expected. If a new regex, prefix-decoded, constant, or similar route is added to a handler without a registry entry, neither the scanner nor this calculation observes it, so the test remains green despite the registry being incomplete. Maintain a source-independent non-literal inventory or extend the scanner to discover those mechanisms.
Useful? React with 👍 / 👎.
| name: "capabilities", | ||
| usage: "ocx capabilities [--json] [--mutating-only] [--route <path>]", | ||
| summary: "Enumerate every CLI capability with the management routes it drives.", |
There was a problem hiding this comment.
This commit adds the public ocx capabilities command and new status/doctor version-skew output, but no docs-site/ file is updated, leaving users without documentation for the command's JSON schema, filters, exit behavior, or warning. Add the corresponding public CLI documentation as required for user-visible source changes.
AGENTS.md reference: src/AGENTS.md:L24-L28
Useful? React with 👍 / 👎.
리뷰 · 우선순위 66 / 80이 PR은 ocx agentic-control 스택의 열쇠입니다. 베이스는 하는 일은 세 가지입니다. (1) 관리 API 라우트를 선언 목록으로 고정합니다. 평등 비교( 면제(exemption) 어휘에 라인 src/server/management/route-registry.ts - MANAGEMENT_ROUTES 선언. mechanism·exempt·mutates 필드가 있습니다. handler를 import하면 안 됩니다. 라인 tests/management-route-registry.test.ts - 소스에서 (method, path)를 모아 선언과 맞춥니다. 빠지면 실패합니다. 라인 src/cli/capabilities.ts / capabilities-command.ts - 이후 동사들이 등록될 테이블의 시작점입니다. 라인 src/cli/version-skew.ts - PATH ocx와 프록시 빌드 불일치를 보고합니다. #2701 핵심입니다. 경로 base branch - 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
… source Nothing in this repository related the CLI surface to the management API. This adds the declared inventory that later phases register against, plus the gate that keeps it honest. The registry is declared, not harvested, because 18 routes are unreachable by any text scan of their own file: four regexes over model paths, three over lab ids, an endsWith, a pathname.slice, a prefix decode, three path constants, and two negated pathname !== guards. The negated pair matters most. GET /api/routing-analytics has no equality literal anywhere, so a scan built on === never sees it. And for GET /api/storage a === scan finds only the dead shadowed copy in logs-usage-routes.ts, never the live guard in storage-log-guard-routes.ts -- it would have reconciled against the corpse and called the surface covered. The reconciliation counts (method, path) pairs, never grep line hits. Line counting cannot balance against this source: /api/codex-auth/pool-strategy registers PUT and PATCH from one guard spanning four lines, and nineteen path guards decide their method somewhere other than their own line. Both nesting orders occur and neither is inferable from the other -- lab-routes.ts:352 narrows to GET before eight path literals, while storage-log-guard-routes.ts:112 opens with the path and puts the method guard inside at :113. So the scanner walks statements in order maintaining a method narrowing context, and when it cannot resolve a method it returns null and the test fails. It never assumes GET; a scanner that guesses produces a number nobody can trust, which is how the earlier route figures became unreproducible. Resolved surface: 192 pairs from source plus 18 declared non-literal, 108 of them mutating. The gate was driven red before being trusted. Injecting POST /api/combos/red-first-probe into a real handler failed two checks independently and named the route in both. Two further failures found during construction were also real: the generator emitted the negated-guard routes twice once the scanner learned to resolve them, and the inertness assertion flagged this file's own header, which explains why it must not import Lab. That inertness is load-bearing. route-registry.ts is imported by src/server/management-api.ts, which tests/core-lab-boundary.test.ts protects: a user with one provider and no Lab must execute no Lab code. Route paths are strings, so declaring /api/lab/status creates no module edge. The module imports nothing at all, and a test asserts it. Exemptions carry a mandatory reason. The seven mutating /api/lab routes are the interesting case: they were previously going to be waved through as "local-transport" alongside the reads, but ocx lab reads local SQLite and cannot start an automation run or import a community bundle. They now carry deferred-verb with an owner phase and a tracked devlog doc the test verifies exists -- deliberately a repository file and not the goalplan, which is gitignored, so the assertion means the same thing in CI as it does locally.
An agent driving ocx had no way to discover what it could do except by parsing help text, and help text was twenty per-module USAGE constants plus a banner a test explicitly licensed to drift from the command registry. This adds the table that relates each CLI capability to the management routes it drives, and the verb that emits it. ocx capabilities human tree ocx capabilities --json full table with routes and flags ocx capabilities --json --mutating-only ocx capabilities --route /api/usage which commands drive this route capabilities.ts is a leaf module and a test enforces it. Every command module declares its usage text as a top-level const USAGE evaluated at import time, so a cycle back into this table would resolve to undefined under ESM rather than throwing -- silently emptying the usage string rejectArgs hands to CliUsageError, degrading the very error reporting these issues are about. The test asserts no relative import and no dynamic import, plus that every rendered invocation and summary is non-empty, so a cycle fails loudly. Head-handled surfaces stay out of CLI_COMMANDS. --version, -v, and version exit in the CLI head at root.ts before dispatch, so none is a runner key; help is excluded there deliberately, documented in cli-registry.test.ts as a head-handled pseudo-case. Listing either as an ordinary command entry would break the runner-key parity assertion, so they live in HEAD_CAPABILITIES and contribute to the banner and JSON output without touching parity. --route on a route no verb drives exits 4 rather than printing an empty list and returning 0. Reporting success for a request that accomplished nothing is the same dishonesty the transport phase just removed; it does not get reintroduced at the discovery layer. The registry cross-check earned its place immediately: the first version of this table declared ocx status as driving GET /api/status, and the test proved that route does not exist. collectStatus identity-probes /healthz and reads local config, so the capability now correctly declares no route and says so. Flag parsing is order-independent by construction, scanning argv for the flag rather than reading a fixed position -- positional reading is why ocx restore back --json ignores its flag today.
Closes #2701. An older ocx earlier on PATH than the running proxy printed help for commands the proxy does not have and described a different build, and nothing surfaced it because the CLI never compared the two versions. It does now, in both human and JSON output, and in doctor. No extra request. The identity probe in findLiveProxy already fetched and validated the healthz body, so the version rides back with the liveness result. Carrying it there took three hops rather than one: isOpencodexHealthz is a boolean predicate and cannot return a value, so proxyIdentityAt's return type widened and all three findLiveProxy construction sites now thread the field. The version is guarded exactly as pid is -- a non-string is absent, not coerced. Threading it broke 12 exhaustive toEqual assertions in proxy-liveness.test.ts, because Bun's toEqual rejects an extra defined key. Each now asserts the version its own mock actually reports (2.6.17 for OURS, 2.6.16 for the legacy pidless body) rather than being loosened to tolerate the new field. The warning gets its own versionSkew field instead of joining warningParts. That array funnels into codexRuntime.warning and prints under the Codex runtime heading, and a stale ocx on PATH is a fact about this install, not about the Codex runtime. schemaVersion stays 1: the field is additive and a consumer that ignores it is unaffected. The printer edit in index.ts is not optional. A stale CLI is exactly the situation where an operator is reading human output and wondering why their CLI disagrees with the dashboard; adding only the JSON field would have satisfied a test and helped nobody. Both placeholder versions suppress the warning rather than reporting skew against them: the server's VERSION falls back to "0.0.0" when it cannot resolve its own package, and packageVersion() answers "unknown" for a non-string version. Either would mean "cannot compare", and a false stale-CLI warning sends an operator to reinstall a healthy setup. doctor reuses computeVersionSkew rather than reimplementing the comparison. Two diagnostics disagreeing about whether an install is stale would be worse than one reporting it. Verified live, not only under test: this worktree's CLI at 2.35.0 against a running 2.32.1-preview proxy prints the warning.
5b0c273 to
c1f41a6
Compare
104a604 to
3216932
Compare
An adversarial review of the wp9 CI triage disproved a premise I had asserted. I claimed this stack added no empty catch and that the hygiene failure was purely a prose match. The prose part was true; the claim was not. `Bun.writeSync?.(0, "")` guarded by a catch whose body is only a comment does nothing: the scanner probe writes its fixture with `node:fs` on the next line, so the call has no purpose and its failure is swallowed. The hygiene regex misses it because the body is not literally empty -- a comment satisfies the pattern -- which is exactly why my own scan came back clean and why the reword alone would have shipped it. Removing the block outright rather than adding a handler: there is nothing to handle, since no later line depends on that write succeeding. Verified with a comment-aware pattern over every file this stack touches: the only semantically-empty catch it added was this one. The seven that remain are pre-existing on `dev` -- confirmed zero added catch lines in each -- so they are not this unit's scope to change. WP9MSG && git log --oneline -1
…port `local-management-direct-transport` went red on an exact-match assertion: the identity payload gained a `version` field. That field is the point of the version-skew fix in this branch (#2701) -- the value rides back with the liveness probe rather than costing a second request -- and the healthz fixture in this test already serves `version: "test"`, so the new field is correct and the assertion was simply written before it existed. Widened rather than loosened to a subset match. An exact-match assertion is what this test is for: it pins that the direct transport passes the identity payload through intact, and a `toMatchObject` would stop noticing a dropped field. Driven red to confirm it is not vacuous: reverting the expectation to `{ pid: PID }` fails this test and no other. WP9MSG && git log --oneline -1 && git status --porcelain | wc -l
…call provider list reads local config, not GET /api/providers. Drop the "every capability" wording, treat a bare --route as usage, and pin the empty --mutating-only filter to the actual mutating count. Co-authored-by: Cursor <cursoragent@cursor.com>
…-registry feat: declare the management route surface and add ocx capabilities
…-registry feat: declare the management route surface and add ocx capabilities
Summary
Third PR in the ocx agentic-control stack. Targets
codex/ocx-transport-honesty(PR #2775), notdev— retarget after the parent lands.Closes #2701.
This is the keystone phase: it introduces the declared route registry and capability table that later phases register verbs into. Building verbs first would mean writing them twice.
Three things land:
src/server/management/route-registry.ts— 210 declared routes (192 resolved from source, 18 non-literal), with a mandatory-reason exemption vocabulary.ocx capabilities— the machine-readable surface index, so an agent drivingocxdoes not have to parse help text.ocx statusandocx doctornow report when theocxon PATH is a different build from the running proxy.The registry is declared, not harvested
18 routes are unreachable by any text scan of their own file: four regexes over model paths, three over lab ids, an
endsWith, apathname.slice, a prefix decode, three path constants, and two negatedpathname !==guards.The negated pair is why this matters.
GET /api/routing-analyticshas no equality literal anywhere, so a scan built on===never sees it. And forGET /api/storagean equality scan finds only the dead shadowed copy inlogs-usage-routes.ts— never the live guard instorage-log-guard-routes.ts. A gate built that way would have reconciled against the corpse and called the surface covered.The reconciliation counts routes, not grep hits
A count keyed on
rgline hits cannot balance against this source:/api/codex-auth/pool-strategyregisters PUT and PATCH from one guard spanning four lines.lab-routes.ts:352narrows to GET before eight path literals, whilestorage-log-guard-routes.ts:112opens with the path and puts the method guard inside at :113.So the scanner walks statements in order maintaining a method-narrowing context, and when it cannot resolve a method it returns
nulland the test fails. It never assumes GET.The gate was driven red before being trusted
Injecting
POST /api/combos/red-first-probeinto a real handler failed two checks independently and named the route in both. A gate nobody has attacked is not evidence.Three further defects were found by the new tests during implementation, not by inspection:
ocx statusdrivesGET /api/status— a route that does not exist.collectStatusprobes/healthzand reads local config.Lab boundary
route-registry.tsis imported bysrc/server/management-api.ts, whichtests/core-lab-boundary.test.tsprotects. Route paths are strings, so declaring/api/lab/statuscreates no module edge. The module imports nothing at all, and a test asserts it.The seven mutating
/api/labroutes were going to be waved through aslocal-transportalongside the reads, butocx labreads local SQLite and cannot start an automation run or import a community bundle. They now carrydeferred-verbwith an owner phase and a tracked devlog doc the test verifies exists — deliberately a repository file and not the goalplan, which is gitignored, so the assertion means the same thing in CI as locally.Version skew (#2701)
No extra request: the identity probe already parsed the healthz body. Carrying the version took three hops, because
isOpencodexHealthzis a boolean predicate and cannot return a value —proxyIdentityAt's return type widened and all threefindLiveProxyconstruction sites thread the field.That broke 12 exhaustive
toEqualassertions inproxy-liveness.test.ts(Bun'stoEqualrejects an extra defined key). Each now asserts the version its own mock actually reports, rather than being loosened.The warning gets its own
versionSkewfield rather than joiningwarningParts, which funnels intocodexRuntime.warningand would print a stale-PATH fact under the Codex runtime heading.schemaVersionstays 1. Both placeholder versions (0.0.0server-side,unknownCLI-side) suppress the warning — a false stale-CLI warning sends an operator to reinstall a healthy install.Verification
bun testover the 12 affected suites: 363 pass, 0 fail, 1216 expect() calls.tsc --noEmit: clean.combo-routes.ts.tests/core-lab-boundary.test.tsandtests/repo-hygiene.test.tsgreen.Local full-suite and CI checks are deliberately deferred to the final stack-wide rebase and CI pass (wp9), per the stack plan.
Checklist
tsc --noEmitcleanbun run test/ CI — deferred to the stack-wide final pass (wp9)Summary by CodeRabbit
New Features
ocx capabilitieswith human-readable or JSON output, mutation filtering, and route lookup.ocx statusandocx doctornow detect and report CLI/proxy version mismatches.Documentation