feat(api): centrs api over rest-api + native-api (Phases 2+3) - #110
Merged
Conversation
…s 2+3) Structured RouterOS API passthrough (gh-api style): one command per operation, structured in/out, can write, validated through `/console/inspect`. The structured middle of the execute/retrieve/api verb trichotomy (#91). - src/api.ts: orchestrator — endpoint normalization, method→verb map, isApiMutating write gate, -f/-d/--input body, --query/--raw-query + --proplist, the /console/inspect gate, write-confirm (reuses execute's promptForWriteConfirmation), envelope + --raw passthrough. - src/cli/api.ts: arg parsing + dispatch; wired into src/cli.ts + index. - ProtocolAdapter.apiRequest seam: REST = HTTP method + URL (id-in-path, .query/.proplist body); native = tagged talk (?-words, =.id=, =.proplist=, add ret→{.id} re-map); console adapters stub capability-unsupported. New error code usage/invalid-method. CHR-passed on 7.23.1: test/integration/api.test.ts (rest examples 1-20) and api-native.test.ts (native N1-N8); api GET added to chr-smoke. Unit: test/unit/api{,-cli-args}.test.ts. MATRIX api row → CHR-passed ×2. Grounding (commands/api/AGENTS.md): native /execute honors =as-string= for synchronous output (without it = fire-and-forget job id) — corrects execute.ts's "native script unsupported" assumption. Deferred to Phase 4: open-ended --listen streaming (native-only; rest --listen → transport/capability-unsupported) and multi-target fan-out. Also: pre-push hook now runs `lint:ci && test && build` so cspell / markdownlint gate the push (a spell error was slipping to CI and skipping the smoke tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a new Changescentrs api command
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds the new centrs api command runtime: a validated, structured RouterOS API passthrough (modeled on gh api) that works over both rest-api and native-api, including CLI wiring, transport adapter support, and CHR-gated integration coverage.
Changes:
- Introduces the
src/api.tsorchestrator +src/cli/api.tsCLI surface, including endpoint normalization, method→verb mapping,/console/inspectvalidation, and write confirmation. - Extends protocol adapters with an
apiRequestseam for REST and native API, with rest-style result shaping on native. - Adds unit + CHR integration tests, updates docs/MATRIX + command docs, and registers the new
usage/invalid-methoderror code (plus page); updates the pre-push hook gate.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/unit/api.test.ts | Unit tests for endpoint normalization, verb mapping, body/query building, and envelope rendering. |
| test/unit/api-cli-args.test.ts | Unit tests for centrs api CLI argument parsing. |
| test/integration/cli-smoke.test.ts | Adds hermetic CLI smoke coverage for api help/index and early usage errors. |
| test/integration/chr-smoke.test.ts | Adds a fast CHR smoke api GET round-trip. |
| test/integration/api.test.ts | CHR integration suite for api over REST (examples 1–20). |
| test/integration/api-native.test.ts | CHR integration suite for api over native API (examples N1–N8). |
| src/protocols/adapter.ts | Adds ProtocolApiRequest/apiRequest to adapters; implements REST/native mappings. |
| src/index.ts | Exports new api surfaces and protocol API request/result types. |
| src/execute.ts | Exposes promptForWriteConfirmation for reuse by api. |
| src/core/error-catalog.ts | Registers new usage/invalid-method error code summary. |
| src/cli/api.ts | Adds api command metadata, arg parsing, and CLI runner. |
| src/cli.ts | Wires api into the top-level CLI dispatch and help index. |
| src/api.ts | New api orchestrator: resolve/validate/confirm/execute + rendering. |
| README.md | Adds api to the command list. |
| package.json | Tightens git:pre-push to run lint:ci && test && build. |
| GLOSSARY.txt | Adds iff to the project glossary. |
| docs/MATRIX.md | Marks api as CHR-passed and documents supported/deferred scope. |
| docs/errors/usage/invalid-method.md | Adds the generated error page stub. |
| commands/api/README.md | Updates command status to CHR-passed and clarifies deferred --listen/fanout. |
| commands/api/examples.md | Adjusts example 20 to explicitly use -X POST + --yes. |
| commands/api/AGENTS.md | Records new CHR-grounded findings from Phase 2+3 integration. |
| commands/AGENTS.md | Adds api to the verbs list. |
Comment on lines
+271
to
+279
| const normalized = normalizeApiEndpoint(request.endpoint); | ||
| const method = parseApiMethod(request.method); | ||
| const verb = mapMethodToVerb(method); | ||
| const listen = (request.listen ?? false) || normalized.listen; | ||
| const scriptMode = pathTokens(normalized.path).at(-1) === "execute"; | ||
| const raw = request.raw ?? false; | ||
| const body = buildApiBody(request); | ||
| const query = buildApiQuery(request); | ||
| const proplist = buildApiProplist(request); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (2)
package.json (1)
60-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign
lint:git:pushwith the updatedgit:pre-pushhook.The
git:pre-pushscript was updated to runlint:ci && test && build, butlint:git:pushat Line 63 still runsbun run ci(which is onlylint && test && build, without markdown/cspell/secretlint checks). If both scripts serve the same purpose, they should be consistent; if they serve different purposes, the difference should be documented.- "lint:git:push": "bun run ci", + "lint:git:push": "bun run lint:ci && bun run test && bun run build",🤖 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 `@package.json` at line 60, The `lint:git:push` script is still out of sync with the updated `git:pre-push` hook, so update the matching script in package.json to use the same command sequence as `git:pre-push` or clearly separate their responsibilities. Use the existing script names `git:pre-push` and `lint:git:push` to locate the entries, and make sure both either run the same checks (`lint:ci`, `test`, `build`) or are explicitly documented as intentionally different.test/integration/api-native.test.ts (1)
164-179: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAvoid using REST as the oracle in the native-api suite.
chr.rest("/system/identity")makes this test fail when REST is unavailable even if the nativeapiEnvelope(... via: "native-api")path is healthy. Fetch the identity through the same native path, or just assert that the execute result is non-empty.Suggested change
- const identity = - ((await chr.rest("/system/identity")) as Record<string, string>)[ - "name" - ] ?? ""; - expect(identity.length).toBeGreaterThan(0); + const identityInfo = expectApiSuccess( + await apiEnvelope({ + ...base, + endpoint: "system/identity", + method: "GET", + }), + ); + const identity = + (identityInfo.data as Record<string, unknown>)["name"] ?? ""; + expect(String(identity).length).toBeGreaterThan(0);🤖 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 `@test/integration/api-native.test.ts` around lines 164 - 179, The native-api test is using chr.rest("/system/identity") as the source of truth, which ties the test to REST availability instead of the native path. Update the identity lookup in api-native.test.ts to use the same native apiEnvelope flow (or remove the identity dependency and assert the execute response is non-empty) so the test only validates the native /execute behavior via apiEnvelope and expectApiSuccess.
🤖 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 `@docs/errors/usage/invalid-method.md`:
- Around line 7-9: The invalid-method error doc still contains placeholder stub
text instead of actionable guidance; replace the “will be expanded” content in
the invalid-method markdown with the real supported-method and remediation
details. Use the existing docs structure in this page and align the wording with
the runtime error contract referenced by docs/CONSTITUTION.md so readers can see
which methods are accepted and how to recover.
In `@src/api.ts`:
- Around line 887-905: The error-envelope builder in src/api.ts is normalizing
an invalid request method into a fake GET/print operation via safeParseMethod,
mapMethodToVerb, and the operation.request metadata. Preserve the caller’s
original raw method on this error path instead of fabricating a valid one, or
make the method/verb fields nullable when parsing fails. Update the envelope
assembly near serializeCentrsError so the error metadata in this branch reflects
the actual invalid input rather than a rewritten request.
- Around line 544-546: The /execute scriptMode branch in api.ts is accepting a
missing or misspelled script key and defaulting to an empty script, which should
instead fail fast. Update the request building logic around the
resolved.scriptMode check to require an explicit resolved.body["script"] value,
validate that it exists and is non-empty, and return an actionable error if it
is missing rather than silently assigning "". Keep the behavior localized to the
request assembly path that sets request.script.
- Around line 599-637: The run-validation branch in src/api.ts is only checking
that the terminal token exists under its parent, so menu-backed POSTs and
malformed command arguments can slip through. Update the resolved.verb === "run"
path to validate against the command schema for the terminal node before
returning passed, using the existing inspectChildrenOrEmpty, isCommandNode, and
buildProtocolApiRequest flow as a guide. Ensure POST requests that carry a body
are rejected unless the terminal is a real command and its arguments/body fields
are valid, so --validate catches the bad run cases instead of only existence.
In `@src/cli/api.ts`:
- Around line 291-293: The unknown-flag handling in api.ts currently throws a
plain Unknown api flag error and bypasses the repo’s standard suggestion UX.
Update the default branch in the api command parsing flow to use the same
closest-match suggestion logic used elsewhere in the CLI so the error includes
canonical flag matches plus accepted aliases; reference the existing api flag
parsing path and any shared unknown-flag helper or matcher already used by other
commands, rather than hardcoding the message.
- Around line 357-378: The pre-envelope error fallback in src/cli/api.ts is
ignoring --raw and emitting plain text via formatCentrsErrorText even when
parsing succeeded in raw mode. Update the error path around the existing
withTips/buildApiErrorEnvelope/renderApiEnvelope logic so that when parsed?.raw
is true it reuses the structured envelope rendering (with the same raw JSON
contract as apiEnvelope()) instead of the text formatter, and keep the non-raw
branch using formatCentrsErrorText as today.
- Around line 384-405: The parse-time error path is ignoring the env-selected
output format because inferApiFormat only checks argv, so the fallback in
apiEnvelope can disagree with buildApiErrorEnvelope metadata. Update
inferApiFormat to use the same precedence as the rest of the API CLI flow by
honoring Bun.env/CENTRS_FORMAT before defaulting, and keep parsed.format and
argv flags as higher-priority overrides. Make the change in inferApiFormat so
the catch path and meta.operation.request.format stay aligned.
- Around line 408-413: The parseIntegerFlag helper currently uses
Number.parseInt, which accepts partially numeric inputs like 8728ms or 10abc, so
update this validation to reject anything that is not a full integer token
before conversion. Keep the check inside parseIntegerFlag in src/cli/api.ts, and
make sure the flag parsing path only accepts strings that match an ամբողջ
integer format, then convert to a number and preserve the existing error message
style when validation fails.
In `@src/protocols/adapter.ts`:
- Around line 251-271: The REST handling in adapter.ts’s print branch returns
GET via requestRest as soon as request.id is present, which skips any
request.query or request.proplist filters/projections. Update the print case to
detect id plus query/proplist together and send that path through the /print
POST flow instead, adding the id as an extra .query term via restQueryBody so
normalized REST behavior matches native. Keep the existing GET-by-id path only
for print requests with an id and no query/proplist, and make the same
adjustment in the duplicated print handling around the later block referenced by
the comment.
In `@test/unit/api-cli-args.test.ts`:
- Around line 117-120: The unknown-flag test is too weak because it only checks
for a throw, not the actionable guidance required by parseApiCliArgs. Update the
assertion in test/unit/api-cli-args.test.ts for parseApiCliArgs so it verifies
the error message includes the closest canonical matches and accepted aliases
for the bogus flag, using the parseApiCliArgs behavior in src/cli/api.ts as the
reference for the expected wording.
---
Nitpick comments:
In `@package.json`:
- Line 60: The `lint:git:push` script is still out of sync with the updated
`git:pre-push` hook, so update the matching script in package.json to use the
same command sequence as `git:pre-push` or clearly separate their
responsibilities. Use the existing script names `git:pre-push` and
`lint:git:push` to locate the entries, and make sure both either run the same
checks (`lint:ci`, `test`, `build`) or are explicitly documented as
intentionally different.
In `@test/integration/api-native.test.ts`:
- Around line 164-179: The native-api test is using chr.rest("/system/identity")
as the source of truth, which ties the test to REST availability instead of the
native path. Update the identity lookup in api-native.test.ts to use the same
native apiEnvelope flow (or remove the identity dependency and assert the
execute response is non-empty) so the test only validates the native /execute
behavior via apiEnvelope and expectApiSuccess.
🪄 Autofix (Beta)
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: CHILL
Plan: Pro Plus
Run ID: c153b2dd-c972-4541-9cc2-c828cd12797d
📒 Files selected for processing (22)
GLOSSARY.txtREADME.mdcommands/AGENTS.mdcommands/api/AGENTS.mdcommands/api/README.mdcommands/api/examples.mddocs/MATRIX.mddocs/errors/usage/invalid-method.mdpackage.jsonsrc/api.tssrc/cli.tssrc/cli/api.tssrc/core/error-catalog.tssrc/execute.tssrc/index.tssrc/protocols/adapter.tstest/integration/api-native.test.tstest/integration/api.test.tstest/integration/chr-smoke.test.tstest/integration/cli-smoke.test.tstest/unit/api-cli-args.test.tstest/unit/api.test.ts
Comment on lines
+7
to
+9
| See [`docs/CONSTITUTION.md`](../../CONSTITUTION.md) for the centrs error | ||
| contract. This stub will be expanded with the typical trigger and remediation | ||
| for `usage/invalid-method`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace the placeholder with the real fix guidance.
This page still says it "will be expanded", so the shipped error doc does not actually tell readers which methods are accepted or how to recover. Mirroring the concrete supported-method list from the runtime error would make the page useful immediately.
Suggested fix
See [`docs/CONSTITUTION.md`](../../CONSTITUTION.md) for the centrs error
-contract. This stub will be expanded with the typical trigger and remediation
-for `usage/invalid-method`.
+contract.
+
+Accepted methods for `centrs api` are `GET`, `PUT`, `PATCH`, `DELETE`, and `POST`.
+Use one of those with `-X/--method`, or omit the flag to use the default `GET`.📝 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.
Suggested change
| See [`docs/CONSTITUTION.md`](../../CONSTITUTION.md) for the centrs error | |
| contract. This stub will be expanded with the typical trigger and remediation | |
| for `usage/invalid-method`. | |
| See [`docs/CONSTITUTION.md`](../../CONSTITUTION.md) for the centrs error | |
| contract. | |
| Accepted methods for `centrs api` are `GET`, `PUT`, `PATCH`, `DELETE`, and `POST`. | |
| Use one of those with `-X/--method`, or omit the flag to use the default `GET`. |
🧰 Tools
🪛 LanguageTool
[grammar] ~7-~7: Ensure spelling is correct
Context: ...ION.md`](../../CONSTITUTION.md) for the centrs error contract. This stub will be expan...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 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 `@docs/errors/usage/invalid-method.md` around lines 7 - 9, The invalid-method
error doc still contains placeholder stub text instead of actionable guidance;
replace the “will be expanded” content in the invalid-method markdown with the
real supported-method and remediation details. Use the existing docs structure
in this page and align the wording with the runtime error contract referenced by
docs/CONSTITUTION.md so readers can see which methods are accepted and how to
recover.
Comment on lines
+544
to
+546
| if (resolved.scriptMode) { | ||
| request.script = resolved.body["script"] ?? ""; | ||
| return request; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require an explicit script field for /execute.
This branch silently drops every body key except script and turns a missing/misspelled one into an empty script. A request like -f scrpit=... now executes something different from what the caller asked for instead of failing fast with a usable error. As per coding guidelines, errors must be actionable for humans and agents.
Suggested fix
if (resolved.scriptMode) {
- request.script = resolved.body["script"] ?? "";
+ const { script, ...extra } = resolved.body;
+ if (typeof script !== "string" || script.trim().length === 0) {
+ throw new CentrsError({
+ code: "input/invalid-command",
+ summary: "`/execute` requires a non-empty `script` field.",
+ remediation:
+ "Pass the script with `-f script='...'`, `-d '{\"script\":\"...\"}'`, or `--input`.",
+ });
+ }
+ if (Object.keys(extra).length > 0) {
+ throw new CentrsError({
+ code: "usage/conflicting-flags",
+ summary: "`/execute` accepts only the `script` body field.",
+ remediation:
+ "Remove unexpected fields or call the concrete RouterOS menu/command instead of `/execute`.",
+ context: { extraFields: Object.keys(extra) },
+ });
+ }
+ request.script = script;
return request;
}📝 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.
Suggested change
| if (resolved.scriptMode) { | |
| request.script = resolved.body["script"] ?? ""; | |
| return request; | |
| if (resolved.scriptMode) { | |
| const { script, ...extra } = resolved.body; | |
| if (typeof script !== "string" || script.trim().length === 0) { | |
| throw new CentrsError({ | |
| code: "input/invalid-command", | |
| summary: "`/execute` requires a non-empty `script` field.", | |
| remediation: | |
| "Pass the script with `-f script='...'`, `-d '{\"script\":\"...\"}'`, or `--input`.", | |
| }); | |
| } | |
| if (Object.keys(extra).length > 0) { | |
| throw new CentrsError({ | |
| code: "usage/conflicting-flags", | |
| summary: "`/execute` accepts only the `script` body field.", | |
| remediation: | |
| "Remove unexpected fields or call the concrete RouterOS menu/command instead of `/execute`.", | |
| context: { extraFields: Object.keys(extra) }, | |
| }); | |
| } | |
| request.script = script; | |
| return request; |
🤖 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/api.ts` around lines 544 - 546, The /execute scriptMode branch in api.ts
is accepting a missing or misspelled script key and defaulting to an empty
script, which should instead fail fast. Update the request building logic around
the resolved.scriptMode check to require an explicit resolved.body["script"]
value, validate that it exists and is non-empty, and return an actionable error
if it is missing rather than silently assigning "". Keep the behavior localized
to the request assembly path that sets request.script.
Source: Coding guidelines
Comment on lines
+599
to
+637
| if (resolved.verb === "run") { | ||
| // The path already carries the command verb (`/interface/monitor-traffic`); | ||
| // verify it exists as a child of its parent menu. | ||
| const parent = tokens.slice(0, -1); | ||
| const terminal = tokens.at(-1) ?? ""; | ||
| const parentChildren = await inspectChildrenOrEmpty(backend, parent); | ||
| if (!parentChildren.some((child) => child.name === terminal)) { | ||
| throw unknownPathError(resolved); | ||
| } | ||
| const isCommand = parentChildren.some((child) => | ||
| isCommandNode(child, terminal), | ||
| ); | ||
| // A bare-collection POST carrying fields (terminal is a menu, not a command) | ||
| // likely meant PUT (RouterOS create). Advise, never rewrite the method. | ||
| if ( | ||
| resolved.method === "POST" && | ||
| !isCommand && | ||
| resolved.id === undefined && | ||
| Object.keys(resolved.body).length > 0 | ||
| ) { | ||
| tips.push( | ||
| buildTip( | ||
| "tip/rest-verb-mapping", | ||
| `POST ${resolved.path} carries fields but its terminal segment is a menu, not a command.`, | ||
| "RouterOS creates with PUT (`-X PUT`), not POST. Use PUT to add a row; centrs never rewrites your method.", | ||
| ), | ||
| ); | ||
| } | ||
| return { | ||
| validation: { | ||
| enabled: true, | ||
| source: "/console/inspect request=child", | ||
| result: "passed", | ||
| syntax: false, | ||
| semantic: true, | ||
| }, | ||
| tips, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
run validation currently accepts requests that cannot run.
This branch only proves that the terminal token exists under its parent. POST /ip/address therefore passes preflight as long as address is a known menu, and command argument typos also skip attribute validation entirely. Since buildProtocolApiRequest() forwards resolved.body for run, --validate is missing exactly the bad POST cases it should stop.
Suggested fix
const isCommand = parentChildren.some((child) =>
isCommandNode(child, terminal),
);
- // A bare-collection POST carrying fields (terminal is a menu, not a command)
- // likely meant PUT (RouterOS create). Advise, never rewrite the method.
- if (
- resolved.method === "POST" &&
- !isCommand &&
- resolved.id === undefined &&
- Object.keys(resolved.body).length > 0
- ) {
- tips.push(
- buildTip(
- "tip/rest-verb-mapping",
- `POST ${resolved.path} carries fields but its terminal segment is a menu, not a command.`,
- "RouterOS creates with PUT (`-X PUT`), not POST. Use PUT to add a row; centrs never rewrites your method.",
- ),
- );
+ if (!isCommand) {
+ throw new CentrsError({
+ code: "validation/unknown-path",
+ summary: `${resolved.path} is a menu, not a runnable command.`,
+ remediation:
+ resolved.method === "POST" && resolved.id === undefined
+ ? "Use `-X PUT` to add a row, or point POST at a concrete RouterOS command path."
+ : "Point the request at a concrete RouterOS command path.",
+ context: { path: resolved.path, method: resolved.method },
+ });
+ }
+ if (Object.keys(resolved.body).length > 0) {
+ const available = await inspectApiAttributes(backend, tokens);
+ const missing = Object.keys(resolved.body).filter(
+ (attribute) => !available.includes(attribute),
+ );
+ if (missing.length > 0) {
+ throw new CentrsError({
+ code: "validation/unknown-attribute",
+ summary: `Unknown RouterOS attribute ${missing.join(", ")} for ${resolved.path}.`,
+ remediation:
+ "Check the command arguments against `/console/inspect`, or disable validation only when intentionally probing an undocumented edge.",
+ });
+ }
}📝 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.
Suggested change
| if (resolved.verb === "run") { | |
| // The path already carries the command verb (`/interface/monitor-traffic`); | |
| // verify it exists as a child of its parent menu. | |
| const parent = tokens.slice(0, -1); | |
| const terminal = tokens.at(-1) ?? ""; | |
| const parentChildren = await inspectChildrenOrEmpty(backend, parent); | |
| if (!parentChildren.some((child) => child.name === terminal)) { | |
| throw unknownPathError(resolved); | |
| } | |
| const isCommand = parentChildren.some((child) => | |
| isCommandNode(child, terminal), | |
| ); | |
| // A bare-collection POST carrying fields (terminal is a menu, not a command) | |
| // likely meant PUT (RouterOS create). Advise, never rewrite the method. | |
| if ( | |
| resolved.method === "POST" && | |
| !isCommand && | |
| resolved.id === undefined && | |
| Object.keys(resolved.body).length > 0 | |
| ) { | |
| tips.push( | |
| buildTip( | |
| "tip/rest-verb-mapping", | |
| `POST ${resolved.path} carries fields but its terminal segment is a menu, not a command.`, | |
| "RouterOS creates with PUT (`-X PUT`), not POST. Use PUT to add a row; centrs never rewrites your method.", | |
| ), | |
| ); | |
| } | |
| return { | |
| validation: { | |
| enabled: true, | |
| source: "/console/inspect request=child", | |
| result: "passed", | |
| syntax: false, | |
| semantic: true, | |
| }, | |
| tips, | |
| }; | |
| } | |
| if (resolved.verb === "run") { | |
| // The path already carries the command verb (`/interface/monitor-traffic`); | |
| // verify it exists as a child of its parent menu. | |
| const parent = tokens.slice(0, -1); | |
| const terminal = tokens.at(-1) ?? ""; | |
| const parentChildren = await inspectChildrenOrEmpty(backend, parent); | |
| if (!parentChildren.some((child) => child.name === terminal)) { | |
| throw unknownPathError(resolved); | |
| } | |
| const isCommand = parentChildren.some((child) => | |
| isCommandNode(child, terminal), | |
| ); | |
| if (!isCommand) { | |
| throw new CentrsError({ | |
| code: "validation/unknown-path", | |
| summary: `${resolved.path} is a menu, not a runnable command.`, | |
| remediation: | |
| resolved.method === "POST" && resolved.id === undefined | |
| ? "Use `-X PUT` to add a row, or point POST at a concrete RouterOS command path." | |
| : "Point the request at a concrete RouterOS command path.", | |
| context: { path: resolved.path, method: resolved.method }, | |
| }); | |
| } | |
| if (Object.keys(resolved.body).length > 0) { | |
| const available = await inspectApiAttributes(backend, tokens); | |
| const missing = Object.keys(resolved.body).filter( | |
| (attribute) => !available.includes(attribute), | |
| ); | |
| if (missing.length > 0) { | |
| throw new CentrsError({ | |
| code: "validation/unknown-attribute", | |
| summary: `Unknown RouterOS attribute ${missing.join(", ")} for ${resolved.path}.`, | |
| remediation: | |
| "Check the command arguments against `/console/inspect`, or disable validation only when intentionally probing an undocumented edge.", | |
| }); | |
| } | |
| } | |
| return { | |
| validation: { | |
| enabled: true, | |
| source: "/console/inspect request=child", | |
| result: "passed", | |
| syntax: false, | |
| semantic: true, | |
| }, | |
| tips, | |
| }; | |
| } |
🤖 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/api.ts` around lines 599 - 637, The run-validation branch in src/api.ts
is only checking that the terminal token exists under its parent, so menu-backed
POSTs and malformed command arguments can slip through. Update the resolved.verb
=== "run" path to validate against the command schema for the terminal node
before returning passed, using the existing inspectChildrenOrEmpty,
isCommandNode, and buildProtocolApiRequest flow as a guide. Ensure POST requests
that carry a body are rejected unless the terminal is a real command and its
arguments/body fields are valid, so --validate catches the bad run cases instead
of only existence.
Comment on lines
+887
to
+905
| const method = safeParseMethod(request.method); | ||
| return { | ||
| ok: false, | ||
| error: serializeCentrsError(centrsError), | ||
| warnings: [], | ||
| tips: [], | ||
| meta: { | ||
| target: { input: request.targetInput }, | ||
| via: requestedVia, | ||
| settings: {}, | ||
| operation: { | ||
| kind: "api", | ||
| objectCount: 0, | ||
| request: { | ||
| endpoint: request.endpoint, | ||
| path: safeNormalizePath(request.endpoint), | ||
| method, | ||
| verb: mapMethodToVerb(method), | ||
| write: false, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Don't rewrite an invalid -X to GET in the error envelope.
When method parsing fails, the envelope metadata reports a valid GET/print request even though the caller asked for something else. That loses the original input on the exact error path humans and agents use for triage. Preserve the raw method or make these metadata fields nullable instead of fabricating a valid request. As per coding guidelines, errors must be actionable for humans and agents.
🤖 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/api.ts` around lines 887 - 905, The error-envelope builder in src/api.ts
is normalizing an invalid request method into a fake GET/print operation via
safeParseMethod, mapMethodToVerb, and the operation.request metadata. Preserve
the caller’s original raw method on this error path instead of fabricating a
valid one, or make the method/verb fields nullable when parsing fails. Update
the envelope assembly near serializeCentrsError so the error metadata in this
branch reflects the actual invalid input rather than a rewritten request.
Source: Coding guidelines
Comment on lines
+291
to
+293
| default: | ||
| if (arg.startsWith("-")) { | ||
| throw new Error(`Unknown api flag: ${arg}`); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Include closest matches for unknown flags.
Unknown api flag: ${arg} drops the repo’s unknown-flag UX contract for this new command. Please route this through the same suggestion logic used elsewhere so users get the closest canonical flags plus accepted aliases. Based on learnings, "On an unknown verb, sub-verb, or flag, the error must include the closest canonical matches plus accepted aliases."
🤖 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/api.ts` around lines 291 - 293, The unknown-flag handling in api.ts
currently throws a plain Unknown api flag error and bypasses the repo’s standard
suggestion UX. Update the default branch in the api command parsing flow to use
the same closest-match suggestion logic used elsewhere in the CLI so the error
includes canonical flag matches plus accepted aliases; reference the existing
api flag parsing path and any shared unknown-flag helper or matcher already used
by other commands, rather than hardcoding the message.
Source: Learnings
Comment on lines
+357
to
+378
| if (!parsed?.raw && (format === "json" || format === "yaml")) { | ||
| const envelope = withTips( | ||
| buildApiErrorEnvelope(parsed ?? { endpoint: "" }, error), | ||
| tips, | ||
| ); | ||
| console.error( | ||
| renderApiEnvelope(envelope, format, { | ||
| verbose: parsed?.verbose ?? false, | ||
| }), | ||
| ); | ||
| } else { | ||
| console.error( | ||
| formatCentrsErrorText( | ||
| asCentrsError(error, { | ||
| code: "input/invalid-command", | ||
| summary: error instanceof Error ? error.message : String(error), | ||
| remediation: | ||
| "Use `centrs api --help` to inspect the supported endpoint shape and flags.", | ||
| }), | ||
| { verbose: parsed?.verbose ?? args.includes("--verbose") }, | ||
| ) + formatTipsText(tips), | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Honor --raw in the pre-envelope error path.
If parsing succeeds with --raw but a later local failure happens before apiEnvelope() runs (for example, --input points to a missing file), this branch falls back to formatCentrsErrorText(...) and emits plain text. renderApiEnvelope(..., { raw: true }) already defines the raw error contract as compact JSON, so machine callers get a different shape on this path. Reuse the structured error envelope here instead of forcing text.
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 363-365: Clear-text logging of sensitive information
This logs sensitive data returned by an access to password as clear text.
This logs sensitive data returned by an access to password as clear text.
This logs sensitive data returned by an access to password as clear text.
This logs sensitive data returned by an access to password as clear text.
This logs sensitive data returned by an access to password as clear text.
This logs sensitive data returned by an access to password as clear text.
This logs sensitive data returned by an access to password as clear text.
[failure] 369-377: Clear-text logging of sensitive information
This logs sensitive data returned by an access to password as clear text.
This logs sensitive data returned by an access to password as clear text.
This logs sensitive data returned by an access to password as clear text.
This logs sensitive data returned by an access to password as clear text.
This logs sensitive data returned by an access to password as clear text.
This logs sensitive data returned by an access to password as clear text.
This logs sensitive data returned by an access to password as clear text.
🤖 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/api.ts` around lines 357 - 378, The pre-envelope error fallback in
src/cli/api.ts is ignoring --raw and emitting plain text via
formatCentrsErrorText even when parsing succeeded in raw mode. Update the error
path around the existing withTips/buildApiErrorEnvelope/renderApiEnvelope logic
so that when parsed?.raw is true it reuses the structured envelope rendering
(with the same raw JSON contract as apiEnvelope()) instead of the text
formatter, and keep the non-raw branch using formatCentrsErrorText as today.
Comment on lines
+384
to
+405
| function inferApiFormat( | ||
| args: readonly string[], | ||
| parsed?: ApiCliArgs, | ||
| ): ApiOutputFormat { | ||
| if (parsed?.format) { | ||
| return parsed.format; | ||
| } | ||
| if (args.includes("--json")) { | ||
| return "json"; | ||
| } | ||
| const index = args.indexOf("--format"); | ||
| if (index >= 0) { | ||
| const value = args[index + 1]; | ||
| if ( | ||
| value !== undefined && | ||
| apiOutputFormats.includes(value as ApiOutputFormat) | ||
| ) { | ||
| return value as ApiOutputFormat; | ||
| } | ||
| } | ||
| // api is machine-first: default to the structured json envelope, not text. | ||
| return "json"; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Parse-time failures ignore env-selected output format.
inferApiFormat() only looks at argv, so errors thrown before apiEnvelope() defaults to JSON even when CENTRS_FORMAT selects YAML/text. That leaves the catch path rendering in one format while buildApiErrorEnvelope(...) computes metadata from Bun.env, so the payload and its meta.operation.request.format can disagree. Mirror the same env-aware precedence here.
🤖 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/api.ts` around lines 384 - 405, The parse-time error path is ignoring
the env-selected output format because inferApiFormat only checks argv, so the
fallback in apiEnvelope can disagree with buildApiErrorEnvelope metadata. Update
inferApiFormat to use the same precedence as the rest of the API CLI flow by
honoring Bun.env/CENTRS_FORMAT before defaulting, and keep parsed.format and
argv flags as higher-priority overrides. Make the change in inferApiFormat so
the catch path and meta.operation.request.format stay aligned.
Comment on lines
+408
to
+413
| function parseIntegerFlag(value: string, flag: string): number { | ||
| const parsed = Number.parseInt(value, 10); | ||
| if (!Number.isInteger(parsed)) { | ||
| throw new Error(`${flag} must be an integer; got ${value}.`); | ||
| } | ||
| return parsed; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject partially numeric values for integer flags.
Number.parseInt accepts prefixes, so inputs like --port 8728ms or --count 10abc are silently treated as valid integers. That bypasses the actionable validation error this parser is trying to provide and can send requests to the wrong port. Validate the whole token before parsing.
🤖 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/api.ts` around lines 408 - 413, The parseIntegerFlag helper currently
uses Number.parseInt, which accepts partially numeric inputs like 8728ms or
10abc, so update this validation to reject anything that is not a full integer
token before conversion. Keep the check inside parseIntegerFlag in
src/cli/api.ts, and make sure the flag parsing path only accepts strings that
match an ամբողջ integer format, then convert to a number and preserve the
existing error message style when validation fails.
Comment on lines
+251
to
+271
| case "print": { | ||
| if (request.id) { | ||
| // GET one by id: RouterOS REST addresses a single object in the URL. | ||
| return { | ||
| data: await this.requestRest("GET", `${base}/${request.id}`), | ||
| }; | ||
| } | ||
| if ( | ||
| (request.query?.length ?? 0) > 0 || | ||
| (request.proplist?.length ?? 0) > 0 | ||
| ) { | ||
| // A GET cannot carry a body, so `.query`/`.proplist` projection rides a | ||
| // POST to the `/print` sub-endpoint (the documented REST idiom). | ||
| return { | ||
| data: await this.requestRest( | ||
| "POST", | ||
| `${base}/print`, | ||
| restQueryBody(request), | ||
| ), | ||
| }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Honor query/proplist when REST print also has an id.
Line 252 returns GET ${base}/${id} before considering request.query or request.proplist, so REST ignores filters/projections that the normalized request can carry and native already applies. Route id+query/proplist through /print and add the id as a .query term.
🐛 Proposed fix
case "print": {
- if (request.id) {
+ const hasQuery = (request.query?.length ?? 0) > 0;
+ const hasProplist = (request.proplist?.length ?? 0) > 0;
+ if (request.id && !hasQuery && !hasProplist) {
// GET one by id: RouterOS REST addresses a single object in the URL.
return {
data: await this.requestRest("GET", `${base}/${request.id}`),
};
}
- if (
- (request.query?.length ?? 0) > 0 ||
- (request.proplist?.length ?? 0) > 0
- ) {
+ if (request.id || hasQuery || hasProplist) {
// A GET cannot carry a body, so `.query`/`.proplist` projection rides a
// POST to the `/print` sub-endpoint (the documented REST idiom).
return {
data: await this.requestRest(
@@
function restQueryBody(request: ProtocolApiRequest): Record<string, unknown> {
const body: Record<string, unknown> = {};
- if (request.query && request.query.length > 0) {
- body[".query"] = request.query;
+ const query = [
+ ...(request.id ? [`.id=${request.id}`] : []),
+ ...(request.query ?? []),
+ ];
+ if (query.length > 0) {
+ body[".query"] = query;
}Also applies to: 967-975
🤖 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/protocols/adapter.ts` around lines 251 - 271, The REST handling in
adapter.ts’s print branch returns GET via requestRest as soon as request.id is
present, which skips any request.query or request.proplist filters/projections.
Update the print case to detect id plus query/proplist together and send that
path through the /print POST flow instead, adding the id as an extra .query term
via restQueryBody so normalized REST behavior matches native. Keep the existing
GET-by-id path only for print requests with an id and no query/proplist, and
make the same adjustment in the duplicated print handling around the later block
referenced by the comment.
Comment on lines
+117
to
+120
| test("an unknown flag is rejected", () => { | ||
| expect(() => parseApiCliArgs(["r", "x", "--bogus"])).toThrow( | ||
| "Unknown api flag", | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert the closest-match/alias guidance for unknown flags.
This only verifies that parsing throws, so it would still pass with the current bare Unknown api flag: ... fallback from src/cli/api.ts:160-304. Please tighten this assertion to cover the required actionable guidance as well. Based on learnings, "On an unknown verb, sub-verb, or flag, the error must include the closest canonical matches plus accepted aliases."
🤖 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 `@test/unit/api-cli-args.test.ts` around lines 117 - 120, The unknown-flag test
is too weak because it only checks for a throw, not the actionable guidance
required by parseApiCliArgs. Update the assertion in
test/unit/api-cli-args.test.ts for parseApiCliArgs so it verifies the error
message includes the closest canonical matches and accepted aliases for the
bogus flag, using the parseApiCliArgs behavior in src/cli/api.ts as the
reference for the expected wording.
Source: Learnings
Functional correctness: - Tighten `scriptMode` to a POST on the single-token `/execute` surface, so a GET or a nested menu that merely ends in `execute` is no longer mis-routed as a script run (Copilot). - Reject PATCH/DELETE without a row id up front (`input/invalid-path`) instead of issuing a `/undefined` REST URL or empty native `=.id=` (Copilot). - Require a non-empty `script` and forbid extra body fields for `/execute` (CodeRabbit) — a misspelled `script` key no longer runs an empty script. - Validate `run`-command arguments through inspect when a body is present, so a mistyped command argument is caught preflight (CodeRabbit). The documented `tip/rest-verb-mapping` advisory for a menu POST is kept per the constitution. - Honor `.query`/`.proplist` on a REST `print` that also carries an id: fold the id into `.query` (`.id=`) via POST `/print`, then unwrap to a single object — matching native's `?.id=` read (CodeRabbit). New CHR example 21 covers it. - Preserve the caller's raw `-X` on the error envelope instead of rewriting an invalid method to GET/print (`verb` is now null when unparseable) (CodeRabbit). CLI: - Add a shared `unknownFlagError` helper (Levenshtein "did you mean?") and wire it into the api parser so unknown flags suggest the closest canonical match (CodeRabbit). Rollout to the other parsers tracked separately. - Honor `--raw` on the pre-envelope error path (compact JSON, not text) and make parse-time format inference env-aware (`CENTRS_FORMAT`) (CodeRabbit). - Reject partially-numeric integer flags like `8728ms` / `10abc` (CodeRabbit). - Surface a missing `--input` file as `input/local-file-not-found` with remediation rather than `internal/unhandled`. Docs / tooling: - Replace the `usage/invalid-method` stub with the concrete accepted-method table (CodeRabbit). - Align `lint:git:push` with `git:pre-push` (`lint:ci && test && build`), the push hook now delegates to it (CodeRabbit). CHR-validated on 7.23.1: api (rest 1-21, 103 asserts), api-native (N1-N8), chr-smoke. lint:ci + 879 unit tests + build green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
Review disposition — commit
|
This was referenced Jul 2, 2026
5 tasks
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Builds the
centrs apicommand — a structured RouterOS API passthrough modeled ongh api— over bothrest-apiandnative-api, single-target. This is the structured middle of theexecute/retrieve/apiverb trichotomy (#91). Phases 0+1 (spec + sharedcore/inspect+core/fanout) merged in #109; this is the runtime.What's here
src/api.ts— transport-agnostic orchestrator: endpoint normalization (ip/address,/rest/ip/address,"ip address",ip/address/*1,…/listen), the gh-api method→verb map, theisApiMutatingwrite gate (keyed on verb/method, not the wire HTTP method),-f/-d/--inputbody,--query/--raw-query+--proplist, the/console/inspectgate (path existence + add/set attribute validity — no:put [:parse]), write-confirmation (reuses execute'spromptForWriteConfirmation), and the standard envelope +--rawpassthrough.src/cli/api.ts— arg parser + dispatch; wired intosrc/cli.tsandsrc/index.ts.ProtocolAdapter.apiRequestseam — REST maps the verb to an HTTP method + URL (id-in-path,.query/.proplistbody); native maps to a taggedtalk(?-words,=.id=,=.proplist=,addret→{.id}re-map,set/remove→null). Console adapters (ssh/mac-telnet) stubcapability-unsupported.usage/invalid-method(+ catalog entry + generated page).Done definition (CHR-passed)
Green on CHR 7.23.1 via
bun run test:integration:test/integration/api.test.ts— rest examples 1–20 (98 assertions)test/integration/api-native.test.ts— native N1–N8 (31 assertions)apiGET added tochr-smoke(the fast PR gate)Unit:
test/unit/api.test.ts+test/unit/api-cli-args.test.ts.docs/MATRIX.mdapi row →CHR-passed/CHR-passed.New grounding (recorded in
commands/api/AGENTS.md)Native
/executehonors=as-string=for synchronous output capture — without it the native API is fire-and-forget and returns a job id (e.g.*31), exactly like REST/rest/execute. This correctssrc/execute.ts's long-standing assumption that native script mode is unsupported (execute still blocks it; theapipath proves native/execute =as-string=works).Tooling fix
The pre-push git hook now runs
lint:ci && test && build(waslint && test && build), socspell/markdownlintgate the push — a spell error was previously slipping through to CI, where it skips the smoke tests (valuable feedback lost).Deferred to Phase 4 (with the
streamfold)--listenstreaming (native-api only).--via rest-api --listenalready errorstransport/capability-unsupported; native--listenerrorsusage/not-implemented.--group/--where/--all/--concurrency). No example exercises it yet, so it isn't CHR-validatable as "done" here; the README marks these flags as a later phase rather than advertising them as working.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
apicommand for structured RouterOS API passthrough, with support for multiple request types, output formats, and transport selection.Bug Fixes
Documentation
apicommand and related usage details.