feat: 3-layer TaxDiagnosticResult model + migrate TDS/ITC/GST-RCM guards (close #39)#45
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughIntroduces Changes3-Layer Diagnostic Model and Guard Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Greptile SummaryThis PR introduces the
Confidence Score: 5/5Safe to merge — the additive API design means no existing call sites are broken, and all three to_diagnostic() paths are fail-closed for non-VERIFIED results. The core model invariants are well-enforced by the frozen dataclass and post_init. The to_diagnostic() methods correctly guard against the empty-evidence proof_ref issue raised in the previous review round. The two remaining concerns are design-level observations: the GSTGuard mismatch mapping is intentional per the PR description, and the proof_ref format validation gap is a hardening opportunity rather than a runtime defect. Neither affects the fail-closed behavior of the non-VERIFIED paths. qwed_tax/jurisdictions/india/guards/gst_guard.py — to_diagnostic() mismatch-to-BLOCKED mapping conflates two distinct failure modes; qwed_tax/diagnostics.py — proof_ref format is not structurally validated in post_init or from_dict. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Guard method returns Dict[str, Any]"] --> B{"verified?"}
B -- "False" --> C{"computed_only?\n(GST only)"}
C -- "True" --> D["TaxDiagnosticResult.unverifiable()\nstatus=UNVERIFIABLE\nproof_ref=None"]
C -- "False" --> E["TaxDiagnosticResult.blocked()\nstatus=BLOCKED\nproof_ref=None"]
B -- "True" --> F{"audit_trace\npresent?"}
F -- "No" --> G["raise ValueError\n'VERIFIED requires audit_trace'"]
F -- "Yes" --> H["compute_proof_ref(audit_trace)\n→ sha256:digest"]
H --> I["TaxDiagnosticResult.verified()\nstatus=VERIFIED\nproof_ref=sha256:..."]
I --> J{"__post_init__ invariants"}
J -- "pass" --> K["Return TaxDiagnosticResult\nis_authoritative=True"]
D --> L["Return TaxDiagnosticResult\nis_authoritative=False"]
E --> L
style G fill:#ff6b6b,color:#fff
style K fill:#51cf66,color:#fff
style L fill:#ffa94d,color:#fff
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A["Guard method returns Dict[str, Any]"] --> B{"verified?"}
B -- "False" --> C{"computed_only?\n(GST only)"}
C -- "True" --> D["TaxDiagnosticResult.unverifiable()\nstatus=UNVERIFIABLE\nproof_ref=None"]
C -- "False" --> E["TaxDiagnosticResult.blocked()\nstatus=BLOCKED\nproof_ref=None"]
B -- "True" --> F{"audit_trace\npresent?"}
F -- "No" --> G["raise ValueError\n'VERIFIED requires audit_trace'"]
F -- "Yes" --> H["compute_proof_ref(audit_trace)\n→ sha256:digest"]
H --> I["TaxDiagnosticResult.verified()\nstatus=VERIFIED\nproof_ref=sha256:..."]
I --> J{"__post_init__ invariants"}
J -- "pass" --> K["Return TaxDiagnosticResult\nis_authoritative=True"]
D --> L["Return TaxDiagnosticResult\nis_authoritative=False"]
E --> L
style G fill:#ff6b6b,color:#fff
style K fill:#51cf66,color:#fff
style L fill:#ffa94d,color:#fff
Reviews (2): Last reviewed commit: "fix: address CodeQL, Sentry, Greptile P1..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@qwed_tax/diagnostics.py`:
- Around line 187-205: The `__post_init__` method assumes `agent_message` is a
string without type validation, which can cause uncontrolled `AttributeError` on
bad payloads instead of clear validation errors. Add explicit type checks in
`__post_init__` to verify that `agent_message` is a string and
`developer_fields` (if used) are of the expected type, raising controlled
`ValueError` messages with clear descriptions if types are incorrect.
Additionally, in the `from_dict` method (referenced in the "Also applies to"
comment), add validation to ensure the `status` field is converted to or
validated as a `TaxDiagnosticStatus` enum value before object construction,
preventing non-enum status values from being accepted.
In `@tests/test_diagnostics.py`:
- Around line 60-61: The pytest.raises block in the frozen dataclass test is
catching too broad an exception type. Replace Exception with FrozenInstanceError
to explicitly assert for the specific exception that should be raised when
attempting to mutate a frozen instance. This ensures the test is checking for
the expected behavior and won't mask unrelated failures or unexpected
exceptions.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5d12e032-581e-4c45-867e-e33a76261505
⛔ Files ignored due to path filters (1)
qwed_tax/__pycache__/models.cpython-311.pycis excluded by!**/*.pyc
📒 Files selected for processing (6)
qwed_tax/audit.pyqwed_tax/diagnostics.pyqwed_tax/guards/indirect_tax_guard.pyqwed_tax/guards/tds_guard.pyqwed_tax/jurisdictions/india/guards/gst_guard.pytests/test_diagnostics.py
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
qwed_tax/guards/tds_guard.py (1)
85-124: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider extracting the common
to_diagnostic()logic into a shared helper.The
to_diagnostic()implementation is identical across all three guards migrated in this PR (TDSGuard,InputCreditGuard,GSTGuard). Since these guards are being added together, consolidating the conversion logic now would prevent future divergence and reduce maintenance burden.♻️ Suggested consolidation approach
Create a shared helper in
qwed_tax/diagnostics.py:def legacy_result_to_diagnostic( result: Dict[str, Any], unknown_constraint_id: str = "UNKNOWN", ) -> TaxDiagnosticResult: """Convert a legacy guard result dict to TaxDiagnosticResult. Backward-compatible migration helper for guards that produce audit_trace. """ verified = result.get("verified", False) audit_trace = result.get("audit_trace") if not verified: return TaxDiagnosticResult.blocked( agent_message="Tax verification could not be completed.", developer_fields={ "constraint_id": audit_trace["rule_id"] if audit_trace else unknown_constraint_id, "audit_trace": audit_trace, "error": result.get("error"), "deduction": result.get("deduction"), "net_payable": result.get("net_payable"), }, ) if audit_trace is None: raise ValueError( "VERIFIED result requires audit_trace — " "use UNVERIFIABLE if no evidence was established." ) return TaxDiagnosticResult.verified( agent_message="Tax verification completed.", developer_fields={ "constraint_id": audit_trace["rule_id"], "statute": audit_trace.get("statute"), "jurisdiction": audit_trace.get("jurisdiction"), "audit_trace": audit_trace, "deduction": result.get("deduction"), "net_payable": result.get("net_payable"), }, evidence=audit_trace, )Then replace each guard's
to_diagnostic()with:`@staticmethod` def to_diagnostic(result: Dict[str, Any]) -> TaxDiagnosticResult: """Convert legacy result to TaxDiagnosticResult.""" return legacy_result_to_diagnostic(result, unknown_constraint_id="TDS_UNKNOWN")🤖 Prompt for 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. In `@qwed_tax/guards/tds_guard.py` around lines 85 - 124, The to_diagnostic() static method in TDSGuard contains identical logic that is duplicated across InputCreditGuard and GSTGuard. Extract this common conversion logic into a shared helper function called legacy_result_to_diagnostic() in qwed_tax/diagnostics.py, parameterizing the unknown_constraint_id so each guard can pass its own identifier (e.g., "TDS_UNKNOWN" for TDSGuard). Then replace the entire to_diagnostic() method body in TDSGuard with a single call to this shared helper, passing "TDS_UNKNOWN" as the constraint identifier argument.
🤖 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.
Nitpick comments:
In `@qwed_tax/guards/tds_guard.py`:
- Around line 85-124: The to_diagnostic() static method in TDSGuard contains
identical logic that is duplicated across InputCreditGuard and GSTGuard. Extract
this common conversion logic into a shared helper function called
legacy_result_to_diagnostic() in qwed_tax/diagnostics.py, parameterizing the
unknown_constraint_id so each guard can pass its own identifier (e.g.,
"TDS_UNKNOWN" for TDSGuard). Then replace the entire to_diagnostic() method body
in TDSGuard with a single call to this shared helper, passing "TDS_UNKNOWN" as
the constraint identifier argument.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 78f9a404-71f5-48d4-97aa-713dd94fbb87
📒 Files selected for processing (6)
qwed_tax/audit.pyqwed_tax/diagnostics.pyqwed_tax/guards/indirect_tax_guard.pyqwed_tax/guards/tds_guard.pyqwed_tax/jurisdictions/india/guards/gst_guard.pytests/test_diagnostics.py
🚧 Files skipped from review as they are similar to previous changes (4)
- qwed_tax/jurisdictions/india/guards/gst_guard.py
- qwed_tax/guards/indirect_tax_guard.py
- qwed_tax/diagnostics.py
- tests/test_diagnostics.py
|
Not fixing the three suggested by coderabbit |
|
Docs PR opened: QWED-AI/docs#226 Added a changelog entry for QWED-Tax adopting the 3-layer DiagnosticResult contract for TDS, ITC, and GST-RCM guards. |
|
Docs PR opened: QWED-AI/docs#227 Added a Structured diagnostics section to the tax guards page documenting the new TaxDiagnosticResult tri-state model and proof references. |
1 participant
Summary
Implements the 3-layer
TaxDiagnosticResultmodel for QWED-Tax, following the same pattern as qwed-verification #204 but as an independent model (no cross-package dependency).What's new
qwed_tax/diagnostics.py— Core model:TaxDiagnosticStatusenum:VERIFIED/UNVERIFIABLE/BLOCKED(tri-state only)TaxDiagnosticResultfrozen dataclass with 3 layers:agent_message: str— agent-safe, no statute/rule IDs/detection logicdeveloper_fields: dict— structured evidence (constraint_id, statute, jurisdiction, audit_trace, deduction, net_payable, etc.)proof_ref: Optional[str]— sha256 hash of retained proof artifactTaxAdvisoryCheck— non-proof-bearing analysis (advisory_only=True enforced)compute_proof_ref()— deterministic sha256 hash of JSON-serialized evidenceverified(),unverifiable(),blocked()__post_init__: VERIFIED requires proof_ref, non-VERIFIED rejects proof_ref, agent_message must be non-emptyqwed_tax/audit.py— Extended:trace_proof_ref(trace)— binds abuild_trace()output to a deterministic hashGuard migrations (3 of 12 — the ones that already have audit_trace)
TDSGuardto_diagnostic()InputCreditGuardto_diagnostic()GSTGuardto_diagnostic()Each guard gets a
to_diagnostic()static method that converts its existing dict return toTaxDiagnosticResult. The legacy dict API is unchanged —to_diagnostic()is additive, not breaking.Tests
tests/test_diagnostics.py— 32 tests:Remaining 9 guards
The remaining 9 guards (CapitalGains, Classification, Speculation, Setoff, Crypto, Valuation, Remittance, PoEM, Withholding) will be migrated in follow-up PRs. This PR establishes the contract and migrates the 3 guards that already have
audit_trace— the lowest-effort, highest-confidence migrations.Closes #39
Summary by CodeRabbit
New Features
Tests