feat: implement Phase B of TOP_01_PLAN — financial math verification - #324
Merged
Conversation
…r function B-1: adds ref_irr (bisection, no numpy dependency) to finance_reference.py. B-2: expands NOI/cap-rate/CoC/DSCR/one-percent-rule/GRM case lists to 50+ rows each and adds a new IRR_CASES block; fixes ref_one_percent_rule and ref_gross_rent_multiplier to raise ValueError matching production's contract on zero/negative input, so those edge cases can't silently diverge from production behavior.
…ctions B-4: noi, cap_rate, cash_on_cash, dscr, and irr in investor_app/finance/utils.py gain full formula + derivation docstrings, following the Args/Returns/Raises style already used by one_percent_rule and gross_rent_multiplier. Those two also gain a one-line derivation note. Docstrings only, no function bodies changed.
…ap_rate UnderwritingInput/UnderwritingMetrics now use Decimal fields instead of float, closing an AGENTS.md Never-Do violation (finance math with float persistence, outside services/utils). The local cap_rate() duplicate is deleted; the module now imports cap_rate/cash_on_cash/to_decimal from investor_app.finance.utils. cash_on_cash_yield() keeps its distinct all-cash-yield semantics but delegates its division to the canonical cash_on_cash() instead of reimplementing it. solve_underwriting()'s target_cap_rate widens to Decimal | float so existing bare-float callers keep working via to_decimal() coercion. orchestrator.py's UnderwritingInput construction boundary (tax/insurance defaults) moves to Decimal arithmetic. Downstream test files are updated for Decimal/float interop gaps found during the fix: pytest.approx comparisons against a Decimal actual need float(...)-wrapping to avoid TypeError on near-matches, Decimal * float arithmetic in derived test assertions needs the same treatment, and bare int/float literals passed to UnderwritingInput's now-Decimal fields need explicit Decimal(...) wrapping to satisfy mypy (pydantic coerces them fine at runtime, but the pydantic mypy plugin isn't enabled in this repo's mypy.ini).
LIMIT-20: bare-function vs. calculate_* contract divergence for the same formulas in investor_app/finance/utils.py, plus a duplicate score_listing_v2 implementation. LIMIT-21: prei/pipeline/handlers/offer.py remains float-based currency. Both found during Phase B implementation but deliberately out of scope for this PR.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
paruff
added a commit
that referenced
this pull request
Jul 28, 2026
PR #324 merged without archiving Phase B's active spec/design/tasks into features/<slug>/, per this repo's own convention. Preserving them here before feat/top01-phase-c merges main and overwrites the root files with Phase C's content.
paruff
added a commit
that referenced
this pull request
Jul 28, 2026
…/top01-phase-c PR #324 (Phase B) merged into main after this branch diverged, causing real conflicts in the root ACTIVE spec/design/tasks files plus docs/KNOWN_LIMITATIONS.md and features/top01-phase-a/specification.md. Resolution: root files take Phase C's content (the active feature on this branch, with Phase B now archived under features/top01-phase-b/), KNOWN_LIMITATIONS.md keeps both Phase B's and Phase C's entries, and the Phase A archive takes the version with the MERGED status note.
paruff
added a commit
that referenced
this pull request
Jul 28, 2026
) * feat(ci): add idempotent seed_zap_scan_user management command Seeds a low-privilege user from ZAP_AUTH_USERNAME/ZAP_AUTH_PASSWORD env vars for authenticated OWASP ZAP scans against an ephemeral CI instance (Phase C, C-2). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * feat(ci): add authenticated OWASP ZAP scan against ephemeral CI instance Adds a zap-authenticated-scan PR-gate job: migrates a throwaway SQLite DB, seeds a low-privilege scan account, starts a local runserver, and runs zap-full-scan.py with a new auth context file so the scan reaches pages behind /accounts/login/. Wired into the pr-gates-pass summary gate. post-deployment.yml's existing unauthenticated scan against the live artifact is unchanged (defense in depth). Documents the scope decision as LIMIT-22: authenticating against the real deployment would need deploy-pipeline access this repo doesn't have, same category as the deferred C-1/C-3 infra gaps. Phase C (C-2) of docs/TOP_01_PLAN.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * feat(test): add flaky test detection, quarantine, and ledger Adds pytest-reportlog + --report-log to pytest.ini so reruns are visible as distinct report-log entries; .github/scripts/flaky_report.py detects rerun-then-pass nodeids and maintains docs/quality/flaky_tests.json (cumulative counts) plus tests/.flaky_quarantine.txt (nodeids at or above the quarantine threshold). A new conftest.py pytest_collection_modifyitems hook marks quarantined nodeids xfail(strict=False) so a known-flaky test can't block a build while it's being fixed. Wiring into CI jobs (report on every PR, write only from the main-branch job) follows in the next commit. Phase C (C-4) of docs/TOP_01_PLAN.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * feat(ci): wire flaky test reporting into PR and main-branch jobs ci-quality.yml: tests-unit/integration/e2e each run flaky_report.py --mode report after pytest (using the report-log pytest.ini already produces) and upload it as an artifact for inspection. Report mode never touches the ledger, so PRs can't race each other or fork PRs need write access. docker-publish.yml: the live-test job (push-to-main only) extracts the report log from the running container, runs flaky_report.py --mode write to update docs/quality/flaky_tests.json and tests/.flaky_quarantine.txt, and bot-commits any change back to main as github-actions[bot] with [skip ci]. This is the single writer for the shared ledger. Needs contents: write, added to the job's permissions. Phase C (C-4) of docs/TOP_01_PLAN.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(feature-flow): archive Phase A, regenerate root spec for Phase C Phase A (docs/TOP_01_PLAN.md) merged via PR #323 but its spec/design/ tasks were never archived per features/README.md's convention; move them to features/top01-phase-a/ and mark MERGED. Regenerate root specification.md/design.md/tasks.json for this PR's scope: Phase C, C-2 (authenticated ZAP scan) + C-4 (flaky test detection/quarantine) only. C-1 and C-3 remain explicitly deferred. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(ci): pin zaproxy/action-full-scan to v0.13.0, stop hardcoding ZAP scan credentials zaproxy/action-full-scan has no v1 tag (latest is v0.13.0) — the zap-authenticated-scan job in ci-quality.yml and the never-yet-run security job in post-deployment.yml both referenced the nonexistent @v1. GitGuardian also flagged a hardcoded username/password pair in ci-quality.yml (the ZAP scan-only account credentials, also embedded as a base64 blob in .zap/prei-auth-context.xml). The password is now generated per-run with secrets.token_urlsafe and the auth context XML is rendered from a __ZAP_AUTH_CREDS_B64__ placeholder at job runtime, so no credential value is ever committed to git history. * fix(ci): correct ZAP auth-context parser config and user auth-method id The urlparser/postparser <config> fields need JSON ({"kvps":"&","kvs":"=","struct":[]}) per ZAP's StandardParameterParser, not a bare "&=" string — the bad format threw a JSONException during context import that cascaded into an NPE decoding the <user> entry (auth-method-type registry never got populated). Also fixes the <user> line's auth-method-type id (1 -> 2) to match <authentication><type>2</type>. * chore(docs): trim stale stack line and extract GitOps principles skill AGENTS.md claimed Django 5.2 but requirements.txt pins 6.0.7 — point at the manifest instead of a version string that drifts. Move the 10-item GitOps Principles section to a lazy-loaded skill so it's only in context when touching workflows/deployment config, not on every request. * ci: retrigger PR checks * docs(phase-b): archive spec/design/tasks before phase C merge PR #324 merged without archiving Phase B's active spec/design/tasks into features/<slug>/, per this repo's own convention. Preserving them here before feat/top01-phase-c merges main and overwrites the root files with Phase C's content. * fix(ci): add missing basic block to ZAP context authorization The authenticated ZAP scan job (zap-authenticated-scan) failed on its first run: ZAP's context import throws a NullPointerException in BasicAuthorizationDetectionMethod when the <authorization> element's <type>0</type> has no accompanying <basic> block, because the config-based constructor calls LogicalOperator.valueOf() on the (missing) logic value with no null-guard. Docker then exits 3 and the scan never runs. Add the <basic> sub-block (empty header/body regex, AND, code -1) that ZAP always expects when an <authorization> element is present, even though this context doesn't use authorization detection. * fix(ci): base64-encode the ZAP context user name The prior fix resolved the context-import NullPointerException, but the authenticated scan still failed: "ZAP failed to find user: zap-ci-scan-only". ZAP's User.encode()/decode() format is id;enabled;base64(name);authTypeId;credentials - the name field must be base64, and our <user> line had it in plaintext. On import ZAP base64-decodes "zap-ci-scan-only" into garbage, so the -U flag's exact-name lookup in zap_set_scan_user() never matches. Encode the name (zap-ci-scan-only -> emFwLWNpLXNjYW4tb25seQ==) and add a comment documenting the field format so this isn't rediscovered by hand next time the context file is edited. * fix(ci): don't block PRs on pre-existing ZAP WARN findings Both blockers preventing the authenticated scan from running at all are now fixed (missing <basic> auth block, plaintext username needing base64). With auth working, ZAP reached pages behind /accounts/login/ for the first time and surfaced 10 WARN-level findings (missing security headers, non-HttpOnly cookie, insecure session ID transmission, missing SRI, etc.) - 0 FAIL-level alerts. zap-full-scan.py exits non-zero on any WARN by default, which would block every future PR on pre-existing app gaps unrelated to their changes. Add -I so the gate only fails on FAIL-level alerts; WARN findings still show up in the job's ZAP log for visibility. Document the deferred findings as LIMIT-23 with a recommended follow-up hardening PR. --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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
tests/test_finance_math.py's edge-case coverage to 50+ cases per function (546 total);ci-quality.yml's existingfinance-mathjob automatically gates on the expansion.noi/cap_rate/cash_on_cash/dscr/irrininvestor_app/finance/utils.py.prei/pipeline/handlers/underwriting.pyfrom float to Decimal and removes its duplicatecap_rate()in favor of the canonicalinvestor_app.finance.utilsimplementation — closes anAGENTS.md"Never Do" violation (finance math outside services/utils, float currency persistence).docs/KNOWN_LIMITATIONS.mdentries (LIMIT-20, LIMIT-21) for issues found but deliberately not fixed in this PR.specification.md/design.md/tasks.jsontofeatures/top01-phase-a/(was merged but never archived) and regenerates the root files for Phase B, perAGENTS.md's ephemeral-file convention.Investigation found Phase B partially done already by a prior actor (B-3 was already wired); this PR completes B-1, B-2, B-4, and the bundled underwriting.py fix.
Test plan
pytest tests/test_finance_math.py -v --tb=short— 546 passedpytest prei/pipeline/tests/test_underwriting.py tests/test_underwriting_integration.py tests/test_offer_integration.py prei/pipeline/tests/test_orchestrator.py -q— 58 passedmypy core/ investor_app/finance/andmypy prei/pipeline/handlers/underwriting.py prei/pipeline/orchestrator.py— cleanpytest tests_bdd/ core/tests/ prei/pipeline/tests/ -q— 1157 passed, 1 skipped, 4 failed (pre-existing: live ATTOM API 401s incore/tests/test_integration_attom.py, unrelated to this PR, normally excluded bypytest.ini's default filter)🤖 Generated with Claude Code