[codex] add pyright LSP validation#3
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
📜 Recent review details🔇 Additional comments (9)
📝 WalkthroughWalkthroughAdds a Pyright Language Server Protocol (LSP) diagnostic path to the Agent Quality MCP server: new shared validator capability/result models, a long-running subprocess launcher, a stdio JSON-RPC framing layer, a full Pyright LSP provider/session/manager with deterministic CLI fallback, service wiring with lifecycle cleanup, and response filtering for transport-fallback warnings. Also adds Phase 2 decision-contract specs and implementation plan documents. ChangesPyright LSP Validator Implementation
Phase 2 Decision Contract and Pyright LSP Planning Documentation
Sequence DiagramsequenceDiagram
participant validate_patch_service
participant _run_pyright_provider
participant PyrightLspProvider
participant RealPyrightLspManager
participant PyrightLspProcessSession
participant PyrightCliAdapter
participant _diagnostics_for_decision
validate_patch_service->>_run_pyright_provider: ValidatorRequest(safety_mode, scope, changed_files)
_run_pyright_provider->>PyrightLspProvider: validate(request)
PyrightLspProvider->>RealPyrightLspManager: session_for(real_workspace_root, config)
RealPyrightLspManager-->>PyrightLspProvider: PyrightLspProcessSession (cached or new)
PyrightLspProvider->>PyrightLspProcessSession: collect_diagnostics(shadow_root, changed_files, scope, timeout)
PyrightLspProcessSession-->>PyrightLspProvider: RawLspDiagnostics | None, fallback_reason | None
alt LSP diagnostics complete
PyrightLspProvider-->>_run_pyright_provider: ValidatorResult (TYPE_DIAGNOSTICS, fallback_to_cli=False)
else LSP failed or incomplete
PyrightLspProvider->>RealPyrightLspManager: discard_session(real_workspace_root)
PyrightLspProvider->>PyrightCliAdapter: check(cwd, changed_files, mode)
PyrightCliAdapter-->>PyrightLspProvider: diagnostics, records
PyrightLspProvider-->>_run_pyright_provider: ValidatorResult (lsp_fallback diagnostic, fallback_to_cli=True)
end
_run_pyright_provider-->>validate_patch_service: diagnostics, command_records
validate_patch_service->>_diagnostics_for_decision: filter lsp_fallback and tool_unavailable diagnostics
_diagnostics_for_decision-->>validate_patch_service: decision-relevant diagnostics only
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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
🤖 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
`@docs/superpowers/specs/2026-06-22-agent-quality-mcp-pyright-lsp-validator-capabilities-design.md`:
- Around line 99-103: Update the architecture section describing the LSP module
structure (lines 95-105) to reflect the actual implementation where all
functionality including PyrightLspManager, PyrightLspProvider, and
RealPyrightLspManager is consolidated into a single lsp/pyright.py file rather
than being split across separate lsp/protocol.py, lsp/manager.py, and
lsp/pyright.py modules. Replace the multi-module proposal with documentation of
the consolidated design in lsp/pyright.py and add an explanation of why
simplicity through consolidation was chosen over module separation.
In `@src/agent_quality_mcp/service.py`:
- Around line 408-413: In the _validator_scope_for_tool function, the branch
that checks for tool == "ruff" and mode == "strict" appears to be unreachable
code since only pyright validation calls this function. Either remove this dead
code branch entirely to improve maintainability, or if it is intentional
future-proofing for a future refactor where ruff validation might use the
validator contract, add a clarifying comment above the ruff condition explaining
why it's included despite not being currently used.
In `@tests/integration/test_validate_patch_demo.py`:
- Around line 110-128: The function
_assert_pyright_evidence_or_structured_unavailable duplicates the logic already
present in the existing helper _assert_tool_recorded_or_structured_unavailable,
differing only in the tool name and log message. Delete the
_assert_pyright_evidence_or_structured_unavailable function entirely and replace
its call (at line 83) with a direct call to
_assert_tool_recorded_or_structured_unavailable passing "pyright" as the tool
name parameter. If preserving the specific error message is important, consider
adding an optional message parameter to the
_assert_tool_recorded_or_structured_unavailable helper to avoid duplication
while maintaining distinct messaging.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5f0b33a0-4cd2-4439-8fd3-defa4e9c70fc
📒 Files selected for processing (24)
README.mddocs/superpowers/plans/2026-06-19-agent-quality-mcp-phase-2-agent-decision-contract.mddocs/superpowers/plans/2026-06-22-agent-quality-mcp-pyright-lsp-validator-capabilities.mddocs/superpowers/specs/2026-06-19-agent-quality-mcp-phase-2-agent-decision-contract-design.mddocs/superpowers/specs/2026-06-19-agent-quality-mcp-phase-2-decision-engine-split-design.mddocs/superpowers/specs/2026-06-22-agent-quality-mcp-pyright-lsp-validator-capabilities-design.mdsrc/agent_quality_mcp/cli/runner.pysrc/agent_quality_mcp/config.pysrc/agent_quality_mcp/lsp/__init__.pysrc/agent_quality_mcp/lsp/protocol.pysrc/agent_quality_mcp/lsp/pyright.pysrc/agent_quality_mcp/models.pysrc/agent_quality_mcp/response.pysrc/agent_quality_mcp/server.pysrc/agent_quality_mcp/service.pysrc/agent_quality_mcp/validators.pytests/integration/test_validate_patch_demo.pytests/unit/test_config.pytests/unit/test_lsp_protocol.pytests/unit/test_pyright_lsp.pytests/unit/test_runner.pytests/unit/test_service.pytests/unit/test_tools_server.pytests/unit/test_validators.py
📜 Review details
🧰 Additional context used
🪛 ast-grep (0.44.0)
tests/unit/test_lsp_protocol.py
[info] 18-18: use jsonify instead of json.dumps for JSON output
Context: json.dumps(message, separators=(",", ":"), ensure_ascii=False)
Note: Security best practice.
(use-jsonify)
src/agent_quality_mcp/lsp/protocol.py
[info] 15-15: use jsonify instead of json.dumps for JSON output
Context: json.dumps(message, separators=(",", ":"), ensure_ascii=False)
Note: Security best practice.
(use-jsonify)
src/agent_quality_mcp/cli/runner.py
[error] 179-187: Command coming from incoming request
Context: subprocess.Popen( # noqa: S603 - executable is allowlist-resolved.
[executable, *args],
cwd=str(launch_cwd),
env=safe_env,
stdin=subprocess.PIPE,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
shell=False,
)
Note: [CWE-20].
(subprocess-from-request)
src/agent_quality_mcp/lsp/pyright.py
[info] 133-133: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload, sort_keys=True, separators=(",", ":"))
Note: Security best practice.
(use-jsonify)
[info] 195-195: use jsonify instead of json.dumps for JSON output
Context: json.dumps(text, ensure_ascii=False)
Note: Security best practice.
(use-jsonify)
🪛 LanguageTool
docs/superpowers/plans/2026-06-22-agent-quality-mcp-pyright-lsp-validator-capabilities.md
[uncategorized] ~1990-~1990: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...` Expected: PASS. - [ ] Step 6: Run full service tests Run: ```bash .venv/bin/python...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
docs/superpowers/specs/2026-06-19-agent-quality-mcp-phase-2-decision-engine-split-design.md
[grammar] ~58-~58: Use a hyphen to join words.
Context: ...remains service-led and shadow-workspace based. CLI adapters continue to produce ...
(QB_NEW_EN_HYPHEN)
docs/superpowers/specs/2026-06-19-agent-quality-mcp-phase-2-agent-decision-contract-design.md
[grammar] ~221-~221: Use a hyphen to join words.
Context: ...remains service-led and shadow-workspace based. Tool adapters continue to report ...
(QB_NEW_EN_HYPHEN)
README.md
[grammar] ~235-~235: Ensure spelling is correct
Context: ...ons - Text unified-diff subset only. - Stdio transport only. - Minimal uv, Ruff, and...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
docs/superpowers/specs/2026-06-22-agent-quality-mcp-pyright-lsp-validator-capabilities-design.md
[style] ~375-~375: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... provider. 8. Run the Ruff provider. 9. Run the Pyright LSP provider. 10. If Pyrigh...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~546-~546: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...tocol parser and fake-process tests. 3. Add command resolution and inspection suppo...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~547-~547: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...on support for pyright-langserver. 4. Add the Pyright LSP manager and provider wi...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (83)
docs/superpowers/specs/2026-06-19-agent-quality-mcp-phase-2-agent-decision-contract-design.md (4)
219-240: 💤 Low valueGrammar: Hyphenate compound adjective modifying noun.
Line 221: "remains service-led and shadow-workspace based" should be "remains service-led and shadow-workspace-based" when the compound adjective modifies the following noun phrase.
53-82: LGTM!
84-115: LGTM!
140-160: LGTM!docs/superpowers/specs/2026-06-19-agent-quality-mcp-phase-2-decision-engine-split-design.md (3)
1-55: LGTM!
56-69: 💤 Low valueGrammar: Hyphenate compound adjective.
Line 58: "remains service-led and shadow-workspace based" should be "shadow-workspace-based" to properly hyphenate the compound adjective.
94-199: LGTM!docs/superpowers/plans/2026-06-19-agent-quality-mcp-phase-2-agent-decision-contract.md (8)
22-36: LGTM!
77-158: LGTM!
295-440: LGTM!
467-602: LGTM!
618-804: LGTM!
949-1157: LGTM!
1311-1566: LGTM!
1587-1877: LGTM!docs/superpowers/plans/2026-06-22-agent-quality-mcp-pyright-lsp-validator-capabilities.md (10)
19-21: LGTM!
51-416: LGTM!
418-692: LGTM!
694-870: LGTM!
872-1129: LGTM!
1131-1469: LGTM!
1470-1741: LGTM!
1743-2007: LGTM!
2009-2197: LGTM!
2199-2298: LGTM!docs/superpowers/specs/2026-06-22-agent-quality-mcp-pyright-lsp-validator-capabilities-design.md (1)
1-554: Comprehensive and well-structured specification with strong safety and testing grounding.This design specification for Pyright LSP validator capabilities is detailed, internally consistent, and well-aligned with the PR objectives. Key strengths:
- Clear scope and boundaries: The "Scope" section (lines 18–52) explicitly states what is and is not included, preventing scope creep.
- Safety-first framing: The "Safety Model" section (lines 412–434) preserves Phase 1 isolation guarantees and documents concrete requirements (shadow workspaces only, no real-workspace mutation, untrusted subprocess handling).
- Diagnostic completion semantics: The "Diagnostic Completion" section (lines 331–359) rigorously defines what "complete" means for both changed-file and workspace scopes, with explicit fallback triggers for ambiguous cases—critical for correctness.
- Comprehensive testing strategy: The "Testing Strategy" section (lines 461–521) provides concrete, actionable test coverage including edge cases, process lifecycle, and fallback paths.
- Pragmatic error handling: LSP failures cleanly degrade to CLI fallback (Section 16), avoiding silent partial results.
The staged implementation plan (Section 21, lines 539–554) properly identifies process lifecycle state as the key risk and recommends explicit cleanup checks before service wiring.
src/agent_quality_mcp/models.py (1)
97-97: LGTM!src/agent_quality_mcp/config.py (1)
50-50: LGTM!src/agent_quality_mcp/validators.py (5)
1-20: LGTM!
22-54: LGTM!
57-74: LGTM!
77-125: LGTM!
128-184: LGTM!tests/unit/test_config.py (1)
305-315: LGTM!tests/unit/test_validators.py (3)
1-49: LGTM!
51-111: LGTM!
113-153: LGTM!src/agent_quality_mcp/response.py (2)
302-306: LGTM!
321-322: LGTM!tests/unit/test_service.py (11)
7-26: LGTM!
74-106: LGTM!
115-141: LGTM!
257-272: LGTM!
536-542: LGTM!Also applies to: 590-605, 647-651, 721-736
754-810: LGTM!
812-868: LGTM!
870-899: LGTM!
901-928: LGTM!
930-976: LGTM!
1022-1033: LGTM!src/agent_quality_mcp/cli/runner.py (4)
16-22: LGTM!
34-44: LGTM!
55-55: LGTM!
163-202: LGTM!tests/unit/test_runner.py (1)
8-14: LGTM!Also applies to: 187-222, 331-380, 381-426, 428-454
src/agent_quality_mcp/lsp/__init__.py (1)
1-1: LGTM!src/agent_quality_mcp/lsp/protocol.py (1)
1-100: LGTM!tests/unit/test_lsp_protocol.py (1)
1-68: LGTM!src/agent_quality_mcp/lsp/pyright.py (7)
1-57: LGTM!Also applies to: 60-111, 114-199, 202-237
240-441: LGTM!
442-578: LGTM!
581-719: LGTM!
722-849: LGTM!
852-968: LGTM!
971-1028: LGTM!tests/unit/test_pyright_lsp.py (11)
1-211: LGTM!
213-258: LGTM!
261-379: LGTM!
382-471: LGTM!
474-545: LGTM!
548-670: LGTM!
673-803: LGTM!
806-938: LGTM!
941-1047: LGTM!
1050-1250: LGTM!
1252-1404: LGTM!src/agent_quality_mcp/service.py (3)
30-30: LGTM!Also applies to: 46-59
163-169: LGTM!Also applies to: 311-317
350-368: LGTM!Also applies to: 398-405, 416-444, 447-451
src/agent_quality_mcp/server.py (1)
7-7: LGTM!Also applies to: 22-25
tests/unit/test_tools_server.py (1)
7-7: LGTM!Also applies to: 59-77
tests/integration/test_validate_patch_demo.py (1)
81-83: LGTM!README.md (1)
25-27: LGTM!Also applies to: 218-223, 224-231, 232-239
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
1 participant
Summary
pyright-langserver --stdiothrough the existing trusted command runner.pyrightandpyright-langserver.Test Plan
uv run pytest-> 298 passeduv run ruff check .-> passeduv run pyright-> 0 errors, 0 warnings, 0 informationsgit diff --check-> passedNotes
.DS_Storethat was left out of the branch.Summary by CodeRabbit
New Features
pyright-langserverexecutable with newAGENT_QUALITY_MCP_PYRIGHT_LANGSERVERenvironment variable configuration.Documentation