feat(sp-skill-catalog): add cyclone-cli skill (CLI conventions)
This commit is contained in:
Tyler
@@ -0,0 +1,187 @@
|
||||
---
|
||||
name: cyclone-cli
|
||||
description: "Cyclone CLI subcommand conventions (cli.py — Click group + subcommands parse-837/parse-835/validate-npi/validate-tax-id/backup, --yes + click.confirm for destructive ops, exit codes 0/1/2, CliRunner smoke tests in backend/tests/test_cli_*.py). Use when: adding a CLI subcommand, changing an exit code, adding a smoke test, or wiring a security-sensitive command (backup, key rotation, anything touching secrets.py)."
|
||||
---
|
||||
|
||||
# cyclone-cli
|
||||
|
||||
The operator-facing CLI is a **Click** group at `cli.py:45` (`@click.group()` for `main`), mounted as the `cyclone` console script in `pyproject.toml:54` (`cyclone = "cyclone.cli:main"`). Seven subcommands ship today: `parse-837`, `parse-835`, `validate-npi`, `validate-tax-id`, plus the `backup` group (`init-passphrase`, `create`, `list`, `verify`, `restore`, `prune`, `status`). `serve` lives separately in `__main__.py:19` (dispatches `uvicorn cyclone.api:app`).
|
||||
|
||||
## When to use
|
||||
|
||||
- **Adding a subcommand.** You need a new operator command (e.g. `cyclone rotate-key`) and want to match the existing Click decorator + smoke-test rhythm.
|
||||
- **Changing an exit code.** You're tweaking which `sys.exit(N)` a subcommand raises and need the 0/1/2 contract used by `parse_837`, `parse_835`, `validate_npi_cmd`, `validate_tax_id_cmd`, and the `backup` group.
|
||||
- **Adding a smoke test.** You need `backend/tests/test_cli_<name>.py` using `click.testing.CliRunner` (NOT `subprocess.run`) and want the canonical fixture + monkeypatch layout (Keychain stub, fresh SQLite, `CYCLONE_BACKUP_DIR`).
|
||||
- **Wiring a security-sensitive command.** Anything touching `cyclone/secrets.py` (Keychain writes), DB key rotation, or destructive restores needs the two-step confirm dance used by `backup restore` / `backup prune` (`cli.py:462,509`).
|
||||
|
||||
## Conventions
|
||||
|
||||
1. **Click decorator pattern, not argparse.** Each subcommand is a
|
||||
top-level function decorated with `@main.command("<name>")` and
|
||||
one `@click.option` / `@click.argument` per parameter. Group
|
||||
dispatch is implicit — no `set_defaults(func=...)` and no
|
||||
`cmd_<name>(args) -> int` signature. Top-level commands at
|
||||
`cli.py:77,151,241,261,303`; the `backup` sub-group nests a
|
||||
second `@main.group()` (`cli.py:303`) with its own
|
||||
`@backup.command("<name>")` children.
|
||||
2. **Long-form flags for safety.** Prefer `--rotate-key`,
|
||||
`--backup-dir`, `--from-stdin` over positional args for anything
|
||||
that mutates state or takes a secret. `init-passphrase`
|
||||
(`cli.py:308-311`) demonstrates the canonical "flag OR stdin"
|
||||
pattern: `--passphrase` for automation, `--from-stdin` for
|
||||
interactive `getpass()` prompting.
|
||||
3. **Exit codes: 0 / 1 / 2.** `0` = success. `1` = user / input
|
||||
error (invalid NPI/EIN at `cli.py:258,277,285`; Keychain write
|
||||
failure at `cli.py:341,351`; tampered-backup verify at
|
||||
`cli.py:459`). `2` = operator / parse error
|
||||
(`CycloneParseError` at `cli.py:106,178`; passphrase
|
||||
empty/mismatch/short at `cli.py:331,337`). Document the codes
|
||||
in the docstring (see `validate_npi_cmd` at `cli.py:244-250`).
|
||||
Use `click.UsageError(...)` for usage mistakes; reserve
|
||||
`sys.exit(2)` for "the file failed to parse" semantics.
|
||||
4. **Smoke test with `click.testing.CliRunner`.** Every new
|
||||
subcommand gets `backend/tests/test_cli_<name>.py` that
|
||||
imports `from cyclone.cli import main` and invokes via
|
||||
`CliRunner().invoke(main, [...], catch_exceptions=False)`. The
|
||||
test must stub Keychain (`monkeypatch.setattr(secrets_mod,
|
||||
"get_secret"/"set_secret", ...)`) and pin a temp SQLite DB via
|
||||
`CYCLONE_DB_URL` + `db._reset_for_tests()` — see
|
||||
`backend/tests/test_cli_backup.py:13-69` for the canonical
|
||||
`_cli_env` fixture. CliRunner captures output and exit codes
|
||||
in-process; do NOT shell out to `subprocess.run`.
|
||||
5. **Destructive ops need `--yes` + `click.confirm(abort=True)`.**
|
||||
`backup restore` (`cli.py:462-506`) and `backup prune`
|
||||
(`cli.py:509-537`) both gate the destructive action behind a
|
||||
`--yes` is_flag and an interactive `click.confirm(..., abort=True)`
|
||||
prompt. CliRunner auto-aborts confirm prompts, so smoke tests
|
||||
assert `exit_code != 0` when `--yes` is omitted
|
||||
(`test_cli_backup.py:122-137`). A `--dry-run` flag is NOT yet
|
||||
implemented anywhere; if needed, mirror the `--yes` pattern.
|
||||
|
||||
## Patterns
|
||||
|
||||
### A Click subcommand — `@main.command("<name>")` + options
|
||||
|
||||
From `cli.py:241-258` (smallest standalone subcommand):
|
||||
|
||||
```python
|
||||
@main.command("validate-npi")
|
||||
@click.argument("npi")
|
||||
@click.option("--log-level", default="WARNING", show_default=True,
|
||||
type=click.Choice(["DEBUG", "INFO", "WARNING", "ERROR"]))
|
||||
def validate_npi_cmd(npi: str, log_level: str) -> None:
|
||||
"""Validate a 10-digit NPI's Luhn checksum locally (SP20). Exit 0 valid, 1 invalid. PHI — don't log the value."""
|
||||
setup_logging(level=log_level)
|
||||
from cyclone.npi import is_valid_npi
|
||||
if is_valid_npi(npi):
|
||||
click.echo(f"OK: {len(npi)}-digit NPI passes Luhn checksum")
|
||||
return
|
||||
click.echo(f"INVALID: {npi!r} fails NPI Luhn checksum", err=True)
|
||||
sys.exit(1)
|
||||
```
|
||||
|
||||
### A smoke test — `CliRunner` + Keychain stub + temp SQLite
|
||||
|
||||
From `test_cli_backup.py:13-69`. Stable hex salt keeps multiple `CliRunner` invocations consistent within one test. The `Batch` seed at `cli_backup.py:27-35` is omitted — only the Keychain + DB plumbing is the convention:
|
||||
|
||||
```python
|
||||
@pytest.fixture
|
||||
def _cli_env(tmp_path, monkeypatch):
|
||||
from cyclone import db, secrets as secrets_mod
|
||||
from cyclone import backup_service as svc_mod
|
||||
|
||||
monkeypatch.setenv("CYCLONE_DB_URL", f"sqlite:///{tmp_path}/test.db")
|
||||
db._reset_for_tests()
|
||||
db.init_db()
|
||||
# ... seed any DB rows the subcommand needs (see cli_backup.py:27-35) ...
|
||||
|
||||
store = {svc_mod.KEYCHAIN_BACKUP_PASSPHRASE_ACCOUNT: "cli-test-passphrase",
|
||||
svc_mod.KEYCHAIN_BACKUP_SALT_ACCOUNT:
|
||||
"0123456789abcdef0123456789abcdef"}
|
||||
monkeypatch.setattr(secrets_mod, "get_secret", lambda n: store.get(n))
|
||||
monkeypatch.setattr(secrets_mod, "set_secret",
|
||||
lambda n, v: store.__setitem__(n, v) or True)
|
||||
monkeypatch.setenv("CYCLONE_BACKUP_DIR", str(tmp_path / "backups"))
|
||||
yield tmp_path / "backups"
|
||||
db._reset_for_tests()
|
||||
|
||||
def test_backup_create_list_verify_status(_cli_env):
|
||||
from cyclone.cli import main
|
||||
runner = CliRunner()
|
||||
r = runner.invoke(main, ["backup", "create"], catch_exceptions=False)
|
||||
assert r.exit_code == 0, r.output
|
||||
assert "created backup id=" in r.output
|
||||
```
|
||||
|
||||
### A destructive subcommand — `--yes` + `click.confirm(abort=True)`
|
||||
|
||||
From `cli.py:462-506` (`backup restore`). Two-step: announce, prompt unless `--yes`, then execute. Restore uses an explicit init/confirm round-trip so the operator can back out between phases. Matching smoke test asserts the guard fires:
|
||||
|
||||
```python
|
||||
@backup.command("restore")
|
||||
@click.argument("backup_id", type=int)
|
||||
@click.option("--yes", is_flag=True, help="Skip the interactive confirm prompt")
|
||||
@click.option("--actor", default="operator-cli", show_default=True)
|
||||
def backup_restore(backup_id: int, yes: bool, actor: str) -> None:
|
||||
"""Restore the live DB from a backup (two-step, requires --yes)."""
|
||||
# ... db.init_db() + service config omitted ...
|
||||
click.echo(f"Initiating restore from backup {backup_id}...")
|
||||
init = svc.restore_initiate(backup_id)
|
||||
# ... echo init summary (filename, fp, table_count, ttl) ...
|
||||
if not yes:
|
||||
click.confirm(
|
||||
"Replace the live DB with this backup? "
|
||||
"This will dispose the engine and rebuild it.",
|
||||
abort=True,
|
||||
)
|
||||
click.echo("Confirming restore...")
|
||||
result = svc.restore_confirm(backup_id, init.restore_token, actor=actor)
|
||||
|
||||
def test_backup_restore_requires_yes_flag(_cli_env):
|
||||
runner = CliRunner()
|
||||
runner.invoke(main, ["backup", "create"], catch_exceptions=False)
|
||||
r = runner.invoke(main, ["backup", "restore", "1"], catch_exceptions=False)
|
||||
# CliRunner auto-aborts confirm prompts → exit_code != 0.
|
||||
assert r.exit_code != 0
|
||||
```
|
||||
|
||||
## Anti-patterns
|
||||
|
||||
- **Don't `sys.exit(2)` for usage errors.** Reserve code 2 for
|
||||
parse/operator errors (`CycloneParseError`, Keychain not
|
||||
initialized, passphrase policy violation). For "you passed the
|
||||
wrong flag" use `raise click.UsageError(...)` — Click formats
|
||||
it as a clean help message and exits 2 on its own.
|
||||
- **Don't print errors to stdout.** Use `click.echo(msg, err=True)`
|
||||
for all error output. `print(..., file=sys.stderr)` and bare
|
||||
`logging.error(...)` bypass Click's stdout/stderr split and leak
|
||||
into test `result.output` — breaking `assert "FAIL" in r.output`
|
||||
style assertions.
|
||||
- **Don't add a subcommand without a smoke test.** Every new
|
||||
`@main.command(...)` ships a sibling
|
||||
`backend/tests/test_cli_<name>.py` exercising the happy path
|
||||
AND at least one error path (missing input, invalid arg,
|
||||
tampered ciphertext — see `test_cli_backup.py:102-119`).
|
||||
- **Don't reuse the `parse` command name.** Existing subcommands
|
||||
are type-specific (`parse-837`, `parse-835`); a generic `parse`
|
||||
would shadow them or force an `--type` flag — neither is the
|
||||
codebase pattern.
|
||||
|
||||
## Related skills
|
||||
|
||||
- **`cyclone-store`** — `backup` subcommands and the parse
|
||||
subcommands both round-trip through `CycloneStore` and the DB
|
||||
session; load when the increment changes write paths or the
|
||||
`<entity>_written` event contract.
|
||||
- **`cyclone-api-router`** — `cyclone serve` (via `__main__.py:19`)
|
||||
launches the FastAPI app; the CLI parse subcommands share the
|
||||
same `CycloneParseError` exception and Pydantic result models
|
||||
as the matching HTTP endpoints.
|
||||
- **`cyclone-edi`** — `parse-837` / `parse-835` are the CLI smoke
|
||||
entry points for the parser/validator surface; load when adding
|
||||
a parser or R-code rule.
|
||||
- **`cyclone-tests`** — every CLI subcommand gets a pytest smoke
|
||||
case under `backend/tests/test_cli_<name>.py`; the `_cli_env`
|
||||
fixture pattern (fresh SQLite + Keychain stub) is documented
|
||||
there.
|
||||
- **`cyclone-spec`** — load when the SP-N spec introduces a new operator command or reserves a new exit-code category.
|
||||
Reference in New Issue
Block a user