Skip to content

Correctness sub-agent: add CLI operational safety checklist for destructive commands #264

Description

@fullsend-ai-retro

What happened

On PR #4081, the review agent's initial pass found 6 findings focused on code architecture (type assertions, interface design, input validation, scope coherence). Across two subsequent rounds, @waynesun09's multi-model review squad found 10 additional findings — all in the category of CLI operational safety — that the review agent missed entirely:

  1. [HIGH] Confirmation prompt only triggered by glob syntax, not explicit bulk lists (comment)
  2. [MEDIUM] Confirmation text said 'remove' when --uninstall also tears down infrastructure (comment)
  3. [MEDIUM] Guard variable deleted last, creating confusing intermediate state on partial failure (comment)
  4. [MEDIUM] --dry-run with --uninstall doesn't preview the uninstall portion (comment)
  5. [MEDIUM] --yes help text says 'glob patterns' but actually triggers on multiple repos (comment)
  6. [MEDIUM] filepath.Match error discarded, giving no feedback on malformed globs (comment)

These findings share a common theme: reasoning about "what does a production operator experience when using this command, especially when things go wrong?" The review agent's existing dimensions (correctness, security, intent, style, docs, cross-repo) do not specifically cover this class of concern. The gap added 4 extra days to the review cycle (Jul 13 squad review to Jul 17 merge).

What could go better

The correctness sub-agent's definition covers error handling gaps generically but has no specific checklist for CLI commands that perform infrastructure mutations. The human-directed squad consistently found issues in categories that are well-defined and checkable: (a) confirmation prompts must cover all destructive paths, not just one trigger condition, (b) --dry-run must preview all effects of all combined flags, (c) help text and error messages must accurately reflect actual behavior, (d) deletion/teardown ordering should be resilient to partial failure (delete the sentinel/guard first so partial failure leaves a clean state). These are not subjective design preferences — they are objective correctness checks that can be verified by reading the code. Confidence: HIGH — the gap is systematic (0 of 10 operational safety findings caught), the patterns are well-defined, and the correctness sub-agent already reads the relevant files but lacks the checklist to apply.

Proposed change

Add a 'CLI operational safety' checklist section to skills/pr-review/sub-agents/correctness.md that activates when the PR modifies CLI command implementations (files under cmd/ or internal/cli/ that register cobra commands or similar). The checklist should include:

  1. Confirmation coverage: Destructive commands (delete, remove, uninstall, teardown) must prompt for confirmation on ALL paths that affect multiple targets — not just glob patterns, but also explicit bulk argument lists. Check that the trigger condition matches the actual scope.
  2. Dry-run completeness: When --dry-run is combined with other flags (e.g., --uninstall, --force), the dry-run preview must cover all effects of all active flags. Check for conditional blocks like if !dryRun && someFlag that skip previewing someFlag's effects.
  3. Help text accuracy: Flag descriptions must match actual runtime behavior. When a behavioral change modifies when a flag takes effect (e.g., confirmation now triggers on multiple repos, not just globs), verify that help text is updated.
  4. Teardown/deletion ordering: When a command deletes multiple related resources, the sentinel or guard resource (the one that determines system state for other tools) should be deleted first, so partial failure leaves the system in the 'uninstalled' state rather than 'installed but broken'.
  5. Error message completeness: When multiple independent operations can fail, all errors must be reported (see parallel error consolidation), and error messages should distinguish which items succeeded vs failed and provide recovery guidance.

Each item should be flagged at medium severity. Items 1 and 2 should be high severity when the command deletes cloud infrastructure or modifies deployment state.

Validation criteria

On the next 5 PRs to fullsend-ai/fullsend that add or modify CLI commands performing destructive operations (under cmd/ or internal/cli/), the correctness sub-agent should evaluate at least 3 of the 5 checklist items and surface findings where applicable. Measure by comparing the sub-agent's findings against a manual audit of the same checklist items — the sub-agent should catch at least 60% of the issues a manual audit would find. Track over a 60-day window after the change lands.


Generated by retro agent from fullsend-ai/fullsend#4081

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions