feat: exclude external-analyzer test files from CRAP scoring - #286
Conversation
|
🤖 Review · Commit: |
|
🤖 Finished Review · ✅ Success · Started 11:30 PM UTC · Completed 11:48 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $11.20 |
|
Risk Assessment: moderate (2/5) DetailsModerate risk. 25-file feature PR with good test ratio (32%), no protected paths, no security-sensitive files, no CI/dependency changes, by an established contributor with a well-scoped linked issue. Primary risk factor is high churn on cmd/gaze/main.go. Changes are well-contained within the adapter/external-analyzer subsystem. |
ReviewFindingsMedium
Low
Next steps:
|
3272205 to
6dd29ae
Compare
Addresses PR unbound-force#286 review feedback from @fullsend-ai-review[bot]. Signed-off-by: Jay Flowers <jay.flowers@gmail.com> Assisted-by: gpt-5.6-luna
Addresses PR unbound-force#286 review feedback from @fullsend-ai-review[bot]. Signed-off-by: Jay Flowers <jay.flowers@gmail.com> Assisted-by: gpt-5.6-luna
Addresses PR unbound-force#286 review feedback from @fullsend-ai-review[bot]. Signed-off-by: Jay Flowers <jay.flowers@gmail.com> Assisted-by: gpt-5.6-luna
A discover timeout kills the shared analyzer subprocess (the transport
kills on context deadline), so continuing with the same client poisons
every subsequent provider call. Split discover failure handling: protocol
errors and unmarshal errors still degrade gracefully (warn + no filtering),
but a timeout now returns an error so Session.Initialize fails fast.
Also fixes review-council LOW findings:
- share an isTestFile helper for the filepath.Clean lookup (D3)
- replace the bare "test_function_no_target_effects" magic string with a
typed ContractCoverageReasonType constant
- return a defensive copy from DiscoverTestFiles()
- correct SetTestFiles -> constructor-injection doc drift in design/tasks
- align json-schemas.md wording ("empty unioned target effects")
- soften the proposal's Go-mode regression-test claim
- add fake-analyzer --hang-discover and --empty-discover modes with tests
Addresses PR unbound-force#286 review feedback from @fullsend-ai-review[bot].
Signed-off-by: Jay Flowers <jflowers@unbound-force.com>
Assisted-by: deepseek-v4-pro
Amend tasks.md 1.2/6.4 and design.md Goals/D2 to reflect the constructor-injection filter and the timeout fail-fast split, removing the final stale SetTestFiles references and "no hard errors" language. Addresses PR unbound-force#286 review feedback from @fullsend-ai-review[bot]. Signed-off-by: Jay Flowers <jflowers@unbound-force.com> Assisted-by: deepseek-v4-pro
|
Addressed in |
- Filter test files out of CRAP scoring using the analyzer's own discover test_files, at the adapter layer (no scoring-engine change) - Add no_contract_expected/reason quality sentinel for test functions with no production target to assert on - Graceful degradation: discover unsupported or failing leaves filtering off and warns to stderr Assisted-by: deepseek-v4-pro Generated with AI assistance (deepseek-v4-pro)
- external-analyzer-filtering: filter test files at the adapter layer via discover test_files, not computeScores - schema-sentinel-pattern: additive no_contract_expected/reason bool+string, never an out-of-band -1 sentinel - review-hallucination-gotcha: verify reviewer-cited file paths against disk before fixing findings Assisted-by: deepseek-v4-pro Generated with AI assistance (deepseek-v4-pro)
Addresses PR unbound-force#286 review feedback from @fullsend-ai-review[bot]. Signed-off-by: Jay Flowers <jay.flowers@gmail.com> Assisted-by: gpt-5.6-luna
Addresses PR unbound-force#286 review feedback from @fullsend-ai-review[bot]. Signed-off-by: Jay Flowers <jay.flowers@gmail.com> Assisted-by: gpt-5.6-luna
Addresses PR unbound-force#286 review feedback from @fullsend-ai-review[bot]. Signed-off-by: Jay Flowers <jay.flowers@gmail.com> Assisted-by: gpt-5.6-luna
A discover timeout kills the shared analyzer subprocess (the transport
kills on context deadline), so continuing with the same client poisons
every subsequent provider call. Split discover failure handling: protocol
errors and unmarshal errors still degrade gracefully (warn + no filtering),
but a timeout now returns an error so Session.Initialize fails fast.
Also fixes review-council LOW findings:
- share an isTestFile helper for the filepath.Clean lookup (D3)
- replace the bare "test_function_no_target_effects" magic string with a
typed ContractCoverageReasonType constant
- return a defensive copy from DiscoverTestFiles()
- correct SetTestFiles -> constructor-injection doc drift in design/tasks
- align json-schemas.md wording ("empty unioned target effects")
- soften the proposal's Go-mode regression-test claim
- add fake-analyzer --hang-discover and --empty-discover modes with tests
Addresses PR unbound-force#286 review feedback from @fullsend-ai-review[bot].
Signed-off-by: Jay Flowers <jflowers@unbound-force.com>
Assisted-by: deepseek-v4-pro
Amend tasks.md 1.2/6.4 and design.md Goals/D2 to reflect the constructor-injection filter and the timeout fail-fast split, removing the final stale SetTestFiles references and "no hard errors" language. Addresses PR unbound-force#286 review feedback from @fullsend-ai-review[bot]. Signed-off-by: Jay Flowers <jflowers@unbound-force.com> Assisted-by: deepseek-v4-pro
7cee3f6 to
1a56035
Compare
jflowers
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Note: Could not post as APPROVE due to insufficient permissions (self-review prohibition). Posted as COMMENT instead. Original verdict: APPROVE.
The implementation thoroughly filters external-analyzer test files from CRAP scoring at the adapter layer and adds a no_contract_expected/reason quality sentinel for test functions with no production contract. All spec scenarios are covered by tests, all 5 CI checks pass, and all 7 prior review findings were addressed.
CI Status
All 5 checks PASS (Unit+Integration Go 1.24/1.25, E2E Go 1.24/1.25, MegaLinter).
Walkthrough
internal/adapter/session.go—testFilesfield,discoverin Initialize,DiscoverTestFiles()accessorinternal/adapter/complexity.go— constructor-injected filter viafilterTestFiles/isTestFileinternal/adapter/quality.go—testFilesparam, no-contract-expected sentinel, summary exclusioninternal/taxonomy/types.go—NoContractExpected/Reason+ContractCoverageReasonTypeinternal/report/schema.go—no_contract_expected/reasonschema properties- Extensive unit + integration tests; graceful degradation on discover failure/timeout
Alignment / Security / Constitution
No issues found. Issue #284 acceptance criteria both satisfied; Go-mode behavior unchanged; no new untrusted inputs or secrets.
This review was generated by /uf.review-pr (AI-assisted).
Addresses PR #286 review feedback from @fullsend-ai-review[bot]. Signed-off-by: Jay Flowers <jay.flowers@gmail.com> Assisted-by: gpt-5.6-luna
Addresses PR #286 review feedback from @fullsend-ai-review[bot]. Signed-off-by: Jay Flowers <jay.flowers@gmail.com> Assisted-by: gpt-5.6-luna
Addresses PR #286 review feedback from @fullsend-ai-review[bot]. Signed-off-by: Jay Flowers <jay.flowers@gmail.com> Assisted-by: gpt-5.6-luna
A discover timeout kills the shared analyzer subprocess (the transport
kills on context deadline), so continuing with the same client poisons
every subsequent provider call. Split discover failure handling: protocol
errors and unmarshal errors still degrade gracefully (warn + no filtering),
but a timeout now returns an error so Session.Initialize fails fast.
Also fixes review-council LOW findings:
- share an isTestFile helper for the filepath.Clean lookup (D3)
- replace the bare "test_function_no_target_effects" magic string with a
typed ContractCoverageReasonType constant
- return a defensive copy from DiscoverTestFiles()
- correct SetTestFiles -> constructor-injection doc drift in design/tasks
- align json-schemas.md wording ("empty unioned target effects")
- soften the proposal's Go-mode regression-test claim
- add fake-analyzer --hang-discover and --empty-discover modes with tests
Addresses PR #286 review feedback from @fullsend-ai-review[bot].
Signed-off-by: Jay Flowers <jflowers@unbound-force.com>
Assisted-by: deepseek-v4-pro
Amend tasks.md 1.2/6.4 and design.md Goals/D2 to reflect the constructor-injection filter and the timeout fail-fast split, removing the final stale SetTestFiles references and "no hard errors" language. Addresses PR #286 review feedback from @fullsend-ai-review[bot]. Signed-off-by: Jay Flowers <jflowers@unbound-force.com> Assisted-by: deepseek-v4-pro
Summary
External analyzers instrument only source files (e.g.
--cov=src), so testfunctions get no coverage entry and are scored at a phantom 0% — inflating
CRAP/CRAPload/quadrant counts and emitting spurious
add_testsflags. Thischange filters test files out of CRAP scoring using the analyzer's own
discovertest_files(consumed at the adapter layer, never reachingcomputeScores), and adds ano_contract_expected/reasonquality sentinelfor test functions whose target has no production contract to assert.
Closes #284.
How to Test
How to Demo
Run
gaze crap --analyzer <fake-analyzer> --language python ./srcagainst thefake analyzer in
internal/protocol/testdata/fake_analyzer/. The test-filefunction
tests/test_ops.py::test_addis absent from scores and fix-strategycounts (3 source functions only). Run
gaze quality --analyzer <fake>to seeno_contract_expected: true+reason: "test_function_no_target_effects"onthe
test_addreport. Analyzers without thediscovercapability (or whosediscoverfails) degrade gracefully to unfiltered scoring with a warning.Key Files Changed
internal/adapter/session.go—testFilesfield,discoverinInitialize,DiscoverTestFiles()accessorinternal/adapter/complexity.go—SetTestFilessetter +filterTestFilesinternal/adapter/quality.go—BuildQualityFromMappingstestFilesparam, sentinel, summary exclusioninternal/taxonomy/types.go—NoContractExpected/Reasonfields onContractCoverageinternal/report/schema.go+report_test.go— schema properties + validation testcmd/gaze/main.go— threadDiscoverTestFiles()into quality pathinternal/adapter/{session_test.go,complexity_internal_test.go,quality_internal_test.go,adapter_test.go,call_test.go}— testsinternal/protocol/testdata/fake_analyzer/main.go+client_test.go— fixture extensionscmd/gaze/external_analyzer_test.go— CRAP/quality integration assertionsdocs/protocol.md,docs/reference/json-schemas.md— docsopenspec/changes/exclude-external-test-functions/— spec artifactsThis PR was generated by /uf.finale (AI-assisted).