Sprint: option economics · the quality evidence chain · vendor memory · a clawback that admits it cannot tell - #110
Merged
Merged
Conversation
Strix Security ReviewNo security issues found. Updated for Reviewed by Strix |
PR Summary by QodoAdd option economics provenance, quality chain, vendor scorecards, clawback status
AI Description
Diagram
High-Level Assessment
Files changed (17)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
55 rules 1. Unknown equity reported zero
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+113
to
+117
| "basis": BASIS_DERIVED, "hard_cost": round(cost, 2) if cost is not None else None, | ||
| "irr_pct": irr_pct, | ||
| "equity": round(_num(su.get("equity")) or 0.0, 2), | ||
| "loan_amount": round(_num(su.get("loan_amount")) or 0.0, 2), | ||
| "equity_multiple": _num(ret.get("equity_multiple")), |
There was a problem hiding this comment.
5. Unknown equity reported zero 🐞 Bug ≡ Correctness
option_economics.option_economics() converts missing/unparseable derived equity and loan_amount into 0.0 using round(_num(...) or 0.0, 2), which can misrepresent unknown financing values as real zeros.
Agent Prompt
## Issue description
For derived scenarios, `equity` and `loan_amount` are computed as `round(_num(...) or 0.0, 2)`. Since `_num()` returns `None` for missing/unparseable values, this coerces unknowns to `0.0`, which is semantically different from “not provided”.
## Issue Context
This is inconsistent with how other derived fields preserve `None` when missing (e.g., `hard_cost`, `irr_pct`). It matters if older cached `Scenario.result` payloads are partial or if future solver changes omit fields.
## Fix Focus Areas
- services/api/src/aec_api/option_economics.py[107-119]
- services/api/src/aec_api/option_economics.py[59-65]
## Suggested fix
- Compute and round only when non-None:
- `eq = _num(su.get('equity'))` then `equity = round(eq, 2) if eq is not None else None`
- same for `loan_amount`
- Keep `0.0` only when it is explicitly present and parsed as numeric zero.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
…wback that admits it cannot tell Three R22 items, all Lane C engines plus their routes (Lane G) and one register field (Lane H). No version files. No module added — the register count stays 133/30. --- R22-OPTION-OBJECT ------------------------------------------------------------- `design_option` carried `hard_cost` and `irr_pct` as plain typed fields with ZERO reference to a deal, while proforma/solve.py sizes debt and returns.py computes the levered IRR right next door. A typed 19.7% and a solved 19.7% rendered identically on the card, and the typed one is the one that goes stale. `option_economics.py` gives every figure a basis: `derived` (the option names a scenario and solve() ran), `declared` (typed, used as given, LABELLED), `unlinked`, `unavailable`. `unlinked` is the one that matters. Pricing an option from the project's other scenario would assert that this massing was underwritten under that deal — provenance nobody recorded. It is refused and the candidate scenarios are named instead. A declared figure is never promoted to derived and a derived one never overwrites what was typed, so `declared_disagrees_with_derived` can exist: a scheme whose typed IRR sits 4 points above what its own scenario solves to is the most useful output here, and it is only visible because both numbers are kept. --- R22-ITP-NCR (2) --------------------------------------------------------------- The registers were never the gap and the entry's estimate was wrong in a specific direction. Verified by reading the files: `itp` carries hold/witness points (`point_type`, `acceptance_criteria`, planned->active->verified), `ncr` a real open->dispositioned->closed lifecycle, and quality.py rolls all three up. But zero GUID fields across all three modules, and turnover.py referenced none of them — so a project could be certified substantially complete with the quality evidence never attached to what it inspected. NO new field and NO migration. Records attach through `element_guids`, the column every module record already has, already routed as POST .../elements. `data.element` has one reader in the codebase; `element_guids` has the module engine. Adding a bespoke field would have created a second convention beside the platform's own. The rule it enforces: an element with no quality record is `unrecorded`, never `clear`. "0 open NCRs" across a building nobody inspected is literally true and completely misleading. `any_attached` and `coverage_pct` separate "this building has a problem" from "nobody has linked records yet". --- clawback status --------------------------------------------------------------- `lp_irr is None` means XIRR did not converge — 26% of shapes in a 729-pattern sweep — not "no restitution owed". That case silently returned the same payload as a healthy deal, so a GP kept money a clawback might have recovered. Now reported as `clawback.status` = applied / not_owed / unavailable / not_requested, with the reason. No fallback is picked, deliberately: inventing a restitution figure out of a failed solve is how a plausible dollar amount gets cited while a missing one gets chased. Whether a non-converging IRR is a zero-clawback case is a deal-terms decision. --- also ------------------------------------------------------------------------- test_proforma: the CPI-fallback forecast is value-checked, not just its label. The old assertion was `forecast_at_completion < 40_000_000`, an upper bound — flipping the branch's `max` to `min` collapsed 27,133,231.54 to 9,000,000 (spend-to-date, i.e. claiming a month-6-of-17 job is finished) and passed. Found by mutation testing. Rebased onto b44ddbf and re-applied on top of #108's field sweep rather than over it — `gross_area_sf` keeps `unit: sf`, `efficiency_pct` and `irr_pct` keep `type: percent`, asserted before the edit was written. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… them across projects Two of the entry's three parts already existed, verified by reading the files before any code was written: `prequalification.py` scores EMR / bonding / capacity into a traceable Q-score with COI-expiry tracking, and `clause_playbook.py` holds the clause-position playbook and its deviation register. The third did not. `prequalification.score_project(db, project_id)` takes ONE project. So a sub is prequalified on job B from the form they filled in themselves, with no reference to what they actually did on job A — while the history sits there, keyed by vendor, in six registers nobody had ever read across projects: subcontract, commitment, sub_invoice, coi, lien_waiver, warranty. `vendor_memory.py` reads them, tenant-scoped the same way `benchmarking._rows` is, so a portfolio roll-up cannot see another tenant's partners. Two refusals carry the design: - **A vendor with no history is `no_history`, never `clear`.** An unknown sub that renders as a clean record is the prequalification equivalent of "0 open NCRs" on a building nobody inspected — and it is the expensive direction, because it reads as a green light. - **A COI with no expiry recorded is UNKNOWN cover, not cover.** Counted separately from an expired one; a missing date must not quietly satisfy an insurance check. The Q-score and this history are reported side by side and never blended. One is what the sub said about themselves and one is what they did; a single composite would hide which half is which, exactly as `declared` vs `derived` does on the option card. And the limit is stated rather than left to be discovered: `ncr` and `inspection` carry NO vendor field (checked — their only near-match is `subject`), and no schedule activity binds one, so QUALITY and SCHEDULE performance are not attributable here. A clean scorecard is silence on that subject, not a pass. `attributable.not_from` says so in the payload, and the test asserts it against the actual module.json schemas rather than trusting the prose — if a vendor field is ever added to `ncr`, that test fails and the claim gets revisited. Router uses a function-local import: a top-level one is removed by the repo's import autofix (the same hook noted from the client-portal sprint). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ibuilder
force-pushed
the
feat/r22-option-object
branch
from
July 30, 2026 14:46
21063a7 to
b45a223
Compare
Owner
Author
|
Merging. Rebase verified, and Your correction is right and mine was worse than a different reader. I said "expect 460 (455 on main + 5)". Your generalisation is the keeper: "your 455 and my 450 were both counts of Evidence this merge rests on, with its base SHA. Integrated Also right to flag the ruff scope. The four things I called out in your sprint still stand as the best of it: asserting your predecessor's content was present before writing over a shared register; stating what you CHECKED rather than concluded; choosing |
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.
Four R22 items. Lane C engines, Lane G routes, one Lane H register field. No version files. No module added — the register count stays 133/30, so the docs count gate stays quiet.
Rebased onto
b44ddbf9and re-applied on top of #108's field sweep, not over it — asserted before writing:1. R22-OPTION-OBJECT — economics with provenance
design_optioncarriedhard_costandirr_pctas typed fields with zero reference to a deal, whileproforma/solve.pysizes debt andreturns.pycomputes the levered IRR right next door. A typed 19.7% and a solved 19.7% render identically; the typed one is the one that goes stale.Every figure now states a
basis:derived·declared·unlinked·unavailable.unlinkedis the point. Pricing an option from the project's other scenario would assert this massing was underwritten under that deal — provenance nobody recorded. Refused; the candidate scenarios are named instead.A declared figure is never promoted to derived and a derived one never overwrites what was typed, so
declared_disagrees_with_derivedcan exist — a scheme whose typed IRR sits 4 points above what its own scenario solves to.2. R22-ITP-NCR ② — the quality evidence chain, per element
The registers were never the gap, verified by reading the files:
itpcarries hold/witness points (point_type,acceptance_criteria),ncra realopen → dispositioned → closedlifecycle, andquality.pyrolls all three up. But zero GUID fields across all three modules, andturnover.pyreferenced none of them — so a project could be certified substantially complete with the quality evidence never attached to what it inspected.No new field, no migration. Records attach through
element_guids, the column every module record already has, already routed asPOST .../elements.data.elementhas one reader in the codebase;element_guidshas the module engine. A bespoke field would have created a second convention beside the platform's own.The rule: an element with no quality record is
unrecorded, neverclear. "0 open NCRs" across a building nobody inspected is literally true and completely misleading.any_attachedandcoverage_pctseparate "this building has a problem" from "nobody has linked records yet".Two real product gates surfaced while testing and are now pinned: closing an NCR requires attached evidence, and verifying an ITP requires acceptance criteria. An element cannot be cleared by fiat.
3. R22-PROCURE-DEPTH ③ — vendor memory across projects
2 of 3 parts already existed (
prequalification.py,clause_playbook.py). The third didn't:score_projecttakes one project, so a sub is prequalified from their own form with no reference to their record with you.Six registers carry a vendor and had never been read across projects. Same refusals:
no_history≠clear— an unknown sub rendering as clean is the expensive direction.The Q-score and this history are reported side by side, never blended — one is what the sub said, the other is what they did.
The limit is stated, not hidden:
ncrandinspectioncarry no vendor field, so quality/schedule performance is not attributable. A clean scorecard is silence on that subject. The test asserts this against the actualmodule.jsonschemas — if a vendor field is ever added toncr, the test fails and the claim gets revisited.4. Clawback —
unavailable, with no number inventedlp_irr is Nonemeans XIRR did not converge — 26% of shapes in a 729-pattern sweep — not "no restitution owed". That case silently returned the same payload as a healthy deal, so a GP kept money a clawback might have recovered.Now
clawback.status=applied/not_owed/unavailable/not_requested, with the reason. No fallback is picked, deliberately — inventing a restitution figure out of a failed solve is how a plausible dollar amount gets cited while a missing one gets chased. Whether a non-converging IRR is a zero-clawback case is a deal-terms decision. The test assertsunavailableandnot_owedcan never collapse into each other.Also
test_proforma: the CPI-fallback forecast is value-checked, not just its label. The old assertion wasforecast_at_completion < 40_000_000, an upper bound — flipping the branch'smaxtomincollapsed 27,133,231.54 → 9,000,000 (spend-to-date, i.e. claiming a month-6-of-17 job is finished) and passed. Found by mutation testing.Verification
b44ddbf9. The one failure,test_project_package, passes standalone twice, produced no traceback under parallelism, and references none of the changed modules — consistent with the documented ifcopenshell/Windows parallel flake.test_reachable312/316, no new orphan · manifest guard clean · ruff clean.