feat: migrate 9 remaining guards to TaxDiagnosticResult (close #47)#48
Merged
Rahul Dass (rahuldass19) merged 3 commits intoJun 22, 2026
Merged
Conversation
- Add RuleRef entries to audit.py for all 9 guards (CG, IRS, Speculation, InterHead, VDA, PoEM, FEMA/LRS, W4, SAFE) - Add build_trace() calls + audit_trace to all verification paths - Add to_diagnostic() static method to all 9 guards - Fail-closed: raise ValueError when verified=True but audit_trace is None - CryptoTaxGuard: add audit_trace field to TaxResult pydantic model - 40 new tests covering VERIFIED, BLOCKED, UNVERIFIABLE paths - 260 total tests pass (220 existing + 40 new) - to_diagnostic() coverage: 12 of 12 guards
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
More reviews will be available in 49 minutes and 46 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR completes the ChangesGuard Migration to TaxDiagnosticResult
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Guard as Guard.verify_*()
participant build_trace
participant to_diagnostic as Guard.to_diagnostic()
participant TaxDiagnosticResult
Caller->>Guard: verify_*(inputs)
Guard->>build_trace: build_trace(RuleRef, outcome, context_dict)
build_trace-->>Guard: audit_trace {rule_id, statute, jurisdiction, outcome, ...}
Guard-->>Caller: {verified, error?, audit_trace}
Caller->>to_diagnostic: to_diagnostic(result)
alt verified=True but audit_trace is None
to_diagnostic-->>Caller: raises ValueError
else outcome in _UNVERIFIABLE_OUTCOMES
to_diagnostic->>TaxDiagnosticResult: .unverifiable(agent_msg, dev_fields)
TaxDiagnosticResult-->>Caller: TaxDiagnosticResult(UNVERIFIABLE)
else verified=False
to_diagnostic->>TaxDiagnosticResult: .blocked(agent_msg, dev_fields)
TaxDiagnosticResult-->>Caller: TaxDiagnosticResult(BLOCKED)
else verified=True
to_diagnostic->>TaxDiagnosticResult: .verified(constraint_id, statute, jurisdiction, evidence=audit_trace)
TaxDiagnosticResult-->>Caller: TaxDiagnosticResult(VERIFIED)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 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 completes the
Confidence Score: 4/5Safe to merge after correcting the VDA statutory reference in InterHeadAdjustmentGuard; all other guards are correctly implemented. The qwed_tax/jurisdictions/india/guards/setoff_guard.py — the Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["guard.verify()"] -->|returns Dict| B{verified?}
B -->|True| C{audit_trace present?}
C -->|No| D["raise ValueError — fail-closed"]
C -->|Yes| E["TaxDiagnosticResult.verified\nproof_ref = sha256 of audit_trace"]
B -->|False| F{outcome in _UNVERIFIABLE_OUTCOMES?}
F -->|Yes| G["TaxDiagnosticResult.unverifiable\nproof_ref = None"]
F -->|No| H["TaxDiagnosticResult.blocked\nproof_ref = None"]
subgraph Unverifiable outcomes per guard
U1["CapitalGainsGuard: NO_RATE, SLAB_RATE"]
U2["ClassificationGuard: AMBIGUOUS"]
U3["SpeculationGuard: UNKNOWN_LOSS or PROFIT_SOURCE"]
U4["CryptoTaxGuard: GAIN_SIDE_NOT_IMPLEMENTED"]
U5["InterHeadAdjustmentGuard: UNKNOWN_HEAD"]
end
G -.-> U1
G -.-> U2
G -.-> U3
G -.-> U4
G -.-> U5
%%{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.verify()"] -->|returns Dict| B{verified?}
B -->|True| C{audit_trace present?}
C -->|No| D["raise ValueError — fail-closed"]
C -->|Yes| E["TaxDiagnosticResult.verified\nproof_ref = sha256 of audit_trace"]
B -->|False| F{outcome in _UNVERIFIABLE_OUTCOMES?}
F -->|Yes| G["TaxDiagnosticResult.unverifiable\nproof_ref = None"]
F -->|No| H["TaxDiagnosticResult.blocked\nproof_ref = None"]
subgraph Unverifiable outcomes per guard
U1["CapitalGainsGuard: NO_RATE, SLAB_RATE"]
U2["ClassificationGuard: AMBIGUOUS"]
U3["SpeculationGuard: UNKNOWN_LOSS or PROFIT_SOURCE"]
U4["CryptoTaxGuard: GAIN_SIDE_NOT_IMPLEMENTED"]
U5["InterHeadAdjustmentGuard: UNKNOWN_HEAD"]
end
G -.-> U1
G -.-> U2
G -.-> U3
G -.-> U4
G -.-> U5
Reviews (3): Last reviewed commit: "fix: use frozenset for _UNVERIFIABLE_OUT..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- Remove unused build_trace import from test file (CodeQL)
- Differentiate BLOCKED vs UNVERIFIABLE based on audit_trace outcome:
- UNVERIFIABLE: NO_RATE, SLAB_RATE, AMBIGUOUS, UNKNOWN_LOSS_SOURCE,
UNKNOWN_PROFIT_SOURCE, UNKNOWN_HEAD, GAIN_SIDE_NOT_IMPLEMENTED
- BLOCKED: RATE_MISMATCH, MISCLASSIFICATION, ILLEGAL_SETOFF,
INVALID_INPUT, PROHIBITED, LIMIT_EXCEEDED, etc.
- PoEMGuard: input validation failures now correctly map to blocked()
instead of unverifiable(); added audit_trace to _unverifiable() helper
- Added 3 new tests: SLAB_RATE unverifiable, gain-side not implemented
unverifiable, unknown head unverifiable
- 263 tests pass (220 existing + 43 new)
![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
🧹 Nitpick comments (5)
qwed_tax/guards/classification_guard.py (1)
113-113: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueUse
frozensetfor immutable class-level constant.Same as
CapitalGainsGuard— usingfrozensetsilences RUF012 and makes immutability explicit.Suggested fix
- _UNVERIFIABLE_OUTCOMES = {"AMBIGUOUS"} + _UNVERIFIABLE_OUTCOMES: frozenset[str] = frozenset({"AMBIGUOUS"})🤖 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/classification_guard.py` at line 113, The _UNVERIFIABLE_OUTCOMES class-level constant is currently defined as a mutable set using braces syntax. Convert this constant to use frozenset instead to make its immutability explicit and silence the RUF012 linter warning. Replace the set definition with a frozenset constructor wrapping the set literal for the _UNVERIFIABLE_OUTCOMES variable in the classification_guard.py file.Source: Linters/SAST tools
qwed_tax/guards/speculation_guard.py (1)
95-95: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueUse
frozensetfor immutable class-level constant.Same pattern as other guards —
frozensetsilences RUF012 and documents intent.Suggested fix
- _UNVERIFIABLE_OUTCOMES = {"UNKNOWN_LOSS_SOURCE", "UNKNOWN_PROFIT_SOURCE"} + _UNVERIFIABLE_OUTCOMES: frozenset[str] = frozenset({"UNKNOWN_LOSS_SOURCE", "UNKNOWN_PROFIT_SOURCE"})🤖 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/speculation_guard.py` at line 95, The class-level constant `_UNVERIFIABLE_OUTCOMES` is currently defined as a regular set but should be an immutable frozenset to follow the established pattern in other guards and comply with the RUF012 linting rule. Convert the set literal containing "UNKNOWN_LOSS_SOURCE" and "UNKNOWN_PROFIT_SOURCE" into a frozenset by wrapping it with frozenset() constructor.Source: Linters/SAST tools
qwed_tax/guards/capital_gains_guard.py (1)
114-114: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueUse
frozensetfor immutable class-level constant.The static analysis warning (RUF012) is valid here. Since
_UNVERIFIABLE_OUTCOMESis only used for membership testing and should never be mutated, usingfrozensetmakes the intent explicit and prevents accidental modification.Suggested fix
- _UNVERIFIABLE_OUTCOMES = {"NO_RATE", "SLAB_RATE"} + _UNVERIFIABLE_OUTCOMES: frozenset[str] = frozenset({"NO_RATE", "SLAB_RATE"})🤖 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/capital_gains_guard.py` at line 114, The _UNVERIFIABLE_OUTCOMES class constant should be changed from a mutable set to an immutable frozenset since it is only used for membership testing and should never be modified. Replace the set literal syntax with the frozenset constructor wrapping the same values to make the immutability explicit and prevent accidental modifications.Source: Linters/SAST tools
qwed_tax/jurisdictions/india/guards/crypto_guard.py (1)
106-106: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse
frozensetfor immutable class-level constant.
_UNVERIFIABLE_OUTCOMESis only read (membership tests), never mutated. Usingfrozensetmakes immutability explicit and silences the Ruff RUF012 warning.♻️ Suggested fix
- _UNVERIFIABLE_OUTCOMES = {"GAIN_SIDE_NOT_IMPLEMENTED"} + _UNVERIFIABLE_OUTCOMES: frozenset[str] = frozenset({"GAIN_SIDE_NOT_IMPLEMENTED"})🤖 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/jurisdictions/india/guards/crypto_guard.py` at line 106, The _UNVERIFIABLE_OUTCOMES constant is defined as a regular set but is only used for membership tests and never modified. Change the set literal to a frozenset by wrapping the set with the frozenset() constructor to make immutability explicit and resolve the Ruff RUF012 warning. This clarifies the intent that this collection is immutable and will not be mutated at runtime.Source: Linters/SAST tools
qwed_tax/jurisdictions/india/guards/setoff_guard.py (1)
133-133: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse
frozensetfor immutable class-level constant.
_UNVERIFIABLE_OUTCOMESis only read (membership tests), never mutated. Usingfrozensetmakes immutability explicit and silences the Ruff RUF012 warning.♻️ Suggested fix
- _UNVERIFIABLE_OUTCOMES = {"UNKNOWN_HEAD"} + _UNVERIFIABLE_OUTCOMES: frozenset[str] = frozenset({"UNKNOWN_HEAD"})🤖 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/jurisdictions/india/guards/setoff_guard.py` at line 133, The `_UNVERIFIABLE_OUTCOMES` class-level constant is defined as a mutable set but is only read for membership tests and never mutated. Convert it to a frozenset by wrapping the set literal with the frozenset constructor to make its immutability explicit and satisfy the Ruff RUF012 linting rule. Change the assignment to use frozenset() around the set definition.Source: Linters/SAST tools
🤖 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 `@tests/test_guard_diagnostics.py`:
- Around line 421-456: The TestAllGuardsSerialization class claims to verify
round-trip serialization for all migrated guards but only implements test
methods for three guards: test_capital_gains_roundtrip, test_crypto_roundtrip,
and test_withholding_roundtrip, leaving six migrated guards untested. Add test
methods for each of the remaining six migrated guards following the same pattern
used in the existing tests, where each test method instantiates the guard, calls
its verification method, converts to a diagnostic using to_diagnostic, performs
a round-trip serialization via to_dict and from_dict, and asserts that critical
properties like status and proof_ref are preserved through the round-trip.
- Around line 186-237: Add a new test method to the TestCryptoTaxGuardDiagnostic
class that validates the fail-closed contract of CryptoTaxGuard.to_diagnostic().
The test should verify that a ValueError is raised when attempting to call
to_diagnostic() with a raw result containing verified=True but missing the
required audit_trace field. This ensures the guard properly validates its inputs
and maintains the fail-closed regression safety net across all migrated guard
tests.
---
Nitpick comments:
In `@qwed_tax/guards/capital_gains_guard.py`:
- Line 114: The _UNVERIFIABLE_OUTCOMES class constant should be changed from a
mutable set to an immutable frozenset since it is only used for membership
testing and should never be modified. Replace the set literal syntax with the
frozenset constructor wrapping the same values to make the immutability explicit
and prevent accidental modifications.
In `@qwed_tax/guards/classification_guard.py`:
- Line 113: The _UNVERIFIABLE_OUTCOMES class-level constant is currently defined
as a mutable set using braces syntax. Convert this constant to use frozenset
instead to make its immutability explicit and silence the RUF012 linter warning.
Replace the set definition with a frozenset constructor wrapping the set literal
for the _UNVERIFIABLE_OUTCOMES variable in the classification_guard.py file.
In `@qwed_tax/guards/speculation_guard.py`:
- Line 95: The class-level constant `_UNVERIFIABLE_OUTCOMES` is currently
defined as a regular set but should be an immutable frozenset to follow the
established pattern in other guards and comply with the RUF012 linting rule.
Convert the set literal containing "UNKNOWN_LOSS_SOURCE" and
"UNKNOWN_PROFIT_SOURCE" into a frozenset by wrapping it with frozenset()
constructor.
In `@qwed_tax/jurisdictions/india/guards/crypto_guard.py`:
- Line 106: The _UNVERIFIABLE_OUTCOMES constant is defined as a regular set but
is only used for membership tests and never modified. Change the set literal to
a frozenset by wrapping the set with the frozenset() constructor to make
immutability explicit and resolve the Ruff RUF012 warning. This clarifies the
intent that this collection is immutable and will not be mutated at runtime.
In `@qwed_tax/jurisdictions/india/guards/setoff_guard.py`:
- Line 133: The `_UNVERIFIABLE_OUTCOMES` class-level constant is defined as a
mutable set but is only read for membership tests and never mutated. Convert it
to a frozenset by wrapping the set literal with the frozenset constructor to
make its immutability explicit and satisfy the Ruff RUF012 linting rule. Change
the assignment to use frozenset() around the set definition.
🪄 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: 73fb2a8f-baca-4434-b7f6-15ee77523165
📒 Files selected for processing (11)
qwed_tax/audit.pyqwed_tax/guards/capital_gains_guard.pyqwed_tax/guards/classification_guard.pyqwed_tax/guards/poem_guard.pyqwed_tax/guards/remittance_guard.pyqwed_tax/guards/speculation_guard.pyqwed_tax/guards/valuation_guard.pyqwed_tax/jurisdictions/india/guards/crypto_guard.pyqwed_tax/jurisdictions/india/guards/setoff_guard.pyqwed_tax/jurisdictions/us/withholding_guard.pytests/test_guard_diagnostics.py
- Convert _UNVERIFIABLE_OUTCOMES to frozenset in 5 guards (CodeRabbit RUF012) - Add CryptoTaxGuard fail-closed test (verified=True without audit_trace) - Add 6 remaining serialization round-trip tests (classification, speculation, interhead, valuation, remittance, poem) - 270 tests pass (220 existing + 50 new)
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Completes the
TaxDiagnosticResultmigration for all remaining 9 guards, achieving 12/12to_diagnostic()coverage. This is the final v0.2.0 release blocker.What changed
audit.py — 15 new RuleRef entries
CG_EQUITY_LTCG_112A,CG_EQUITY_STCG_111A,CG_DEBT_FUND_50AA,CG_NO_RATE_CONFIGUREDSPECULATIVE_43_5,SPECULATIVE_SETOFF_73INTERHEAD_SETOFF_71,CAPITAL_GAINS_SETOFF_74VDA_115BBH,VDA_SETOFF_PROHIBITIONPOEM_SECTION_6_3,POEM_CBDT_6_2017LRS_LIMIT,FEMA_SCHEDULE_I,TCS_LRS_206CRIRS_COMMON_LAW,W4_EXEMPT_PUB505SAFE_CONVERSIONJURISDICTION_US = "US"constant9 guards migrated
guards/capital_gains_guard.pybuild_traceinverify_tax_rate,to_diagnostic()guards/classification_guard.pybuild_traceinverify_classification_claim,to_diagnostic()guards/speculation_guard.pybuild_traceinverify_setoff,to_diagnostic()india/guards/setoff_guard.pybuild_trace+_RULE_REFSmap,to_diagnostic()india/guards/crypto_guard.pyaudit_tracefield added toTaxResultmodel,build_tracein both methods,to_diagnostic()guards/valuation_guard.pybuild_traceinverify_conversion,to_diagnostic()guards/remittance_guard.pybuild_traceinverify_lrs_limit,to_diagnostic()guards/poem_guard.pybuild_traceindetermine_residency,to_diagnostic()us/withholding_guard.pybuild_traceinverify_exempt_status,to_diagnostic()Pattern (same as #45)
build_trace()called on every verification path with the relevantRuleRefaudit_traceadded to return dict (additive — existing callers unaffected)to_diagnostic()static method:TaxDiagnosticResult.verified()withproof_refTaxDiagnosticResult.blocked()with fallback constraint_idraise ValueError(fail-closed)unverifiable()instead ofblocked()for invalid input pathsTests — 40 new (
tests/test_guard_diagnostics.py)verified=Truewithoutaudit_traceraisesValueErrorto_dict/from_dict) for 3 guardsTest results
Migration status
to_diagnostic()coverage: 12 of 12 guards ✅Closes #47
Summary by CodeRabbit
New Features
Tests