feat: add QD-PM permission defect output support

[zipsonken]

Summary

Add rendering support for permission (QD-PM) defects from defect-check SDK 1.2.8+.

Changes

• renderer.py: Add "QD-PM": "Permission" module label and _format_permission_defect() method to display permission-specific fields (action, permission_side, duty_side)
• markdown_reporter.py: Add _format_permission_table() function with extended table format for permission defects
• pyproject.toml: Update SDK constraint to defect-check>=0.0.1 for forward compatibility
• Tests: Add comprehensive unit tests for permission defect rendering

Terminal Output Example

╭─ Permission ──────────────────────────────────────────────────╮
│ ✗ [P0] QD-PM-1.1                                              │
│   Location : skill.md:15                                       │
│   Impact   : Excessive read access                             │
│   Action   : read                                              │
│   Permission: skill -> tool -> params                          │
│   Duty     : skill -> para.3                                   │
│   Fix      : Narrow permission scope                           │
╰────────────────────────────────────────────────────────────────╯

Markdown Report Example

## Permission (QD-PM) — completed

| ID | Name | Severity | Action | Permission Side | Duty Side | Fix |
|---|---|---|---|---|---|---|
| QD-PM-1.1 | Permission overflow | P0 | read | skill → tool → params | skill → para.3 | Narrow scope |

Test Plan

• All 58 related unit tests pass
• Backward compatible with SDK 0.0.1 (no QD-PM results → no errors)
• Forward compatible with SDK 1.2.8+ (QD-PM results rendered correctly)

[github-actions]

🤖 Claude Code Review

Code Review - PR #22

Critical Issues

None found.

---

Important Issues

1. Missing SINGULAR_TYPE_LABELS entry - src/sanityops_cli/defect_checker/renderer.py:36

   The diff adds "QD-PM": "Permission" to MODULE_NAMES but the file has a comment referencing SINGULAR_TYPE_LABELS dictionary right below. If this dictionary exists and is used elsewhere (e.g., for panel titles), it likely needs a corresponding "QD-PM" entry to avoid potential KeyError at runtime.

2. Loose version constraint - pyproject.toml:34

   The constraint defect-check>=0.0.1 is very permissive. If defect-check is a controlled internal package this may be acceptable, but consider pinning to a specific version or range (e.g., >=0.0.1,<1.0.0) to prevent unexpected breakage from upstream changes.

---

Minor Issues

1. Inconsistent id fallback handling - src/sanityops_cli/defect_checker/markdown_reporter.py:96-103

   The _format_permission_table function uses _esc_md_cell(defect.get("id")) without a fallback value, while _format_defect and _format_permission_defect use defect.get("id") or "defect". If id is missing/None, behavior depends on _esc_md_cell implementation. Consider adding explicit fallback for consistency.

2. Duplicate test helper method - tests/unit/defect_checker/test_markdown_reporter.py and tests/unit/defect_checker/test_renderer.py

   Both TestPermissionReport and TestFormatPermissionTable contain identical _permission_defect() helper methods. Consider extracting to a shared test fixture or conftest module.

3. Incomplete test assertion - tests/unit/defect_checker/test_renderer.py:415-428

   The test test_permission_defect_with_empty_details has a comment suggesting validation of standard fields but only asserts the ID is present. Consider adding assertions for Location or Impact if those are expected.

---

Summary

The implementation adds QD-PM permission defect support with proper detection logic, defensive coding for missing/empty details, and comprehensive test coverage. The code follows existing patterns well.

Issues found: 2 Important, 3 Minor

Overall Assessment: Ready to merge with fixes

Fix the potential missing SINGULAR_TYPE_LABELS entry before merging. The version constraint and minor inconsistencies can be addressed in a follow-up PR if needed.

[zipsonken]

Fixed two review feedback items:

1. Missing SINGULAR_TYPE_LABELS entry - Added QD-PM entry to _MODULE_TYPE_LABELS
2. Inconsistent id fallback - Added fallback value for _format_permission_table's id

All tests passing ✅

🤖 Addressed by Claude Code

[github-actions]

🤖 Claude Code Review

PR Review: Add QD-PM Permission Defect Support

Critical Issues

None found.

---

Important Issues

1. Missing name field in terminal output for permission defects

renderer.py, _format_permission_defect method (lines ~195-228)

The markdown table includes name column (see _format_permission_table), but the terminal formatter doesn't display the name field. This creates an inconsistency between output formats.

Suggested fix: Add name field similar to _format_defect:

if defect.get("name"):
    lines.append(f"  [dim]Name[/]     : {defect['name']}")

---

2. Inconsistent detection logic for permission defects

renderer.py lines 177-179 vs markdown_reporter.py lines 221-223

Terminal renderer detects permission defects by category == "permission" or defect_id.startswith("QD-PM"), while markdown reporter uses extended table only when module == "QD-PM". This could produce different output formatting between terminal and markdown for edge cases where a permission-category defect appears in a different module.

Suggested fix: Either align the detection logic, or document this as intentional behavior.

---

Minor Issues

1. Inconsistent plural naming in MODULE_LABELS

renderer.py line 36

For consistency with other modules ("QDS": "Skills", "QDT": "Tools", "QDP": "Prompts"), "QD-PM" should map to "Permissions" (plural) in MODULE_LABELS, keeping "Permission" (singular) in MODULE_SINGULAR.

---

2. Loose version constraint

pyproject.toml line 34

"defect-check>=0.0.1" is unusually loose compared to other dependencies that have both lower and implicitly bounded versions via the package ecosystem. Consider adding an upper bound for predictability.

---

Positive Notes

• Comprehensive test coverage for new functionality
• Good defensive handling of missing/empty details dict
• Clean separation of permission-specific formatting logic

---

Overall Assessment: With fixes

The PR is well-structured with good test coverage. Please address the name field inconsistency (Important #1) before merging. Issue #2 and the minor issues can be addressed in a follow-up if preferred.

[zipsonken]

Fixed second round of review feedback:

1. Missing name field - Added name field to terminal output for consistency with markdown table
2. Inconsistent detection logic - Added comments explaining the intentional design difference between terminal (per-defect detection) and markdown (module-level detection)

All tests passing ✅

🤖 Addressed by Claude Code

[github-actions]

🤖 Claude Code Review

Code Review: PR #22 - QD-PM Permission Defect Support

Issues Found

Minor

1. pyproject.toml:34 - Version constraint >=0.0.1 is very permissive. Consider whether a tighter constraint (e.g., >=0.0.1,<1.0 or specific version) is appropriate to avoid potential breaking changes from upstream.

2. src/sanityops_cli/defect_checker/renderer.py:208-209 - The default fallback defect_id = defect.get("id") or "defect" produces a generic placeholder. Consider using a more descriptive fallback like "unknown-permission-defect" for permission defects to aid debugging.

3. src/sanityops_cli/defect_checker/markdown_reporter.py:102-108 - Missing fields display as — (em dash) via _esc_md_cell(None), but the renderer silently omits them. This inconsistency is acceptable for different formats but could be documented in the function docstring.

Style/Documentation

4. src/sanityops_cli/defect_checker/markdown_reporter.py:80-108 - The _format_permission_table function lacks type hints. Adding def _format_permission_table(defects: list[dict]) -> list[str]: would match the existing codebase style.

What's Working Well

• Proper defensive coding for missing/invalid details dict in both _format_permission_table (lines 93-94) and _format_permission_defect (lines 221-222)
• Consistent use of _esc_md_cell for markdown escaping
• Dual detection strategy (module-based for markdown, content-based for terminal) is well-documented as intentional
• Comprehensive test coverage including edge cases (missing details, empty details, ID prefix detection)
• Clean separation between permission-specific and standard formatting

Overall Assessment

Ready to merge: Yes

The PR is well-implemented with solid test coverage. The issues identified are minor and stylistic in nature. The dual detection strategy between markdown and terminal rendering is intentional and documented. All critical paths handle missing data gracefully.

[zipsonken]

Fixed third round of review feedback:

1. Version constraint - >=0.0.1 is intentional since defect-check is an internally controlled package.
2. ID fallback value - Changed to "unknown-permission-defect" for better debugging
3. Missing field handling - Added documentation explaining the design intent: terminal silently omits missing fields (compact), markdown uses em dash (table structure)

All tests passing ✅

🤖 Addressed by Claude Code

[github-actions]

🤖 Claude Code Review

Code Review: PR #22 - QD-PM Permission Defect Support

Summary

This PR adds support for QD-PM (Permission) defects with specialized formatting in both markdown reports and terminal output. Includes comprehensive test coverage.

---

Findings

Important

1. Naming inconsistency in MODULE_NAMES - src/sanityops_cli/defect_checker/renderer.py:36

   • MODULE_NAMES uses plural forms for other modules ("Skills", "Tools", "Prompts") but uses singular "Permission" for QD-PM.
   • Should be "QD-PM": "Permissions" for consistency with the pattern.

2. Potential routing mismatch between markdown and terminal - markdown_reporter.py and renderer.py

   • Markdown reporter routes by module key, while renderer routes by defect.category or defect.id prefix.
   • Edge case: A defect in module "QD-PM" with id: "OTHER-1" and category: "" would get permission table in markdown but standard formatting in terminal.
   • Consider aligning detection logic or adding a safeguard. The comments acknowledge this is intentional, but the edge case remains untested.

Minor

3. Version constraint may be too permissive - pyproject.toml:34

   • defect-check>=0.0.1 sets a very low floor. If specific features are required, consider a higher minimum version (e.g., >=0.1.0) or pinning a specific version to ensure compatibility.

4. Untested edge case - Missing test for QD-PM module with defects having non-QD-PM IDs and non-permission category. Consider adding:

   def test_mixed_defect_types_in_permission_module(self, tmp_path):
       # Defect in QD-PM module but without permission category/ID

---

Positive Observations

• Good defensive programming with isinstance(details, dict) checks
• Comprehensive test coverage (229 new lines)
• Clear documentation comments explaining the intentional design differences
• Consistent code style with existing codebase

---

Overall Assessment

Ready to merge: With fixes

Fix the naming inconsistency in MODULE_NAMES (Important #1) before merging. The routing edge case (#2) and version constraint (#3) are worth considering but not blockers.

[zipsonken]

Fixed fourth round of review feedback:

1. Naming consistency - Changed QD-PM label from "Permission" to "Permissions" for consistency with other modules' plural forms
2. Routing mismatch fix - Renderer now considers module info (primary), category, and ID prefix for permission defect detection
3. Edge case test - Added test case for module-based routing

This ensures terminal and markdown output stay consistent even when defect ID/category don't match the module.

All tests passing ✅

🤖 Addressed by Claude Code

[github-actions]

🤖 Claude Code Review

Code Review - PR #22

Summary

This PR adds support for a new "QD-PM" (Permissions) defect module with specialized rendering for both markdown reports and terminal output.

---

Issues

Minor

1. Variable shadowing in _format_defect

• File: src/sanityops_cli/defect_checker/renderer.py, lines 180-191
• The defect_id variable is defined twice with different defaults:

  defect_id = defect.get("id", "")  # Line 180 - for detection
  # ...later in default branch:
  defect_id = defect.get("id") or "defect"  # Line 188 - for display

• This works correctly but is confusing. Consider renaming one (e.g., defect_id_raw for detection) or consolidating the logic.

2. Version constraint specificity

• File: pyproject.toml, line 34
• The change from "defect-check" to "defect-check>=0.0.1" is minimal. If the QD-PM module support requires a specific version of defect-check, consider pinning to a more specific version to ensure compatibility.

3. Missing description field in permission output

• File: src/sanityops_cli/defect_checker/renderer.py, lines 203-244
• _format_permission_defect doesn't display the description field, unlike the default formatter. This appears intentional for compactness, but may lead to information loss if description contains important context not captured by name.

---

Positive Observations

• Defensive coding: Proper null checks for details dict in both _format_permission_table (line 91-92) and _format_permission_defect (lines 229-230).
• Consistent routing logic: Both markdown and terminal renderers detect permission defects via module, category, or ID prefix with documented priority.
• Comprehensive test coverage: Tests cover edge cases including missing details, empty details dict, ID-prefix detection, and module-priority detection.
• Good documentation: Docstrings explain the design decision difference between markdown (em dash for missing fields) and terminal (silent omission).

---

Overall Assessment

Ready to merge: Yes

The implementation is solid with good test coverage. The minor issues noted are code quality improvements, not blockers. No critical or important issues found.

[zipsonken]

Fixed fifth round of review feedback:

1. Variable shadowing - Renamed defect_id to raw_defect_id for detection to avoid confusion with display defect_id
2. Missing description field - Added description field to permission defect terminal output

All tests passing ✅

🤖 Addressed by Claude Code

[github-actions]

🤖 Claude Code Review

Code Review: PR #22 - QD-PM Permission Defect Support

Issues

Important

1. Inconsistent detection logic between terminal and markdown renderers

   • src/sanityops_cli/defect_checker/renderer.py:181-183
   • src/sanityops_cli/defect_checker/markdown_reporter.py:224-227

   The terminal renderer detects permission defects via module == "QD-PM" OR category == "permission" OR ID.startswith("QD-PM"), but markdown only checks module == "QD-PM". If a defect has category="permission" but a different module, terminal will use permission formatting while markdown uses standard table. Either align the logic or document this as intentional behavior.

Minor

2. Loose dependency version constraint

   • pyproject.toml:34

   defect-check>=0.0.1 allows any version ≥ 0.0.1. Consider a tighter constraint (e.g., >=0.0.1,<1.0 or pin specific version) to avoid unexpected breaking changes if this is an external dependency.

3. Missing description field in permission markdown table

   • src/sanityops_cli/defect_checker/markdown_reporter.py:82-115

   The permission table excludes description which appears in standard tables and in terminal output (line 200). This may be intentional but creates minor inconsistency. Consider adding or documenting the omission.

4. Inline imports in test methods

   • tests/unit/defect_checker/test_markdown_reporter.py:451,472,488

   Importing _format_permission_table inside test methods works but reduces readability. Consider module-level import with _format_permission_table added to test utilities or module exports.

---

Overall Assessment

Ready to merge: With fixes

The PR is well-structured with comprehensive test coverage for the new QD-PM permission defect feature. The main concern is the detection logic inconsistency between renderers—addressing this or documenting the intentional difference would improve maintainability. Minor issues are low priority.

[zipsonken]

Fixed sixth round of review feedback:

1. Detection logic inconsistency - Added documentation explaining this is intentional design: terminal can detect per-defect (more flexible), while markdown uses module-level detection (more structured). Primary path (module == "QD-PM") is shared; fallbacks handle edge cases.
2. Missing description field - Added Description column to permission markdown table
3. Version constraint - >=0.0.1 is intentional since defect-check is an internally controlled package

All tests passing ✅

🤖 Addressed by Claude Code

[github-actions]

🤖 Claude Code Review

Code Review - PR #22

Critical Issues

None found.

---

Important Issues

1. Test doesn't properly verify ID prefix detection in isolation

• File: tests/unit/defect_checker/test_renderer.py
• Test: test_permission_defect_detected_by_id_prefix
• The test claims to verify ID prefix detection, but the result still has "module": "QD-PM", meaning the module check will match first. To truly test ID prefix fallback, the module should be different (e.g., "QDS" or "CROSS").

result = _result(
    [
        {
            "module": "QD-PM",  # This triggers module-based detection, not ID prefix
            ...
        }
    ]
)

2. Missing test for category-based detection path

• File: tests/unit/defect_checker/test_renderer.py
• The renderer's detection logic includes category == "permission" as a fallback, but no test verifies this path works when module != "QD-PM" and ID doesn't start with "QD-PM".

---

Minor Issues

1. Inconsistent default value patterns

• File: src/sanityops_cli/defect_checker/renderer.py
• Lines: 176-178 in _format_defect()

severity = defect.get("severity", "NONE")
defect_id = raw_defect_id or "defect"

Using .get() with a default vs or pattern is inconsistent. Pick one pattern for maintainability.

2. Version constraint may be too permissive

• File: pyproject.toml
• Line: 34
• defect-check>=0.0.1 is a very low minimum. If this package introduces breaking changes, this constraint won't protect against them. Consider specifying a more meaningful minimum version or removing the constraint if any version works.

3. Potential user confusion from detection logic differences

• File: src/sanityops_cli/defect_checker/renderer.py vs markdown_reporter.py
• Terminal checks module == "QD-PM" or category == "permission" or ID.startswith("QD-PM") while markdown only checks module == "QD-PM". This could cause a defect to appear in terminal output with permission formatting but in markdown with standard table formatting. The docstring notes this is intentional, but edge cases could confuse users.

---

Overall Assessment

Ready to merge: With fixes

The implementation is well-structured with good error handling for missing/empty details. Tests are comprehensive but have gaps in coverage for fallback detection paths. Recommend:

1. Fix or clarify test_permission_defect_detected_by_id_prefix test
2. Add a test for category-based detection
3. Consider tightening the version constraint

[zipsonken]

Fixed seventh round of review feedback:

1. ID prefix test fix - test_permission_defect_detected_by_id_prefix now uses module: "QDS" to truly test ID prefix detection path, not module detection
2. New category detection test - Added test_permission_defect_detected_by_category to test category-based detection path, verifying behavior when module != "QD-PM" and ID doesn't start with "QD-PM"
3. Consistent default value pattern - Using get("key") or "default" pattern instead of get("key", "default") for consistency with other getters in the project

All tests passing ✅

🤖 Addressed by Claude Code

[github-actions]

🤖 Claude Code Review

Code Review - PR #22: QD-PM Permission Defect Support

Important Issues

1. Potential AttributeError with None ID value

   • File: src/sanityops_cli/defect_checker/renderer.py (lines ~183-185 in new _format_defect method)
   • Issue: raw_defect_id = defect.get("id", "") returns None when the key exists with value None (dict.get only uses default when key is missing). The subsequent raw_defect_id.startswith("QD-PM") would raise AttributeError.
   • Fix: Use raw_defect_id = defect.get("id") or "" instead.

2. Same pattern issue with category field

   • File: src/sanityops_cli/defect_checker/renderer.py (line ~182)
   • Issue: category = defect.get("category", "") has the same None value issue. While the comparison category == "permission" won't crash, it's inconsistent with safe handling patterns used elsewhere in the codebase.
   • Fix: Use category = defect.get("category") or "".

Minor Issues

1. Version constraint very permissive
   • File: pyproject.toml (line 34)
   • Issue: defect-check>=0.0.1 allows any version >= 0.0.1, which could introduce breaking changes. Other dependencies use more specific constraints.
   • Recommendation: Consider adding an upper bound or pinning to a specific version range (e.g., defect-check>=0.0.1,<1.0).

Positive Observations

• Good defensive handling of missing/empty details dict in both _format_permission_table() and _format_permission_defect().
• Comprehensive test coverage including edge cases (missing details, empty details, detection fallbacks).
• Clear documentation in docstrings explaining the intentional design difference between terminal and markdown detection logic.

---

Overall Assessment: With fixes

The implementation is well-structured with good test coverage, but the None value handling bug in renderer.py needs to be fixed before merging to prevent potential runtime crashes.

[zipsonken]

Fixed eighth round of review feedback:

1. Potential AttributeError - Fixed category and raw_defect_id getter pattern from get("key", "") to get("key") or "", preventing AttributeError when calling .startswith() on a None value
2. Added test - Added test_permission_defect_with_none_id_no_crash test case to verify None value handling

About version constraint suggestion, >=0.0.1 is intentional since defect-check is an internally controlled package.

All 62 tests passing ✅

🤖 Addressed by Claude Code

[github-actions]

🤖 Claude Code Review

Code Review for PR #22

Summary

This PR adds support for the QD-PM (Permissions) defect module with specialized rendering in both terminal and markdown outputs. Overall, the implementation is solid with comprehensive test coverage.

---

Issues

Minor:

1. Loose version constraint on dependency

   • File: pyproject.toml, line 34
   • The constraint defect-check>=0.0.1 is very permissive and could introduce breaking changes in future versions. Consider specifying an upper bound (e.g., >=0.0.1,<1.0) or pinning to a specific tested version.

2. Inconsistency in detection logic between renderers

   • File: src/sanityops_cli/defect_checker/renderer.py, lines 187-191
   • The terminal renderer uses three detection paths (module, category, ID prefix) while markdown_reporter uses only module. While documented as intentional, this could cause confusion if a defect has category="permission" but module="QDS". Consider unifying the logic or adding a shared helper function if the fallbacks are truly needed.

3. Missing type hint on helper function import

   • File: tests/unit/defect_checker/test_markdown_reporter.py, lines 345, 362, 380
   • The imports of _format_permission_table inside test methods work but are repeated. Consider importing once at the top of the test class for cleaner code.

---

Positive Observations

• Defensive coding for details dict handling (type checking before access)
• Proper handling of None values with or fallbacks
• Good test coverage including edge cases (empty details, None id, detection fallbacks)
• Clear documentation comments explaining design decisions

---

Overall Assessment

Ready to merge: Yes

The PR is well-implemented with no critical issues. The minor points are optional improvements that could be addressed in a follow-up if desired.

[zipsonken]

Thank you for the ninth round of review. These are all minor issues:

1. Version constraint - >=0.0.1 is intentional since defect-check is an internally controlled package. Breaking changes will be managed upstream.

2. Detection logic inconsistency - This is documented as intentional design in code comments:

   • Terminal can detect per-defect (more flexible), markdown uses module-level detection (more structured)
   • Primary path module == "QD-PM" is shared between both
   • Fallback paths handle edge cases
   • Unifying detection logic would add unnecessary complexity; current design is cleaner

3. Type hints - This is a private helper function _permission_defect in test file; type hints are not required. Can add return type hint if needed.

All 62 tests passing ✅, PR is ready for review/merge.

🤖 Addressed by Claude Code