Skip to content

feat: consolidate requirement verdict computation across status and MCP#414

Merged
jimisola merged 8 commits into
mainfrom
refactor/consolidate-requirement-verdict
Jun 23, 2026
Merged

feat: consolidate requirement verdict computation across status and MCP#414
jimisola merged 8 commits into
mainfrom
refactor/consolidate-requirement-verdict

Conversation

@jimisola

@jimisola jimisola commented Jun 22, 2026

Copy link
Copy Markdown
Member

Summary

  • Adds the OpenSpec change consolidate-requirement-verdict (proposal, design, spec delta, tasks) and implements it.
  • Extracts compute_requirement_status() (per-requirement implementation/automated/manual verdict) and a shared _requirement_to_dict() serializer in statistics_service.py, queried through the repository's scoped per-requirement getters.
  • StatisticsService (status/report/export/MCP get_status) and the MCP get_requirement_status/get_requirements_status tools now derive their verdict from this single predicate, instead of two independent implementations that had already drifted once (fix(mcp): correctly flag skipped/missing automated tests as unmet #411).
  • Deletes the duplicate _compute_meets/_build_automated_test_summary predicate from common/queries/details.py.
  • Exposes include_post_build (default False) on the MCP status tools for parity with status --with-post-tests.
  • BREAKING (MCP output): get_requirement_status/get_requirements_status now emit the unified status shape (completed, implementation_type, automated_tests/manual_tests), replacing the old meets_requirements/flat test_summary shape. Some verdict values also change for requirements with post-build-only SVCs, not-applicable cases, and unrecorded test executions — these now match status exactly.
  • Adds MCP_0005/SVC_MCP_0005 to the reqstool SSOT and a test asserting get_requirement_status/get_requirements_status agree with StatisticsService in both scoping modes, covering the previously divergent REQ_ext002_300.
  • Adds a CLAUDE.md convention note: the verdict has one source of truth; no parallel re-derivation.

Closes #412

Test plan

  • hatch run dev:pytest --cov=reqstool — 940 passed
  • hatch run dev:flake8 — clean
  • CLAUDE.md regression smoke (status/report on test_standard/test_basic fixtures) — byte-identical to main
  • reqstool status local -p docs/reqstool — 72/72 complete, SVC_MCP_0005 covered
  • openspec validate --all --strict — all pass

@jimisola jimisola left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Full PR Review — Consolidated Findings

Scope: docs-only (OpenSpec proposal/design/tasks/spec-delta + CLAUDE.md note + OpenSpec config scaffold). No code changes. Every factual claim in design.md was verified against the repo at the PR HEAD — all referenced symbols exist and MCP_0005 is the correct next ID. openspec validate --strict passes.

# Prio Location Finding Fix
1 Medium specs/mcp/spec.md:3-7 Spec delta is a tautological placeholder ("The system SHALL implement MCP_0005" / "SHALL pass SVC_MCP_0005"). It carries none of the behavioral requirement that proposal.md describes. openspec validate --strict passes (structure-only), so the stub slips through. Write a real requirement + WHEN/THEN scenario expressing that the per-requirement MCP status tools report a verdict/structure identical to the status command, from one shared computation.
2 Low CLAUDE.md:164 "silently drifted and caused bugs (#411, #412)" — #412 is the open consolidation tracking issue (this work), not a bug caused by drift; only #411 was the drift bug. Reword to "caused bug #411; consolidation tracked in #412".
3 Low tasks.md:210 Task 5.2 hardcodes "72/72 complete"; tasks 1.1/1.2 add a new requirement, so the count is stale on arrival and brittle. Use "all requirements complete with SVC_MCP_0005 covered" instead of a fixed count.
4 Low design.md/proposal.md Refer to the details.py function as get_requirements_status, but the impl is get_requirements_status_all (only the MCP tool is get_requirements_status). tasks.md uses the correct name. Align prose with get_requirements_status_all.
5 Info openspec/config.yaml Committed as the default scaffold with all context/rules commented out — optional opportunity to populate context:. Optional; fine to leave as standard scaffold.

Net: clean, well-reasoned proposal. #1 is the only near-blocking item — the spec delta should express the real requirement since that file survives into the archive.


Automated — /x:full-pr-review

@jimisola jimisola left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correction to the review above

After checking the existing openspec/specs/mcp/spec.md, two of my findings were wrong — retracting them:

  • #1 (spec delta "placeholder") — RETRACTED. The existing mcp spec uses the exact same The system SHALL implement MCP_000X / SHALL pass SVC_MCP_000X form by design: its Purpose note states requirement/SVC content is owned by reqstool (SSOT) and injected at read time via reqstool enrich. The MCP_0005 delta correctly follows that convention; there is nothing to fix.
  • #4 (function name) — RETRACTED. design.md/proposal.md correctly name the MCP tools (get_requirements_status); tasks.md correctly names the impl function (get_requirements_status_all). Both accurate in context.

Applied fixes (committed to the branch):

  • #2 CLAUDE.md — reworded "caused bugs (#411, #412)" → "caused bug #411 (consolidation tracked in #412)", since #412 is the tracking issue, not a drift-caused bug.
  • #3 tasks.md 5.2 — replaced the hardcoded "72/72 complete" with "all requirements complete with SVC_MCP_0005 covered".

#5 (config.yaml scaffold) left as-is (optional). openspec validate --strict still passes.


Automated — /x:full-pr-review

jimisola added 2 commits June 22, 2026 18:16
Adds the OpenSpec proposal/design/tasks for unifying the per-requirement
"complete" verdict behind a single computation, replacing the duplicate
predicate in common/queries/details.py that has already drifted from
StatisticsService once (#411).

Signed-off-by: Jimisola Laursen <jimisola@jimisola.com>
Signed-off-by: Jimisola Laursen <jimisola@jimisola.com>
@jimisola jimisola force-pushed the refactor/consolidate-requirement-verdict branch from 77d1225 to 6eeee17 Compare June 22, 2026 16:16
jimisola added 2 commits June 22, 2026 18:39
…, shared serializer, flag parity, LSP constraint

Signed-off-by: Jimisola Laursen <jimisola@jimisola.com>
Extracts compute_requirement_status() and the shared _requirement_to_dict()
serializer so StatisticsService (status/report/export) and the MCP
get_requirement_status/get_requirements_status tools derive the "complete"
verdict from one predicate instead of two that had already drifted (#411).
Deletes the duplicate _compute_meets/_build_automated_test_summary path in
details.py, exposes include_post_build on the MCP tools for parity with
`status --with-post-tests`, and adds MCP_0005/SVC_MCP_0005 plus an
agreement test covering the previously divergent REQ_ext002_300.

Closes #412

Signed-off-by: Jimisola Laursen <jimisola@jimisola.com>
@jimisola jimisola changed the title docs(openspec): propose consolidating requirement verdict computation feat: consolidate requirement verdict computation across status and MCP Jun 22, 2026
@jimisola jimisola self-assigned this Jun 23, 2026

@jimisola jimisola left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Full PR Review — Consolidated Findings

7 checks run: review, code-review, smells, advanced-smells, security, cruft, testing. Security: no findings.

# Prio Location Finding Fix
1 High docs/modules/ROOT/pages/mcp.adoc:177,189,194-206 Docs still describe the deleted MCP shape (implementation, test_summary, meets_requirements) — actively misleading for MCP API consumers. Update to the new shape and document include_post_build.
2 High tests/unit/reqstool/common/queries/test_details.py:959-983 include_post_build parametrized test can't prove the flag changes anything — no SVC in the fixture sets phase, so both modes select the identical SVC set. Add a fixture SVC with phase: post-build and assert behavior actually differs.
3 High src/reqstool/storage/requirements_repository.py:131-136 New get_svc() — load-bearing for every verdict — has zero test coverage, including the None path. Add a unit test for found/not-found.
4 Medium statistics_service.py:22, details.py:8 _requirement_to_dict is underscore-prefixed yet listed in __all__ and imported across a module boundary. (4/7 checks) Rename to requirement_to_dict.
5 Medium statistics_service.py:101-130 N+1 query pattern: per-requirement, per-SVC get_svc() round-trips replace 4 bulk fetches. Indexed, so sub-ms at today's scale per design doc. (4/7 checks) No fix required now; optional follow-up if profiling shows it matters.
6 Medium statistics_service.py:170-183 _compute_requirement_automated_stats double-queries annotations (checks truthiness, then get_test_results_for_svc re-fetches them internally). Reuse the already-fetched annotations list inline.
7 Medium statistics_service.py:101 compute_requirement_status has zero direct unit tests; the TypeError branch in _check_implementation is untested anywhere. Add direct unit tests covering not_applicable, missing MVR/test, and the TypeError branch.
8 Medium test_details.py:961-983 The new agreement test only proves both callers agree — they call the same predicate now, so it could pass trivially even with a predicate bug. Add assertions on known expected values for a specific requirement.
9 Medium test_details.py:167 _make_db_with_req default verification change (MANUAL_TESTAUTOMATED_TEST) silently shifts which gate several pre-existing tests exercise. Document/assert which gate each test exercises; cover the now-orphaned MANUAL_TEST+MVR combination explicitly.
10 Low statistics_service.py:101 req parameter is untyped while siblings are. Annotate as req: RequirementData.
11 Low test_details.py Test names retain old meets_requirements/compute_meets vocabulary. Rename to test_completed_*/test_verdict_*.
12 Low mcp/server.py:69,109 No negative-path test for include_post_build combined with an invalid id. Parametrize the not-found test over include_post_build.

Not flagged (verified, no action needed): get_svc() SQL correctly parameterized; compute_requirement_status faithfully replicates pre-PR StatisticsService logic; no dead code left from the deleted predicate; include_post_build plumbing proportionate; no security findings.

Fixes for all of the above are being pushed to this branch.


Automated — /x:full-pr-review

…tion

- docs/modules/ROOT/pages/mcp.adoc: update stale MCP status tool docs to
  the new shape (completed/implementation_type/automated_tests/manual_tests)
  and document include_post_build, replacing the deleted meets_requirements/
  test_summary/implementation shape.
- Rename _requirement_to_dict -> requirement_to_dict: it's a deliberate
  shared serializer imported across module boundaries, not module-private.
- requirements_repository.py: extract get_test_results_for_annotations so
  _compute_requirement_automated_stats reuses the annotation list it already
  fetched instead of re-querying it inside get_test_results_for_svc.
- Type the req parameter on compute_requirement_status.
- Add direct unit tests for compute_requirement_status (not_applicable,
  missing MVR/test, completed, and the TypeError guard), for the new
  RequirementsRepository.get_svc() getter (found/not-found), and a
  dedicated test proving include_post_build actually changes the verdict
  (the existing fixture never varies SVC phase, so the prior agreement
  test alone couldn't prove this).
- Strengthen the cross-path agreement test with known-correct expected
  values for REQ_ext002_300, not just cross-path equality.
- Rename test_meets_requirements_*/test_compute_meets_* to test_completed_*
  to match the new vocabulary, and document which gate (automated_tests vs
  manual_tests) each _make_db_with_req-based test exercises.
- Add a negative-path test for get_requirement_status with an invalid id
  combined with include_post_build.

Addresses findings from the automated full-PR-review on #414.

Signed-off-by: Jimisola Laursen <jimisola@jimisola.com>

@jimisola jimisola left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Full PR Review — Round 2 (commit e1a0145)

All 12 round-1 findings verified as correctly and completely fixed. 854 unit + 17 integration tests pass; flake8/black clean. Round 2 surfaced 2 new, minor follow-ups the fix commit's scope didn't reach:

# Prio Location Finding Fix
1 Medium src/reqstool/common/queries/details.py:91-92 (get_urn_details) Same duplicate-query pattern eliminated in compute_requirement_status still exists here: fetches test_annotations then calls get_test_results_for_svc(svc.id), which re-fetches the same annotations internally. Change to repo.get_test_results_for_annotations(svc.id.urn, test_annotations).
2 Low src/reqstool/storage/requirements_repository.py:270 (get_test_results_for_annotations) New repository method has no direct unit test — only exercised transitively via compute_requirement_status tests. Its sibling get_svc got a direct found/not-found pair in the same commit; this one didn't. Add a direct unit test covering the CLASS and METHOD annotation branches.

Everything else checked out clean: all 6 round-1 test-coverage fixes verified substantive (including mutation-testing the include_post_build test to confirm it's load-bearing), the _requirement_to_dictrequirement_to_dict rename is complete repo-wide, docs are consistent, no new cruft, no security findings, no over-engineering.

Fixes for both are being pushed to this branch.


Automated — /x:full-pr-review

- get_urn_details (details.py) still had the duplicate-annotations-query
  pattern that get_test_results_for_annotations was extracted to fix;
  route it through the shared method like compute_requirement_status does.
- Add direct unit tests for get_test_results_for_annotations (METHOD found,
  METHOD missing, CLASS aggregation) and a test pinning that
  get_test_results_for_svc delegates to it without behavior drift.

Signed-off-by: Jimisola Laursen <jimisola@jimisola.com>
@jimisola jimisola merged commit 1b7436f into main Jun 23, 2026
7 checks passed
@jimisola jimisola deleted the refactor/consolidate-requirement-verdict branch June 23, 2026 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consolidate duplicated automated-test-completeness logic between details.py and statistics_service.py

1 participant