Cleanup 418 (v2): doctor + GPG signing + command reduction, with Copilot review fixes#1243
Cleanup 418 (v2): doctor + GPG signing + command reduction, with Copilot review fixes#1243jpoley wants to merge 5 commits into
Conversation
…ixes Add `flowspec doctor` health-check command and `flowspec gpg` agent signing module, fully addressing all Copilot findings from PRs #1236, #1237, #1238, and #1239 plus a thorough independent security review. Key design decisions in signing module: - Fingerprint normalization centralized in get_key_fingerprint() (strip, uppercase, 40-char hex validation, logging on failure) - Key generation uses --status-fd for reliable fingerprint extraction - Secret key verification uses exact fingerprint match (case-insensitive) - All test mocks use realistic 40-char hex fingerprints via TEST_FP constant Doctor module improvements: - httpx imported at module level (coding standard compliance) - Strict flow.<phase>.agent.md validation (rejects flow.agent.md) - UTF-8 encoding on all file I/O - Version check distinguishes "confirmed current" from "could not check" Documentation quality: - All GPG commands use fingerprint (not email) to avoid ambiguity - Broken related-doc links replaced with anchor links - Security note about ~/.gnupg file permissions added - Manual deletion warns about keyring cleanup Closes #1221 #1222 #1223 #1224 #1225 #1226 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: jason poley <jason.poley@gmail.com>
Remove commands: - /flow:research (rarely used) - /flow:intake (rarely used) - /flow:custom (power user only) - /flow:security_* (5 commands - not core workflow) - /vibe (casual mode) Rewrite README.md: - Document all 15 remaining commands - Organize into Core (5), Setup (2), Sub-commands (5), Utilities (2), Automation (1) - Remove bloated ASCII diagrams - Remove false "fully supported" claims for agents - Remove legacy /speckit section - Accurate project structure Command count: 22 → 15 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: jason poley <jason.poley@gmail.com>
Four quality issues flagged by Copilot review on the consolidation PR: - doctor.py: the "beads CLI" check was probing the wrong binary (`beads` instead of `bd`). Reworked `check_tool_installed` to accept a separate `display_name` and an optional `version_getter`, then wired `run_all_checks` to look up `bd` while still showing "beads CLI" in output. - doctor.py: reuse existing `check_backlog_installed_version` / `check_beads_installed_version` helpers from the package root so version parsing stays consistent with the rest of the CLI (replaces the generic `split()[-1]` heuristic). Lazy-imported to avoid a circular import with `flowspec_cli/__init__.py`. - doctor.py: remove the misleading `flowspec doctor --fix` hint — auto-fix is not implemented, so the hint was followed by a "not implemented" notice. Per-check `fix_command` guidance is still rendered inline. - gpg_cli.py: in `setup_command`, read keyring state once (`existing_fingerprint`) and branch on the cached value. Previously repeated `key_exists()` / `get_key_fingerprint()` calls could race on transient keyring errors and report "key already exists" while later failing with "no agent GPG key found". Also adds regression tests: - `test_display_name_separate_from_binary` - `test_version_getter_preferred_over_subprocess` - `test_version_getter_none_falls_back_to_subprocess` - `test_beads_check_probes_bd_binary` Validation: - uv run ruff format --check . → 323 files clean - uv run ruff check . → all checks passed - uv run pytest tests/ -x -q → 3532 passed, 42 skipped Signed-off-by: jason poley <jason.poley@gmail.com>
🟢 Security Scan Results (Advisory)
No critical or high severity issues found. How to remediate# View detailed scan results
gh run download 24635895367 -n security-scan-results-a846e686b23a1a04b1777d0cbb0eeb24f09b7a06
# Triage findings with AI assistance
specify security triage
# Generate fix suggestions
specify security fix
# Apply fixes and re-scan
flowspec security scanSee the Security tab for detailed findings. |
There was a problem hiding this comment.
Pull request overview
This PR consolidates the new flowspec doctor environment health check and agent GPG commit signing support, while reducing the slash-command surface area and updating documentation to match the streamlined workflow.
Changes:
- Adds
flowspec doctorwith tool/version checks and result rendering, plus regression tests around tool probing/version getters. - Adds agent GPG key management + CLI (
flowspec gpg setup/status/rotate) with tests and a new signing guide. - Removes several legacy
.claudeslash-command templates and refreshes README positioning/command reference.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_signing.py |
Adds unit tests for GPG key generation/config/keyring and subprocess helpers. |
tests/test_doctor.py |
Adds tests for doctor checks, including binary vs display name and version-getter behavior. |
src/flowspec_cli/signing.py |
Implements GPG key generation/storage, git signing configuration, and rotation helpers. |
src/flowspec_cli/gpg_cli.py |
Adds Typer CLI commands for GPG signing management. |
src/flowspec_cli/doctor.py |
Implements health checks and formatted output for flowspec doctor. |
src/flowspec_cli/__init__.py |
Registers new doctor command and gpg sub-app on the main Typer app. |
docs/guides/gpg-signing.md |
Documents setup, status, rotation, verification, and troubleshooting for agent GPG signing. |
README.md |
Updates product messaging and simplifies workflow/command documentation. |
.claude/commands/vibe/vibe.md |
Removes legacy “vibe” command template. |
.claude/commands/flow/security_workflow.md |
Removes legacy security workflow template. |
.claude/commands/flow/security_web.md |
Removes legacy web security testing template. |
.claude/commands/flow/security_triage.md |
Removes legacy security triage template. |
.claude/commands/flow/security_report.md |
Removes legacy security report template. |
.claude/commands/flow/security_fix.md |
Removes legacy security fix template. |
.claude/commands/flow/intake.md |
Removes legacy intake template. |
.claude/commands/flow/custom.md |
Removes legacy custom workflow template. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Read key state once to avoid inconsistent behavior from repeated | ||
| # keyring access (e.g., a transient read failure could otherwise make | ||
| # us report "already exists" and then fail configuring git signing | ||
| # because no fingerprint is available). | ||
| existing_fingerprint = get_key_fingerprint() if key_exists() else None | ||
|
|
There was a problem hiding this comment.
existing_fingerprint = get_key_fingerprint() if key_exists() else None still performs two separate keyring reads (because key_exists() calls get_key_fingerprint() internally). That means the transient/inconsistent keyring behavior this block is trying to prevent can still occur. Read the fingerprint once via get_key_fingerprint() and branch on that cached value, or refactor key_exists() to accept a cached fingerprint.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Address Copilot review feedback on PR #1243: the previous guard ``existing_fingerprint = get_key_fingerprint() if key_exists() else None`` still performed two keyring reads, because ``key_exists()`` itself calls ``get_key_fingerprint()`` internally. That left the original race back in place — a transient keyring failure between the two reads could make ``setup`` report "key already exists" and then fail configuring git signing because no fingerprint is available. Drop the redundant ``key_exists()`` call and branch on the cached ``existing_fingerprint`` alone. Also adds tests/test_gpg_cli.py with three regression tests pinning ``get_key_fingerprint.call_count == 1`` and ``key_exists.call_count == 0`` on the existing-key, no-key, and --force paths so this pattern can't silently reappear. Signed-off-by: jason poley <jason.poley@gmail.com>
Summary
Replaces closed PR #1242. Same consolidation work —
flowspec doctor, agent GPG commit signing, and slash-command reduction — plus the four quality issues Copilot flagged on the previous PR.Copilot review fixes
doctor.pycheck_tool_installed("beads")probed the wrong binary — the executable isbdcheck_tool_installedto take a separatedisplay_nameand optionalversion_getter;run_all_checksnow probesbdwhile still labeling it "beads CLI"doctor.pysplit()[-1]version parsing duplicated / diverged from the existingcheck_backlog_installed_versionandcheck_beads_installed_versionhelpersrun_all_checksnow passes those helpers asversion_getters (lazy-imported to avoid a circular import withflowspec_cli/__init__.py)gpg_cli.pysetup_commandcalledkey_exists()/get_key_fingerprint()multiple times; a transient keyring miss could produce "key already exists" followed by "no agent GPG key found"existing_fingerprint; branch on the cached valuedoctor.pyrun_doctor()printed aflowspec doctor --fixhint that was immediately followed by "auto-fix is not implemented"fix_commandsuggestions are still rendered inline bydisplay_resultsRegression tests added
test_display_name_separate_from_binary— display_name vs binary_name diverge cleanlytest_version_getter_preferred_over_subprocess— custom getter wins over--versionprobetest_version_getter_none_falls_back_to_subprocess— falls back when getter returns Nonetest_beads_check_probes_bd_binary— pins the exact binary name looked up byrun_all_checks, so this regression can't silently reappearTest plan
uv run ruff format --check .— 323 files cleanuv run ruff check .— all checks passeduv run pytest tests/ -x -q— 3532 passed, 42 skippedNotes
The bulk of the diff (doctor, GPG signing, slash-command removal) is carried over verbatim from #1242 — only the four files flagged by Copilot review changed in this iteration.
🤖 Generated with Claude Code