diff --git a/docs/maintainer-guide.md b/docs/maintainer-guide.md index 3d57b27c..82ee2bd9 100644 --- a/docs/maintainer-guide.md +++ b/docs/maintainer-guide.md @@ -136,7 +136,7 @@ When multiple sessions are active: | Card copy on the branches no persona reaches | `skills/fomo-kernel/copy/*.json` ↔ `tests/golden/copy-corpus.txt`, generated by `tests/copy_corpus.py --update` from real `render_private`/`render_html`/`render_public` output (#402 knife 5). Never hand-edit the golden, and never pin a catalog sentence as a literal inside a test again: add a scene instead. `tests/copy_corpus.py` also fails when a key in a claimed register reaches no surface, so dead copy and a renderer that silently stopped emitting a sentence both surface as red rather than as a passing suite. | | Design-bundle preview CSS | `tools/design_bundle.py` `TOKENS`/`CARD` (`.rc2`/`--` aliases) are derived at build time from `_HTML_WIDGET_CSS` / `_HTML_SHIM_CSS`, not hand-copied. Rerun `python3 skills/fomo-kernel/tools/design_bundle.py` after a runtime CSS change to refresh `ds-bundle/`. | | Coach-root persistence registry (#452) | `coach.py`'s `DATA_FILES` (name, kind, user-facing description) is the only registry — `data-status`/`data-export`/`data-reset` all read it through `_scan_root(root)`, never a second list. Completeness is generated, not hand-verified: `tests/test_coach_data_cli.py`'s `test_data_files_registry_covers_every_engine_written_path` parses every non-test module under `skills/fomo-kernel/engine/` for the `os.path.join(, "")` idiom every writer already uses, and fails if a literal it finds is not registered — `condition_checks.jsonl` was exactly this gap (shipped in #412's second half, never registered, nothing failed until this check existed). `review.py`'s `_engine_version()` is named `repo_root` rather than `root` specifically so the scan needs no hand-written exclusion for the skill's own checkout path. Two already-registered entries the scan cannot see — `profile.md` (the agent writes it directly per `SKILL.md`) and `ux/` (`tools/ux_receipt.py`'s pathlib writer, outside `engine/`) — are covered instead by their own dedicated tests in the same file, not by this one. | -| Pre-trade evaluation (Layer 2, `review.py consider`) | `schemas/trade-evaluation.schema.json` (`premise` is `$ref`-ed straight from `trade-premise.schema.json`, `context` from `schemas/decision-context.schema.json`, and `agent_case` from `schemas/answer-provenance.schema.json` — never restated) ↔ `review.py`'s `cmd_consider` / `_rows_from_ledger` (the ledger-reconstruction fallback for a caller with no CSV in hand) / `_evaluation_id` / `_append_evaluation_row` / `_validate_decision_context` / `_validate_agent_case` ↔ `engine/answer_provenance.py`'s `validate_agent_case` (#414; the semantic gate `cmd_consider` calls after `consequence`/`rule_collisions` exist and before either is persisted or returned — #479 Wave B) ↔ `coach.py`'s `DATA_FILES` (so `data-export`/`data-reset` see `trade_evaluations.jsonl` like every other stored file — #452 is exactly the bug that shipped when a file skipped this step) ↔ `references/trade-consequence.md` ↔ `tests/test_consider.py`. Every stored field is a frozen value, never a pointer into mutable state the ledger keeps growing past — the frozen-subject design ratified in issue #446's specification comment. `--resolve` appends a new row carrying the same `evaluation_id` rather than rewriting the old one; `_fold_evaluations` (mirroring `conditions.fold_slots`'s supersede-by-chain precedent) is the only reader that decides which row is current. `review.CONSIDER_DECISIONS` and the schema's `decision` enum are locked together by a drift test in `tests/test_consider.py`, the same discipline the question-kind enum row above holds; so are `EVALUATION_EVIDENCE_REFS_CAP` / `EVALUATION_CONTEXT_TEXT_MAX` / `EVALUATION_EVIDENCE_REF_MAX` and the context schema's own `maxItems`/`maxLength`. Three rules the DecisionContext (#479 Wave A) adds. **Absent means absent**: `_evaluation_id` omits the `context` key from its seed and `cmd_consider` omits it from the row on the identical `is not None` test — a seed carrying `"context": null` moves the hash of *every* context-free call, so an existing user's next plain re-ask mints a new id instead of converging on the row already on disk, duplicating it, with nothing else in the suite pinning an id value. `test_a_context_free_evaluation_id_is_exactly_what_it_was_before_context_existed` pins the literal digest this function returned at `main@52df7f9`; that value must never move. **Identity, never arithmetic**: the context seeds the id so the same premise re-asked with a different `why_now` is a distinct evaluation, and `consequence`/`rule_collisions` are computed from the premise and the book before the seed is taken — the paired test asserts both halves at once, because seeding on the context is only legitimate while the arithmetic is byte-identical. **Every bound refuses, none truncates**: a shortened reason or a clipped evidence list is a statement the user never made, so `_validate_decision_context` raises with the limit named rather than repairing the envelope. `linked` stays out of `CONSIDER_DECISIONS` and out of the `decision` enum — that tuple is `--decision`'s argparse `choices`, so admitting it would let a user assert a link the engine never made (#490 derives link status at read time from its own stream and never writes it back here). Two rules #479 Wave B's provenance-gate integration adds, on top of #414's already-frozen validator. **The gate reads exactly what gets stored, and reads it once**: `cmd_consider` calls `validate_agent_case` with the same `consequence_stored`/`collisions` objects the row itself carries, never a separately assembled copy, and passes `user_statements=(context["reason"], context["why_now"])` — the exact, unparaphrased strings, never a summary — when a `--decision-context` was supplied, `()` otherwise; `agent_case` still never enters `_evaluation_id`'s seed, matching the reasoning the `context` row above already states: it is the agent's interpretation, not the subject being evaluated. `_validate_agent_case` (the cheap structural precheck that still runs first) was narrowed to require only that a claim carry `claim`/`provenance`, not *exactly* those two fields — the full field set is provenance-dependent (`anchor`/`worsens` for `engine_fact`, `source`/`as_of` for `public_fact`) and is now checked in exactly the one place that also enforces it, `answer_provenance.py`. **Reconciling the two claim shapes was Wave B's call, made once, not left open**: `trade-evaluation.schema.json`'s `agent_case` property is a bare `$ref` to `answer-provenance.schema.json` rather than a second, narrower claim `$defs` of its own — the old inline shape (`additionalProperties: false`, only `claim`/`provenance`) could not express what `engine_fact`/`public_fact` claims are required to carry at all, so leaving it unreconciled would have made both of those provenance kinds permanently unusable through the CLI even after the validator was wired in. What `consequence.DISCLOSURES` may contain is settled in one place and read three times (#598/#599/#600): the constant itself, `trade-evaluation.schema.json`'s stored enum, and `evaluation-challenge.schema.json`'s live one, locked by two drift tests in `tests/test_consider.py`. The stored enum is `DISCLOSURES` **plus** `RETIRED_DISCLOSURES` and nothing else — a key no path emits stays valid on a row written before it stopped being emitted, the replay posture #549's `declared_partial` already has — while the challenge enum is `DISCLOSURES` alone, because that block is emitted fresh and never stored, so offering a retired key there would tell an agent it may owe a disclosure nothing can produce. Three rules about what the keys themselves are for. **A blind spot is disclosed with its size, never only its existence**: `unclassified_book`/`etf_not_decomposed` name the *book's* illegible positions where `unmapped_driver` had only ever looked at the premise's own ticker, and each carries an identity list beside it (`unclassified_holdings`, `undecomposed_etfs`) on #515's exact division of labour — the key says THAT the concentration figures were measured over part of the book, the list says WHICH positions and at what weight. Both feed `answer_provenance.required_coverage`, which reads `disclosures` generically, so adding a key is what makes an answer owe the fact; the enforcement is free and the naming is the whole decision. The lists are stamped by `consequence.book_legibility`, called from `portfolio_state` rather than from `consequence()`, so a state carries its concentration readings and the positions those readings were measured *without* in the same dict — `consequence()` forwards `after`'s pair instead of deriving a second one, and `review._canonical_consider_before` is the only other call site, recomputing because it is the one place a state's denominator is replaced after the state was built. Placing this in `consequence()` alone was the shipped defect: a probe workflow that read the book and printed `ai_pct` received the reading with none of its limitations, offline suite green — the same failure shape as a reader of the book not being handed the split map, and fixed the same way. **The two lists are disjoint and the split is the remedy, not tidiness**: a fund the instrument map does not recognize is an unclassified single name, where `--driver-map` is the fix, and declaring it a fund moves it to the limitation that is true, where no fix exists yet — naming one position twice would point the user at a remedy that cannot work. Nothing here is look-through; #599 still owns prorating a fund's constituents across sector/AI buckets. **A currency gap is not a disclosure at all**: `portfolio_state` refuses a book whose held currency has no rate, because `usd_view` resolves a missing one as 1.0 and a ~31:1 currency summed at face value does not make the aggregate incomplete, it inverts which holding is the largest — AGENTS.md boundary 6's fail-closed rule, and #497's canonical-lane treatment finally reaching the legacy CSV lane that never had it. `_canonical_consider_before` converts that `ConsequenceError` rather than letting it escape, and `portfolio_state` carries no `fx_gaps` companion any more: past the refusal it could only ever be empty. One rule about the price day the basis freezes (#618). **A date is recorded only where one was observed, per instrument, and the summary is derived from the instruments rather than declared over them**: `basis.price_observations` is stamped by `review._price_observation_record` from the parsed feed's own `observed_date`, scoped to the same `priced_universe` the recovery kit is built over, and is *absent* — never null, never a placeholder, never today's — whenever `valuation_basis` is `unpriced`, which is what keeps an offline answer from growing a date it never had. Its `as_of` is `max(by_ticker)` and not `feed["as_of"]`: the envelope's declared frame date is an upper bound `price_feed.parse` enforces on its rows, so a supplied envelope may legitimately declare today over yesterday's closes, and copying it forward would put the summary ahead of every number it summarizes — the same confusion at frame level that per-ticker observation exists to prevent (#583 §2), and the definition `market_data.to_price_feed_envelope` already takes for its own `as_of`. A priced call's `evaluation_id` moves because of this field and that is correct — the id seeds on the frozen answer and the answer genuinely differs — while a context-free *unpriced* call's must not, which `test_a_context_free_evaluation_id_is_exactly_what_it_was_before_context_existed`'s literal digest is what pins. | +| Pre-trade evaluation (Layer 2, `review.py consider`) | `schemas/trade-evaluation.schema.json` (`premise` is `$ref`-ed straight from `trade-premise.schema.json`, `context` from `schemas/decision-context.schema.json`, and `agent_case` from `schemas/answer-provenance.schema.json` — never restated) ↔ `review.py`'s `cmd_consider` / `_rows_from_ledger` (the ledger-reconstruction fallback for a caller with no CSV in hand) / `_evaluation_id` / `_append_evaluation_row` / `_validate_decision_context` / `_validate_agent_case` ↔ `engine/answer_provenance.py`'s `validate_agent_case` (#414; the semantic gate `cmd_consider` calls after `consequence`/`rule_collisions` exist and before either is persisted or returned — #479 Wave B) ↔ `coach.py`'s `DATA_FILES` (so `data-export`/`data-reset` see `trade_evaluations.jsonl` like every other stored file — #452 is exactly the bug that shipped when a file skipped this step) ↔ `references/trade-consequence.md` ↔ `tests/test_consider.py`. Every stored field is a frozen value, never a pointer into mutable state the ledger keeps growing past — the frozen-subject design ratified in issue #446's specification comment. `--resolve` appends a new row carrying the same `evaluation_id` rather than rewriting the old one; `_fold_evaluations` (mirroring `conditions.fold_slots`'s supersede-by-chain precedent) is the only reader that decides which row is current. `review.CONSIDER_DECISIONS` and the schema's `decision` enum are locked together by a drift test in `tests/test_consider.py`, the same discipline the question-kind enum row above holds; so are `EVALUATION_EVIDENCE_REFS_CAP` / `EVALUATION_CONTEXT_TEXT_MAX` / `EVALUATION_EVIDENCE_REF_MAX` and the context schema's own `maxItems`/`maxLength`. Three rules the DecisionContext (#479 Wave A) adds. **Absent means absent**: `_evaluation_id` omits the `context` key from its seed and `cmd_consider` omits it from the row on the identical `is not None` test — a seed carrying `"context": null` moves the hash of *every* context-free call, so an existing user's next plain re-ask mints a new id instead of converging on the row already on disk, duplicating it, with nothing else in the suite pinning an id value. `test_a_context_free_evaluation_id_is_exactly_what_it_was_before_context_existed` pins the literal digest this function returned at `main@52df7f9`; that value must never move. **Identity, never arithmetic**: the context seeds the id so the same premise re-asked with a different `why_now` is a distinct evaluation, and `consequence`/`rule_collisions` are computed from the premise and the book before the seed is taken — the paired test asserts both halves at once, because seeding on the context is only legitimate while the arithmetic is byte-identical. **Every bound refuses, none truncates**: a shortened reason or a clipped evidence list is a statement the user never made, so `_validate_decision_context` raises with the limit named rather than repairing the envelope. `linked` stays out of `CONSIDER_DECISIONS` and out of the `decision` enum — that tuple is `--decision`'s argparse `choices`, so admitting it would let a user assert a link the engine never made (#490 derives link status at read time from its own stream and never writes it back here). Two rules #479 Wave B's provenance-gate integration adds, on top of #414's already-frozen validator. **The gate reads exactly what gets stored, and reads it once**: `cmd_consider` calls `validate_agent_case` with the same `consequence_stored`/`collisions` objects the row itself carries, never a separately assembled copy, and passes `user_statements=(context["reason"], context["why_now"])` — the exact, unparaphrased strings, never a summary — when a `--decision-context` was supplied, `()` otherwise; `agent_case` still never enters `_evaluation_id`'s seed, matching the reasoning the `context` row above already states: it is the agent's interpretation, not the subject being evaluated. `_validate_agent_case` (the cheap structural precheck that still runs first) was narrowed to require only that a claim carry `claim`/`provenance`, not *exactly* those two fields — the full field set is provenance-dependent (`anchor`/`worsens` for `engine_fact`, `source`/`as_of` for `public_fact`) and is now checked in exactly the one place that also enforces it, `answer_provenance.py`. **Reconciling the two claim shapes was Wave B's call, made once, not left open**: `trade-evaluation.schema.json`'s `agent_case` property is a bare `$ref` to `answer-provenance.schema.json` rather than a second, narrower claim `$defs` of its own — the old inline shape (`additionalProperties: false`, only `claim`/`provenance`) could not express what `engine_fact`/`public_fact` claims are required to carry at all, so leaving it unreconciled would have made both of those provenance kinds permanently unusable through the CLI even after the validator was wired in. What `consequence.DISCLOSURES` may contain is settled in one place and read three times (#598/#599/#600): the constant itself, `trade-evaluation.schema.json`'s stored enum, and `evaluation-challenge.schema.json`'s live one, locked by two drift tests in `tests/test_consider.py`. The stored enum is `DISCLOSURES` **plus** `RETIRED_DISCLOSURES` and nothing else — a key no path emits stays valid on a row written before it stopped being emitted, the replay posture #549's `declared_partial` already has — while the challenge enum is `DISCLOSURES` alone, because that block is emitted fresh and never stored, so offering a retired key there would tell an agent it may owe a disclosure nothing can produce. `consequence.EXCLUSION_REASONS` is the same shape of fact for `excluded_holdings[].reason` and is read from the same three places (#515/#673), locked by `test_exclusion_reasons_constant_is_exactly_what_both_schemas_declare` — with both enums identical, since an exclusion reason is emitted live as well as stored and so has no retired half. Three rules about what the keys themselves are for. **A blind spot is disclosed with its size, never only its existence**: `unclassified_book`/`etf_not_decomposed` name the *book's* illegible positions where `unmapped_driver` had only ever looked at the premise's own ticker, and each carries an identity list beside it (`unclassified_holdings`, `undecomposed_etfs`) on #515's exact division of labour — the key says THAT the concentration figures were measured over part of the book, the list says WHICH positions and at what weight. Both feed `answer_provenance.required_coverage`, which reads `disclosures` generically, so adding a key is what makes an answer owe the fact; the enforcement is free and the naming is the whole decision. The lists are stamped by `consequence.book_legibility`, called from `portfolio_state` rather than from `consequence()`, so a state carries its concentration readings and the positions those readings were measured *without* in the same dict — `consequence()` forwards `after`'s pair instead of deriving a second one, and `review._canonical_consider_before` is the only other call site, recomputing because it is the one place a state's denominator is replaced after the state was built. Placing this in `consequence()` alone was the shipped defect: a probe workflow that read the book and printed `ai_pct` received the reading with none of its limitations, offline suite green — the same failure shape as a reader of the book not being handed the split map, and fixed the same way. **The two lists are disjoint and the split is the remedy, not tidiness**: a fund the instrument map does not recognize is an unclassified single name, where `--driver-map` is the fix, and declaring it a fund moves it to the limitation that is true, where no fix exists yet — naming one position twice would point the user at a remedy that cannot work. Nothing here is look-through; #599 still owns prorating a fund's constituents across sector/AI buckets. **A currency gap is not a disclosure at all**: `portfolio_state` refuses a book whose held currency has no rate, because `usd_view` resolves a missing one as 1.0 and a ~31:1 currency summed at face value does not make the aggregate incomplete, it inverts which holding is the largest — AGENTS.md boundary 6's fail-closed rule, and #497's canonical-lane treatment finally reaching the legacy CSV lane that never had it. `_canonical_consider_before` converts that `ConsequenceError` rather than letting it escape, and `portfolio_state` carries no `fx_gaps` companion any more: past the refusal it could only ever be empty. One rule about the integrity record (#673). **A whole-book summary never gates a per-holding fact** — the second instance, after `cost_basis`: `basis["integrity"]` is read through `consequence.integrity_exclusions`, which scopes each warning to the holding it names and excludes that holding with reason `integrity_oversell`, so one unmatched sell anywhere in the history no longer disables `consider` for the whole account — permanently, since the rows derive from history that has already happened — while the review lane discloses the identical condition and delivers its card. What stays whole-book fatal: a warning with no ticker, a warning this route has no reason for (every `bad_*` row, which `portfolio_basis._bad_integrity` already drops the whole basis for), and a book with no usable holding left. `_canonical_consider_before` takes the integrity half out of the denominator — re-dividing `sizing_projection`'s own per-holding values, never re-valuing anything — and checks the projection-agreement gate against the *valuation* half only, since that is the one thing the two readers may not disagree about and an integrity exclusion is not a claim about valuation at all. A premise *about* an excluded holding is refused by name, before `validate_premise` can call it "not currently held": the recorded zero is exactly what the warning puts in doubt. One rule about the price day the basis freezes (#618). **A date is recorded only where one was observed, per instrument, and the summary is derived from the instruments rather than declared over them**: `basis.price_observations` is stamped by `review._price_observation_record` from the parsed feed's own `observed_date`, scoped to the same `priced_universe` the recovery kit is built over, and is *absent* — never null, never a placeholder, never today's — whenever `valuation_basis` is `unpriced`, which is what keeps an offline answer from growing a date it never had. Its `as_of` is `max(by_ticker)` and not `feed["as_of"]`: the envelope's declared frame date is an upper bound `price_feed.parse` enforces on its rows, so a supplied envelope may legitimately declare today over yesterday's closes, and copying it forward would put the summary ahead of every number it summarizes — the same confusion at frame level that per-ticker observation exists to prevent (#583 §2), and the definition `market_data.to_price_feed_envelope` already takes for its own `as_of`. A priced call's `evaluation_id` moves because of this field and that is correct — the id seeds on the frozen answer and the answer genuinely differs — while a context-free *unpriced* call's must not, which `test_a_context_free_evaluation_id_is_exactly_what_it_was_before_context_existed`'s literal digest is what pins. | | Pre-trade evaluation → review reconciliation | `review.py`'s `_evaluation_reconciliation` (calls `_fold_evaluations`, and reads real dated trade events through `_ledger_trade_events` — **never** `_rows_from_ledger`, whose synthesized anchor row raises on a position declared with shares alone and would break `prepare` for an ordinary snapshot user) ↔ `evaluation_reconciliation` in `schemas/review-plan.schema.json` (declared, `additionalProperties: false` at every level, deliberately not dropped into schema-open `state_snapshot`; optional rather than `required`, matching the `engine_version`/`authoring_contract` replay-compatibility precedent) ↔ `EVALUATION_RECONCILE_CAP` and its `summary.beyond_cap` disclosure ↔ `references/trade-consequence.md` ↔ `tests/test_consider.py`. Two invariants. **The engine states a fact, never a cause**: a matching trade may be reported, but `decision` is the user's word and moves only through `consider --resolve` — an engine-written decision would manufacture an adjudication nobody made, the prohibition `condition-check.schema.json` states about `user_response`. **A capped list discloses what it dropped**, or a bounded read reads as a complete one. This is the consumer that keeps `trade_evaluations.jsonl` from being a written-never-read store (#429). **It is no longer the only reader outside `consider`**, and the second one deliberately shares none of its filters: `_evaluation_recall` (#636) reads the same store to quote the user's own `context` words back into the `initial_thesis` question stem, and ignores `decision` entirely — a resolved evaluation is still something the user said, so this row's open-only scope is the wrong one for recall, and this surface's own schema declares itself a fact surface rather than a question. Its bounds live with it: `_cycle_entry` matches `trade_recap.CYCLE_ID_RE` instead of splitting on `#`, and `_recalled_entry_statement` fails closed on any cycle after the first, because a cycle id carries no lower bound and a re-entry would otherwise quote the previous position's reason. A third reader of this store must state which of those two filter sets it takes and why. | | What a `consider` answer owes the user (#479 Wave B cut 2) | `engine/evaluation_challenge.py`'s `build_challenge` is the single statement — `must_state`, `quote_verbatim`, `unchecked`, `case_required`, `required_coverage` — emitted by `review.py` `cmd_consider` beside the row. Its readers: `schemas/evaluation-challenge.schema.json` ↔ `references/trade-consequence.md` "What the answer owes" ↔ `SKILL.md` rule 3 / `AGENTS.md` boundary 2 ↔ `tests/test_evaluation_challenge.py` + `tests/test_consider.py` section M. Wave B's first cut made a *fabricated* case unstorable and left the user's side untouched; this is the other half, and it is this guide's own "Honesty decisions belong in code" applied to the surface that never had a `build_honesty_ledger()`. Four rules. **The floor and the gate are one list, not two**: `required_coverage` is `answer_provenance.required_coverage`, the same function `validate_agent_case` calls internally, so what the agent is *told* it owes and what a `--agent-case` is *refused* for dropping cannot drift — deriving it in both places is the hand-mirrored surface this file forbids, and `test_dropping_any_single_entry_the_challenge_named_is_refused` fails in both directions (an obligation nothing enforces, and an obligation nobody was told about). Widening it is now the only way to add an enforced obligation; the `would_breach`/`already_over` arm is what this cut added, and before it a rule the trade breaks could vanish from a case with the whole suite green. The `price_basis` arm (#618) is the second widening and it carried a latent defect out with it: **one claim pays one debt, the narrowest it matches**. `basis.price_observations` is the first required path ever to sit *under* another (`basis`), and plain prefix matching let a single price-day citation discharge the staleness obligation too — a case that never mentioned staleness accepted, whole suite green, because until then no two required paths nested. `answer_provenance._paid_path` is the repair: the longest matching path wins and the broader obligation stays open. Adding any nested required path is now safe; adding one without that rule was not. **`must_state` is deliberately the larger list**: an `unjudged`/`unmapped` collision must be *named* — an unevaluated rule presented as no issue tells the user something the engine never checked — but forcing a claim for each would make a book with eight behavioral rules need eight claims saying nothing was measured, which is answer-padding to satisfy a checker. **An anchor is never offered unless it resolves**, through `answer_provenance.resolve_anchor` rather than a second walk, so the surface that says how to cite a fact and the gate that judges the citation agree by construction; a real ticker containing a dot (`2330.TW`) cannot be addressed by a dotted path, and such an entry keeps its value and loses only its `anchor` — dropping the fact instead would have been the wrong repair. **Emitted, never stored and never in the id seed**: it is a pure function of fields the row already freezes, so storing it would be a derived duplicate able to disagree with its own inputs, and no reader needs the historical version (#429 in the other direction). Boundary with rule 8: this is the "which facts it owes" clause made computable, and says nothing about length — the delivery half is instruction-only, the same footing `docs/development-guide.md` §4 admits for the recommendation ban, and is observed by an owner-live receipt on `tools/ux_receipt.py`'s `consider` route (#544 Slice B; the receipt-route row above holds its contract), which machine-checks what containment and digits can decide and leaves the rest to the owner's `comprehension` verdict. | | Snapshot position provenance (`carried`, `since`/`since_basis`/`cycle_seq` — #485 Slice C, #531, #539/#536) | `ledger.SNAPSHOT_POSITION_KEYS` and `ledger.ENGINE_ASSIGNED_POSITION_KEYS`/`SINCE_BASES` are the single declaration — `snapshot_adapter.POSITION_KEYS` reads them rather than restating them, which is exactly the hand-mirrored whitelist that let `carried` ship broken with a green suite. Three rules no surface may weaken. **`since` and `since_basis` are one fact, validated as a pair** in `snapshot_adapter._normalize_provenance`: a date that could travel without its stamp is a reconstruction every renderer is free to print as an exact day, so the pairing — not renderer discipline — is what makes rule 2 of #531's ruling true. **A caller never supplies them**: `normalize_envelope`/`normalize_book` refuse both by default and `_load` (the file path, i.e. everything an agent can write) has no way to opt in; `book_refresh.carry_recorded_starts` is the single `allow_engine_provenance=True` call site, counted mechanically by `tests/test_book_refresh.py::test_only_the_refresh_lane_may_write_engine_assigned_provenance`. **`ledger.derive_holdings` is the only reader**, via `_anchored_cycle_start`, and it degrades to the anchor date with a named `integrity` issue rather than trusting a hand-edited row. **Both adoption lanes reach that one primitive** (#539/#536) — `build_adoption` for the refresh lane, `review._carried_declaration` for the review lane, which calls it before `snapshot_adapter.prepare` so the plan's cycle ids and the ledger anchor come from one stamped envelope rather than two derivations. The count test is what keeps that true, so read its assertion as "one implementation, both lanes through it", not "one lane"; a second literal anywhere is a second place a cycle start could be authored. **`since_basis` is an evidence vocabulary, not a transport marker** — four values, carried unchanged, never collapsed (owner ruling, 2026-07-31). The two new ones exist because the stamp was never the fact worth carrying: the appearance question is only asked for a position that appears *after* the book exists, so every position in a user's first declaration is unstampable and reminted on every adoption until the *start itself* was carried. A single "came from the record" value was refused because it would state how the date travelled and destroy why it is believed, and the loss is unrecoverable: after an adoption `origin` describes the new snapshot writer, not the original evidence for the start. So `_carried_start` decides the date and the basis together and **prefers the recorded stamp over recomputation** — a position the record still traces to its anchor keeps that anchor's own basis whatever it says, and only one the anchor does not describe is labelled fresh (`trade_event` when the ledger watched the cycle open, `snapshot_anchor` when a declaration first put it on the books). That branch order is the whole protection: an adopted trade-proven position reads `origin == "snapshot"` like every other row, so a basis recomputed from origin would demote an exact start to a lower bound on the second declaration, silently and permanently — `tests/test_book_refresh.py::test_a_start_the_ledger_can_prove_is_not_overwritten_by_a_declaration` adopts twice for exactly that reason. Nothing is written when the carried start is later than the declaration's `as_of` (the declaration predates the record, and that refusal belongs to the lane that owns it), nor when a `snapshot_anchor` start equals it (restating the default would rewrite the anchor's content address and break idempotent replay); the equality guard is deliberately not extended to the other bases, for which the stamp carries something the default does not. No surface reads the distinction yet — #532 owns visibility — so what this contract owes is that it survives, which is what the repeated-adoption assertions gate. **`cycle_seq` travels with the start or not at all**, because `cycle_id` is `ticker#since#seq` and the start alone is not the identity: two cycles opened on one day — a position flattened and bought back in one session — differ in the sequence and nothing else, so carrying the date without it hands the live position the *sold* cycle's id, along with that cycle's thesis, standing conditions and closed status. That is strictly worse than the reminting being fixed, and #539 widens the door to it (before, the live position landed on the declaration's own date and collided only by coincidence), which is why the two are one change. `_normalize_provenance` refuses a sequence with no dated start and refuses one on `unknown` (a two-segment cycle has none to state); `_anchor` emits it only past the first, for the same content-addressing reason `carried` is conditional — an unconditional key would rewrite every existing `snapshot_id`. `ledger.cycle_sequence` is the only inverse of the composition in `derive_holdings` and lives beside it; `_anchored_cycle_seq` degrades to 1 with a named `integrity` issue exactly as `_anchored_cycle_start` degrades to the anchor date. It stays out of `portfolio_basis._normalized_anchor` by construction (that function builds a fresh dict), which is also the right semantics: a sequence is provenance, so two declarations of one book keep one `state_version`. Its readers: `flows/book-refresh.md` step 3 ↔ `references/data-contract.md` ↔ `docs/prd-ledger.md` ↔ `schemas/book-refresh.schema.json`'s lane description, and the value set itself is asserted once, in `tests/test_book_refresh.py::test_the_classification_enum_matches_the_engine_constant`. Neither key enters `portfolio_basis._normalized_anchor` (provenance stays out of the book's identity) or the `holdings` dict (`portfolio_basis`'s `holding_keys` is an exact-key gate and `since` must stay a real date there) — `cycle_id` is what carries an unknown start, as `ticker#unknown`. Kinds, options, and answer fields are locked across `book_refresh.CONFIRMATION_KINDS`/`CLASSIFICATIONS_BY_KIND`/`HELD_MONTHS_MAX` ↔ `schemas/book-refresh.schema.json` ↔ `flows/book-refresh.md` step 2 ↔ `references/data-contract.md` ↔ `docs/prd-ledger.md` by two drift tests in `tests/test_book_refresh.py`. Widening what refresh raises automatically widens `review._refuse_if_the_book_must_catch_up`'s refusal (#530), which calls `plan_refresh` rather than restating its criterion. | diff --git a/skills/fomo-kernel/engine/consequence.py b/skills/fomo-kernel/engine/consequence.py index cf6bf013..b61d1f9e 100644 --- a/skills/fomo-kernel/engine/consequence.py +++ b/skills/fomo-kernel/engine/consequence.py @@ -144,6 +144,35 @@ class ConsequenceError(Exception): # and never stored — there is no historical version of it to keep readable. RETIRED_DISCLOSURES = ("mixed_currency_no_fx",) +# Why `rows_from_portfolio_basis` left a holding out of the usable book. The +# tuple is the single definition; schemas/trade-evaluation.schema.json and +# schemas/evaluation-challenge.schema.json mirror it, and tests/test_consider.py +# pins the two against this constant. +# +# The first two are missing facts about the holding itself (#515). The third is +# a different kind of gap and #673's whole subject: the ledger's integrity +# record names this holding, so its recorded share count and cost are the +# numbers in doubt — not absent, but not derivable from the history supplied. +# The distinction is load-bearing downstream, because an integrity-excluded +# holding is also the one thing a premise may not be *about* (see +# `_refuse_premise_on_integrity`), while a holding with no cost on record is +# merely outside the denominator. +EXCLUSION_REASONS = ("unusable_shares", "unavailable_cost", "integrity_oversell") + +# The subset of the above that means "this holding's own record is untrustworthy". +INTEGRITY_EXCLUSION_REASONS = ("integrity_oversell",) + +# ledger integrity `issue` → the exclusion reason it produces. An issue absent +# from this table is refused whole-book by `integrity_exclusions` rather than +# excluded, however clearly it names a ticker: an exclusion is a *disclosed* +# degradation, and this route cannot disclose a warning whose meaning for one +# holding it has no vocabulary for. Only `oversell` can reach here from a +# canonical basis — portfolio_basis.validate_portfolio_basis rejects every +# other issue outright, and `_bad_integrity` drops the whole basis for any +# `bad_*` row before that — so the table is exhaustive in production and the +# refusal below is the defensive floor under a hand-built basis. +_INTEGRITY_EXCLUSION_REASON = {"oversell": "integrity_oversell"} + # rule_collision's own vocabulary — deliberately distinct from # conditions.VERDICTS and problems.check_rules' broke/held/skipped. Those # answer "what happened over a realized period"; this answers "does one @@ -246,6 +275,104 @@ def _fifo_held(rows): return trade_recap.fifo_held(open_lots) +def integrity_exclusions(basis): + """``{ticker: exclusion reason}`` for every integrity warning on ``basis``. + + Owner ruling on #673. The canonical integrity record is a list of + per-holding accounting warnings — one ``oversell`` row per sell with no + matching prior buy, each naming its own ticker — and gating the *whole* + basis on that list being non-empty turned one such sell anywhere in the + history into a permanent, account-wide refusal of every ``consider`` call. + Permanent because the rows are derived from history that has already + happened: no future trade clears them. The review lane treats the identical + condition as a disclosure and delivers the card, so a book the review lane + could review was a book the pre-trade lane could not evaluate at all. + + What an ``oversell`` actually damages is that one ticker's share count. The + replay clamps the sell to the shares it can see, so a user who bought 100 + before the export window, then bought 10 and sold 30 inside it, has a + recorded position of zero and a true position of 80. Every *other* holding + is replayed independently and is untouched. So the warning is scoped to the + holding it names, and this function is where that scoping happens. + + Raises ``ConsequenceError`` for a warning that cannot be scoped — no + ticker, or an ``issue`` outside ``_INTEGRITY_EXCLUSION_REASON``. That + refusal is still whole-book, deliberately: a warning this route cannot name + a reason for is one it cannot disclose, and silently absorbing it into the + usable book is the failure mode the exclusion exists to prevent. + """ + if not isinstance(basis, Mapping): + raise ConsequenceError("canonical PortfolioBasis is not an object") + warnings = basis.get("integrity") or () + if not isinstance(warnings, (list, tuple)): + raise ConsequenceError("canonical PortfolioBasis integrity record is not a list") + out = {} + for row in warnings: + issue = row.get("issue") if isinstance(row, Mapping) else None + ticker = row.get("ticker") if isinstance(row, Mapping) else None + reason = _INTEGRITY_EXCLUSION_REASON.get(issue) if isinstance(issue, str) else None + if reason is None or not isinstance(ticker, str) or not ticker.strip(): + named = issue if isinstance(issue, str) and issue else "an unnamed issue" + raise ConsequenceError( + "canonical PortfolioBasis has an integrity warning that cannot be scoped " + f"to one holding: {named}") + out[ticker.strip()] = reason + return out + + +def integrity_excluded_tickers(excluded_holdings): + """The tickers in ``excluded_holdings`` that were excluded for an integrity + reason rather than a missing valuation fact. + + One definition, read by every consumer that has to tell the two apart — the + canonical-`before` facade in review.py must not accuse an integrity-excluded + holding of having no cost on record, and the premise gate below must refuse + only for this half. A caller re-deriving the distinction from the reason + string beside its own presentation is how the two would come to disagree. + """ + integrity = set(INTEGRITY_EXCLUSION_REASONS) + return {row["ticker"] for row in excluded_holdings or () + if isinstance(row, Mapping) and row.get("reason") in integrity + and isinstance(row.get("ticker"), str)} + + +def _refuse_premise_on_integrity(premise, excluded_holdings): + """Refuse a premise that is *about* an integrity-excluded holding, naming + the ticker and the reason. + + The bounded answer #673 restores is an answer about the rest of the book. + It is not an answer about the damaged holding itself: the whole content of + a consequence for ``ORPH`` is what the position becomes, and the position it + starts from is exactly the number the integrity warning says is not + derivable. + + Runs before ``validate_premise`` on purpose. That validator would reach a + sell of an excluded holding first and refuse it as "not currently held", + which is a claim about the book — and the wrong claim, since the recorded + zero is precisely what is in doubt. + + Scoped to integrity exclusions. A holding excluded for a *missing* fact + (`unavailable_cost`) is a different case that #528 owns, and this leaf does + not change it. + """ + if not isinstance(premise, Mapping): + return + ticker = premise.get("ticker") + if not isinstance(ticker, str) or not ticker.strip(): + return + ticker = ticker.strip() + for row in excluded_holdings or (): + if (isinstance(row, Mapping) and row.get("ticker") == ticker + and row.get("reason") in INTEGRITY_EXCLUSION_REASONS): + raise ConsequenceError( + f"this book's record for {ticker} carries an integrity warning " + f"({row['reason']}), so its recorded shares and cost are not derivable from the " + f"supplied history and no consequence for a trade in {ticker} can be computed " + "from them. The rest of the book is unaffected and can still be asked about; " + f"supplying the transactions that {ticker}'s history is missing is what makes " + "this question answerable.") + + def rows_from_portfolio_basis(basis): """Adapt one canonical current book to consequence input rows. @@ -259,12 +386,14 @@ def rows_from_portfolio_basis(basis): Returns ``(rows, excluded)``. - A damaged (claim-affecting integrity warning) or empty basis cannot make a - current-book verdict at all. A basis where *some* holding cannot be valued - is a different thing, and the owner ruled on it (#515, 2026-07-28): compute - over the part that can be used and name what was left out. A user holding - six positions where one has no cost gets an answer about the priced part of - their book, not a refusal -- a refusal gives them nothing and is + An empty basis cannot make a current-book verdict at all. A basis where + *some* holding cannot be used is a different thing, and the owner ruled on + it twice: for a holding that cannot be valued (#515, 2026-07-28) and then + for a holding the integrity record names (#673, 2026-07-31). Both rulings + are the same sentence: compute over the part that can be used and name what + was left out. A user holding six positions where one has no cost -- or one + whose history carries an unmatched sell -- gets an answer about the usable + part of their book, not a refusal. A refusal gives them nothing and is indistinguishable from a broken product. So a holding that cannot become a usable row is EXCLUDED and recorded here, @@ -278,15 +407,23 @@ def rows_from_portfolio_basis(basis): ``basis["cost_basis"]`` is deliberately not read. It is a whole-book summary of a per-holding fact (``partial_average_cost`` means *some* holding lacks a cost), and gating on it refused six positions because of - one -- the exact defect this leaf removes. How the book was declared -- - snapshot anchor versus replayed trade history, partial versus unverified -- - never gates this adapter either (#485); only a genuinely missing fact does, - and now only for the holding actually missing it. + one -- the exact defect #515 removed. ``basis["integrity"]`` was the same + shape of whole-book summary over a per-holding fact and was still gated + whole until #673; it is now read through :func:`integrity_exclusions`, + which scopes each warning to the holding it names. How the book was + declared -- snapshot anchor versus replayed trade history, partial versus + unverified -- never gates this adapter either (#485); only a genuinely + missing or genuinely underivable fact does, and only for the holding it is + actually missing or underivable for. + + An integrity warning naming a ticker this book does not currently hold is + still recorded in ``excluded``. It removes nothing from the denominator -- + there was nothing of it there to remove -- but a recorded zero is exactly + what an unmatched sell puts in doubt, so "this book has no position in X" + is not a fact this answer may rest on, and the caller has to be able to say + so. It is also what the premise gate reads. """ - if not isinstance(basis, Mapping): - raise ConsequenceError("canonical PortfolioBasis is not an object") - if basis.get("integrity"): - raise ConsequenceError("canonical PortfolioBasis has claim-affecting integrity warnings") + integrity = integrity_exclusions(basis) current_book = basis.get("current_book") holdings = current_book.get("holdings") if isinstance(current_book, Mapping) else None if not isinstance(holdings, Mapping) or not holdings: @@ -301,6 +438,12 @@ def rows_from_portfolio_basis(basis): holding = holdings[ticker] if not isinstance(ticker, str) or not isinstance(holding, Mapping): raise ConsequenceError("canonical PortfolioBasis has invalid holding") + if ticker in integrity: + # Checked before shares and cost, because those are the values the + # warning is about: a clamped replay leaves a perfectly well-formed + # positive share count that is simply not the user's position. + excluded.append({"ticker": ticker, "reason": integrity[ticker]}) + continue try: qty = float(holding["shares"]) except (KeyError, TypeError, ValueError): @@ -321,10 +464,22 @@ def rows_from_portfolio_basis(basis): rows.append({"ticker": ticker, "side": "buy", "qty": qty, "price": price, "date": as_of, "market": holding.get("market", "US"), "currency": holding.get("currency", "USD")}) + # A warning naming a ticker this book does not hold excludes nothing from + # the denominator, and is recorded anyway -- see the docstring. Appended + # after the holdings loop, then sorted, so the list stays one ticker-ordered + # record whichever half an entry came from. + for ticker in sorted(set(integrity) - set(holdings)): + excluded.append({"ticker": ticker, "reason": integrity[ticker]}) + excluded.sort(key=lambda row: row["ticker"]) if not rows: + # The floor under exclude-and-disclose: an empty denominator is not a + # bounded answer. Named with each holding's own reason, because "no + # holding could be valued" is false for a book emptied by integrity + # warnings, and the reason is what tells the user which repair -- + # a cost, or the missing transactions -- would make the book answerable. raise ConsequenceError( - "canonical PortfolioBasis has no holding that can be valued: " - + ", ".join(row["ticker"] for row in excluded)) + "canonical PortfolioBasis has no usable holding: " + + ", ".join(f"{row['ticker']} ({row['reason']})" for row in excluded)) return rows, excluded @@ -649,9 +804,10 @@ def consequence(rows, premise, last_px=None, max_pos_override=None, cash_anchor= itself carries positions the concentration figures could not read — an unclassified single name (`unclassified_book`) or a fund nothing decomposes (`etf_not_decomposed`), both #598/#599 and both named in the - fields below; or a held position could not be valued at all and was left - out of the denominator these numbers are measured against - (`partial_book`, #515). + fields below; or a position was left out of the usable book these numbers + are measured against, because it could not be valued (#515) or because the + integrity record names it (#673) — `partial_book` either way, with + `excluded_holdings[].reason` saying which. `unclassified_holdings` and `undecomposed_etfs` are to their keys what `excluded_holdings` is to `partial_book`: the key says THAT the book was @@ -672,7 +828,13 @@ def consequence(rows, premise, last_px=None, max_pos_override=None, cash_anchor= the partial denominator; the disclosure key says *that* something was excluded, this says *what* (#515's first invariant: the excluded holding is named wherever the derived number appears). + + A premise that is *about* an integrity-excluded holding is refused by name + rather than answered over the bounded book (#673) — see + `_refuse_premise_on_integrity` for why that one case is not a bounded + answer at all. """ + _refuse_premise_on_integrity(premise, excluded_holdings) normalized = validate_premise(premise, rows) premise_row = _premise_row(normalized) diff --git a/skills/fomo-kernel/engine/portfolio_basis.py b/skills/fomo-kernel/engine/portfolio_basis.py index f10cf6ec..7f392298 100644 --- a/skills/fomo-kernel/engine/portfolio_basis.py +++ b/skills/fomo-kernel/engine/portfolio_basis.py @@ -334,7 +334,10 @@ def _semantically_known_events(events: Sequence[Mapping[str, Any]]) -> bool: def _bad_integrity(integrity: Sequence[Mapping[str, Any]]) -> bool: # A malformed input is unknown state. An oversell is a known, claim-affecting # accounting warning: retain it in the basis so downstream claim gates can - # fail closed without pretending the replay never happened. + # fail closed without pretending the replay never happened. What a consumer + # does with a retained row is that consumer's policy, not this one's: since + # #673, `consequence.integrity_exclusions` scopes it to the holding it names + # rather than reading the list as a whole-book verdict. return any(str(item.get("issue", "")).startswith("bad_") for item in integrity) diff --git a/skills/fomo-kernel/engine/review.py b/skills/fomo-kernel/engine/review.py index c6cfc48d..6c023027 100644 --- a/skills/fomo-kernel/engine/review.py +++ b/skills/fomo-kernel/engine/review.py @@ -6295,12 +6295,23 @@ def _canonical_consider_before(rows, basis, projection, last_px, max_pos_overrid different books, and the weight the user is shown would be measured against a denominator no other surface uses. That stays a refusal, because the alternative is a silently wrong percentage. + + An *integrity* exclusion (#673) is the one exclusion the two readers are + expected to disagree about, and it is not a disagreement about valuation. + The projection can value the holding perfectly well — it has a price and a + cost; what the ledger's integrity record says is that the share count those + are multiplied by is not derivable from the supplied history. So the + agreement gate above is checked against the *valuation* half of the + excluded set only, and the integrity half is taken out of the denominator + here instead. """ projected = projection.to_dict() coverage = projected["coverage"] if not projected["applicable"] or coverage["scope"] == "unavailable_mixed_currency": raise ReviewError("canonical PortfolioBasis has no usable current-book sizing projection") excluded_tickers = {row["ticker"] for row in excluded_holdings or ()} + integrity_excluded = consequence.integrity_excluded_tickers(excluded_holdings) + valuation_excluded = excluded_tickers - integrity_excluded projection_unavailable = set(coverage["unavailable"]) # A holding the projection CAN value but this adapter cannot use: it has a # current price and no cost on record. The projection weighs it at market; @@ -6312,18 +6323,18 @@ def _canonical_consider_before(rows, basis, projection, last_px, max_pos_overrid # denominators. Refusing here is narrow and actionable -- the two paths # below both work today -- and #528 tracks teaching the consequence engine # about value-only positions so it need not refuse at all. - priced_without_cost = sorted(excluded_tickers - projection_unavailable) + priced_without_cost = sorted(valuation_excluded - projection_unavailable) if priced_without_cost: raise ReviewError( f"{', '.join(priced_without_cost)} has a current price but no cost on record, and a " "consequence answer needs a cost for every position it reasons about. Add the " "average cost for it, or ask again without --prices to get the answer over the " "part of the book that has costs.") - if projection_unavailable != excluded_tickers: + if projection_unavailable != valuation_excluded: raise ReviewError( "canonical PortfolioBasis and the consider adapter disagree about which " "holdings cannot be valued: projection " - f"{sorted(projection_unavailable)} vs adapter {sorted(excluded_tickers)}") + f"{sorted(projection_unavailable)} vs adapter {sorted(valuation_excluded)}") try: before = consequence.portfolio_state(rows, last_px=last_px, max_pos_override=max_pos_override, @@ -6344,8 +6355,25 @@ def _canonical_consider_before(rows, basis, projection, last_px, max_pos_overrid if (abs(held["shares"] - float(holding["shares"])) > 1e-6 or abs(held["cost"] - float(holding["cost_total"])) > 1e-6): raise ReviewError("canonical PortfolioBasis cost disagrees with consider adapter") - weights = {ticker: entry["weight"] for ticker, entry in projected["values"].items() - if entry["applicable"]} + # #673. The projection is the canonical *whole-book* denominator and stays + # that way -- it is the record, and nothing here rewrites it. What this + # builds is the denominator for the bounded book the answer is actually + # about, from the projection's own per-holding values, restricted to the + # holdings that survived. Re-dividing those values is deliberately not a + # second valuation: no price, cost, share count or FX rate is read here, so + # a value that reached this line came from `sizing_projection` and nowhere + # else. `math.fsum` is `value_partition`'s own summation, and it is exactly + # rounded, so with nothing integrity-excluded the sum is bit-for-bit the + # projection's denominator and every weight is the projection's weight -- + # the pre-#673 answer, unchanged. + usable_values = {ticker: entry["value"] for ticker, entry in projected["values"].items() + if entry["applicable"] and ticker not in integrity_excluded} + denominator = math.fsum(usable_values.values()) if usable_values else 0.0 + if not math.isfinite(denominator) or denominator <= 0: + raise ReviewError( + "canonical PortfolioBasis has no holding left to size this answer against once " + f"{sorted(integrity_excluded)} is excluded for an integrity warning") + weights = {ticker: value / denominator for ticker, value in usable_values.items()} if set(weights) != set(before["held"]): raise ReviewError("canonical PortfolioBasis sizing coverage is incomplete") before = dict(before) @@ -6372,9 +6400,10 @@ def _consider_rows(args, root, valuation_manifest=None, last_px=None, splits=Non says which one it used rather than implying a currency it does not have). Fails closed when neither source yields a usable row: an empty book cannot answer a pre-trade question, and inventing one would be worse than - refusing. A book where only *some* holding cannot be valued is not that - case (#515): those holdings come back in the fifth return value and must - be carried to wherever the derived numbers are stated. + refusing. A book where only *some* holding cannot be used is not that case + -- because it could not be valued (#515) or because the ledger's integrity + record names it (#673): those holdings come back in the fifth return value + and must be carried to wherever the derived numbers are stated. ``splits`` reaches both routes, because both accumulate share counts and a split is what makes two of them incomparable (#550/#558). The two apply it diff --git a/skills/fomo-kernel/references/trade-consequence.md b/skills/fomo-kernel/references/trade-consequence.md index 9175f970..a9caf0f5 100644 --- a/skills/fomo-kernel/references/trade-consequence.md +++ b/skills/fomo-kernel/references/trade-consequence.md @@ -69,9 +69,13 @@ The response carries `before` and `after` — the book's own state without and w | `unmapped_driver` | The premise's ticker has no sector/AI classification, so it cannot be accounted for in concentration. | | `unclassified_book` | At least one *held* position has no sector/AI classification. It contributes zero to `ai_pct` and is dropped from `max_sector_pct`'s numerator, so both figures are measured over less than the whole book. `unclassified_holdings` names which, and at what weight. | | `etf_not_decomposed` | At least one held position is a fund whose constituents nothing here inspects. `undecomposed_etfs` names which, at what weight, and whether it was exempted from concentration wholesale or counted as one opaque ticker. | -| `partial_book` | At least one held position could not be valued at all and is outside the denominator every percentage here is measured against. `excluded_holdings` names which, and why. | +| `partial_book` | At least one position is outside the usable book every percentage here is measured against — it could not be valued at all, or the ledger's integrity record names it. `excluded_holdings` names which, and `reason` says which of the two. | -`partial_book` obliges the answer, not just the payload. When it is present, state the denominator in the same breath as the number — *"this would become 23% of the priced part of your book; ACME has no cost on record and is excluded"* — and name the excluded holdings wherever a derived percentage appears. A partial denominator presented as a whole one is worse than the refusal this replaced, because the user cannot tell it happened. The engine will not answer at all when *nothing* can be valued: that is an empty denominator, not a bounded one. +`partial_book` obliges the answer, not just the payload. When it is present, state the denominator in the same breath as the number — *"this would become 23% of the priced part of your book; ACME has no cost on record and is excluded"* — and name the excluded holdings wherever a derived percentage appears. A partial denominator presented as a whole one is worse than the refusal this replaced, because the user cannot tell it happened. The engine will not answer at all when *nothing* is usable: that is an empty denominator, not a bounded one. + +Say the `reason` too, because the two lead somewhere different. `unusable_shares` and `unavailable_cost` are facts missing from the holding, and the repair is to supply them. `integrity_oversell` is not a missing fact: the history carries a sell of that ticker with no matching prior buy — ordinary for an export that starts mid-account — so the replay clamped it and the recorded share count is not what the history can account for. The repair is the earlier transactions, and telling that user their position "has no cost on record" sends them after a number they already gave you. An `integrity_oversell` entry can also name a ticker the book records *no* position in; that is not a stray, it is the point, because a recorded zero is exactly what an unmatched sell puts in doubt. Never state "you don't hold X" from a book that excluded X for this reason. + +A premise about an `integrity_oversell` holding is refused rather than answered, and the refusal names the ticker and the reason. That refusal is narrow on purpose: it is one position, the rest of the book still answers, and the user is told what would make the question answerable. Do not read it as the account being unusable — that whole-book refusal was #673's defect. The same applies to a rule collision. Each row carries `partial_book: true` when the book it was judged against was bounded, and that qualifier belongs in what you say: the user wrote their cap against their whole book, an excluded position reads as weight zero, and a cap that a hidden position is breaching comes back `clear`. Report the state and say which book it was measured on. diff --git a/skills/fomo-kernel/schemas/evaluation-challenge.schema.json b/skills/fomo-kernel/schemas/evaluation-challenge.schema.json index 9f06e07e..4bc8920c 100644 --- a/skills/fomo-kernel/schemas/evaluation-challenge.schema.json +++ b/skills/fomo-kernel/schemas/evaluation-challenge.schema.json @@ -18,7 +18,7 @@ "properties": { "topic": { "enum": ["basis", "price_basis", "position", "concentration", "cash", "rule_collision", "disclosure", "excluded_holding"], - "description": "Which family of fact this is. basis: which book answered, how current it was, and its exact identity -- unconditional, because a weight quoted without the book it was measured on is a number about an unknown denominator. price_basis: which market session that book was valued at (#618), present only when the basis carries price observations and never manufactured for an unpriced answer -- basis.as_of is the last row of the RECORD (the last trade or the snapshot anchor) and says nothing about when the market was observed, while since #611 every weight below is a share of a current close, so the same premise re-asked returns different numbers with no attributable cause unless the answer says which close. The frame summary (basis.price_observations.as_of) is stated unconditionally and a per-instrument entry is added only where that instrument's own session differs from it: naming every ticker on a same-day frame would pad the floor with one entry per holding saying the same thing, while a frame date alone lets one fresh instrument stand in for a stale one (#583 section 2). position: what this trade does to the position the user actually asked about; its before-weight entry appears only when the ticker is already held, since a weight of zero for a name the user does not own is arithmetic rather than a fact worth a sentence. concentration: book concentration and driver overlap after the trade; ai_pct/max_sector_pct are omitted rather than stated as a false zero on a book with no sector/AI classification at all -- the separate unclassified_book disclosure covers that case, and unmapped_driver covers the narrower one where it is the premise's own ticker that cannot be classified. A PARTIALLY classified book is the case this omission does not reach: max_sector is present, so both readings are stated as measurements, and they are -- of the classified part of the book only. unclassified_book is what says so, and names the weight it was measured without (#598) -- and the two boolean triggers are stated only when true, never as an explicit false. cash: what the trade leaves in cash; whether that balance can be trusted rides the disclosure topic instead of being folded in here. rule_collision: every one of the user's own rules this trade would_breach or leaves already_over, plus every rule that could not be judged at all (unjudged/unmapped) -- clear is the only state that may pass in silence. disclosure: every limitation the engine attached to these numbers, addressed by the same index a required_coverage disclosure entry and an --agent-case anchor would use. excluded_holding: which held positions sat outside the denominator every percentage above was measured against (#515) -- partial_book (a disclosure entry) says THAT something was excluded; this says WHAT." + "description": "Which family of fact this is. basis: which book answered, how current it was, and its exact identity -- unconditional, because a weight quoted without the book it was measured on is a number about an unknown denominator. price_basis: which market session that book was valued at (#618), present only when the basis carries price observations and never manufactured for an unpriced answer -- basis.as_of is the last row of the RECORD (the last trade or the snapshot anchor) and says nothing about when the market was observed, while since #611 every weight below is a share of a current close, so the same premise re-asked returns different numbers with no attributable cause unless the answer says which close. The frame summary (basis.price_observations.as_of) is stated unconditionally and a per-instrument entry is added only where that instrument's own session differs from it: naming every ticker on a same-day frame would pad the floor with one entry per holding saying the same thing, while a frame date alone lets one fresh instrument stand in for a stale one (#583 section 2). position: what this trade does to the position the user actually asked about; its before-weight entry appears only when the ticker is already held, since a weight of zero for a name the user does not own is arithmetic rather than a fact worth a sentence. concentration: book concentration and driver overlap after the trade; ai_pct/max_sector_pct are omitted rather than stated as a false zero on a book with no sector/AI classification at all -- the separate unclassified_book disclosure covers that case, and unmapped_driver covers the narrower one where it is the premise's own ticker that cannot be classified. A PARTIALLY classified book is the case this omission does not reach: max_sector is present, so both readings are stated as measurements, and they are -- of the classified part of the book only. unclassified_book is what says so, and names the weight it was measured without (#598) -- and the two boolean triggers are stated only when true, never as an explicit false. cash: what the trade leaves in cash; whether that balance can be trusted rides the disclosure topic instead of being folded in here. rule_collision: every one of the user's own rules this trade would_breach or leaves already_over, plus every rule that could not be judged at all (unjudged/unmapped) -- clear is the only state that may pass in silence. disclosure: every limitation the engine attached to these numbers, addressed by the same index a required_coverage disclosure entry and an --agent-case anchor would use. excluded_holding: which positions sat outside the denominator every percentage above was measured against, whether because they could not be valued (#515) or because the ledger's integrity record names them (#673) -- partial_book (a disclosure entry) says THAT something was excluded; this says WHAT, and `detail.reason` says why, which is what tells the two apart." }, "value": { "type": ["string", "number", "boolean"], @@ -53,8 +53,8 @@ "description": "Set only when the frozen state is already_over: whether this trade moves the relevant reading in the bad direction. null everywhere else -- a boolean that means nothing outside that one state is worse than an absent one." }, "reason": { - "enum": ["unusable_shares", "unavailable_cost"], - "description": "Why this excluded holding could not be valued, mirroring trade-evaluation.schema.json's consequence.excluded_holdings[].reason exactly. Engine-assigned; never a user or agent claim." + "enum": ["unusable_shares", "unavailable_cost", "integrity_oversell"], + "description": "Why this holding is outside the usable book, mirroring trade-evaluation.schema.json's consequence.excluded_holdings[].reason exactly (and consequence.EXCLUSION_REASONS, which both mirror). Engine-assigned; never a user or agent claim. `integrity_oversell` says the holding was left out because the ledger recorded a sell with no matching prior buy, not because a valuation fact was missing (#673) -- an answer that reports it as unvalued would send the user after a cost that is already on record." } } } diff --git a/skills/fomo-kernel/schemas/trade-evaluation.schema.json b/skills/fomo-kernel/schemas/trade-evaluation.schema.json index 437d989e..9368f6d4 100644 --- a/skills/fomo-kernel/schemas/trade-evaluation.schema.json +++ b/skills/fomo-kernel/schemas/trade-evaluation.schema.json @@ -127,7 +127,7 @@ }, "excluded_holdings": { "type": "array", - "description": "Held positions that could not be valued, and are therefore outside the denominator every number above is measured against (#515). Empty when the whole book was usable. The identities live here rather than inside `disclosures`, which is machine-readable keys and never carries data: `partial_book` says THAT something was excluded, this says WHAT. Any consumer stating a derived number must name these alongside it -- a partial denominator that does not say it is partial is worse than the refusal it replaced.", + "description": "Positions left out of the usable book, and therefore outside the denominator every number above is measured against (#515, #673). Empty when the whole book was usable. The identities live here rather than inside `disclosures`, which is machine-readable keys and never carries data: `partial_book` says THAT something was excluded, this says WHAT. Any consumer stating a derived number must name these alongside it -- a partial denominator that does not say it is partial is worse than the refusal it replaced. Nearly always a currently-held position; an `integrity_*` entry can also name a ticker the book records no position in, because a recorded zero is exactly what an unmatched sell puts in doubt (#673).", "items": { "type": "object", "additionalProperties": false, @@ -136,8 +136,8 @@ "ticker": {"type": "string", "minLength": 1}, "reason": { "type": "string", - "enum": ["unusable_shares", "unavailable_cost"], - "description": "Which fact was missing. Engine-assigned; never a user or agent claim." + "enum": ["unusable_shares", "unavailable_cost", "integrity_oversell"], + "description": "Why this holding is outside the usable book, mirroring consequence.EXCLUSION_REASONS. Engine-assigned; never a user or agent claim. The first two are facts missing from the holding (#515). `integrity_oversell` is not a missing fact but an underivable one (#673): the ledger recorded a sell with no matching prior buy, so the replay clamped it and this holding's share count is not what the history can account for. That distinction is load-bearing -- a premise about an `integrity_*` holding is refused by name, while a holding with no cost on record is merely outside the denominator." } } } diff --git a/tests/test_consider.py b/tests/test_consider.py index b08ce381..4ad4a65c 100644 --- a/tests/test_consider.py +++ b/tests/test_consider.py @@ -649,10 +649,234 @@ def test_a_book_where_nothing_can_be_valued_still_refuses(): ]) run = _run("consider", "--root", tmp, "--premise", '{"ticker": "NVDA", "side": "buy", "price": 130.0, "qty": 5}') - _fails(run, "no holding that can be valued: NVDA") + _fails(run, "no usable holding: NVDA (unavailable_cost)") assert not os.path.exists(_evaluation_path(tmp)) +# ───── D2. an integrity warning is scoped to the holding it names (#673) ───── +# +# `_INTEGRITY_BOOK` is one mid-history export in ledger shape: five declared +# positions, one of which (ORPH) is sold beyond what the history can account +# for and then bought back. That is the ordinary shape of an export that starts +# mid-account -- the earlier buy is simply outside the window -- and it is why +# the whole-book gate this section replaces was permanent: the warning is +# derived from history that has already happened, so no future trade clears it. +# +# Every position costs 1000 except ORPH, whose re-buy costs 2000, which is what +# makes the two denominators tell each other apart: over the whole book AAA is +# 1000/6000, over the usable book it is 1000/4000. A fixture where the excluded +# holding happened to weigh the same as the others could not fail. +_INTEGRITY_CLEAN = [{"ticker": t, "shares": 10, "avg_cost": 100.0, + "market": "US", "currency": "USD"} for t in ("AAA", "BBB", "CCC", "DDD")] +_INTEGRITY_BOOK = [ + _snapshot_event("2026-01-01", _INTEGRITY_CLEAN + [ + {"ticker": "ORPH", "shares": 10, "avg_cost": 100.0, "market": "US", "currency": "USD"}]), + # 30 sold against 10 the history can see: `derive_holdings` clamps to zero + # and records {"issue": "oversell", "ticker": "ORPH", ...}. + _trade_event("2026-01-05", "ORPH", "sell", 30, 150.0), + _trade_event("2026-01-06", "ORPH", "buy", 20, 100.0), +] + + +def test_an_unmatched_sell_excludes_that_holding_instead_of_disabling_the_account(): + """#673's owning outcome: a book the review lane can review is a book the + pre-trade lane can evaluate. + + Before this leaf `consider` exited 2 with "canonical PortfolioBasis has + claim-affecting integrity warnings" for every premise on this book, forever, + while the review lane disclosed the identical condition and delivered its + card. The gate read a whole-book summary of a per-holding fact -- exactly + the shape #515 had already removed one line above it. + + The numbers are the assertion, not the presence of an answer. ORPH's re-buy + is 2000 of a 6000 whole book, so if the denominator had not shrunk AAA would + read 0.1667 here; over the four usable holdings it reads exactly 0.25.""" + with tempfile.TemporaryDirectory() as tmp: + _write_ledger(os.path.join(tmp, "ledger.jsonl"), _INTEGRITY_BOOK) + payload = _ok(_run("consider", "--root", tmp, + "--premise", '{"ticker": "AAA", "side": "buy", "qty": 10, ' + '"price": 100.0, "currency": "USD"}')) + row = payload["evaluation"] + result = row["consequence"] + assert result["excluded_holdings"] == [ + {"ticker": "ORPH", "reason": "integrity_oversell"}], ( + "the excluded holding and the reason it was excluded must both be named -- " + "an exclusion the user cannot see is the silent partial denominator #515 " + "refused, and a reason of 'unavailable_cost' would send them after a cost " + "that is on record") + assert "partial_book" in result["disclosures"] + assert "ORPH" not in result["before"]["weights"] + assert set(result["before"]["weights"]) == {"AAA", "BBB", "CCC", "DDD"} + for ticker in ("AAA", "BBB", "CCC", "DDD"): + assert abs(result["before"]["weights"][ticker] - 0.25) < 1e-9, ( + f"{ticker} must be sized against the 4000 that is usable, not the 6000 " + f"whole book: got {result['before']['weights'][ticker]}") + assert abs(result["before"]["max_pct"] - 0.25) < 1e-9 + # 2000 of the 5000 the bounded book becomes -- the question was answered, + # and answered on the same denominator it was asked on. + assert abs(result["after"]["weights"]["AAA"] - 0.4) < 1e-9 + _check_evaluation_shape(row) + # The exclusion is owed to the user, not merely present in the payload: + # the challenge is where a bounded denominator becomes a fact the answer + # has to state, and the reason is what points the repair at the missing + # transactions instead of at a cost that is on record. + _check_challenge_shape(payload["challenge"]) + owed = [entry for entry in payload["challenge"]["must_state"] + if entry["topic"] == "excluded_holding"] + assert owed == [{"topic": "excluded_holding", + "anchor": "consequence.excluded_holdings.0.ticker", + "value": "ORPH", "detail": {"reason": "integrity_oversell"}}], owed + assert "partial_book" in [entry["key"] for entry + in payload["challenge"]["required_coverage"]], ( + "an answer that does not state the bounded denominator must be refused, " + "not merely discouraged") + + +def test_a_rule_is_judged_against_the_book_the_integrity_exclusion_left(): + """#515's second invariant, reached through #673's exclusion: the verdict + is computed on the bounded book and says so. + + The cap is 20% and AAA is already 25% of the usable book, so the honest + verdict is `already_over` -- this trade digs deeper into a line the user is + past, it is not what crosses it. Over the whole 6000 book AAA's before + weight is 16.7%, under the line, and the same rule would come back + `would_breach` instead. So the state, not merely the `partial_book` marker, + is what pins which denominator the rule was judged on.""" + with tempfile.TemporaryDirectory() as tmp: + _write_ledger(os.path.join(tmp, "ledger.jsonl"), _INTEGRITY_BOOK) + with open(os.path.join(tmp, "rules.jsonl"), "w", encoding="utf-8") as handle: + handle.write(json.dumps({ + "type": "rule", "rule_id": "r1", "date": "2026-01-01", + "text": "Cap any single position at 20%.", + "metric_key": "max_pos_pct", "problem_key": "oversize", + "status": "tracking"}) + "\n") + _ok(_run("set-cap", "--root", tmp, "--pct", "0.20")) + payload = _ok(_run("consider", "--root", tmp, + "--premise", '{"ticker": "AAA", "side": "buy", "qty": 10, ' + '"price": 100.0, "currency": "USD"}')) + collisions = payload["evaluation"]["rule_collisions"] + assert collisions, "the tracked rule must still be evaluated, not suppressed" + assert all(row.get("partial_book") is True for row in collisions), ( + "a collision judged against a bounded book must carry the qualifier") + cap = [row for row in collisions if row["metric_key"] == "max_pos_pct"] + assert cap and cap[0]["state"] == "already_over", ( + f"the cap must be judged on the usable book's own weights: {cap}") + + +def test_a_premise_on_the_integrity_excluded_holding_refuses_by_name(): + """The bounded answer is an answer about the rest of the book. It is not an + answer about the damaged holding: the entire content of a consequence for + ORPH is what its position becomes, and the position it starts from is the + number the warning says is not derivable. + + Both sides, because the sell is the one that would otherwise be refused for + the wrong reason -- `validate_premise` reaches it first and calls it "not + currently held", which is a claim about the book, and the wrong claim.""" + for side in ("buy", "sell"): + with tempfile.TemporaryDirectory() as tmp: + _write_ledger(os.path.join(tmp, "ledger.jsonl"), _INTEGRITY_BOOK) + run = _run("consider", "--root", tmp, + "--premise", '{"ticker": "ORPH", "side": "%s", "qty": 5, ' + '"price": 100.0, "currency": "USD"}' % side) + payload = _fails(run, "ORPH") + assert "integrity_oversell" in payload["error"], ( + "a refusal the user can act on names the reason, not only the ticker: " + f"got {payload['error']!r}") + assert "not currently held" not in payload["error"], ( + "the recorded zero is exactly what the warning puts in doubt") + assert not os.path.exists(_evaluation_path(tmp)), ( + "a refused question leaves no row behind") + + +def test_a_book_whose_every_holding_is_integrity_excluded_still_refuses(): + """The floor under exclude-and-disclose, on the integrity side. Excluding + the last holding leaves an empty denominator, which is not a bounded answer + -- it is no answer, and inventing one would be worse than refusing. The + refusal names each holding's own reason, because "nothing could be valued" + is false here and would send the user after costs that are on record.""" + with tempfile.TemporaryDirectory() as tmp: + _write_ledger(os.path.join(tmp, "ledger.jsonl"), [ + _snapshot_event("2026-01-01", [ + {"ticker": t, "shares": 10, "avg_cost": 100.0, "market": "US", "currency": "USD"} + for t in ("AAA", "BBB")]), + _trade_event("2026-01-05", "AAA", "sell", 30, 150.0), + _trade_event("2026-01-05", "BBB", "sell", 30, 150.0), + _trade_event("2026-01-06", "AAA", "buy", 5, 100.0), + _trade_event("2026-01-06", "BBB", "buy", 5, 100.0), + ]) + run = _run("consider", "--root", tmp, + "--premise", '{"ticker": "CCC", "side": "buy", "qty": 1, "price": 100.0}') + _fails(run, "no usable holding: AAA (integrity_oversell), BBB (integrity_oversell)") + assert not os.path.exists(_evaluation_path(tmp)) + + +def test_a_bad_ledger_warning_stays_whole_book_fatal_even_though_it_names_a_ticker(): + """The other half of the ruling: only warnings this route can scope AND + name a reason for degrade locally. A `bad_*` row means the replay could not + read the source at all, and `portfolio_basis` drops the entire basis for one + -- naming a ticker does not make unknown state a bounded subset. Pinned end + to end so #673's per-holding exclusion cannot later be widened into it.""" + with tempfile.TemporaryDirectory() as tmp: + _write_ledger(os.path.join(tmp, "ledger.jsonl"), [ + _snapshot_event("2026-01-01", _INTEGRITY_CLEAN + [ + {"ticker": "ORPH", "shares": 10, "avg_cost": "not a number", + "market": "US", "currency": "USD"}]), + ]) + run = _run("consider", "--root", tmp, + "--premise", '{"ticker": "AAA", "side": "buy", "qty": 1, "price": 100.0}') + _fails(run, "no trustworthy canonical current book") + assert not os.path.exists(_evaluation_path(tmp)) + + +def test_an_integrity_warning_this_route_cannot_scope_refuses_the_whole_book(): + """The adapter's own defensive floor, at the layer that decides. + + Both warning shapes are `ledger.derive_holdings`' own rows. `bad_trade_event` + (ledger.py:568) carries an issue and a `detail` and no ticker at all; + `bad_avg_cost` (ledger.py:550) does name its ticker -- and is still refused + whole-book, because naming a holding is not the same as this route having a + per-holding reason it can disclose for it. Neither can reach a canonical + basis today (portfolio_basis drops the basis for any `bad_*` row), which is + exactly why the refusal is asserted here rather than only through the CLI: + an exclusion is a *disclosed* degradation, and a warning with no reason in + `EXCLUSION_REASONS` would be absorbed into a book that then looks whole.""" + import consequence as cq # noqa: E402 (engine dir already on sys.path) + for warning, named in (({"issue": "bad_trade_event", "detail": '{"type": "trade"}'}, + "bad_trade_event"), + ({"issue": "bad_avg_cost", "ticker": "ORPH"}, "bad_avg_cost")): + basis = {"as_of": "2026-01-01", "integrity": [warning], + "current_book": {"holdings": { + "AAA": {"shares": 10, "cost_total": 1000.0, + "currency": "USD", "market": "US"}}}} + try: + cq.rows_from_portfolio_basis(basis) + except cq.ConsequenceError as exc: + assert "cannot be scoped to one holding" in str(exc) and named in str(exc), ( + f"the refusal must name the warning it could not scope: {exc}") + else: + raise AssertionError( + f"an integrity warning this route has no reason for ({named}) must not " + "yield a usable book") + + +def test_exclusion_reasons_constant_is_exactly_what_both_schemas_declare(): + """One vocabulary, three files. `excluded_holdings[].reason` is written by + consequence.py, stored under trade-evaluation.schema.json and restated live + under evaluation-challenge.schema.json, and the challenge schema's own + description promises it mirrors the stored one exactly. A reason added to + the engine without both enums is a row the validator rejects at delivery.""" + import consequence as cq # noqa: E402 + stored = set(EVALUATION_SCHEMA["properties"]["consequence"]["properties"] + ["excluded_holdings"]["items"]["properties"]["reason"]["enum"]) + assert stored == set(cq.EXCLUSION_REASONS) + live = set(CHALLENGE_SCHEMA["properties"]["must_state"]["items"]["properties"]["detail"] + ["properties"]["reason"]["enum"]) + assert live == stored, "the challenge schema promises it mirrors the stored one exactly" + assert set(cq.INTEGRITY_EXCLUSION_REASONS) < set(cq.EXCLUSION_REASONS), ( + "the integrity subset must be a strict subset -- it is what decides which " + "exclusions also refuse a premise about them") + + def test_consider_refuses_mixed_usd_twd_current_book_before_any_verdict(): """#497: native USD/TWD values have no canonical aggregate without an FX frame. The #496 projection must therefore stop consider before it can @@ -2172,6 +2396,13 @@ def _check_challenge_shape(challenge): assert entry["anchor"].split(".")[0] in ("basis", "consequence", "rule_collisions") if "detail" in entry: assert set(entry["detail"]) <= detail_allowed + # #673: the enum, not only the key. `reason` is what tells an + # excluded holding the answer must send the user after a missing + # cost from one it must send them after missing transactions, so a + # value the schema does not declare is a sentence nothing defines. + if entry["detail"].get("reason") is not None: + assert entry["detail"]["reason"] in set( + entry_props["detail"]["properties"]["reason"]["enum"]), entry quote_props = props["quote_verbatim"]["items"]["properties"] for quoted in challenge["quote_verbatim"]: