fix(parsers): reject boolean values in accounting float coercion

[stranske]

Summary

src/counter_risk/parsers/_xlsx_reader.py coerce_accounting_float matched booleans on its isinstance(value, (int, float)) branch, because bool is a subclass of int. coerce_accounting_float(True) returned 1.0 and coerce_accounting_float(False) returned 0.0, so an accidental boolean flag in a workbook cell was read as a dollar/notional amount instead of being rejected.

This adds an explicit isinstance(value, bool) guard at the top of the function that raises ValueError, and covers it with unit tests.

Closes #1048

Tasks

• In src/counter_risk/parsers/_xlsx_reader.py, add explicit boolean rejection to coerce_accounting_float — guard added at the top of the function (_xlsx_reader.py:174), raising ValueError("Boolean value is not a valid accounting number: ...") before the int/float branch is reached.
• In tests/parsers/test_numeric.py, add unit tests verifying coerce_accounting_float(True) and coerce_accounting_float(False) raise ValueError — test_coerce_accounting_float_rejects_booleans, plus test_coerce_accounting_float_rejects_booleans_under_strip_percent_false (the guard must not depend on the strip_percent branch) and test_coerce_accounting_float_still_accepts_numeric_zero_and_one (non-regression for the int/float path the guard sits in front of).

Acceptance Criteria

• uv run pytest tests/parsers/test_numeric.py -q passes; boolean values raise ValueError on coercion — 12 passed in 0.18s (run as python -m pytest tests/parsers/test_numeric.py -q against the repo's Python 3.12 environment with PYTHONPATH=src; module resolution verified to this worktree).

• Direct inspection confirms coerce_accounting_float(True) raises ValueError:

  ValueError for True -> Boolean value is not a valid accounting number: True
  ValueError for False -> Boolean value is not a valid accounting number: False
  numeric 1 -> 1.0 | numeric 0 -> 0.0
  

• Deliberate-break gate: removed the boolean check from coerce_accounting_float, reran the suite, restored, reran.

  state	result
  guard removed (deliberate break)	2 failed, 10 passed — test_coerce_accounting_float_rejects_booleans and test_coerce_accounting_float_rejects_booleans_under_strip_percent_false both Failed: DID NOT RAISE ValueError
  guard restored	12 passed

  The gate is therefore non-vacuous: it fails when the behaviour is absent.

Regression / quality checks

• pytest tests/parsers -q — 86 passed in 111.14s (whole parsers suite, no regression).
• black --check on both changed files — unchanged.
• ruff check on both changed files — all checks passed.
• mypy src/counter_risk/parsers/_xlsx_reader.py — Success: no issues found in 1 source file.

Non-Goals

No changes to regex-based accounting string parsing. XLSX t="b" cells are unaffected: cell_value already decodes them to the strings "TRUE"/"FALSE", which continue to take the existing string path. This PR only closes the Python-level bool hole in front of the numeric branch.

🤖 Generated with Claude Code

Source: Issue #1048

Closes #1048

Automated Status Summary

Scope

src/counter_risk/parsers/_xlsx_reader.py:171 implements coerce_accounting_float(value). In Python, isinstance(True, (int, float)) evaluates to True. Because coerce_accounting_float lacks an explicit isinstance(value, bool) check before checking isinstance(value, (int, float)), boolean values are converted to numeric floats (coerce_accounting_float(True) == 1.0, coerce_accounting_float(False) == 0.0) instead of raising ValueError. This causes accidental boolean flags in Excel workbook cells to be parsed as numerical dollar/notional amounts.

Tasks

• In src/counter_risk/parsers/_xlsx_reader.py, add explicit boolean rejection to coerce_accounting_float.
• In tests/parsers/test_numeric.py, add unit tests verifying coerce_accounting_float(True) and coerce_accounting_float(False) raise ValueError.

Acceptance criteria

• uv run pytest tests/parsers/test_numeric.py -q passes; boolean values raise ValueError on coercion.
• Direct inspection confirms coerce_accounting_float(True) raises ValueError.
• Deliberate-break gate: deliberately break coerce_accounting_float in src/counter_risk/parsers/_xlsx_reader.py by removing the boolean check; boolean test must fail; restore and rerun.

Summary by CodeRabbit

• Bug Fixes

  • Boolean values are no longer interpreted as numeric values when processing accounting data.
  • Numeric zero and one values continue to be accepted correctly.

• Tests

  • Added coverage confirming consistent validation with and without percentage stripping.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: eef37ed7-38aa-4092-999a-a9918efd7003

📥 Commits

Reviewing files that changed from the base of the PR and between 6bebc39 and 1796f8d.

📒 Files selected for processing (2)

• src/counter_risk/parsers/_xlsx_reader.py
• tests/parsers/test_numeric.py

Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

---

📝 Walkthrough

Walkthrough

The XLSX accounting float coercion now rejects boolean inputs before numeric conversion. Tests cover both strip_percent modes and confirm that integer and float zero and one values remain valid.

Changes

Accounting float validation

Layer / File(s)	Summary
Boolean rejection and regression tests src/counter_risk/parsers/_xlsx_reader.py, tests/parsers/test_numeric.py	coerce_accounting_float raises ValueError for boolean values. Tests cover both strip_percent settings and preserve acceptance of numeric zero and one values.

Priority: ➖ Normal

Estimated code review effort: 1 (Trivial) | ~5 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 1796f

This change rejects boolean accounting values while preserving valid numeric zero and one handling, with regression coverage in place.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: rejecting boolean values during accounting float coercion.
Linked Issues check	✅ Passed	Issue #1048 requires an explicit boolean rejection and unit tests. The PR adds an isinstance(value, bool) guard at the start of coerce_accounting_float that raises ValueError. Tests cover True…
Out of Scope Changes check	✅ Passed	The changes remain within issue #1048. The source change, its docstring update, and the focused unit tests directly support boolean rejection and regression coverage. No unrelated implementation or te…
Docstring Coverage	✅ Passed	Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/issue-1048-xlsx-bool-reject

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Workflow state fingerprint for Keepalive Loop Reporter. Do not edit.

[stranske-keepalive]

🤖 Keepalive Loop Status

PR #1055 | Agent: Claude | Iteration 1/12

Current State

Metric	Value
Iteration progress	[#---------] 1/12
Action	review (progress-review-4)
Gate	success
Tasks	0/5 complete
Timeout	45 min (default)
Timeout usage	6m elapsed (15%, 39m remaining)
Keepalive	✅ enabled
Autofix	❌ disabled

🔍 Failure Classification

| Error type | infrastructure |
| Error category | unknown |
| Suggested recovery | Capture logs and context; retry once and escalate if the issue persists. |

[stranske-keepalive]

Keepalive Work Log (click to expand)

#	Time (UTC)	Agent	Action	Result	Files	Tasks	Progress	Commit	Gate
0	2026-09-12 14:53:46	Claude	wait (gate-not-success)	skipped	—	0	0/5	—	—
1	2026-09-12 14:56:42	Claude	run (bypass-rate-limit-gate)	success	—	0	0/5	—	cancelled
1	2026-09-12 14:57:28	Claude	skip (needs-human)	skipped	—	0	0/5	—	cancelled
1	2026-09-12 15:03:37	Claude	run (agent-run-skipped)	skipped	—	0	0/5	—	success
1	2026-09-12 15:06:56	Claude	wait (gate-pending-transient)	skipped	—	0	0/5	—	—
1	2026-09-12 15:13:03	Claude	review (progress-review-4)	skipped	—	0	0/5	—	success

[github-actions]

Workflow state fingerprint for Agents Gate Followups. Do not edit.

[Copilot]

🟢 Approval recommended

The fix and regression coverage address the issue with no unresolved comments.

Pull request overview

This pull request prevents Python booleans from being coerced into accounting amounts.

Changes:

• Rejects booleans with ValueError.
• Adds regression tests for booleans and numeric 0/1.

File summaries

File	Summary
tests/parsers/test_numeric.py	Tests boolean rejection and numeric compatibility.
src/counter_risk/parsers/_xlsx_reader.py	Adds the boolean validation guard.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[github-actions]

Runner dispatch state for claude on PR #1055. Do not edit.

[stranske]

Autofix attempts exhausted for this head.
Attempts: 4 / 3

Latest Gate summary:

Gate run: https://github.com/stranske/Counter_Risk/actions/runs/34700547701
Conclusion: cancelled
PR: #1055
Head SHA: 1796f8dbf4a36cd68c9557415ff83c4bb9cb5ee2
Autofix attempts for this head: 4 / 3
Fix scope: src/, tests/, tools/, scripts/, agents/, templates/, .github/
Failing jobs:
- classify changed paths (cancelled)
  - steps: Classify changed paths (cancelled)

Please investigate manually.

[stranske]

Autofix attempts exhausted for this head.
Attempts: 5 / 3

Latest Gate summary:

Gate run: https://github.com/stranske/Counter_Risk/actions/runs/34700554188
Conclusion: cancelled
PR: #1055
Head SHA: 1796f8dbf4a36cd68c9557415ff83c4bb9cb5ee2
Autofix attempts for this head: 5 / 3
Fix scope: src/, tests/, tools/, scripts/, agents/, templates/, .github/
Failing jobs:
- Python CI / python 3.12 (cancelled)
  - steps: Pytest (unit tests with coverage) (cancelled); Finalize check results (failure)
- Python CI / python 3.13 (cancelled)
  - steps: Pytest (unit tests with coverage) (cancelled); Finalize check results (failure)

Please investigate manually.

[stranske]

Closer disposition: removing needs-human — it is autofix-loop debris, not a human decision.

Evidence (head 1796f8dbf4a36cd68c9557415ff83c4bb9cb5ee2, the only commit on this PR):

The needs-human label was applied at 2026-09-12T14:54:16Z by the "Autofix attempts exhausted" path, reporting Attempts: 4 / 3 against a head that was 2 minutes 24 seconds old. Its quoted Gate summary says Conclusion: cancelled with the sole failing job listed as classify changed paths (cancelled).

Cancelled is not failed. Enumerating every run on this exact head shows duplicate dispatches colliding in their concurrency groups:

• Gate dispatched three times on the same SHA — 34700552400 (cancelled), 34700554188 (cancelled), 34700635780 (in progress right now).
• Autofix dispatched three times — 34700552589, 34700554157, 34700635887, all cancelled.
• Agents PR Event Hub dispatched eight times, Agents Gate Followups four times; most cancelled by their own successors.

Within 34700554188 the Python CI legs did not fail either — python 3.12 and python 3.13 both ended cancelled, after running clean to ~59% (gh pr checks renders a cancelled conclusion as fail, which is what makes this read like a red PR). No assertion failure, lint error or type error appears in any log on this head; lint-ruff, typecheck-mypy, Validate inputs and classify changed paths all concluded success.

So the retry counter tallied four "attempts" that were self-inflicted concurrency cancellations, exhausted its budget of 3, and asked for a human — two seconds before a fresh Gate (34700635780) started on the same unchanged head. Nothing here states a product, design or access decision, which is what needs-human is for.

What automation checked: every workflow run for this SHA and its conclusion; the job-level conclusions inside the cancelled Gate run; the full job log for python 3.13; the label timeline; the keepalive work log (iteration 1/12, last action wait with reason gate-not-success); the CodeRabbit review (no actionable comments); and PR mergeability (MERGEABLE).

Action taken: needs-human removed. Routing (agent:claude), agents:keepalive and autofix are correct and untouched — exactly one routing label, no agent:auto co-presence. No branch, source or CI change was made; the live Gate 34700635780 is left to finish on its own.

Left for the normal pipeline: if 34700635780 goes green, ordinary closer merge gates apply on the next round (exact head unchanged, seven-minute review floor, zero active review threads, every normally-reporting check actually reported). If it reports a real failure, keepalive owns the next iteration on its own budget. A human is needed only if a genuine failure then survives that.

Underlying defect, recorded rather than fixed here (out of this lane's scope): a PR that triggers duplicate same-head dispatches can exhaust its autofix budget purely on concurrency cancellations and self-apply needs-human before its first real Gate ever runs. The counter should discount cancelled conclusions, or not count an attempt whose Gate never reached a terminal non-cancelled state — otherwise the label latches on evidence that can never turn green, because a cancelled run is not re-run by anything.

[stranske]

Pre-merge absent-check audit (mandatory rule), head 1796f8dbf4a36cd68c9557415ff83c4bb9cb5ee2.

The prescribed reporter could not be used: ~/.codex/orchestrator-mirror/check_checks_reported.py does not exist at that path (the script lives at scripts/check_checks_reported.py) and it hardcodes REPO = "stranske/Orchestrator" at line 44, so it cannot audit this repo. Per the rule, a missing reporter is treated as "unknown, do not merge on green alone", so the inventory was audited by hand against a reference PR in this same repo: #1052, merged earlier today.

Result: no check that normally reports is absent on this head. #1055's check inventory is a structural superset of #1052's. Every job present-and-reporting there is present here, plus conformance / Backplane run-contract conformance, emit-reference-run, classify changed paths, gate, gate-summary, guard and check. Where #1052 shows a flat parent name (Fetch PR context, PR meta handler) as SKIPPED, this head shows the nested child jobs reporting SUCCESS instead — more reporting, not less.

The Gate and the full Python CI matrix both reported SUCCESS on this exact unchanged head via Gate run 34700635780: Gate / gate, Python CI / python 3.12, Python CI / python 3.13, Python CI / lint-ruff, Python CI / typecheck-mypy, Python CI / Validate inputs, Python CI / select reusable CI scope, Python CI / logs summary, Health 45 Agents Guard / guard, conformance, emit-reference-run, CodeRabbit.

Two names on this head carry a cancelled conclusion with no successful twin: Resolve Context and Record autofix dispatch completion. Both belong to the Autofix workflow — the workflow whose three duplicate same-head dispatches caused the concurrency pile-up documented above. They are that workflow's completion bookkeeping, not correctness gates; the branch has no protection rules and the repository has no rulesets, so nothing here is a required check. Autofix had nothing to repair in any case, since the Gate is green. Recording the reason explicitly, as the rule requires, rather than merging on the strength of a green list.

Remaining merge gates, all satisfied on this exact head: non-draft; in-scope agent issue work (agent:claude, branch claude/issue-1048-xlsx-bool-reject, body closes #1048); CLEAN/MERGEABLE; zero review threads total and zero active (GraphQL, no further pages); Copilot review is COMMENTED, not changes-requested; no auto-merge armed; the seven-minute post-push review floor from the 14:51:52Z commit elapsed well before this audit and the head has not changed since.

The full diff was read rather than trusted from the body: +8/-0 in src/counter_risk/parsers/_xlsx_reader.py (docstring plus an isinstance(value, bool) guard raising ValueError ahead of the int/float branch) and +25/-0 in tests/parsers/test_numeric.py (three tests: boolean rejection, boolean rejection under strip_percent=False, and a non-regression check that numeric 0/1 still coerce). No out-of-scope paths. This matches #1048's acceptance criteria exactly, and the PR body carries a non-vacuous deliberate-break demonstration — guard removed gives 2 failed / 10 passed, guard restored gives 12 passed.

One note for whoever reads the keepalive summary: its "Tasks 0/5" figure is a counting artifact, not incomplete work. It counts the checkboxes in the generated auto-status-summary block, which is a stale unchecked duplicate of the task list; the author's own Tasks and Acceptance Criteria sections are fully checked with evidence attached. Merging also stops keepalive from spending its remaining 11 iterations on an already-complete PR.

Sequencing note: this audit was completed and the head re-read as 1796f8dbf4a36cd68c9557415ff83c4bb9cb5ee2 before the merge fired, but the command that was supposed to post this record ahead of the merge failed on a shell filename mismatch and the merge in the same script proceeded anyway. The record is therefore being posted immediately after the merge rather than immediately before it. Nothing in the audit changed: the PR merged at the exact audited head via --match-head-commit, squashed to 86fd2e0e1f77bb34297c488d98bce48dbd791bcc. Flagging the ordering slip explicitly rather than letting the timestamp imply a pre-merge post.

[github-actions]

Provider Comparison Report

Provider Summary

Provider	Model	Verdict	Confidence	Summary
openai	gpt-5.6-terra	PASS	99%	The implementation explicitly rejects bool values before the int/float isinstance path, so True and False now raise ValueError rather than coercing to 1.0 and 0.0. The guard is correctly placed and...
anthropic	claude-sonnet-5	PASS	95%	The change adds an explicit isinstance(value, bool) check at the top of coerce_accounting_float, placed before the None check and any int/float coercion logic, ensuring booleans are rejected...

📋 Full Provider Details (click to expand)

openai

• Model: gpt-5.6-terra
• Verdict: PASS
• Confidence: 99%
• Scores:
  • Correctness: 10.0/10
  • Completeness: 10.0/10
  • Quality: 10.0/10
  • Testing: 10.0/10
  • Risks: 10.0/10
• Summary: The implementation explicitly rejects bool values before the int/float isinstance path, so True and False now raise ValueError rather than coercing to 1.0 and 0.0. The guard is correctly placed and does not affect valid numeric zero/one inputs. Tests cover both boolean values, the strip_percent=False path, and preservation of normal numeric behavior. Removing the new guard would cause the added boolean-rejection tests to fail, satisfying the deliberate-break intent. The change is focused, readable, and introduces no meaningful compatibility, security, or performance risk beyond the intended rejection of previously accepted invalid boolean inputs.

anthropic

• Model: claude-sonnet-5
• Verdict: PASS
• Confidence: 95%
• Scores:
  • Correctness: 10.0/10
  • Completeness: 9.0/10
  • Quality: 9.0/10
  • Testing: 9.0/10
  • Risks: 9.0/10
• Summary: The change adds an explicit isinstance(value, bool) check at the top of coerce_accounting_float, placed before the None check and any int/float coercion logic, ensuring booleans are rejected via ValueError regardless of the strip_percent flag. This directly and correctly addresses the bug where bool being a subclass of int allowed True/False to silently coerce to 1.0/0.0. The docstring is updated to explain the rationale, improving maintainability. Tests are thorough: they verify rejection of True/False under default and strip_percent=False conditions, and include a regression test confirming that legitimate numeric 0/1 (int and float) still coerce correctly, guarding against overly broad guards. This satisfies all stated acceptance criteria: the test suite will pass, direct inspection confirms the boolean guard exists and raises ValueError, and removing the guard would cause the added tests to fail (satisfying the deliberate-break gate in principle, though it's not automated in the diff itself). No security, performance, or compatibility risks are introduced by this small, well-scoped change.
• Concerns:
  • Deliberate-break gate is a manual verification step not encoded in CI, but this is a process check outside the code diff itself and doesn't affect the correctness of the implementation.
  • Docstring/tests reference a specific error message substring; future refactors changing the message would need corresponding test updates, but this is a minor coupling concern.

Agreement

• Verdict: PASS (all providers)
• Correctness: scores within 1 point (avg 10.0/10, range 10.0-10.0)
• Completeness: scores within 1 point (avg 9.5/10, range 9.0-10.0)
• Quality: scores within 1 point (avg 9.5/10, range 9.0-10.0)
• Testing: scores within 1 point (avg 9.5/10, range 9.0-10.0)
• Risks: scores within 1 point (avg 9.5/10, range 9.0-10.0)

Disagreement

No major disagreements detected.

Unique Insights

• openai: The implementation explicitly rejects bool values before the int/float isinstance path, so True and False now raise ValueError rather than coercing to 1.0 and 0.0. The guard is correctly placed and does not affect valid numeric zero/one inputs. Tests cover both boolean values, the strip_percent=F...
• anthropic: Deliberate-break gate is a manual verification step not encoded in CI, but this is a process check outside the code diff itself and doesn't affect the correctness of the implementation.; Docstring/tests reference a specific error message substring; future refactors changing the message would need corresponding test updates, but this is a minor coupling concern.

🔍 LangSmith Traces

• openai
• anthropic

[github-actions]

Workflow state fingerprint for Agents Verifier. Do not edit.