Skip to content

Fix stale/incorrect CLI claims in skills and AGENTS.md - #27

Open
ndenny wants to merge 8 commits into
developfrom
fix/skill-docs-live-cli-review
Open

ndenny wants to merge 8 commits into
developfrom
fix/skill-docs-live-cli-review

Conversation

@ndenny

@ndenny ndenny commented Sep 19, 2026

Copy link
Copy Markdown
Member

Summary

Live-tested all 8 claude-skills workflows plus the 3 embedded onboarding skills (skills/embed/*.md) against a real apimetrics-qc account, project, and org (creating and fully cleaning up a scratch org project along the way), and fixed every confirmed discrepancy between the docs and actual CLI behavior.

Command-name / flag fixes

  • list-call-results doesn't exist → list-results-by-call. Its flags are --from/--result-category/--cursor/--limit, not --since/--before (same fix applied to list-results itself, which also has --from/--time, not --since/--before).
  • run-call doesn't exist → run-monitor <id> runs API, browser, and MCP monitors alike.
  • list-slos/get-slo/create-slo/update-slo/delete-slo don't exist. SLOs are one per project: get-project-slo / update-project-slo (creates one if none exists) / delete-project-slo, scoped via include_tags/exclude_tags — there's no scope/scope_id field or per-SLO ID. Rewrote apimetrics-slo-review's whole model around this.
  • list-schedules-for-call doesn't exist → list-schedules-by-call.

Behavioral corrections

  • get-result is documented everywhere as "always a summary" — that's only true for below-ANALYST callers. ANALYST+ (most project members) get the full request/response/timing/DNS/TLS object back by default, confirmed live. Updated AGENTS.md, apimetrics-project-bootstrap, apimetrics-failure-investigation, and apimetrics-weekly-health-review.
  • apimetrics-project-bootstrap claimed "the CLI cannot create a project" — the reasoning was wrong (create-project-in-org exists as a flat top-level command outside the project noun-group; the skill's step 1 only checked apimetrics project --help, which is a different, narrower command group), even though it happened to be right in effect for non-admin accounts (403). Documented the org-admin path and a previously-undocumented gotcha: creating a project does not grant the creator access to it — a follow-up create-project-access call is required before any create-call/monitor command in it will work. Verified this live end-to-end.
  • Fixed a result vs result_category field-name bug in the embedded browser/MCP skills — result carries a transport-completion value (COMPLETE), the PASS/FAIL/WARN/ERROR/TIMEOUT/QUEUED enum lives in result_category.

bulk init scheme bug (source + docs)

  • apimetrics-config-as-code told agents to prefer apimetrics:/<collection> as "the CLI's own API-name scheme." It doesn't work — there's no scheme-resolution logic anywhere in bulk/*.go; it's only a cosmetic example string in the bulk command's own --help (bulk/commands.go), and passing it does a literal DNS lookup on host apimetrics and fails. Fixed the example in commands.go to a resolvable placeholder host (matching bulk init --help's own existing convention), and pointed the skill at a real host+path, confirmed working live against qc-client.apimetrics.io.

Embedded onboarding skills (skills/embed/*.md)

  • All three (setup-api-monitor, setup-mcp-monitor, setup-browser-monitor) referenced a nonexistent --api-key flag in their error-recovery sections (AGENTS.md already correctly says this flag doesn't exist).
  • setup-api-monitor.md additionally had the run-call/list-call-results bugs above, plus a result.success/result.failure_reason response shape that doesn't exist.

Left unchanged

  • apimetrics-performance-analytics — every command, enum, and schema checked out live, no changes needed.
  • apimetrics-incident-triage and apimetrics-failure-investigation's already-correct sections — only the confirmed bugs above were touched.

Detail

How this was tested

Used apimetrics-qc (non-admin login, then a temporary org-admin grant on companyb for the mutating workflows). Created a scratch project via create-project-in-org, granted self access via create-project-access, ran the full create-call → set-call-conditions/get-call-conditions → create-schedule → run-monitor → get-result/list-results-by-call → cleanup (delete-schedule, delete-call, delete-project-from-org) loop, and separately exercised bulk init/status/list/diff in an isolated scratch directory. All test objects were deleted; the CLI's active project was restored to its original selection.

Not touched

skills/skills.go, skills/embed/* wiring, and anything outside doc/example text — no functional CLI behavior changed except the one cosmetic Example: string in bulk/commands.go. go build ./... passes.

🤖 Generated with Claude Code

Live-tested all 8 claude-skills workflows plus the three embedded
onboarding skills against a real apimetrics-qc account and org, and
fixed every confirmed discrepancy:

- list-call-results doesn't exist -> list-results-by-call, whose flags
  are --from/--result-category/--cursor/--limit, not --since/--before
  (also fixed on list-results itself).
- run-call doesn't exist -> run-monitor <id> runs API, browser, and MCP
  monitors alike.
- list-slos/get-slo/create-slo/update-slo/delete-slo don't exist. SLOs
  are one per project: get-project-slo/update-project-slo (creates if
  none exists)/delete-project-slo, scoped by include_tags/exclude_tags
  with no scope/scope_id field. Rewrote apimetrics-slo-review's model
  accordingly.
- list-schedules-for-call doesn't exist -> list-schedules-by-call.
- get-result's "always a summary" claim only holds for below-ANALYST
  callers; ANALYST+ (most project members) get the full
  request/response/timing/DNS/TLS object back by default.
- project-bootstrap's "the CLI cannot create a project" claim was
  wrong reasoning (create-project-in-org exists as a flat command
  outside the `project` noun-group) even though it happened to be
  right in effect for non-admin accounts. Documented the org-admin
  path, and the previously-undocumented gotcha that creating a project
  does not grant the creator access to it (needs a follow-up
  create-project-access call).
- bulk init's apimetrics:/<collection> scheme doesn't resolve -- it's
  a cosmetic example string in the CLI's own --help (bulk/commands.go)
  with no actual scheme-resolution logic behind it, and fails with a
  literal DNS lookup on host "apimetrics". Fixed the example in
  commands.go to a resolvable placeholder host, and pointed the skill
  at a real host+path instead.
- Three embedded onboarding skills (skills/embed/*.md) referenced a
  nonexistent --api-key flag in their error-recovery sections, and
  setup-api-monitor.md additionally had the run-call/list-call-results
  bugs above plus a result.success/failure_reason shape that doesn't
  exist (real field is result_category, PASS/FAIL/WARN/ERROR/TIMEOUT/
  QUEUED).

performance-analytics and the shared operating-rules boilerplate were
otherwise verified correct and left alone where no bug was found.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YKRzfBhwWbRrARvJsbFnaY

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Several updated docs are internally inconsistent about result status field naming and one bulk init example contradicts its surrounding “URL/link” guidance, which could mislead users copying these workflows.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 4 Low severity

Open (4)
What changed in this PR

Updates the CLI’s agent-facing documentation (AGENTS.md, installable skills, and embedded onboarding skills) to remove stale command/flag/response-shape claims and align guidance with the current observed CLI surface, plus a small help-text tweak in the hidden bulk command group.

Changes:

  • Fixes multiple incorrect command names/flags across skills (e.g., run-monitor vs run-call, list-results-by-call, --from/--time, per-project SLO commands).
  • Updates embedded onboarding skills to remove the nonexistent --api-key recovery path and to use the correct result-status field naming.
  • Replaces the bulk help example that previously used an unsupported apimetrics:/...-style address with a standard host/path example.
File Description
skills/​embed/​setup-mcp-monitor.md Updates polling/validation field name and removes nonexistent --api-key guidance.
skills/​embed/​setup-browser-monitor.md Updates polling/validation field name and removes nonexistent --api-key guidance.
skills/​embed/​setup-api-monitor.md Replaces nonexistent run-call/list-call-results usage with run-monitor/list-results-by-call and updates polling guidance.
skills/​claude-skills/​apimetrics-weekly-health-review/​SKILL.md Aligns list/result commands and flags; updates SLO/list envelope guidance.
skills/​claude-skills/​apimetrics-slo-review/​SKILL.md Reworks SLO model and commands to “one SLO per project” semantics.
skills/​claude-skills/​apimetrics-project-bootstrap/​SKILL.md Updates project creation/access notes and run/result guidance.
skills/​claude-skills/​apimetrics-monitoring-estate-audit/​SKILL.md Updates SLO and schedule-by-call guidance plus list flag corrections.
skills/​claude-skills/​apimetrics-incident-triage/​SKILL.md Updates list-results flags and related guidance.
skills/​claude-skills/​apimetrics-failure-investigation/​SKILL.md Updates list/run commands and result-detail guidance.
skills/​claude-skills/​apimetrics-config-as-code/​SKILL.md Documents bulk init address behavior and updates recommended bulk init usage.
bulk/​commands.go Updates the bulk group example string to use a standard host/path address format.
AGENTS.md Updates command naming, list envelope notes, SLO model, and bulk-init guidance.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread AGENTS.md Outdated
Comment thread skills/claude-skills/apimetrics-config-as-code/SKILL.md Outdated
Comment thread skills/claude-skills/apimetrics-incident-triage/SKILL.md Outdated
Comment thread skills/claude-skills/apimetrics-project-bootstrap/SKILL.md Outdated
…lk init example

- Standardize on result_category as the PASS/FAIL/WARN/ERROR/TIMEOUT/QUEUED
  field everywhere (AGENTS.md, failure-investigation, incident-triage,
  project-bootstrap), matching the embedded skills and the rest of this PR.
  A separate result field carries a transport-completion value (e.g.
  COMPLETE), which is now called out explicitly where the two could be
  confused.
- config-as-code: drop the redundant/inconsistent first bulk init example
  (it set url to a bare id without --url-template, contradicting the
  documented url/uri/self/link-or-url-template guidance) and keep the
  single --url-template form, which was confirmed working live.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YKRzfBhwWbRrARvJsbFnaY
@ndenny
ndenny marked this pull request as draft September 19, 2026 03:44
@ndenny
ndenny requested a balanced review from Copilot September 19, 2026 03:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The changes are documentation corrections plus one cosmetic example string; the verifiable claim (broken apimetrics:/ scheme) checks out against cli.FixAddress and the apimetrics config key, and the edits are internally consistent with prior feedback resolved.

Review effort: Balanced
Findings: None

Resolved since last review (4)

Several fixes from the previous commits were written as a diff against
the prior (incorrect) text -- "there is no list-call-results (use
list-results-by-call)", "the apimetrics:/<collection> scheme does not
work", "there is no run-call" -- which only makes sense to someone who
saw the old version. A fresh reader (a new agent invoking the skill,
someone who never saw the prior draft) has no context for what's being
negated. Rewrote all of these to state the current CLI surface
directly and positively, and swapped the placeholder `<call-or-monitor-id>`
positional argument name for `<monitor-id>`, matching this codebase's
convention of "monitor" as the umbrella term for an API call/browser
test/MCP test.

Left alone the handful of pre-existing "there is no --api-key flag" /
"there is no --body/--data/-d flag" warnings, which predate this PR,
don't reference anything removed from these docs, and read fine
standalone as guardrails against a very plausible wrong guess.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YKRzfBhwWbRrARvJsbFnaY
@ndenny
ndenny requested a balanced review from Copilot September 21, 2026 14:31
@ndenny
ndenny marked this pull request as ready for review September 21, 2026 14:31
@ndenny
ndenny enabled auto-merge September 21, 2026 14:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The slo-review skill introduces an incorrect flag --apimetrics-project-id, whereas the CLI's actual global flag is --project-id, contradicting this PR's goal of removing inaccurate CLI claims.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 Low severity

Open (1)

Comment thread skills/claude-skills/apimetrics-slo-review/SKILL.md Outdated
--apimetrics-project-id does exist (confirmed live in get-project-slo
and update-project-slo's own generated option schema), but it isn't
the documented, general-purpose mechanism for targeting a different
project. That's the global --project-id flag (registered in
cli/cli.go, and what cli/config.go's "no active project" error tells
users to pass), so point at that instead.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YKRzfBhwWbRrARvJsbFnaY

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The updated bulk top-level help example still uses a potentially stale /monitors path and should be aligned with the repo’s other bulk init examples to avoid reintroducing misleading CLI help output.

Review effort: Lite
Findings: None

Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Low severity Top-level bulk help uses stale /monitors path

bulk/​commands.go:317

The top-level bulk command help example uses the path /monitors, but this endpoint/path isn't referenced anywhere else in the repo (and the bulk init subcommand examples use /users). Keeping /monitors here risks reintroducing a stale or misleading hint in CLI help text; consider aligning it with the bulk init examples.

commands.go:317's example used /monitors while the bulk init subcommand's
own examples (line 346, pre-existing) use /users -- inconsistent for no
reason. Match them.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YKRzfBhwWbRrARvJsbFnaY

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The documentation's correctness depends on many command-name and behavioral claims that are generated at runtime from the live OpenAPI spec and cannot be independently verified from repository source, so a human familiar with the live CLI should confirm them.

Review effort: Balanced
Findings: None

### 1. Discover SLO operations

The SLO command set is CRUD only: `list-slos`, `get-slo <slo-id>` (not `read-slo`), `create-slo`, `update-slo`, `delete-slo`. There is **no SLO attainment, status, or error-budget endpoint** — the CLI returns SLO *definitions*, not computed attainment. Plan to derive attainment yourself from result and performance data (step 4).
**There is exactly one SLO per project, not a list of named SLOs.** The real commands are `get-project-slo`, `update-project-slo` (also *creates* the project's SLO if none exists yet), and `delete-project-slo` — none of them take an SLO ID. Use the global `--project-id` flag only if targeting a different project than the active one. There is **no attainment, status, or error-budget endpoint** — the CLI returns the SLO *definition*, not computed attainment. Plan to derive attainment yourself from result and performance data (step 4).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of the changes in this file are contrasting the behaviour against what was previously reported in the skill. This is a waste of tokens since it wouldn't assume otherwise if not for the previous skill description.

@rujames rujames Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A search for ", not" (e.g. "one SLO per project , not a list") is pretty good at picking up other instances of this pattern.

Comment thread skills/embed/setup-mcp-monitor.md Outdated

- **400 on create:** Confirm `name` and `url` are both provided and that the URL is a valid SSE endpoint.
- **401/403:** Confirm `--api-key` or project is configured. Run `apimetrics project show` to check the active project.
- **401/403:** There is no `--api-key` flag — authentication is via `apimetrics login` (OAuth). Confirm login state and that a project is active with `apimetrics project show`.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, I feel like specifying There is no '--api-key' flag is at best a waste of tokens and at worst going to get it confused:

Suggested change
- **401/403:** There is no `--api-key` flag — authentication is via `apimetrics login` (OAuth). Confirm login state and that a project is active with `apimetrics project show`.
- **401/403:** Confirm login state and that a project is active with `apimetrics project show`.

Comment thread AGENTS.md Outdated
- **Output**: `-o json` for machine use; `-f` projects with a shorthand query; `-q` adds confirmed query params. The command spec is cached ~24h and refreshes automatically; `--rsh-no-cache` forces a refresh.
- **List envelopes are not uniform.** `list-calls`, `list-results`, `list-call-results`, and `list-auth-settings` return `{ "meta": ..., "results": [...] }`; `list-schedules` returns `{ "data": [...] }`; `list-slos`, `list-browser-monitors`, and `list-mcp-monitors` return a bare `{ "results": [...] }`. Inspect each command's own output before writing an `-f` path.
- **`get-result` is a summary** (`result`, `http_code`, `response_time` ms, `location_id`, `test`, `created`; `result` ∈ `PASS`/`FAIL`/`WARN`/`ERROR`/`TIMEOUT`/`QUEUED`). Deeper data comes from `get-result-content`, `get-result-screenshot`, the `query-*-performance`/`query-*-dns-diagnostics` commands, and `conformance-results`.
- **List envelopes are not uniform.** `list-calls`, `list-results`, `list-results-by-call`, and `list-auth-settings` return `{ "meta": ..., "results": [...] }`; `list-schedules` returns `{ "data": [...] }`; `list-browser-monitors` and `list-mcp-monitors` return a bare `{ "results": [...] }`; `get-project-slo` returns a bare single SLO object (one SLO per project, not a list — see below). Inspect each command's own output before writing an `-f` path.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- **List envelopes are not uniform.** `list-calls`, `list-results`, `list-results-by-call`, and `list-auth-settings` return `{ "meta": ..., "results": [...] }`; `list-schedules` returns `{ "data": [...] }`; `list-browser-monitors` and `list-mcp-monitors` return a bare `{ "results": [...] }`; `get-project-slo` returns a bare single SLO object (one SLO per project, not a list — see below). Inspect each command's own output before writing an `-f` path.
- **List envelopes are not uniform.** `list-calls`, `list-results`, `list-results-by-call`, and `list-auth-settings` return `{ "meta": ..., "results": [...] }`; `list-schedules` returns `{ "data": [...] }`; `list-browser-monitors` and `list-mcp-monitors` return a bare `{ "results": [...] }`. Inspect each command's own output before writing an `-f` path.

- For availability/pass objectives, count `result` categories over the objective `period` using `list-results`/`list-call-results` with `--since`/`--before`.
- For latency objectives (`measure: mean`, metrics like `total`), use `query-api-performance` / `query-api-monitor-performance` with matching `metrics`/`measures` and window; align the query `interval` to the objective `period`.
- For availability/pass objectives, count `result_category` values over the objective `period` using `list-results`/`list-results-by-call` with `--from`/`--time`.
- For latency objectives, use `query-api-performance` / `query-api-monitor-performance` with matching `metrics`/`measures` and window; align the query `interval` to the objective `period`. Confirm the objective's actual `metric` name (e.g. `slow`, `dns`, `tcp`, `casc`) maps to a real `query-*-performance` metric/measure before querying — don't assume it's called `total`.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now this refers to the project SLOs and not the new ones, we must have an endpoint which exposes attainment, right? We shouldn't be getting the client to derive aggregate statistics from the list endpoints.

ndenny and others added 3 commits September 23, 2026 16:43
- State facts directly instead of contrasting with what earlier skill
  text claimed (one SLO per project, list-results flags, run-monitor,
  project creation).
- Remove the --api-key references from AGENTS.md and the embedded
  setup skills.
- Drop get-project-slo from the list-envelope summaries.
- slo-review: derive availability attainment from get-call-passfail-range/
  get-call-passfail-total and latency from query-*-performance, keeping
  list-results for drilling into violations; add the same guidance to
  AGENTS.md.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VcnM8iRzyY2ZEyNi6iyp28
- Drop the "not noun/verb groups" and "Do not invent --body" lines from
  the shared skill preamble, and the matching "There is no --body" line
  from the embedded setup skills.
- skills.go: generated agent guide states the flat-command and stdin
  facts directly, matching AGENTS.md.
- weekly-health-review: take pass/warning/failure counts from
  get-call-passfail-range instead of paging list-results and counting;
  keep list-results for failure detail. SLO attainment points at the
  slo-review method. AGENTS.md table row updated to match.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VcnM8iRzyY2ZEyNi6iyp28
…e-cli-review

# Conflicts:
#	skills/skills.go

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

One embedded-skill "hard rule" claims list-results-by-call works across API/browser/MCP monitors, contradicting the API-call-only scoping used everywhere else in this PR.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 Low severity

Open (1)

## Hard rules

- Always verify the call ID before attaching to a schedule — attaching the wrong ID silently succeeds.
- `run-monitor` and `list-results-by-call` work the same way across monitor types — API, browser, or MCP.
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.

3 participants