fix(parser): raise InvalidTaxCodeError when birthplace not found#72
fix(parser): raise InvalidTaxCodeError when birthplace not found#72matteoredz wants to merge 1 commit into
Conversation
…ities or countries Previously the birthplace method returned nil implicitly when the stop: true recursive call found nothing, masking invalid tax codes silently. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR tightens birthplace validation in the tax code parser: the ChangesBirthplace Validation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
AGENTS.md (1)
1-181:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the required AGENTS.md sections for agent contracts and lifecycle.
This file still does not document agent responsibilities/capabilities/interaction patterns, clear agent interfaces/contracts, and lifecycle/state handling as required.
As per coding guidelines: "Document agent responsibilities, capabilities, and interaction patterns in AGENTS.md", "Maintain clear agent interface definitions and API contracts in AGENTS.md", and "Include agent lifecycle management and state handling documentation in AGENTS.md".
🤖 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 `@AGENTS.md` around lines 1 - 181, Add new sections to AGENTS.md titled "Agent Responsibilities", "Agent Interfaces & Contracts", "Agent Lifecycle & State Management", and "Interaction Patterns" that explicitly describe each agent's responsibilities and capabilities, define clear interface contracts (inputs, outputs, message/event names, error behaviors and retry semantics) for every agent referenced in the repo, and list concrete lifecycle hooks/state transitions (init/start/ready/running/paused/failed/terminated) including allowed transitions, persisted state keys, recovery/retry policies and timeouts; include examples of API/message payload shapes, expected return values/errors, and monitoring/observability hooks (metrics, logs, health checks) so implementers can wire agents to the system correctly.
🤖 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 `@AGENTS.md`:
- Line 81: Update the Error Hierarchy documentation to reflect the expanded
behavior of InvalidTaxCodeError: mention both the existing length/format
validation failure and the new unknown birthplace lookup failure (e.g., when
birthplace CSV lookup fails for "birthplace"). Locate the Error Hierarchy
section and add a sentence alongside the InvalidTaxCodeError entry indicating it
is raised for invalid length/format AND when birthplace lookup is not found,
matching the decode section wording for consistency.
In `@test/itax_code/parser_test.rb`:
- Around line 32-33: This test relies on Date.today during
klass.new("BRRDRN70M41ZXXXE").decode so make it deterministic by wrapping the
assertion in a Timecop.freeze block (choose a fixed Date, e.g. a specific
YYYY,MM,DD) around the assert_raises call; locate the test case that calls
klass.new(...).decode and change it to use Timecop.freeze (block form) so the
date-dependent decode path is stable.
---
Outside diff comments:
In `@AGENTS.md`:
- Around line 1-181: Add new sections to AGENTS.md titled "Agent
Responsibilities", "Agent Interfaces & Contracts", "Agent Lifecycle & State
Management", and "Interaction Patterns" that explicitly describe each agent's
responsibilities and capabilities, define clear interface contracts (inputs,
outputs, message/event names, error behaviors and retry semantics) for every
agent referenced in the repo, and list concrete lifecycle hooks/state
transitions (init/start/ready/running/paused/failed/terminated) including
allowed transitions, persisted state keys, recovery/retry policies and timeouts;
include examples of API/message payload shapes, expected return values/errors,
and monitoring/observability hooks (metrics, logs, health checks) so
implementers can wire agents to the system correctly.
🪄 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: fa6ce204-cee9-42a3-a180-2ffc19bea5e3
📒 Files selected for processing (3)
AGENTS.mdlib/itax_code/parser.rbtest/itax_code/parser_test.rb
| # gender: "M" | "F", | ||
| # birthdate: String, # "YYYY-MM-DD" | ||
| # birthplace: { # nil if not found in either CSV | ||
| # birthplace: { # raises InvalidTaxCodeError if not found in either CSV |
There was a problem hiding this comment.
InvalidTaxCodeError documentation is now incomplete across sections.
The decode section was updated, but the Error Hierarchy still describes InvalidTaxCodeError only as a length error; it should also mention unknown birthplace lookup failure for consistency.
Based on learnings: "Whenever you modify code, check whether the change affects any section of this file ... and update the relevant section in the same commit."
🤖 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 `@AGENTS.md` at line 81, Update the Error Hierarchy documentation to reflect
the expanded behavior of InvalidTaxCodeError: mention both the existing
length/format validation failure and the new unknown birthplace lookup failure
(e.g., when birthplace CSV lookup fails for "birthplace"). Locate the Error
Hierarchy section and add a sentence alongside the InvalidTaxCodeError entry
indicating it is raised for invalid length/format AND when birthplace lookup is
not found, matching the decode section wording for consistency.
| test "raises InvalidTaxCodeError when birthplace code is not found in cities or countries" do | ||
| assert_raises(klass::InvalidTaxCodeError) { klass.new("BRRDRN70M41ZXXXE").decode } |
There was a problem hiding this comment.
Freeze time in this new decode test to keep it deterministic.
This assertion exercises date-dependent decode logic (Date.today path in parser), so it should be wrapped in Timecop.freeze to prevent future time-based flakes.
As per coding guidelines: "Use Timecop.freeze for any date-sensitive tests to control time".
🤖 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 `@test/itax_code/parser_test.rb` around lines 32 - 33, This test relies on
Date.today during klass.new("BRRDRN70M41ZXXXE").decode so make it deterministic
by wrapping the assertion in a Timecop.freeze block (choose a fixed Date, e.g. a
specific YYYY,MM,DD) around the assert_raises call; locate the test case that
calls klass.new(...).decode and change it to use Timecop.freeze (block form) so
the date-dependent decode path is stable.
1 participant
Summary
Parser#birthplacesilently returnednilwhen neither cities nor countries contained the decoded birthplace code. Thestop: truebranch fell off the end without an explicit return or raise.raise InvalidTaxCodeErrorafter the recursive countries fallback fails.assert_raisesand removed the FIXME comment.AGENTS.mdto document the new behaviour (raises InvalidTaxCodeErrorinstead ofnil).Test plan
bundle exec rakegreen with 100% line + branch coveragebundle exec rubocopclean on changed filestest/coverage-gaps(that branch's T4 test depends on this fix)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation