refactor: transport-neutral verification core + gpd CLI parity for the built-in MCP tools - #268
Open
jungdaesuh wants to merge 2 commits into
Open
refactor: transport-neutral verification core + gpd CLI parity for the built-in MCP tools#268jungdaesuh wants to merge 2 commits into
jungdaesuh wants to merge 2 commits into
Conversation
…P tools Extract error-catalog, protocol-catalog, contract-check, and convention logic from the MCP server modules into transport-neutral gpd.core modules (error_catalog, protocol_catalog, contract_checks, convention_checks, envelopes, verification_bindings); the servers become thin wrappers over the same code, verified by the existing tests/mcp suite passing unmodified. Add nine CLI subcommands (gpd verify contract-check / suggest-checks / bundle-checklist / checklist / coverage, gpd convention validate-assert / subfield-defaults, gpd refs errors / protocols) emitting the same schema-versioned envelopes, with fail-fast TTY stdin guards, strict exit codes, loud flag-conflict errors, and parity + regression tests. Migrate prompts, specs, the verify-work stage manifest, and the staging tool registry to the CLI forms. The MCP servers remain installed and fully functional; nothing depends on them anymore. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe change centralizes convention, error-catalog, protocol-catalog, and contract verification logic in core modules, exposes CLI parity commands, simplifies MCP servers into wrappers, migrates verification workflows to shell-based CLI commands, and adds extensive catalog, CLI, workflow, and prompt tests. ChangesVerification surfaces and transport parity
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Verifier
participant CLI
participant CoreChecks
participant Catalogs
Verifier->>CLI: run gpd --raw verify suggest-checks
CLI->>CoreChecks: parse contract and suggest check keys
CoreChecks->>Catalogs: resolve checks, bundles, and references
Catalogs-->>CoreChecks: catalog metadata
CoreChecks-->>CLI: stable suggestion envelope
Verifier->>CLI: run gpd --raw verify contract-check
CLI->>CoreChecks: execute request payload
CoreChecks-->>CLI: verification verdict envelope
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
src/gpd/core/protocol_catalog.py (3)
461-513: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winProcess-wide cached catalog state is handed out by reference. Both the cached domain manifest and the query payloads return objects owned by long-lived caches, so a single caller mutating what it receives corrupts the catalog for every later request in the process.
src/gpd/core/protocol_catalog.py#L461-L513: copy the list values (load_when,steps,checkpoints) inprotocol_detail_payloadandprotocol_checkpoints_payloadinstead of returning the store's internal lists.src/gpd/core/protocol_catalog.py#L180-L183: return an immutable view (or a copy) fromload_protocol_domain_manifestso thelru_cached dict cannot be mutated by callers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/gpd/core/protocol_catalog.py` around lines 461 - 513, The cached catalog data is exposed by reference, allowing callers to mutate shared state. In src/gpd/core/protocol_catalog.py lines 461-513, update protocol_detail_payload and protocol_checkpoints_payload to return copies of load_when, steps, and checkpoints; in lines 180-183, update load_protocol_domain_manifest to return an immutable view or copy so the lru_cache-owned dictionary cannot be modified by callers.
75-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocstring says H2/H3 but
_HEADING_REmatches H1–H4. The returned sections include the document title, which callers of this exported helper may not expect.📝 Suggested doc fix
- """Extract H2/H3 sections from markdown body.""" + """Extract all H1–H4 sections from a markdown body, in document order."""🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/gpd/core/protocol_catalog.py` around lines 75 - 86, Update extract_protocol_sections and its _HEADING_RE usage so the exported helper extracts only H2/H3 headings, excluding document-level H1 and deeper H4 sections. Keep the existing section boundaries and returned fields unchanged for supported heading levels.
180-183: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCached manifest dict is shared mutable state.
lru_cachehands the samedictto every caller of this exported function; any accidental mutation permanently corrupts the manifest for the process. Returning an immutable view (or a copy) would make the cache safe by construction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/gpd/core/protocol_catalog.py` around lines 180 - 183, The cached result returned by load_protocol_domain_manifest is a shared mutable dict. Update this function to return an immutable mapping or an independent copy on each call, while preserving the cached manifest data and the existing dict[str, str] contents for callers.src/gpd/core/convention_checks.py (1)
182-185: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
exceptpair. The secondexcept Exceptionalready covers the first clause's types, so the explicit tuple only documents intent without changing behavior.♻️ Optional simplification
- except (ConventionError, OSError, ValueError, TimeoutError) as exc: - return stable_mcp_error(exc) - except Exception as exc: # pragma: no cover - defensive envelope + except Exception as exc: # defensive envelope: any parse/IO failure becomes a stable error return stable_mcp_error(exc)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/gpd/core/convention_checks.py` around lines 182 - 185, Remove the redundant typed exception clause in the surrounding convention-check handling and retain a single `except Exception as exc` path that returns `stable_mcp_error(exc)`. Preserve the existing defensive behavior and error conversion.src/gpd/core/contract_checks.py (2)
3438-3444: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
resolved_subjectreads branch-local variables.source_reference_idandregime_labelare only bound inside their respectiveelifbranches; the mapping is correct today, but the enclosingexcept Exceptionat Line 3506 would silently convert any future mismatch into a generic error envelope. Initializing both toNonebefore the dispatch makes the invariant explicit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/gpd/core/contract_checks.py` around lines 3438 - 3444, Initialize source_reference_id and regime_label to None before the check_key dispatch that assigns resolved_subject. Preserve the existing branch mappings for contract.benchmark_reproduction, contract.limit_recovery, and _PROOF_CHECK_KEYS while ensuring the enclosing exception path cannot encounter unbound variables.
1363-1381: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
contract is Nonebranch is identical to the code that follows it. Lines 1367-1370 duplicate Lines 1372-1373, so the branch has no effect.♻️ Suggested simplification
for target in check_targets: values = _binding_values_for_target(binding, target) if not values: continue - if contract is None: - valid_by_target[target] = values - contract_impacts.extend(values) - continue - valid_by_target[target] = values contract_impacts.extend(values)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/gpd/core/contract_checks.py` around lines 1363 - 1381, Remove the redundant contract is None conditional in the target-processing loop. In the binding validation logic, keep a single unconditional valid_by_target assignment and contract_impacts extension after the empty-values check, preserving the existing behavior and return values.src/gpd/core/error_catalog.py (1)
331-355: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winTraceability columns are mapped purely by position with no header validation. If the markdown table's column order or set changes, values are silently attributed to the wrong verification check instead of failing closed — unlike the catalog loader, which validates declared ID ranges. Consider asserting the header row matches
TRACEABILITY_COLUMNSbefore mapping cells.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/gpd/core/error_catalog.py` around lines 331 - 355, The traceability loader maps cells by fixed position without validating the table schema. Before processing data rows in the traceability-loading method, identify and validate the markdown header against TRACEABILITY_COLUMNS, rejecting mismatched column order or sets with a clear error; only perform the existing positional mapping after validation.src/gpd/mcp/servers/verification_server.py (1)
1235-1240: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDead local validation in
get_bundle_checklist: the rejection is discarded and the normalized ids are not passed on.
_validate_string_listresult is only used for the span'sbundle_count,_rejectionis dropped, and the rawbundle_idsgoes to core (which re-validates). Behavior is still correct because core owns validation, but the code reads as if the wrapper enforces it. Either return the rejection or drop the local pass.♻️ Suggested simplification
def get_bundle_checklist(bundle_ids: BundleIdListInput) -> dict: """Return additive verifier checklist extensions for selected protocol bundles.""" - validated_bundle_ids, _rejection = _validate_string_list(bundle_ids, field_name="bundle_ids") - normalized_bundle_ids = _unique_strings(validated_bundle_ids or []) - with gpd_span("mcp.verification.bundle_checklist", bundle_count=len(normalized_bundle_ids)): + validated_bundle_ids, rejection = _validate_string_list(bundle_ids, field_name="bundle_ids") + if rejection is not None: + return rejection + normalized_bundle_ids = _unique_strings(validated_bundle_ids or []) + with gpd_span("mcp.verification.bundle_checklist", bundle_count=len(normalized_bundle_ids)): - return contract_checks.get_bundle_checklist(bundle_ids, bundle_lookup=get_protocol_bundle) + return contract_checks.get_bundle_checklist(normalized_bundle_ids, bundle_lookup=get_protocol_bundle)Confirm the envelope text still matches the CLI path (
gpd verify bundle-checklist) before adopting, since the CLI passes raw ids straight to core.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/gpd/mcp/servers/verification_server.py` around lines 1235 - 1240, Remove the unused local validation in get_bundle_checklist, including _validate_string_list, _unique_strings, and the discarded rejection, and derive the span’s bundle_count without implying wrapper-level enforcement; continue passing raw bundle_ids to contract_checks.get_bundle_checklist so validation remains owned by core and matches the CLI path.src/gpd/cli.py (1)
3606-3635: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: fold the two identical "not found" blocks into one helper.
The
--nameand--checkpointsmisses build the same envelope-and-exit sequence.♻️ Suggested consolidation
+ def _not_found(selector: str) -> NoReturn: + _output( + stable_mcp_response( + {"available": available_protocol_names(store)}, + error=f"Protocol '{selector}' not found", + ) + ) + raise typer.Exit(code=1) + try: store = get_protocol_store() if name is not None: payload = protocol_detail_payload(store, name) if payload is None: - _output( - stable_mcp_response( - {"available": available_protocol_names(store)}, - error=f"Protocol '{name}' not found", - ) - ) - raise typer.Exit(code=1) + _not_found(name) elif checkpoints is not None: payload = protocol_checkpoints_payload(store, checkpoints) if payload is None: - _output( - stable_mcp_response( - {"available": available_protocol_names(store)}, - error=f"Protocol '{checkpoints}' not found", - ) - ) - raise typer.Exit(code=1) + _not_found(checkpoints)Note the closure must be defined after
storeis resolved.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/gpd/cli.py` around lines 3606 - 3635, Consolidate the duplicate missing-protocol handling in the command’s try block into one local helper that builds the available-protocol error response and exits with code 1. Define the helper only after get_protocol_store() resolves store, then invoke it for both the name and checkpoints lookup misses while preserving each lookup’s requested value in the error message.tests/test_cli_verify_contract.py (1)
46-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd CLI-level coverage for
--project-dirincontract-check/suggest-checks.
tests/test_cli_verify_contract.pyexercises these commands without ever passing--project-dir, so CI does not lock in that the flag is resolved and threaded through torun_contract_check(..., project_dir=...)/suggest_contract_checks(..., project_dir=...)before contracting path resolution. Add atmp_pathfixture-based parity test for each command that compares--project-dir <dir>output to the MCP function with the sameproject_dir.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_cli_verify_contract.py` around lines 46 - 127, Add tmp_path-based CLI parity tests for both contract-check and suggest-checks that pass --project-dir and compare CLI JSON output with run_contract_check or suggest_contract_checks invoked using the same project_dir. Use a request whose path resolution depends on the temporary project directory, and assert the command’s exit status and payload match the corresponding MCP function.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/gpd/core/contract_checks.py`:
- Around line 3770-3777: Deduplicate error_class_ids before passing them into
_coverage_inner, while preserving their validated integer values and existing
order. Apply the same normalization in the related coverage path around the
second occurrence so total, covered, partial, uncovered, and coverage_percent
count each distinct error class once; leave active_checks handling unchanged.
- Around line 593-594: Update the contract-check hint construction around the
else branch assigning required_request_fields so binding.forbidden_proxy_ids is
removed from optional_request_fields whenever it is marked required. Preserve
the existing required field assignment and ensure the final hint cannot
advertise this field in both lists, without limiting the change to proof checks.
- Around line 396-397: Update the allowed_families and forbidden_families fields
in ContractMetadataRequest to use the optional annotation list[str] | None while
retaining their None defaults, matching the model’s other optional list fields.
---
Nitpick comments:
In `@src/gpd/cli.py`:
- Around line 3606-3635: Consolidate the duplicate missing-protocol handling in
the command’s try block into one local helper that builds the available-protocol
error response and exits with code 1. Define the helper only after
get_protocol_store() resolves store, then invoke it for both the name and
checkpoints lookup misses while preserving each lookup’s requested value in the
error message.
In `@src/gpd/core/contract_checks.py`:
- Around line 3438-3444: Initialize source_reference_id and regime_label to None
before the check_key dispatch that assigns resolved_subject. Preserve the
existing branch mappings for contract.benchmark_reproduction,
contract.limit_recovery, and _PROOF_CHECK_KEYS while ensuring the enclosing
exception path cannot encounter unbound variables.
- Around line 1363-1381: Remove the redundant contract is None conditional in
the target-processing loop. In the binding validation logic, keep a single
unconditional valid_by_target assignment and contract_impacts extension after
the empty-values check, preserving the existing behavior and return values.
In `@src/gpd/core/convention_checks.py`:
- Around line 182-185: Remove the redundant typed exception clause in the
surrounding convention-check handling and retain a single `except Exception as
exc` path that returns `stable_mcp_error(exc)`. Preserve the existing defensive
behavior and error conversion.
In `@src/gpd/core/error_catalog.py`:
- Around line 331-355: The traceability loader maps cells by fixed position
without validating the table schema. Before processing data rows in the
traceability-loading method, identify and validate the markdown header against
TRACEABILITY_COLUMNS, rejecting mismatched column order or sets with a clear
error; only perform the existing positional mapping after validation.
In `@src/gpd/core/protocol_catalog.py`:
- Around line 461-513: The cached catalog data is exposed by reference, allowing
callers to mutate shared state. In src/gpd/core/protocol_catalog.py lines
461-513, update protocol_detail_payload and protocol_checkpoints_payload to
return copies of load_when, steps, and checkpoints; in lines 180-183, update
load_protocol_domain_manifest to return an immutable view or copy so the
lru_cache-owned dictionary cannot be modified by callers.
- Around line 75-86: Update extract_protocol_sections and its _HEADING_RE usage
so the exported helper extracts only H2/H3 headings, excluding document-level H1
and deeper H4 sections. Keep the existing section boundaries and returned fields
unchanged for supported heading levels.
- Around line 180-183: The cached result returned by
load_protocol_domain_manifest is a shared mutable dict. Update this function to
return an immutable mapping or an independent copy on each call, while
preserving the cached manifest data and the existing dict[str, str] contents for
callers.
In `@src/gpd/mcp/servers/verification_server.py`:
- Around line 1235-1240: Remove the unused local validation in
get_bundle_checklist, including _validate_string_list, _unique_strings, and the
discarded rejection, and derive the span’s bundle_count without implying
wrapper-level enforcement; continue passing raw bundle_ids to
contract_checks.get_bundle_checklist so validation remains owned by core and
matches the CLI path.
In `@tests/test_cli_verify_contract.py`:
- Around line 46-127: Add tmp_path-based CLI parity tests for both
contract-check and suggest-checks that pass --project-dir and compare CLI JSON
output with run_contract_check or suggest_contract_checks invoked using the same
project_dir. Use a request whose path resolution depends on the temporary
project directory, and assert the command’s exit status and payload match the
corresponding MCP function.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 171bd341-5f97-484c-a02e-fb9fdf98f4f3
📒 Files selected for processing (35)
src/gpd/agents/gpd-verifier.mdsrc/gpd/cli.pysrc/gpd/commands/verify-work.mdsrc/gpd/core/contract_checks.pysrc/gpd/core/convention_checks.pysrc/gpd/core/envelopes.pysrc/gpd/core/error_catalog.pysrc/gpd/core/protocol_catalog.pysrc/gpd/core/verification_bindings.pysrc/gpd/core/workflow_staging.pysrc/gpd/mcp/servers/__init__.pysrc/gpd/mcp/servers/conventions_server.pysrc/gpd/mcp/servers/errors_mcp.pysrc/gpd/mcp/servers/protocols_server.pysrc/gpd/mcp/servers/verification_server.pysrc/gpd/mcp/verification_contract_policy.pysrc/gpd/specs/references/verification/audits/verification-gap-analysis.mdsrc/gpd/specs/references/verification/core/verification-quick-reference.mdsrc/gpd/specs/templates/contract-results-schema.mdsrc/gpd/specs/templates/research-verification.mdsrc/gpd/specs/workflows/verify-phase.mdsrc/gpd/specs/workflows/verify-work-stage-manifest.jsonsrc/gpd/specs/workflows/verify-work/inventory-build.mdtests/core/test_error_catalog.pytests/core/test_prompt_exactness_budget.pytests/core/test_prompt_wiring.pytests/core/test_protocol_catalog.pytests/core/test_research_correctness_prompt_visibility.pytests/core/test_review_contract_prompt_visibility.pytests/core/test_verifier_prompt_contract_visibility.pytests/core/test_workflow_staging.pytests/mcp/test_servers_integration.pytests/test_cli_convention_gaps.pytests/test_cli_refs.pytests/test_cli_verify_contract.py
💤 Files with no reviewable changes (2)
- src/gpd/commands/verify-work.md
- src/gpd/core/workflow_staging.py
Drop binding.forbidden_proxy_ids from optional_request_fields when the hint promotes it to required; dedupe error_class_ids before computing verification coverage. The allowed_families/forbidden_families typing suggestion is intentionally not taken: the fields are optional-but-not-nullable by pinned contract (test_contract_metadata_family_lists_are_optional_but_ not_nullable rejects explicit null and requires a plain array schema); a clarifying comment now records that. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jungdaesuh
force-pushed
the
feat/verification-cli-parity
branch
from
July 27, 2026 22:22
02f3525 to
219e526
Compare
Author
|
Addressed the review in the latest push:
Full suite after the changes: 13,021 passed, 0 failed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Makes GPD's verification/convention/reference surfaces transport-neutral: the logic behind the built-in MCP servers moves into
gpd.coremodules, the servers become thin wrappers over the same code, and a matchinggpdCLI surface is added. The MCP servers remain installed and fully functional — this PR removes nothing. After it, no GPD workflow depends on them: the two prompt files that referencedmcp__gpd_verification__*tools now shell out to the CLI.Motivation
Auditing tool usage across the 71 commands + 24 agents showed only 3 of the ~41 built-in MCP tools were referenced by any prompt (
run_contract_check,suggest_contract_checks,get_bundle_checklistinverify-work.md/gpd-verifier.md). Meanwhile each session pays for 7 resident server processes. Decoupling the logic from the transport lets every runtime reach the same functionality through one-shot CLI calls, keeps a single source of truth, and leaves the MCP layer as a pure optional transport.Changes
gpd.coremodules (moved, not copied):error_catalog,protocol_catalog,contract_checks,convention_checks,envelopes,verification_bindings. Servers now delegate; the existingtests/mcp/suite passes unmodified (830 tests) as the parity oracle, plus an AST-level move audit (zero dropped functions/constants).gpd verify contract-check | suggest-checks | bundle-checklist | checklist | coverage,gpd convention validate-assert | subfield-defaults,gpd refs errors | protocols. Same schema-versioned envelopes as the MCP tools; strict exit codes (failed check / missing bundle id / empty id list are non-zero); fail-fast guard for-payloads on an interactive stdin; loud flag-conflict errors.verify-work.md,gpd-verifier.md, verify-work stage manifest,workflow_staging.pyregistry, and related spec prose now usegpd --raw verify ...(with--project-dirpreserved socontract_warningsstill ground against the project root, matching 8f4eadc's intent).tests/test_cli_verify_contract.py,tests/test_cli_refs.py,tests/test_cli_convention_gaps.py,tests/core/test_error_catalog.py,tests/core/test_protocol_catalog.py); prompt-pinning tests updated to the CLI contract.exact_assertion_countratchet raised 5165→5171 for six new deliberate contract locks (commented in the file).Validation
ruff check/ruff formatclean.tests/mcp/unmodified-green throughout (parity oracle).schema_versionparity, flag-conflict handling).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests