fix(code-review): coerce pylint line 0 and empty messages for ReviewFinding#208
fix(code-review): coerce pylint line 0 and empty messages for ReviewFinding#208
Conversation
…inding. Pylint JSON can use line 0 for file-scoped messages; ReviewFinding requires line >= 1, so Pydantic ValidationError was raised, caught as ValueError, and the whole pylint step collapsed into a single tool_error. Normalize line to at least 1, substitute a placeholder for blank messages, strip stdout before JSON parse, and add regression tests. Made-with: Cursor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🧰 Additional context used📓 Path-based instructions (2)**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php,cpp,c,h}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
tests/**/*.py⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (2)📓 Common learnings📚 Learning: 2026-04-13T10:38:43.535ZApplied to files:
🔀 Multi-repo context nold-ai/specfact-cliLinked repositories findingsnold-ai/specfact-cli
Notes on tests: rg hits refer to tests/unit/specfact_code_review/tools/test_pylint_runner.py, but that path could not be printed in this repo checkout (file not found when attempting to read). The openspec and scripts confirm the relevant contract points above. [::nold-ai/specfact-cli::tests/unit/specfact_code_review/tools (referenced by search)] 🔇 Additional comments (3)
📝 Walkthroughpylint Findings Normalization and Line/Message CoercionBundle & Module Surface
Manifest & Integrity
Tests
Cross-Repo / CLI Alignment
Docs / Registry / OpenSpec
Reviewer notes
WalkthroughBumped package version to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2ee98360f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/specfact-code-review/src/specfact_code_review/tools/pylint_runner.py`:
- Around line 95-98: The code currently treats whitespace-only stdout as a
successful empty payload by returning [] when "stripped = stdout.strip()" is
falsy; instead, preserve adapter boundaries by treating empty/whitespace stdout
as a parsing/tool failure. Update the block in pylint_runner.py so that if
"stripped" is empty you do not return [] but raise or propagate an error (e.g.,
raise a descriptive exception or call the existing tool_error/pathway) that
includes the raw "stdout" and any "stderr"/exit info, and otherwise call
json.loads(stripped) to produce "payload"; keep references to "stripped",
"stdout", "json.loads" and "payload" so the change is localized and observable.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a209d390-43ce-4542-92de-d2c1baa10219
📒 Files selected for processing (3)
packages/specfact-code-review/module-package.yamlpackages/specfact-code-review/src/specfact_code_review/tools/pylint_runner.pytests/unit/specfact_code_review/tools/test_pylint_runner.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: quality (3.12)
- GitHub Check: quality (3.11)
- GitHub Check: quality (3.13)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php,cpp,c,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Preserve the clean-code compliance gate and its category references (naming, kiss, yagni, dry, and solid)
Files:
packages/specfact-code-review/src/specfact_code_review/tools/pylint_runner.pytests/unit/specfact_code_review/tools/test_pylint_runner.py
packages/**/src/**/*.py
⚙️ CodeRabbit configuration file
packages/**/src/**/*.py: Focus on adapter and bridge patterns: imports from specfact_cli (models, runtime, validators),
Typer/Rich command surfaces, and clear boundaries so core upgrades do not silently break bundles.
Flag breaking assumptions about registry loading, lazy imports, and environment/mode behavior.
Files:
packages/specfact-code-review/src/specfact_code_review/tools/pylint_runner.py
packages/**/module-package.yaml
⚙️ CodeRabbit configuration file
packages/**/module-package.yaml: Validate metadata: name, version, commands, dependencies, and parity with packaged src.
Call out semver and signing implications when manifests or payloads change.
Files:
packages/specfact-code-review/module-package.yaml
tests/**/*.py
⚙️ CodeRabbit configuration file
tests/**/*.py: Contract-first and integration tests: migration suites, bundle validation, and flakiness.
Ensure changes to adapters or bridges have targeted coverage.
Files:
tests/unit/specfact_code_review/tools/test_pylint_runner.py
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Fix SpecFact code review findings, including warnings, unless a rare explicit exception is documented
📚 Learning: 2026-04-13T10:38:22.837Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-13T10:38:22.837Z
Learning: Signed module or manifest changes require version-bump review and verify-modules-signature
Applied to files:
packages/specfact-code-review/module-package.yaml
📚 Learning: 2026-04-13T10:38:22.837Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-13T10:38:22.837Z
Learning: This repository enforces the clean-code review gate through hatch run specfact code review run --json --out .specfact/code-review.json
Applied to files:
packages/specfact-code-review/module-package.yaml
📚 Learning: 2026-04-13T10:38:43.524Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Fix SpecFact code review findings, including warnings, unless a rare explicit exception is documented
Applied to files:
packages/specfact-code-review/module-package.yaml
🔀 Multi-repo context nold-ai/specfact-cli
nold-ai/specfact-cli
-
ReviewFinding model used/defined in pre-commit script:
- scripts/pre_commit_code_review.py:33,62 — ReviewFinding model and findings list (consumes ReviewFinding contract). [::nold-ai/specfact-cli::scripts/pre_commit_code_review.py]
-
Multiple openspec requirements and runner specs reference run_pylint / pylint-runner behavior (these are consumers / contract docs that expect run_pylint to map tool output into ReviewFinding and to return tool_error on parse error):
- openspec/specs/pylint-runner/spec.md — scenarios for run_pylint and expected ReviewFinding mapping. [::nold-ai/specfact-cli::openspec/specs/pylint-runner/spec.md]
- openspec/changes/archive/2026-03-17-code-review-03-type-governance-runners/specs/pylint-runner/spec.md — same. [::nold-ai/specfact-cli::openspec/changes/archive/2026-03-17-code-review-03-type-governance-runners/specs/pylint-runner/spec.md]
- openspec/specs/review-finding-model/spec.md — requirement that ReviewFinding Pydantic model validates fields including line and message. [::nold-ai/specfact-cli::openspec/specs/review-finding-model/spec.md]
-
Unit tests and TDD evidence referencing test_pylint_runner and run_pylint expectations:
- openspec/changes/archive/.../TDD_EVIDENCE.md mentions tests/unit/specfact_code_review/tools/test_pylint_runner.py and that tests assert mapping/filtering/parse-error handling. [::nold-ai/specfact-cli::openspec/changes/archive/2026-03-17-code-review-03-type-governance-runners/TDD_EVIDENCE.md]
- Test references across repo asserting the module bundle presence and install behavior (specfact-code-review is expected to be installed/available by profiles and e2e tests): tests/unit/specfact_cli/registry/test_profile_presets.py and related tests assert inclusion/installation of "specfact-code-review". [::nold-ai/specfact-cli::tests/unit/specfact_cli/registry/test_profile_presets.py]
- skills/specfact-code-review/SKILL.md — metadata indicating module name and linkage. [::nold-ai/specfact-cli::skills/specfact-code-review/SKILL.md]
-
Behavior expectations that relate to this PR:
- Several specs/design docs state "Parse error in tool output → ReviewFinding(category='tool_error')" but also that run_pylint maps each finding to ReviewFinding. The PR's coercion of line/message and whitespace-trim before JSON parsing reduces the chance of tool_error collapsing the pylint step and better satisfies the per-finding mapping expected by the specs/tests. Relevant doc pointers above. [::nold-ai/specfact-cli::openspec/specs/pylint-runner/spec.md] [::nold-ai/specfact-cli::openspec/specs/review-finding-model/spec.md]
Assessment / cross-repo impact
- specfact-cli contains specs, tests, and scripts that depend on the specfact-code-review module exposing run_pylint and producing ReviewFinding instances with validated fields. The change in the specfact-code-review pylint runner (coercing line/message, trimming stdout) aligns with these expectations and should reduce spurious tool_error results that downstream tests/specs consider exceptional.
- No direct code-level consumers of the internal helper names (coerce_pylint*) were found in this repo; impact is limited to behavior/contract of run_pylint / ReviewFinding parsing rather than changing public APIs or types referenced here.
🔇 Additional comments (4)
packages/specfact-code-review/module-package.yaml (2)
2-2: Patch bump is appropriate for this fix scope.
0.47.9 → 0.47.10is a clean semver patch increment for a parsing robustness fix.
26-26:⚠️ Potential issue | 🟠 MajorRestore
integrity.signaturefor the updated manifest payload.At Line 26, checksum changed but the manifest has no signature field. This can break signature-required verification paths and weakens release integrity guarantees. Please re-sign this manifest (or ensure CI signing injects it before merge/publish).
As per coding guidelines:
packages/**/module-package.yaml— “Validate metadata... Call out semver and signing implications when manifests or payloads change.”
Based on learnings: “Signed module or manifest changes require version-bump review and verify-modules-signature.”⛔ Skipped due to learnings
Learnt from: CR Repo: nold-ai/specfact-cli-modules PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2026-04-13T10:38:22.837Z Learning: Signed module or manifest changes require version-bump review and verify-modules-signatureLearnt from: CR Repo: nold-ai/specfact-cli-modules PR: 0 File: AGENTS.md:0-0 Timestamp: 2026-04-13T10:38:43.524Z Learning: Enforce module signatures and version bumps when signed module assets or manifests are affectedLearnt from: djm81 Repo: nold-ai/specfact-cli-modules PR: 136 File: registry/modules/specfact-spec-0.40.17.tar.gz.sha256:1-1 Timestamp: 2026-04-02T21:49:11.371Z Learning: In nold-ai/specfact-cli-modules, module tarball signatures (registry/signatures/*.tar.sig) are generated by the `publish-modules` GitHub Actions runner during the publish workflow, not committed locally to the branch. Missing signature files should NOT be flagged as a pre-merge blocker in PRs.packages/specfact-code-review/src/specfact_code_review/tools/pylint_runner.py (1)
47-80: Normalization at the adapter boundary is solid.Coercing
line/messagebeforeReviewFindingconstruction preserves per-finding mapping and avoids collapsing valid payloads into a single error path.tests/unit/specfact_code_review/tools/test_pylint_runner.py (1)
100-153: Good targeted regression coverage for the adapter change.These tests directly exercise the new normalization paths and whitespace-tolerant parsing behavior in
run_pylint.As per coding guidelines:
tests/**/*.py— “Ensure changes to adapters or bridges have targeted coverage.”
djm81
added
bug
Blank or whitespace-only stdout no longer maps to zero findings; raise with stdout, stderr preview, and returncode so run_pylint surfaces a tool_error. Adds regression tests for empty and whitespace-only stdout. Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/specfact-code-review/src/specfact_code_review/tools/pylint_runner.py`:
- Around line 96-102: The error raised when pylint output is empty embeds the
full stdout, which can be huge; update the block in parse section where it
checks `if not stripped:` to apply the same truncation logic used for `stderr`
to `stdout` before composing the ValueError message—create a preview variable
(e.g., `out = stdout`) and if `len(out) > 4096` truncate it to
`f"{out[:4096]}... ({len(stdout)} chars total)"`, then use that truncated `out`
(and the already-truncated `err`) in the `ValueError` message that references
`stdout`, `stderr`, and `returncode`.
In `@tests/unit/specfact_code_review/tools/test_pylint_runner.py`:
- Around line 100-138: Add a targeted unit test that verifies the stderr
truncation branch in run_pylint: mock subprocess.run (as in the existing tests
using completed_process) to return a very long stderr string (>4096 chars) and
call run_pylint([file_path]); then assert the returned finding is a tool_error
from "pylint" and that the message contains the truncated stderr (e.g., does not
contain the full original stderr and respects the 4096-char truncation contract,
or contains the expected ellipsis/truncation marker used by the parser).
Reference run_pylint, subprocess.run, and the completed_process helper when
locating where to add the new test.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 56d81e69-d126-4fe7-b8c2-10085580735b
📒 Files selected for processing (3)
packages/specfact-code-review/module-package.yamlpackages/specfact-code-review/src/specfact_code_review/tools/pylint_runner.pytests/unit/specfact_code_review/tools/test_pylint_runner.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: verify-module-signatures
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php,cpp,c,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Preserve the clean-code compliance gate and its category references (naming, kiss, yagni, dry, and solid)
Files:
packages/specfact-code-review/src/specfact_code_review/tools/pylint_runner.pytests/unit/specfact_code_review/tools/test_pylint_runner.py
packages/**/src/**/*.py
⚙️ CodeRabbit configuration file
packages/**/src/**/*.py: Focus on adapter and bridge patterns: imports from specfact_cli (models, runtime, validators),
Typer/Rich command surfaces, and clear boundaries so core upgrades do not silently break bundles.
Flag breaking assumptions about registry loading, lazy imports, and environment/mode behavior.
Files:
packages/specfact-code-review/src/specfact_code_review/tools/pylint_runner.py
tests/**/*.py
⚙️ CodeRabbit configuration file
tests/**/*.py: Contract-first and integration tests: migration suites, bundle validation, and flakiness.
Ensure changes to adapters or bridges have targeted coverage.
Files:
tests/unit/specfact_code_review/tools/test_pylint_runner.py
packages/**/module-package.yaml
⚙️ CodeRabbit configuration file
packages/**/module-package.yaml: Validate metadata: name, version, commands, dependencies, and parity with packaged src.
Call out semver and signing implications when manifests or payloads change.
Files:
packages/specfact-code-review/module-package.yaml
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Fix SpecFact code review findings, including warnings, unless a rare explicit exception is documented
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Enforce module signatures and version bumps when signed module assets or manifests are affected
📚 Learning: 2026-04-13T10:38:22.837Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-13T10:38:22.837Z
Learning: Signed module or manifest changes require version-bump review and verify-modules-signature
Applied to files:
packages/specfact-code-review/module-package.yaml
📚 Learning: 2026-04-02T21:49:11.371Z
Learnt from: djm81
Repo: nold-ai/specfact-cli-modules PR: 136
File: registry/modules/specfact-spec-0.40.17.tar.gz.sha256:1-1
Timestamp: 2026-04-02T21:49:11.371Z
Learning: In nold-ai/specfact-cli-modules, module tarball signatures (registry/signatures/*.tar.sig) are generated by the `publish-modules` GitHub Actions runner during the publish workflow, not committed locally to the branch. Missing signature files should NOT be flagged as a pre-merge blocker in PRs.
Applied to files:
packages/specfact-code-review/module-package.yaml
📚 Learning: 2026-04-13T10:38:22.837Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-13T10:38:22.837Z
Learning: This repository enforces the clean-code review gate through hatch run specfact code review run --json --out .specfact/code-review.json
Applied to files:
packages/specfact-code-review/module-package.yaml
📚 Learning: 2026-04-13T10:38:43.524Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Fix SpecFact code review findings, including warnings, unless a rare explicit exception is documented
Applied to files:
packages/specfact-code-review/module-package.yaml
🔀 Multi-repo context nold-ai/specfact-cli
Linked repositories findings
nold-ai/specfact-cli
-
scripts/pre_commit_code_review.py — defines ReviewFinding Pydantic model and CodeReviewReport; pre-commit uses validated ReviewFinding fields (line/message expectations used indirectly by tooling that writes/reads .specfact/code-review.json). [::nold-ai/specfact-cli::scripts/pre_commit_code_review.py]
-
openspec/specs/pylint-runner/spec.md — spec describing run_pylint → ReviewFinding mapping and that parse errors must yield a single tool_error finding. Changes to pylint_runner parsing/coercion affect compliance with this spec. [::nold-ai/specfact-cli::openspec/specs/pylint-runner/spec.md]
-
openspec/specs/review-finding-model/spec.md — requirement for ReviewFinding Pydantic model validated fields (category,severity,tool,rule,file,line,message,fixable). The PR's coercion changes how inputs convert to this validated model. [::nold-ai/specfact-cli::openspec/specs/review-finding-model/spec.md]
-
openspec/changes/archive/2026-03-17-code-review-03-type-governance-runners/... (proposal/design/TDD_EVIDENCE) — contains tasks, TDD evidence and tests referencing tests/unit/specfact_code_review/tools/test_pylint_runner.py and expectations that run_pylint maps findings and returns tool_error on parse failure. Updated pylint_runner behavior directly relates to these archived specs/tests. [::nold-ai/specfact-cli::openspec/changes/archive/2026-03-17-code-review-03-type-governance-runners/TDD_EVIDENCE.md]
- TDD evidence shows earlier test expectations: parse errors -> single tool_error finding; new coercion reduces cases that become parse errors.
-
tests/unit/specfact_cli/registry/test_profile_presets.py and multiple tests / docs assume the external module/package specfact-code-review is installed/available (module id nold-ai/specfact-code-review). The changed package manifest in the PR (module-package.yaml) affects the module version consumers when installing from the marketplace, referenced by these tests/docs. [::nold-ai/specfact-cli::tests/unit/specfact_cli/registry/test_profile_presets.py]
-
skills/specfact-code-review/SKILL.md and numerous docs/specs reference the specfact-code-review module and the runners it provides; changes to pylintrunner behavior influence the runtime behavior expected by these docs and by the module integration points. [::nold-ai/specfact-cli::skills/specfact-code-review/SKILL.md]
Summary assessment:
- The primary consumers/contract owners in this repo are the ReviewFinding model (scripts/pre_commit_code_review.py) and the openspec documents/specs that define expected behavior of pylintrunner (parse error -> tool_error, per-finding mapping). The PR's coercion of line/message and stricter whitespace handling reduces spurious parse errors and aligns with these specs and TDD evidence, so it is likely beneficial to consumers here. Also note test suites and installation docs reference the external module package (specfact-code-review) and would pick up the updated module-package.yaml when installing the module. [::nold-ai/specfact-cli::]
🔇 Additional comments (3)
packages/specfact-code-review/src/specfact_code_review/tools/pylint_runner.py (1)
47-63: Good contract-preserving coercion forReviewFindingfields.This safely prevents payload-wide failures from
line=0and empty messages while keeping findings mappable per item.Also applies to: 79-80
tests/unit/specfact_code_review/tools/test_pylint_runner.py (1)
100-193: Nice contract-focused regression coverage for parser/coercion edge cases.These tests directly protect the changed adapter behavior and error mapping path.
packages/specfact-code-review/module-package.yaml (1)
25-26: Module manifest signature removal is expected in dev-targeted PRs; ensure branch policy compliance for main.On Line 26, removing
integrity.signatureis standard practice in dev-targeted PRs: thesign-modules-on-approvalworkflow adds signatures upon approval, and thepublish-modulesworkflow re-signs any missing signatures before registry packaging (see.github/workflows/publish-modules.ymllines 300–309 and.github/workflows/sign-modules.ymllines 117–147).If this PR targets
main, ensure the manifest signature is present before merge (per.github/pull_request_template.mdline 44 requirement for--require-signaturecompliance).
Mirror stderr truncation for huge stdout when pylint emits no JSON, and add a unit test for the long stderr branch. Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/specfact_code_review/tools/test_pylint_runner.py`:
- Around line 169-221: Add two unit tests to cover the edge cases for the
coercion helpers: one named test_run_pylint_coerces_negative_line_to_one that
calls run_pylint with a payload containing "line": -5 and asserts the resulting
finding's line == 1, and one named
test_run_pylint_coerces_whitespace_only_message that calls run_pylint with a
payload whose "message" is whitespace-only (e.g. " \t\n ") and asserts the
resulting finding's message == "(pylint provided no message text)"; both tests
should use the same pattern as existing tests (create tmp_path/target.py,
monkeypatch subprocess.run to return completed_process("pylint",
stdout=json.dumps(payload))) so they exercise the coercion logic in run_pylint
and the existing helpers.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3caeb4b5-5fad-4ad0-aced-7832de5aac96
📒 Files selected for processing (3)
packages/specfact-code-review/module-package.yamlpackages/specfact-code-review/src/specfact_code_review/tools/pylint_runner.pytests/unit/specfact_code_review/tools/test_pylint_runner.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: quality (3.12)
- GitHub Check: quality (3.11)
- GitHub Check: quality (3.13)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (4)
packages/**/module-package.yaml
⚙️ CodeRabbit configuration file
packages/**/module-package.yaml: Validate metadata: name, version, commands, dependencies, and parity with packaged src.
Call out semver and signing implications when manifests or payloads change.
Files:
packages/specfact-code-review/module-package.yaml
**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php,cpp,c,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Preserve the clean-code compliance gate and its category references (naming, kiss, yagni, dry, and solid)
Files:
packages/specfact-code-review/src/specfact_code_review/tools/pylint_runner.pytests/unit/specfact_code_review/tools/test_pylint_runner.py
packages/**/src/**/*.py
⚙️ CodeRabbit configuration file
packages/**/src/**/*.py: Focus on adapter and bridge patterns: imports from specfact_cli (models, runtime, validators),
Typer/Rich command surfaces, and clear boundaries so core upgrades do not silently break bundles.
Flag breaking assumptions about registry loading, lazy imports, and environment/mode behavior.
Files:
packages/specfact-code-review/src/specfact_code_review/tools/pylint_runner.py
tests/**/*.py
⚙️ CodeRabbit configuration file
tests/**/*.py: Contract-first and integration tests: migration suites, bundle validation, and flakiness.
Ensure changes to adapters or bridges have targeted coverage.
Files:
tests/unit/specfact_code_review/tools/test_pylint_runner.py
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Fix SpecFact code review findings, including warnings, unless a rare explicit exception is documented
📚 Learning: 2026-04-13T10:38:22.837Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-13T10:38:22.837Z
Learning: Signed module or manifest changes require version-bump review and verify-modules-signature
Applied to files:
packages/specfact-code-review/module-package.yaml
📚 Learning: 2026-04-13T10:38:22.837Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-13T10:38:22.837Z
Learning: This repository enforces the clean-code review gate through hatch run specfact code review run --json --out .specfact/code-review.json
Applied to files:
packages/specfact-code-review/module-package.yaml
📚 Learning: 2026-04-13T10:38:43.524Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Fix SpecFact code review findings, including warnings, unless a rare explicit exception is documented
Applied to files:
packages/specfact-code-review/module-package.yamlpackages/specfact-code-review/src/specfact_code_review/tools/pylint_runner.py
🔀 Multi-repo context nold-ai/specfact-cli
Linked repositories findings
nold-ai/specfact-cli
-
ReviewFinding model (consumer of pylint_runner outputs)
- scripts/pre_commit_code_review.py defines ReviewFinding and uses validated fields including line >= 1 and non-empty message. [::nold-ai/specfact-cli::scripts/pre_commit_code_review.py]
-
Runner spec / contract expectations
- openspec/specs/pylint-runner/spec.md — describes mapping from pylint output → ReviewFinding and that parse errors must yield a single tool_error finding. Changes to pylintrunner coercion directly affect compliance with this spec. [::nold-ai/specfact-cli::openspec/specs/pylint-runner/spec.md]
- openspec/specs/review-finding-model/spec.md — documents required ReviewFinding fields (category,severity,tool,rule,file,line,message,fixable). Coercion of line/message impacts model validation expectations. [::nold-ai/specfact-cli::openspec/specs/review-finding-model/spec.md]
-
Module manifest / packaging consumer
- packages/specfact-code-review/module-package.yaml exists under this repo (the PR updates this manifest version/integrity). Installer/registry code and tests read module-package.yaml when installing or verifying modules. Relevant code paths:
- src/specfact_cli/registry/module_installer.py (reads/manipulates module-package.yaml during install). [::nold-ai/specfact-cli::src/specfact_cli/registry/module_installer.py]
- Multiple tests expect specfact-code-review to be installable/present (tests/unit/specfact_cli/registry/test_profile_presets.py, tests/unit/modules/module_registry/test_commands.py, tests/unit/specfact_cli/modules/test_multi_module_install_uninstall.py). These will pick up the changed module-package.yaml when installing from this workspace or marketplace. [::nold-ai/specfact-cli::tests/unit/specfact_cli/registry/test_profile_presets.py][::nold-ai/specfact-cli::tests/unit/specfact_cli/modules/test_multi_module_install_uninstall.py]
- packages/specfact-code-review/module-package.yaml exists under this repo (the PR updates this manifest version/integrity). Installer/registry code and tests read module-package.yaml when installing or verifying modules. Relevant code paths:
-
Docs / guidance referencing the module and integrity fields
- docs and openspec reference that module-package.yaml contains integrity.checksum and optional signature; marketplace publishing and signing flows validate integrity fields. The PR removal of integrity.signature and checksum change are relevant to these flows (e.g., openspec/specs/marketplace-publishing/spec.md and docs/guides/using-module-security-and-extensions.md). [::nold-ai/specfact-cli::openspec/specs/marketplace-publishing/spec.md][::nold-ai/specfact-cli::docs/guides/using-module-security-and-extensions.md]
-
Tests & TDD evidence relating to runner behavior
- openspec/changes/archive/2026-03-17-code-review-03-type-governance-runners and related TDD_EVIDENCE reference expected runner behaviour and tests around parse failures mapping to a tool_error. The pylintrunner coercions reduce parse failures and change which cases become per-finding ReviewFinding vs a single tool_error. [::nold-ai/specfact-cli::openspec/changes/archive/2026-03-17-code-review-03-type-governance-runners/TDD_EVIDENCE.md]
Assessment
- The pylintrunner changes (coercing line 0 → 1 and providing placeholder messages) affect the primary consumer (ReviewFinding model) and the openspec contracts that define runner behavior. They will also change runtime/test outcomes where previously some pylint outputs produced a single tool_error; now they may produce individual validated findings. The updated module-package.yaml (version/integrity) is relevant to installation/signing flows and marketplace publishing tests in this repo.
🔇 Additional comments (11)
packages/specfact-code-review/module-package.yaml (2)
2-2: Version bump appears appropriate for the bugfix scope.Patch increment from 0.47.9 → 0.47.12 aligns with the behavioral fix (coercing pylint edge-case fields). Based on learnings: "Signed module or manifest changes require version-bump review and verify-modules-signature."
25-26: Signature removal is expected in pre-publish state; version bump is appropriate.The
integrity.signaturefield removal aligns with the publishing workflow: signatures are generated by thepublish-modulesCI job, not committed to source. The consumer code innold-ai/specfact-cli(module_installer.py) gracefully handles missing signatures—verification only fails whenrequire_signature=Trueis explicitly set, which defaults toFalse. The version bump (0.47.12) confirms the manifest update is intentional. No cross-repo blocker.packages/specfact-code-review/src/specfact_code_review/tools/pylint_runner.py (5)
47-56: Coercion logic is sound; negative integers are correctly handled.The helper properly clamps any
line < 1to1, satisfyingReviewFinding.line >= 1. Handling ofboolbeforeintis correct sinceisinstance(True, int)returnsTruein Python. The fallback chain (None → bool → int → float → default) is defensive and complete.
59-63: Placeholder message satisfiesReviewFindingnon-empty constraint.The fallback
"(pylint provided no message text)"passes the_validate_non_empty_textfield validator inReviewFinding(per Context snippet 1). Good defensive design.
79-80: Contract change: missingline/messagekeys now coerce instead of raisingKeyError.Using
.get("line")and.get("message")silently coerces missing keys to defaults. This is intentional per the PR objective, but note it subtly changes the adapter contract — previously a malformed pylint item missing these fields would bubble up as aKeyErrorcaught at Line 159 and produce atool_error. Now such items produce valid findings with coerced values.This aligns with the goal of preserving individual findings and avoiding collapse into
tool_error, but consumers relying on the previous behavior should be aware.
94-105: Empty stdout handling now raisesValueErrorwith truncated diagnostics — addresses past review.This resolves the prior concern about treating whitespace-only stdout as a successful empty payload. The truncation logic for both
stdoutandstderrat 4096 chars prevents oversizedtool_errormessages.
148-152: Passingstderrandreturncodeenables richer tool-error diagnostics.The updated call signature provides execution context that helps developers diagnose pylint failures. The outer exception handler at Lines 153-154 converts the
ValueErrorinto a governedtool_errorfinding.tests/unit/specfact_code_review/tools/test_pylint_runner.py (4)
100-122: Solid coverage for empty stdout → tool_error contract.The test asserts all key diagnostic components (
stdout='', stderr content,returncode) appear in the error message. This locks the contract for the updated_payload_from_outputbehavior.
140-166: Regression test for stderr truncation addresses prior review feedback.This test verifies the 4096-char truncation by checking:
- Unique tail is NOT present in message (truncation occurred)
- Truncation marker format
... (N chars total)IS presentGood contract-first approach per coding guidelines for
tests/**/*.py.
169-184: Test confirmsline: 0→line == 1coercion.Directly validates the
_coerce_pylint_linebehavior for the primary edge case (pylint emitting0for file-scoped messages).
187-202: Test confirms empty message coercion to placeholder.Validates
_coerce_pylint_messagereturns the governed fallback string when pylint emits"message": "".
Add tests for negative line numbers and whitespace-only pylint messages. Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/specfact_code_review/tools/test_pylint_runner.py`:
- Around line 241-258: The test
test_run_pylint_parses_json_with_surrounding_whitespace only asserts
findings[0].rule; update the test to also assert findings[0].line == 7,
findings[0].message == "No exception type(s) specified" and findings[0].category
(or findings[0].type if your model uses a different name) matches the parsed
category expected from run_pylint so the entire finding object produced by
run_pylint([file_path]) is validated against the payload; reference the payload
JSON in the test and the run_pylint function to locate where to add these
assertions.
- Around line 169-185: The test suite lacks an assertion that valid (≥1) line
numbers are preserved by the coercion logic; update the test function
test_run_pylint_maps_bare_except_to_architecture to include an assertion that
the parsed finding keeps its original line (e.g., assert findings[0].line == 7)
so you verify that run_pylint does not coerce already-valid line numbers.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 93e5efda-363e-4fd7-a1c9-ca74fe81f8b8
📒 Files selected for processing (1)
tests/unit/specfact_code_review/tools/test_pylint_runner.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: quality (3.12)
- GitHub Check: quality (3.11)
- GitHub Check: quality (3.13)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php,cpp,c,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Preserve the clean-code compliance gate and its category references (naming, kiss, yagni, dry, and solid)
Files:
tests/unit/specfact_code_review/tools/test_pylint_runner.py
tests/**/*.py
⚙️ CodeRabbit configuration file
tests/**/*.py: Contract-first and integration tests: migration suites, bundle validation, and flakiness.
Ensure changes to adapters or bridges have targeted coverage.
Files:
tests/unit/specfact_code_review/tools/test_pylint_runner.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.535Z
Learning: Fix SpecFact code review findings, including warnings, unless a rare explicit exception is documented
📚 Learning: 2026-04-13T10:38:43.535Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.535Z
Learning: Fix SpecFact code review findings, including warnings, unless a rare explicit exception is documented
Applied to files:
tests/unit/specfact_code_review/tools/test_pylint_runner.py
🔀 Multi-repo context nold-ai/specfact-cli
Linked repositories findings
nold-ai/specfact-cli
-
ReviewFinding Pydantic model (consumer of pylint_runner outputs):
- scripts/pre_commit_code_review.py — class ReviewFinding definition and usages; model enforces fields including line >= 1 and non-empty message. [::nold-ai/specfact-cli::scripts/pre_commit_code_review.py]
-
Pylint runner contract and expectations:
- openspec/specs/pylint-runner/spec.md — requires mapping pylint findings → ReviewFinding and documents that parse errors should produce a single tool_error finding. Coercion changes in the PR modify when parse errors occur vs. per-finding ReviewFinding. [::nold-ai/specfact-cli::openspec/specs/pylint-runner/spec.md]
-
ReviewFinding model spec / governance:
- openspec/specs/review-finding-model/spec.md — documents required ReviewFinding fields (category,severity,tool,rule,file,line,message,fixable) and that invalid values should raise ValidationError; PR coercion changes interact with these model constraints. [::nold-ai/specfact-cli::openspec/specs/review-finding-model/spec.md]
-
Tests & TDD evidence for pylint runner:
- openspec/changes/archive/2026-03-17-code-review-03-type-governance-runners/TDD_EVIDENCE.md — records tests for pylint runner and that parse errors previously produced tool_error; relevant to semantics changed by coercion. [::nold-ai/specfact-cli::openspec/changes/archive/2026-03-17-code-review-03-type-governance-runners/TDD_EVIDENCE.md]
- tests/unit/specfact_code_review/tools/test_pylint_runner.py — existing test suite for pylint runner behavior (mapping, filtering, parse error handling). The PR updates/adds tests to assert coercion behavior; ensure these align with the openspec expectations. [::nold-ai/specfact-cli::tests/unit/specfact_code_review/tools/test_pylint_runner.py]
-
Module-package manifest in this repo:
- packages/specfact-code-review/module-package.yaml — not present in this repository checkout (file not found here). The PR modifies this manifest in the module repository; verify installer/publishing tooling in this repo will accept the changed checksum/signature semantics when consuming the updated module package. [::nold-ai/specfact-cli::packages/specfact-code-review/module-package.yaml] (not found)
Summary assessment:
- The pylintrunner coercion (line 0 → 1, blank message → placeholder) directly affects the ReviewFinding model and the pylint-runner spec: it reduces parse failures and will convert some cases previously turned into a single tool_error into multiple validated ReviewFinding entries. Tests and openspec TDD evidence in this repo reference the prior tool_error behavior — verify that updated tests/specs accept the new per-finding outcome. [::nold-ai/specfact-cli::openspec/specs/pylint-runner/spec.md][::nold-ai/specfact-cli::openspec/specs/review-finding-model/spec.md][::nold-ai/specfact-cli::tests/unit/specfact_code_review/tools/test_pylint_runner.py]
🔇 Additional comments (6)
tests/unit/specfact_code_review/tools/test_pylint_runner.py (6)
100-123: LGTM! Comprehensive tool_error test for empty stdout.The test correctly verifies that empty stdout produces a tool_error finding with diagnostic information including stdout, stderr, and returncode. The assertions align with the
_payload_from_outputimplementation.
125-138: LGTM! Edge case test for whitespace-only stdout.The test verifies that whitespace-only stdout is correctly treated as empty and produces a tool_error with the appropriate message. This aligns with the
stripped = stdout.strip()logic in_payload_from_output.
140-167: LGTM! Excellent stderr truncation test design.The test uses a unique tail marker to definitively prove truncation occurred, and verifies both the ellipsis marker and the exact character count format. This locks the 4096-char truncation contract and addresses the past review suggestion.
187-203: LGTM! Correct test for empty message coercion.The test verifies that an empty message string is coerced to the placeholder text
"(pylint provided no message text)", which satisfies the ReviewFinding non-empty message constraint. This aligns with the_coerce_pylint_messageimplementation.
205-221: LGTM! Addresses past review suggestion.The test correctly verifies that negative line numbers are coerced to 1, as expected by the
_coerce_pylint_lineimplementation. This addresses the edge case identified in previous review feedback.
223-239: LGTM! Addresses past review suggestion.The test correctly verifies that whitespace-only messages are coerced to the placeholder text, as the
_coerce_pylint_messageimplementation uses.strip()to detect empty content. This addresses the edge case identified in previous review feedback.
…ayload. Assert valid lines are preserved for W0702 and validate full finding fields when JSON is wrapped in surrounding whitespace. Made-with: Cursor
1 participant
Pylint can emit findings at line
0or with an emptymessage, which violatesReviewFinding(line >= 1, non-empty message). Those values were caught as genericValueErrorand collapsed the pylint step into a singletool_error.This change coerces line 0 and fills empty messages so pylint output parses into normal findings and scoring behaves as intended.
Tests:
hatch run pytest tests/unit/specfact_code_review/tools/test_pylint_runner.py -q