cleanup-418 (v6): fix all 5 Copilot review findings from PR #1246 by jpoley · Pull Request #1247 · jpoley/flowspec

jpoley · 2026-04-20T01:58:09Z

Summary

Follows closed PR #1246 (same cleanup-418 branch). Fixes all 5 inline Copilot review findings from that PR. No behavior changes outside the targeted issues.

Copilot findings addressed

1. Misleading test name (tests/test_signing.py:385)

test_get_key_info_cached_none_returns_none_without_keyring — name said "without_keyring" but the test asserted a keyring read (call_count == 1). Renamed to test_get_key_info_explicit_none_falls_back_to_keyring.

2. README rewrite (README.md:15) — intentional

The README rewrite was intentional and in-scope for this branch (it rewrites outdated product messaging). No code change needed; confirming here.

3. `get_key_info` docstring inconsistency (signing.py:272)

The docstring distinguished "when omitted" vs "pass fingerprint=None" as if they had different behavior, but the implementation treats both identically (both fall back to the keyring). Updated to "when not provided or None".

4. `setup_command` multiple keyring reads (gpg_cli.py:92)

The CLI-layer comment said "read key state exactly once" but configure_git_signing also called get_key_fingerprint() internally. Fix: added fingerprint: Optional[str] = None to configure_git_signing; setup_command now passes the already-held fingerprint to skip the redundant read. Also changed the "GPG key generated" message to "GPG key ready" since generate_agent_key may return an existing key (not always generating).

5. `rotate_command` multiple keyring reads (gpg_cli.py:281)

delete_agent_key called get_key_fingerprint() internally even though rotate_command already held old_fingerprint. Fix: added cached_fingerprint: Optional[str] = None to delete_agent_key; rotate_command now passes the cached value. Similarly, configure_git_signing now receives the new fingerprint from generate_agent_key.

New regression tests

test_configure_git_signing_uses_cached_fingerprint — verifies no keyring read when fingerprint is provided
test_delete_agent_key_uses_cached_fingerprint — verifies no keyring read when cached_fingerprint is provided
Updated CLI keyring-read tests to pin that fingerprints are forwarded to configure_git_signing and delete_agent_key

Validation

uv run ruff format --check . — 324 files clean
uv run ruff check . — all checks passed
uv run pytest tests/ -x -q — 3546 passed, 42 skipped
CI green on this PR
All commits DCO-signed (git commit -s)

🤖 Generated with Claude Code

…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>

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>

Addresses all three Copilot findings on PR #1244. PR #1244 only fixed the caching pattern in ``gpg setup`` — ``gpg status`` and ``gpg rotate`` still repeated the same double/triple-read pattern, and the new ``_check_installed_tool_version`` probe cascade in ``doctor`` shipped without ``subprocess`` timeouts. Caching (src/flowspec_cli/gpg_cli.py) ------------------------------------- ``key_exists()`` is implemented in terms of ``get_key_fingerprint()``, and the old ``get_key_info()`` also called ``get_key_fingerprint()`` internally. Chaining these issued multiple keyring reads per command invocation and could produce inconsistent output under transient keyring failures (e.g. "Configured" paired with fingerprint "unknown"). - ``status_command``: now reads the fingerprint exactly once and passes it through as ``get_key_info(fingerprint=fingerprint)``. Previously: ``key_exists()`` + ``get_key_fingerprint()`` + ``get_key_info()`` = 3 reads. - ``rotate_command``: now reads once and branches on the cached value. Previously: ``key_exists()`` + ``get_key_fingerprint()`` = 2 reads. - ``key_exists`` is no longer imported in this module — the fingerprint value itself is now the single source of truth. Supporting change (src/flowspec_cli/signing.py) ----------------------------------------------- ``get_key_info`` gains an optional ``fingerprint`` keyword argument. When provided, the keyring read is skipped entirely; when omitted, behavior is unchanged so existing callers and tests keep working. Subprocess timeout (src/flowspec_cli/doctor.py) ----------------------------------------------- ``_check_installed_tool_version`` cascades three ``subprocess.run`` probes (``--version`` / ``version`` / ``-V``). Each now uses ``timeout=5`` and catches ``subprocess.TimeoutExpired`` alongside ``OSError``, so a hung or interactive tool can no longer block ``flowspec doctor`` indefinitely. Regression tests ---------------- 9 new tests total; all pin invariants that can silently regress: tests/test_gpg_cli.py (rewritten + expanded, 8 tests): - ``TestModuleSurface::test_key_exists_is_not_imported`` — static guardrail: ``key_exists`` must not reappear as a gpg_cli import. - Setup (3): kept from #1244 — one keyring read on existing-key, no-key, and --force paths. - Status (2, new): one keyring read on configured and not-configured paths; ``get_key_info`` is called with ``fingerprint=<cached>``. - Rotate (2, new): one keyring read on no-key and rotating-with-yes paths. tests/test_signing.py (2 new): - ``test_get_key_info_accepts_cached_fingerprint`` — passing ``fingerprint=`` bypasses the keyring entirely. - ``test_get_key_info_cached_none_returns_none_without_keyring`` — ``fingerprint=None`` falls back to the keyring exactly once (cache, not suppression flag). tests/test_doctor.py (2 new): - ``test_timeout_expired_is_treated_as_unknown_version`` — a ``TimeoutExpired`` from any probe is swallowed, not re-raised. - ``test_timeout_is_configured_on_each_probe`` — every ``subprocess.run`` call receives an explicit positive ``timeout``. Validation ---------- - ``uv run ruff format --check .`` → 324 files clean - ``uv run ruff check .`` → all checks passed - ``uv run pytest tests/ -x -q`` → 3544 passed, 42 skipped Signed-off-by: jason poley <jason.poley@gmail.com>

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Both ``Potential fix for pull request finding`` commits introduced on PR #1245 (d08b27a, 48d2134) need to be reverted. The third autofix commit, 9a76497 (doctor.py), is benign and is kept. d08b27a — hallucinated kwargs ----------------------------- The autofix changed ``setup_command`` to call:: generate_agent_key(fingerprint=existing_fingerprint) configure_git_signing(project_root=root, fingerprint=fingerprint) but neither function accepts a ``fingerprint`` keyword argument: - ``src/flowspec_cli/signing.py:99`` ``def generate_agent_key() -> str`` - ``src/flowspec_cli/signing.py:166`` ``def configure_git_signing( project_root: Optional[Path] = None) -> None`` Both calls would raise ``TypeError`` at runtime the first time an actual setup was attempted. This is why the ``test`` / ``validate-dev-role`` / ``validate-qa-role`` CI jobs failed. 48d2134 — duplicate safety, wrong contract ------------------------------------------ The autofix added a post-delete keyring re-read in ``rotate_command``:: delete_agent_key() remaining_fingerprint = get_key_fingerprint() if remaining_fingerprint == old_fingerprint: raise GPGConfigurationError( "Failed to delete the existing agent GPG key." ) Problems: 1. ``delete_agent_key()`` already raises ``GPGError`` if either the ``gpg --delete-secret-and-public-key`` call or the ``keyring.delete_password`` call fails (signing.py:349-356). The extra verification is duplicate safety. 2. The extra read is the exact pattern PR #1244/#1245 set out to eliminate, and it breaks the ``TestRotateCommandKeyringReads::test_reads_fingerprint_once_when_rotating`` regression test. 3. The stray long-line also left ``gpg_cli.py`` failing ``ruff format --check`` — that was the ``lint`` CI failure. Reverting this restores the single-read invariant and the format check. 9a76497 (doctor.py) kept ------------------------ That commit only splits ``(result.stdout or result.stderr).strip()`` into two separate strips. Semantics unchanged; no reason to revert. Validation ---------- - ``uv run ruff format --check .`` → 324 files clean - ``uv run ruff check .`` → all checks passed - ``uv run pytest tests/ -x -q`` → 3544 passed, 42 skipped This reverts commits d08b27a and 48d2134. Signed-off-by: jason poley <jason.poley@gmail.com>

- Rename test_get_key_info_cached_none_returns_none_without_keyring to test_get_key_info_explicit_none_falls_back_to_keyring (name matched opposite of behavior) - Fix get_key_info docstring: "when not provided or None" (both cases behave identically; prior wording implied different behavior) - Add fingerprint param to configure_git_signing; callers now pass the already-held fingerprint to skip the redundant keyring read - Add cached_fingerprint param to delete_agent_key; rotate_command now passes old_fingerprint to avoid a second keyring read - Thread fingerprints through setup_command and rotate_command; update test assertions to pin the forwarded-fingerprint contract - Change "GPG key generated" message to "GPG key ready" (generate_agent_key may return an existing key if one appears between the CLI check and the internal check; "ready" is always accurate) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: jason poley <jason.poley@gmail.com>

github-actions · 2026-04-20T01:59:17Z

🟢 Security Scan Results (Advisory)

Note: Security scanning is advisory and does not block PR merges.

Total Findings: 11
Critical: 0
High: 0

No critical or high severity issues found.

How to remediate

# View detailed scan results
gh run download 24644901933 -n security-scan-results-632631658bbde5df361bd15ae2151de38b76ba0c

# Triage findings with AI assistance
specify security triage

# Generate fix suggestions
specify security fix

# Apply fixes and re-scan
flowspec security scan

See the Security tab for detailed findings.

Copilot

Pull request overview

This PR follows up on PR #1246 to address prior review findings around GPG keyring-read caching, test naming/docstring accuracy, and CLI messaging consistency for agent GPG signing and doctor checks.

Changes:

Extend signing primitives (configure_git_signing, delete_agent_key, get_key_info) to accept cached fingerprints to avoid redundant keyring reads.
Update GPG CLI commands to forward cached fingerprints; add/adjust regression tests for “single keyring read per invocation”.
Add/extend doctor command wiring and tests; update docs/README messaging.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
tests/test_signing.py	Adds regression tests for cached fingerprint behavior in signing primitives.
tests/test_gpg_cli.py	Adds CLI-level tests asserting cached fingerprints are forwarded and keyring reads are constrained.
tests/test_doctor.py	Adds/updates tests covering doctor checks (including timeout behavior).
src/flowspec_cli/signing.py	Implements cached-fingerprint parameters and docstring updates; core signing/key management logic.
src/flowspec_cli/gpg_cli.py	Forwards cached fingerprints through CLI commands; updates user-facing messages.
src/flowspec_cli/doctor.py	Implements doctor checks with time-bounded version probing and reporting.
src/flowspec_cli/init.py	Registers `gpg` sub-app and `doctor` command in the main Typer app.
docs/guides/gpg-signing.md	Documents agent GPG signing usage and workflows.
README.md	Updates high-level product messaging and command overview.
.claude/commands/vibe/vibe.md	Removes the `vibe` command documentation.
.claude/commands/flow/security_workflow.md	Removes deprecated/obsolete security workflow command documentation.
.claude/commands/flow/security_web.md	Removes deprecated/obsolete security web command documentation.
.claude/commands/flow/security_triage.md	Removes deprecated/obsolete security triage command documentation.
.claude/commands/flow/security_report.md	Removes deprecated/obsolete security report command documentation.
.claude/commands/flow/security_fix.md	Removes deprecated/obsolete security fix command documentation.
.claude/commands/flow/intake.md	Removes deprecated/obsolete intake command documentation.
.claude/commands/flow/custom.md	Removes deprecated/obsolete custom workflow command documentation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-20T02:02:48Z

+# Output:
+# ✓ GPG key generated
+# ✓ Git configured for signing
+#
+# Fingerprint: A1B2C3D4E5F6A7B8C9D0E1F2A3B4C5D6E7F8A9B0
+```


The guide’s sample output still shows “✓ GPG key generated”, but the CLI now prints “GPG key ready”. Update the documentation snippet so it matches the current command output.

Copilot · 2026-04-20T02:02:49Z

+        else:
+            if existing_fingerprint and force:
+                console.print("[yellow]Regenerating agent GPG key...[/yellow]")
+                delete_agent_key()


In the --force path you already have existing_fingerprint, but delete_agent_key() is called without passing it. That triggers an extra get_key_fingerprint() read inside delete_agent_key, contradicting the “read key state exactly once” contract and reintroducing transient keyring inconsistency risk. Pass cached_fingerprint=existing_fingerprint here (and update the related test expectation).

Suggested change

delete_agent_key()

delete_agent_key(cached_fingerprint=existing_fingerprint)

Copilot · 2026-04-20T02:02:49Z

+        # Read key state exactly once. ``key_exists()`` itself calls
+        # ``get_key_fingerprint()`` internally, so using both here would still
+        # incur two keyring reads and reintroduce the inconsistency this
+        # caching was meant to prevent. Branch on the fingerprint alone.
+        existing_fingerprint = get_key_fingerprint()
+
+        if existing_fingerprint and not force:
+            console.print("[yellow]Agent GPG key already exists.[/yellow]")
+            console.print("[dim]Use --force to regenerate the key.[/dim]")
+            fingerprint = existing_fingerprint
+        else:
+            if existing_fingerprint and force:
+                console.print("[yellow]Regenerating agent GPG key...[/yellow]")
+                delete_agent_key()
+
+            # Generate new key
+            console.print("[cyan]Generating agent GPG key...[/cyan]")
+            fingerprint = generate_agent_key()
+            console.print("[green]✓[/green] GPG key ready")


The setup flow calls generate_agent_key() after an initial get_key_fingerprint() read. Since generate_agent_key() calls get_key_fingerprint() internally, flowspec gpg setup still performs a second keyring read on the “no key” path, despite the surrounding comments/tests asserting “exactly once per invocation”. Consider adding an optional cached/known-fingerprint parameter (or a lower-level helper) so callers that already checked the keyring can avoid the redundant read.

Copilot · 2026-04-20T02:02:49Z

+        mock_configure.assert_called_once_with(
+            project_root=mock_configure.call_args[1]["project_root"], fingerprint=fp
+        )


These assertions set the expected project_root to whatever was actually passed (mock_configure.call_args[...]), which makes the check tautological and weaker than intended. Use unittest.mock.ANY (or assert the type/Path value explicitly) for project_root, and keep the assertion focused on verifying that the fingerprint is forwarded.

Copilot · 2026-04-20T02:02:49Z

+        mock_configure.assert_called_once_with(
+            project_root=mock_configure.call_args[1]["project_root"], fingerprint=new_fp
+        )


Same issue as above: using mock_configure.call_args[...] in the expected kwargs makes the project_root assertion self-fulfilling. Prefer ANY (or an explicit Path) for project_root and assert fingerprint=new_fp to ensure the forwarding contract is actually tested.

Copilot · 2026-04-20T02:02:50Z

+        mock_get_fp.return_value = "C" * 40
+        mock_generate.return_value = "D" * 40
+
+        result = runner.invoke(gpg_app, ["setup", "--force"])
+
+        assert result.exit_code == 0, result.output
+        assert mock_get_fp.call_count == 1
+        mock_delete.assert_called_once()


test_force_regenerates_with_one_fingerprint_read currently only asserts that delete_agent_key() was called, but not that the cached fingerprint is forwarded. If setup_command is meant to avoid duplicate keyring reads, this test should assert delete_agent_key(cached_fingerprint=<existing>) (and fail if the call is made without the cache).

Suggested change

mock_get_fp.return_value = "C" * 40

mock_generate.return_value = "D" * 40

result = runner.invoke(gpg_app, ["setup", "--force"])

assert result.exit_code == 0, result.output

assert mock_get_fp.call_count == 1

mock_delete.assert_called_once()

existing_fp = "C" * 40

mock_get_fp.return_value = existing_fp

mock_generate.return_value = "D" * 40

result = runner.invoke(gpg_app, ["setup", "--force"])

assert result.exit_code == 0, result.output

assert mock_get_fp.call_count == 1

mock_delete.assert_called_once_with(cached_fingerprint=existing_fp)

github-actions · 2026-05-20T04:43:39Z

This pull request has been automatically marked as stale because it has not had recent activity.
It will be closed in 7 days if no further activity occurs.

If this PR is still in progress:

Comment or push new commits to keep it open
Add the work-in-progress label to prevent automatic closure

Thank you for your contributions to flowspec!

jpoley and others added 12 commits April 18, 2026 11:25

Update src/flowspec_cli/doctor.py

431117a

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update src/flowspec_cli/__init__.py

0fda399

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Potential fix for pull request finding

d08b27a

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Potential fix for pull request finding

48d2134

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Potential fix for pull request finding

9a76497

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings April 20, 2026 01:58

github-actions Bot added documentation Improvements or additions to documentation config github source tests claude labels Apr 20, 2026

Copilot AI reviewed Apr 20, 2026

View reviewed changes

github-actions Bot added the stale Marked stale due to inactivity label May 20, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cleanup-418 (v6): fix all 5 Copilot review findings from PR #1246#1247

cleanup-418 (v6): fix all 5 Copilot review findings from PR #1246#1247
jpoley wants to merge 12 commits into
mainfrom
cleanup-418

jpoley commented Apr 20, 2026

Uh oh!

github-actions Bot commented Apr 20, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 20, 2026

Uh oh!

Copilot AI Apr 20, 2026

Uh oh!

Copilot AI Apr 20, 2026

Uh oh!

Copilot AI Apr 20, 2026

Uh oh!

Copilot AI Apr 20, 2026

Uh oh!

Copilot AI Apr 20, 2026

Uh oh!

github-actions Bot commented May 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	delete_agent_key()
	delete_agent_key(cached_fingerprint=existing_fingerprint)

Conversation

jpoley commented Apr 20, 2026

Summary

Copilot findings addressed

1. Misleading test name (tests/test_signing.py:385)

2. README rewrite (README.md:15) — intentional

3. get_key_info docstring inconsistency (signing.py:272)

4. setup_command multiple keyring reads (gpg_cli.py:92)

5. rotate_command multiple keyring reads (gpg_cli.py:281)

New regression tests

Validation

Uh oh!

github-actions Bot commented Apr 20, 2026

🟢 Security Scan Results (Advisory)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented May 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

3. `get_key_info` docstring inconsistency (signing.py:272)

4. `setup_command` multiple keyring reads (gpg_cli.py:92)

5. `rotate_command` multiple keyring reads (gpg_cli.py:281)