Skip to content

chore(cli): reconcile monitors -> alerts API parity drift - #106

Merged
callumreid merged 1 commit into
mainfrom
chore/reconcile-monitors-alerts-parity
Aug 5, 2026
Merged

chore(cli): reconcile monitors -> alerts API parity drift#106
callumreid merged 1 commit into
mainfrom
chore/reconcile-monitors-alerts-parity

Conversation

@callumreid

Copy link
Copy Markdown
Contributor

Reconciles pre-existing API-parity drift found incidentally on 2026-08-04. Deliberately left out of #102 to keep that PR scoped to reports merge / reports rows.

The drift

python scripts/audit_api_coverage.py on clean main fails:

FAIL: 117/174 published operations have first-class CLI command coverage; 50 reviewed gaps remain.
new_gaps:                      unexpected_cli_operations:
  - GET /alerts                  - GET /monitors
  - POST /alerts                 - POST /monitors
  - GET /alerts/{alert_id}       - GET /monitors/{id}
  - PATCH /alerts/{alert_id}     - PATCH /monitors/{id}
  - DELETE /alerts/{alert_id}    - DELETE /monitors/{id}
  - GET /alerts/{alert_id}/events            - GET /monitors/{monitor_id}/events
  - POST /alerts/{alert_id}/test-evaluate    - POST /monitors/{monitor_id}/test-evaluate
snapshot_mismatches:
  - cli_supported_operations: recorded 124, current 117

coval-ai/backend#6368 (merged 2026-07-31) renamed the published spec monitors-v1 -> alerts-v1. The live catalog now lists an alerts spec and no monitors spec, while the CLI client still calls /v1/monitors/*.

The CLI is not broken — verified, not assumed

The rename was spec-only. /v1/monitors/* was retained as a serving legacy alias, so no shipped command regressed. Confirmed three independent ways:

  1. Live production probe (read-only GETs). GET /v1/monitors and GET /v1/alerts both return 200 with byte-identical payloads carrying both keys: {"alerts": [], "monitors": [], ...}.

  2. Control for route existence. Real routes reach the Lambda and return a Coval-shaped body; unrouted paths return API Gateway's own. This distinguishes "route missing" from "record missing":

    Request Response Meaning
    GET /v1/monitors/{ulid} {"error":{"code":"NOT_FOUND","message":"Monitor not found"}} route exists
    GET /v1/monitors/{ulid}/events {"error":{"code":"NOT_FOUND", ...}} route exists
    GET /v1/bogusresource {"message":"Not Found"} route absent
    GET /v1/monitors/{ulid}/bogus {"message":"Not Found"} route absent
  3. Backend origin/main. terraform/api-v1/modules/monitors_api_gateway.tf declares all 14 routes — 7 /alerts plus 7 /monitors — against the same integrations, and tests/integration/api/v1/test_monitors_e2e.py still exercises /monitors alongside the new test_alerts_e2e.py. That PR's own title says it retains the alias.

So this is coverage-manifest drift, not a broken-command problem. No command surface changes here.

Changes

api-coverage.toml only, plus the regenerated report:

  • 7 [[allowed_extra]] entries for the legacy /v1/monitors client operations, matching the client's actual path literals (the {id} / {monitor_id} mix is real — src/client/mod.rs uses different format! locals per method).
  • 7 [[known_gap]] entries for the canonical published /alerts operations, each naming the coval monitors subcommand that serves it today.
  • Snapshot: cli_supported_operations 124 -> 117, reviewed_at -> 2026-08-04.

Result — audit passes, exit 0:

PASS: 117/174 published operations have first-class CLI command coverage; 57 reviewed gaps remain.

python -m unittest scripts/test_audit_api_coverage.py — 21 tests, OK. No Rust files touched; nothing in src/ or tests/ reads this manifest.

Recommended follow-up (not in this PR)

Point the client at the canonical /v1/alerts paths and rename the command to coval alerts, keeping monitors as an alias so existing scripts keep working. That returns these 7 reviewed gaps to first-class coverage (117 -> 124) and stops the CLI depending on an undocumented alias that will eventually be withdrawn. Worth a ticket — the reviewed-gap reasons here are written to point at it.

The weekly automation is broken independently

.github/workflows/api-parity-audit.yml should have caught this in its rolling chore/weekly-api-parity PR. It didn't, and no such PR exists, because the workflow is failing — run 30808061069, 2026-08-03:

##[error]Input 'token' not supplied. Unable to continue.

It fails at Open or update the parity PR (peter-evans/create-pull-request@v7). The audit steps before it succeeded — that run printed PASS: 124/174, which is why the drift went unrecorded: the spec rename landed in the published catalog after 08-03.

secrets.REGEN_PR_TOKEN resolves to an empty value. The secret name exists (created 2026-07-31, never updated), but the step's input dump echoes commit-message, title, body, and branch while omitting token: entirely — an empty secret renders as absent. An expired-but-present PAT would instead fail with a 401.

This needs a repo-admin action I can't take: re-add REGEN_PR_TOKEN with a valid PAT (Contents + Pull requests write). Until then the weekly parity PR cannot open, and this manual reconciliation is the only thing keeping the manifest current. Issue #101 is open and tracking it; the workflow auto-closes it on the next green run.

Note continue-on-error on CI's audit step is false only for PRs from chore/weekly-api-parity, so on this branch the step is advisory — but it passes here regardless.

Merge-order note

#102 also edits api-coverage.toml (it removes the GET /reports/{report_id}/rows known_gap). Whichever lands second needs the snapshot's cli_supported_operations re-derived by rerunning the audit — the two changes move that number in opposite directions.

🤖 Generated with Claude Code

The public API renamed its published OpenAPI spec from monitors-v1 to
alerts-v1 in coval-ai/backend#6368, but deliberately retained /v1/monitors
as a serving legacy alias rather than removing it.

Verified against production on 2026-08-04 that all seven /v1/monitors routes
still reach their Lambda and return Coval-shaped responses, while an unrouted
path returns API Gateway's own not-found body. The shipped coval monitors
commands are therefore not broken, so this is coverage-manifest drift only and
no command surface changes.

Record the seven published /alerts operations as reviewed gaps, record the
seven legacy /v1/monitors client operations as allowed extras, and refresh the
coverage snapshot from 124 to 117 supported operations.

Follow-up, deliberately not in this PR: point the client at the canonical
/v1/alerts paths and rename the command to coval alerts with monitors kept as
an alias, which would return the seven reviewed gaps to first-class coverage.
@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: 9e3f6f1b-7352-499d-b806-b48ffb11a4d0

📥 Commits

Reviewing files that changed from the base of the PR and between a6a0de4 and 990693d.

📒 Files selected for processing (2)
  • api-coverage-report.md
  • api-coverage.toml

Walkthrough

The API coverage metadata now uses the 2026-08-04 review date and records 117 first-class CLI operations. It allows seven retained /monitors aliases for canonical /alerts operations. It records seven canonical /alerts operations as known gaps. The coverage report updates its summary to 57 reviewed gaps and lists the added alert operations.

Suggested reviewers: jakelevi

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the CLI API parity drift between monitors and alerts, which is the main change.
Description check ✅ Passed The description directly explains the API coverage reconciliation, manifest updates, audit results, and scope of the changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@callumreid
callumreid requested a review from jakelevi August 5, 2026 16:21
@callumreid
callumreid marked this pull request as ready for review August 5, 2026 16:21
@greptile-apps

greptile-apps Bot commented Aug 5, 2026

Copy link
Copy Markdown

Greptile Summary

Reconciles API coverage metadata after the published API renamed the monitors specification to alerts while retaining monitors as a legacy route alias.

  • Records seven legacy /monitors client operations as allowed extras.
  • Records seven canonical /alerts operations as reviewed CLI gaps.
  • Regenerates the coverage report and updates supported-operation counts from 124 to 117.

Confidence Score: 5/5

The PR appears safe to merge with no actionable defects identified.

The manifest operation names normalize to the existing client paths, all seven canonical alert gaps are accounted for, and the regenerated report and snapshot totals are mutually consistent.

Reviews (1): Last reviewed commit: "chore(cli): reconcile monitors -> alerts..." | Re-trigger Greptile

@callumreid
callumreid merged commit 22937d6 into main Aug 5, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants