fix: fail-closed on ambiguous classification + claim comparison (#17, #18)#42
Merged
Rahul Dass (rahuldass19) merged 8 commits intoJun 21, 2026
Conversation
…18) #17 — guessing classifications instead of blocking: - CapitalGainsGuard.determine_term: raise ValueError on bad dates and unknown asset types (was returning ERROR_DATE_FORMAT sentinel that silently flowed to verified=True) - CapitalGainsGuard SLAB rates: return verified=False — slab rates cannot be deterministically verified without taxpayer's income bracket - SpeculationGuard: require known vocabulary (intraday, f&o, delivery, etc.) — reject unrecognized source strings instead of substring guess - SetoffGuard: add allowlist for explicitly allowed loss heads, add SALARY to prohibition matrix (salary losses cannot be set off inter-head), block heads not in prohibition matrix or allowlist - GSTGuard RCM: fail-closed on unknown service/entity — no silent coercion to OTHER/INDIVIDUAL (was suppressing statutory RCM) #18 — success without comparing claim to computed truth: - GSTGuard RCM: add optional claimed_is_rcm parameter — when provided, compare computed is_rcm against claim and return verified=True only on exact match. When omitted, return computed_only=True flag (backward compatible, separates calculation from verification) - CryptoTaxGuard: distinguish vda_income==0 (verify claimed_tax==0) from vda_income<0 (loss — route to set-off, not 'no income') Test updated: test_rcm.py test_unknown_string_service_is_forward_charge → test_unknown_string_service_fail_closed (asserts verified=False) Also cleaned: unused imports in setoff_guard.py and crypto_guard.py, pre-existing unused variables in crypto_guard.py verify_set_off 115 tests pass, 0 regressions. Ruff lint clean.
- verify_worker_status: return None for mixed signals (some but not all employee indicators) instead of defaulting to CONTRACTOR - verify_classification_claim: handle None → verified=False with 'ambiguous classification' error - Contractor only returned when NO employee indicators are present Fixes the original #17 finding: 'Worker classification defaults to contractor in mixed-signal cases' — e.g. provides_tools=False, reimburses_expenses=True, indefinite_relationship=True was collapsing to CONTRACTOR despite two employee indicators being present. 115 tests pass.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughMultiple tax guard classes are hardened to fail closed: unknown or ambiguous inputs now raise ChangesFail-closed guard hardening
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Greptile SummaryThis PR hardens six tax guards against fail-open behavior: bad dates and unknown asset types now raise
Confidence Score: 3/5Not safe to merge without updating the qwed-open-responses TaxGuard to drop gains from its verify_set_off call, or the behavior change will silently break all crypto gain+loss tool calls in production. The fail-closed improvements to classification, speculation, and capital gains are well-reasoned and thoroughly tested. The critical gap is that
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[verify_set_off called] --> B{gains provided\nand non-empty?}
B -- Yes --> C[verified=False\n'not implemented']
B -- No --> D{VDA loss\npresent?}
D -- Yes --> E[verified=False\nSection 115BBH]
D -- No --> F[verified=True\nAllowed]
G[qwed-open-responses\nTaxGuard._verify_crypto_tax] --> H[verify_set_off\nlosses=...\ngains=args.get gains or empty]
H --> A
style C fill:#ff6b6b,color:#fff
style E fill:#ff6b6b,color:#fff
style F fill:#51cf66,color:#fff
style C stroke:#c92a2a
%%{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[verify_set_off called] --> B{gains provided\nand non-empty?}
B -- Yes --> C[verified=False\n'not implemented']
B -- No --> D{VDA loss\npresent?}
D -- Yes --> E[verified=False\nSection 115BBH]
D -- No --> F[verified=True\nAllowed]
G[qwed-open-responses\nTaxGuard._verify_crypto_tax] --> H[verify_set_off\nlosses=...\ngains=args.get gains or empty]
H --> A
style C fill:#ff6b6b,color:#fff
style E fill:#ff6b6b,color:#fff
style F fill:#51cf66,color:#fff
style C stroke:#c92a2a
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
qwed_tax/guards/classification_guard.py (1)
14-24:⚠️ Potential issue | 🟠 MajorUpdate return type annotation to
Optional[WorkerType]to reflect that the function returnsNonefor ambiguous cases.Line 44 returns
Nonewhen mixed signals are detected, but the signature declares-> WorkerType. This breaks the type contract and prevents static type checkers from validating the None check at line 60. AddOptionalto the imports and update the return type annotation.Proposed fix
-from typing import Dict, Any +from typing import Dict, Any, Optional @@ - def verify_worker_status(self, behavioral_control: bool, financial_control: bool, relationship_permanence: bool) -> WorkerType: + def verify_worker_status( + self, + behavioral_control: bool, + financial_control: bool, + relationship_permanence: bool, + ) -> Optional[WorkerType]:🤖 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` around lines 14 - 24, The verify_worker_status method has a return type annotation of WorkerType but the docstring and implementation indicate it can return None when mixed signals are detected. Update the return type annotation from WorkerType to Optional[WorkerType] by importing Optional from the typing module (if not already imported) and modifying the function signature of verify_worker_status to declare -> Optional[WorkerType]. This will align the type signature with the actual behavior where None is returned for ambiguous cases.Source: Linters/SAST tools
🧹 Nitpick comments (1)
tests/test_rcm.py (1)
145-150: ⚡ Quick winAdd tests for dual-mode contract (
claimed_is_rcmvscomputed_only).Good fail-closed test for unknown service. Please also add explicit assertions for verification mode (match/mismatch) and computation mode (
computed_onlybehavior), since that contract is security-critical in this PR.🤖 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 `@tests/test_rcm.py` around lines 145 - 150, The current test_unknown_string_service_fail_closed method only covers the fail-closed behavior for unknown service types, but does not test the security-critical dual-mode contract behavior. Add additional test methods that explicitly verify the verification mode behavior (testing both matching and mismatching scenarios between claimed_is_rcm and computed RCM status) and the computed_only mode behavior where verification is bypassed. Include explicit assertions for these modes in the guard.verify_rcm_applicability call to ensure the contract between claimed_is_rcm and computed_only parameters is properly enforced and tested.
🤖 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/guards/capital_gains_guard.py`:
- Around line 15-20: The try-except block in _check_capital_gains only catches
ValueError, but other exceptions like TypeError (from strptime receiving
non-string dates) and AttributeError (from calling .lower() on non-string
asset_type) can escape unhandled. Expand the except clause to catch all relevant
exception types (ValueError, TypeError, AttributeError) or use a broader
exception handler to ensure all unexpected errors from strptime, asset_type
operations, and date calculations are properly caught and converted to a
consistent error response that maintains the fail-closed flow.
- Around line 24-36: The debt_fund threshold is set to 0 in the thresholds
dictionary, but the comparison logic `days > limit` means any holding period
greater than 0 days will be classified as LTCG, which contradicts the intended
behavior stated in the comment. To fix this, change the debt_fund threshold
value to a number large enough that the condition `days > limit` will always
evaluate to false for any realistic holding period (such as a very large integer
like 10000 or float('inf')), ensuring debt_fund assets are always classified as
STCG regardless of holding period.
In `@qwed_tax/guards/classification_guard.py`:
- Around line 69-74: Add a type guard check before calling `.upper()` on
`llm_claim` in the normalization block. If `llm_claim` is not a string, return
`verified=False` with an explicit error message instead of allowing the
AttributeError to be raised. The type check should occur before the comment "#
Normalize claim" and the subsequent call to `llm_claim.upper()`, ensuring the
fail-closed path returns a controlled error response rather than crashing.
In `@qwed_tax/guards/speculation_guard.py`:
- Around line 67-76: The _classify_source method uses simple substring matching
with the `keyword in source` check in both the cls._KNOWN_SPECULATIVE and
cls._KNOWN_NON_SPECULATIVE loops, which allows keywords to match within larger
words or tokens rather than as complete words. Replace the substring matching
with word boundary matching (e.g., using regex word boundaries \b or by
splitting the source string into tokens and comparing against individual words)
to ensure keywords only match as complete, standalone words and prevent false
classifications from ambiguous inputs.
In `@qwed_tax/jurisdictions/india/guards/gst_guard.py`:
- Around line 203-206: The `_try_coerce` method currently only catches
`ValueError` when attempting enum coercion, but malformed JSON values such as
lists or dicts can raise `TypeError` instead, which will escape the exception
handler and break the fail-closed behavior. Update the except clause to catch
both `ValueError` and `TypeError` so that any enum coercion failure (whether
ValueError or TypeError) returns `None` consistently.
- Around line 180-184: The computation mode block in the RCM/FCM decision logic
incorrectly returns verified=True without performing any claim comparison,
creating a false success state that could lead callers to accept unproven
decisions. Change the verified field to False in the return statement where the
liability is set to "RECIPIENT (RCM)" or "PROVIDER (FCM)" to accurately reflect
that this is a computed-only result without claim verification. Reserve
verified=True only for cases where the computation has been explicitly validated
against a claim comparison.
---
Outside diff comments:
In `@qwed_tax/guards/classification_guard.py`:
- Around line 14-24: The verify_worker_status method has a return type
annotation of WorkerType but the docstring and implementation indicate it can
return None when mixed signals are detected. Update the return type annotation
from WorkerType to Optional[WorkerType] by importing Optional from the typing
module (if not already imported) and modifying the function signature of
verify_worker_status to declare -> Optional[WorkerType]. This will align the
type signature with the actual behavior where None is returned for ambiguous
cases.
---
Nitpick comments:
In `@tests/test_rcm.py`:
- Around line 145-150: The current test_unknown_string_service_fail_closed
method only covers the fail-closed behavior for unknown service types, but does
not test the security-critical dual-mode contract behavior. Add additional test
methods that explicitly verify the verification mode behavior (testing both
matching and mismatching scenarios between claimed_is_rcm and computed RCM
status) and the computed_only mode behavior where verification is bypassed.
Include explicit assertions for these modes in the
guard.verify_rcm_applicability call to ensure the contract between
claimed_is_rcm and computed_only parameters is properly enforced and tested.
🪄 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: 93a20313-e5a1-4b3d-aa16-50f079219796
📒 Files selected for processing (8)
qwed_tax/guards/capital_gains_guard.pyqwed_tax/guards/classification_guard.pyqwed_tax/guards/speculation_guard.pyqwed_tax/jurisdictions/india/guards/crypto_guard.pyqwed_tax/jurisdictions/india/guards/gst_guard.pyqwed_tax/jurisdictions/india/guards/setoff_guard.pyqwed_tax/verifier.pytests/test_rcm.py
- CapitalGainsGuard: debt_fund special-case returns STCG always (was threshold=0 which classified as LTCG for any holding > 0 days) - CapitalGainsGuard: catch TypeError in date parsing (non-string dates) - CapitalGainsGuard: raise from exc (Ruff B904) - SpeculationGuard: exact match instead of substring (was classifying 'intraday_equity_position' as speculative, 'side_business_income' as non-speculative) - ClassificationGuard: Optional[WorkerType] return type annotation - ClassificationGuard: type guard for non-string llm_claim - GSTGuard: computation mode verified=False (was True — calculator masquerading as verifier, #18 core fix) - GSTGuard: _try_coerce catches TypeError (malformed JSON values) - GSTGuard: remove no-op ternary on reason field - Tests: 22 new tests for dual-mode RCM, debt_fund STCG, SLAB block, speculation exact match, classification ambiguity, crypto zero/negative, setoff allowlist, entity fail-closed - Test: update test_trade_tax_setoff to use exact known source names 137 tests pass, 0 regressions. Ruff lint clean.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
qwed_tax/jurisdictions/india/guards/crypto_guard.py (1)
22-42:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail closed when
gainsis provided but not validated.At Line 22,
gainsis accepted but ignored, and the method can still returnverified=Trueat Line 38. That creates a verification false-positive surface for inputs the guard does not actually check. Until gain-side validation is implemented, reject non-emptygainsexplicitly.Proposed fail-closed patch
def verify_set_off(self, losses: Dict[str, Decimal], gains: Optional[Dict[str, Decimal]] = None) -> TaxResult: @@ - # Rule 1: Check for VDA Losses being used + # Fail closed: gain-side verification is not implemented yet. + if gains: + return TaxResult( + verified=False, + message="Gain-side set-off verification is not implemented in CryptoTaxGuard. Provide losses-only payload or route to inter-head set-off guard.", + allowed_set_off=Decimal(0), + ) + + # Rule 1: Check for VDA Losses being used if "VDA" in losses and losses["VDA"] < 0: return TaxResult(🤖 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` around lines 22 - 42, The verify_set_off method accepts an optional gains parameter but does not validate it, causing the method to return verified=True even when unhandled gains data is provided. To fail closed, add an explicit check at the beginning of the verify_set_off method (before the VDA loss check) that rejects any non-empty gains dictionary. If gains is provided and contains entries, return a TaxResult with verified=False and a message indicating that inter-head adjustment verification is not yet implemented. This prevents false positives for validation scenarios the guard does not currently handle.Source: Linters/SAST tools
qwed_tax/jurisdictions/india/guards/gst_guard.py (1)
157-159:⚠️ Potential issue | 🟠 MajorAdd strict boolean type validation for
claimed_is_rcmto prevent false verification with non-bool truthy values.The comparison
verified = (claimed_is_rcm == is_rcm)is unsafe: Python's equality operator treats integers like1and0as equivalent toTrueandFalserespectively (1 == Trueevaluates toTrue). Ifclaimed_is_rcmreceives an integer from JSON deserialization or dynamic input despite theOptional[bool]type hint, verification can incorrectly pass.Proposed fix
# Verification mode: compare computed RCM against claimed RCM if claimed_is_rcm is not None: + if not isinstance(claimed_is_rcm, bool): + return { + "verified": False, + "error": ( + "Invalid claimed_is_rcm. Expected a boolean true/false " + "for deterministic verification." + ), + "is_rcm": is_rcm, + } - verified = (claimed_is_rcm == is_rcm) + verified = (claimed_is_rcm is is_rcm) return { "verified": verified,🤖 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/gst_guard.py` around lines 157 - 159, The equality comparison in the verification statement does not perform strict type checking, allowing Python's truthy behavior to treat integers like 1 and 0 as equivalent to boolean True and False values. Add explicit type validation using isinstance(claimed_is_rcm, bool) to ensure claimed_is_rcm is actually a boolean before performing the equality comparison with is_rcm. This prevents false verification when claimed_is_rcm receives integer values from JSON deserialization or other dynamic input sources despite the Optional[bool] type hint.
🤖 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.
Outside diff comments:
In `@qwed_tax/jurisdictions/india/guards/crypto_guard.py`:
- Around line 22-42: The verify_set_off method accepts an optional gains
parameter but does not validate it, causing the method to return verified=True
even when unhandled gains data is provided. To fail closed, add an explicit
check at the beginning of the verify_set_off method (before the VDA loss check)
that rejects any non-empty gains dictionary. If gains is provided and contains
entries, return a TaxResult with verified=False and a message indicating that
inter-head adjustment verification is not yet implemented. This prevents false
positives for validation scenarios the guard does not currently handle.
In `@qwed_tax/jurisdictions/india/guards/gst_guard.py`:
- Around line 157-159: The equality comparison in the verification statement
does not perform strict type checking, allowing Python's truthy behavior to
treat integers like 1 and 0 as equivalent to boolean True and False values. Add
explicit type validation using isinstance(claimed_is_rcm, bool) to ensure
claimed_is_rcm is actually a boolean before performing the equality comparison
with is_rcm. This prevents false verification when claimed_is_rcm receives
integer values from JSON deserialization or other dynamic input sources despite
the Optional[bool] type hint.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a25b5b1e-ab65-4afa-bc46-befee178ddfa
⛔ Files ignored due to path filters (2)
qwed_tax/__pycache__/__init__.cpython-311.pycis excluded by!**/*.pycqwed_tax/__pycache__/models.cpython-311.pycis excluded by!**/*.pyc
📒 Files selected for processing (7)
qwed_tax/guards/capital_gains_guard.pyqwed_tax/guards/classification_guard.pyqwed_tax/guards/speculation_guard.pyqwed_tax/jurisdictions/india/guards/crypto_guard.pyqwed_tax/jurisdictions/india/guards/gst_guard.pytests/test_guards_coverage.pytests/test_rcm.py
🚧 Files skipped from review as they are similar to previous changes (3)
- qwed_tax/guards/classification_guard.py
- qwed_tax/guards/capital_gains_guard.py
- qwed_tax/guards/speculation_guard.py
|
|
Docs PR opened: QWED-AI/docs#220 Added a changelog entry for QWED-Tax PR #42 covering fail-closed hardening across six guards for ambiguous classifications and unverified claims. |
|
Docs PR opened: QWED-AI/docs#221 Documented fail-closed behavior across six tax guards and the new claim-comparison mode for GST RCM verification. |
This was referenced Jun 21, 2026
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
Fixes #17 (guessing classifications) and #18 (success without comparing claim) across 6 guards.
#17 — Guessing classifications instead of blocking
determine_term"ERROR_DATE_FORMAT"sentinel → flows toverified=TrueValueError— caller catches and blocksdetermine_termValueError— known types onlyverify_tax_rateSLABverified=Trueignoringclaimed_rateentirelyverified=False— slab rates can't be proven without income bracketverify_worker_statusNone→verified=Falsewith "ambiguous classification" error. Contractor only when NO employee indicators presentverify_setoff"intraday" in source— unrecognized = non-speculativeintraday,f&o,futures,options,delivery,business,capital_gains) — unknown sources blockedverify_setoffverify_rcm_applicabilityOTHER, unknown entity toINDIVIDUAL— could suppress statutory RCM_try_coercereturnsNone→verified=Falsewith error naming the unrecognized value#18 — Success without comparing claim to computed truth
verify_rcm_applicabilityverified=True— calculator masquerading as verifierclaimed_is_rcmparameter — when provided, compares computedis_rcmagainst claim, returnsverified=Trueonly on exact match. When omitted, returnscomputed_only=Trueflag (backward compatible — separates calculation from verification)verify_flat_tax_ratevda_income <= 0→verified=Trueignoringclaimed_tax. Negative income (loss) treated as "no income"vda_income == 0→ verifiesclaimed_tax == 0.vda_income < 0→verified=False(loss, not "no income" — route to set-off/lapse logic)Caller updates
verifier.py_check_capital_gains— now catchesValueErrorfromdetermine_termand produces a block with the error messageTest changes
test_rcm.py::test_unknown_string_service_is_forward_charge→test_unknown_string_service_fail_closed— assertsverified=Falsefor unknown service (was assertingis_rcm=False, enforcing the fail-open behavior)Cleanup
setoff_guard.py(List,Dict) andcrypto_guard.py(List,Optional)crypto_guard.pyverify_set_off(total_gains,non_vda_losses)Test results
115 tests pass, 0 regressions. Ruff lint clean.
Summary by CodeRabbit
Bug Fixes
Refactor
Tests