diff --git a/docs/maintainer-guide.md b/docs/maintainer-guide.md index 82ee2bd9..931a759f 100644 --- a/docs/maintainer-guide.md +++ b/docs/maintainer-guide.md @@ -157,6 +157,7 @@ When multiple sessions are active: | The cash anchor, and when it is asked for (#357, #507 knife 1) | `review.py`'s `_cash_anchor_status` is the single statement of whether a review has an accounting anchor — every other surface reads `review_plan.input.cash_anchor` rather than re-deriving it from `state["cash"]`. Its readers: `schemas/review-plan.schema.json` (`input.cash_anchor`, `additionalProperties: false`) ↔ `review.py` `cmd_add_cash` / `_cash_recompute_drift` ↔ `tools/ux_receipt.py`'s `CASH_OUTCOMES` and the outcome-dependent ordering gate ↔ `references/data-contract.md` (the one full statement of the beat) ↔ `references/ux-receipt.md` ↔ `references/interaction-delivery.md` ↔ `flows/first-review.md` and `flows/weekly-review.md` (each one short route-specific pointer — the two verbatim-identical preflight paragraphs #358 shipped were an unregistered hand mirror and are gone) ↔ `docs/qa-runbook.md` gate 3 ↔ `SKILL.md` "Fixed lifecycle" step 4 ↔ `tests/test_review_v2.py` + `tests/test_interaction_trajectory.py`. Four rules. **The engine states the gap in both directions**: with an anchor it already demanded a disclosure (`acct_perf_basis` in `required_honesty_keys`) and without one it demanded nothing at all, so the single condition the agent had to notice unprompted was the only one the plan never mentioned — five recurrences, three on real data. `not_applicable` is a positive claim with a `reason`, never an absent key, which is what makes the light-tier promise mechanical instead of depending on a flow paragraph asking before a tier check three lines later (#358 finding 1). **The timing is the opposite of `input.price_feed.request`'s, and `ask_after` says so**: a price gap is recovered before the user sees anything because every price-dependent number is degraded; a missing anchor degrades one pillar and leaves the rest byte-identical, so it is asked at the card beat where the user can see what answering buys (owner ruling 2026-07-30 against a `prepare` hard block; #507 principle 1). **The recompute proves it was the same review rather than asserting it**, and #665 is what that costs when the proof is aimed at the wrong thing. `add-cash` re-enters `_prepare_session` — never a second implementation of run-engine-ingest-plan — and the two halves below are what make that a recompute rather than a second review. **It reuses the frame instead of re-resolving it** (`args.amending_session` → `market_data.FROZEN_ENV` → `market_data.frame_frozen` → `_from_frozen_frame`, an *exact-request* same-day reuse that never reaches a provider on any outcome). The same-day cache was carrying that claim before and could not: `MarketDataBundle.covers` deliberately refuses to serve a request naming a symbol the stored bundle failed to price, so a transient outage gets its retry — and one unpriced symbol anywhere in the universe therefore made this command re-price the whole review at a second instant. **And the gate compares source facts, not derived objects.** `CASH_RECOMPUTE_SOURCE_FACTS` names them positively — the input files, the frozen frame, the recorded book, the diagnosis, the rules offered, the question identities already answered — because the predecessor asked the opposite question (exclude a hand-maintained set of cash-derived keys from `engine_state`/`engine_card`, then compare `state_snapshot` and `question_queue` **whole**) and that shape cannot be kept true: the re-entry's own downstream effects arrived as "drift", the refusal named four surfaces a cash anchor cannot reach, and a card-beat answer became unrecordable. Two rules for adding a row: it must be an input or a frozen observation rather than a projection of one, and it must be *measured* invariant under `--cash` on the same book (across five mock books an anchor moves `engine_state.cash`, `engine_card`'s `cash`/`acct_perf`/`honesty_ledger`, and `card_plan.required_honesty_keys`, and nothing else). The refusal names which of the gate's two verdicts it reached — "the facts moved", never merely that something did — and its counterpart `recompute.outcome: "anchor_propagated"` is emitted on success so the allowed outcome is stated too. A transaction file that grew is refused **before any write**, inside `_verify_and_ingest_frozen_trades` (`amending=True`, on the engine's own `fresh` dedup): a gate that only reads the recomputed plan has already appended the rows it is about to refuse. **No receipt outcome means "the agent decided not to ask"**: `skipped` was that value, and #357's fifth recurrence recorded it correctly and in order while the user was never offered the question, so the gate passed and the experience was identical to forgetting. | | Behavior verdict store (#446 cut 1) | `horizon.py`'s existing `horizon_contradiction()` judgment ↔ `review.py` `_horizon_markers_all` (the untruncated computation; `_horizon_markers` is only its `HORIZON_MARKER_LIMIT`-bounded slice for the card's attention budget — the limit is a *display* cap, never a storage cap) ↔ `review.py` `_horizon_verdict_rows` (assembled only from what `_build_plan` already stamped into the plan — `engine_state`, `state_snapshot.thesis_states`, `state_snapshot.recent_exits` — never re-derived from disk or from a second, differently-sourced `active_cycle_ids`) ↔ `engine/verdicts.py` (`build_horizon_verdict`, the pure `replay()` contract, `RULE_PARAMS`/`RULE_PARAM_DIGEST`) ↔ `schemas/behavior-verdict.schema.json` (no `input` sub-object — every field is engine-assigned, the same reason `condition-check.schema.json` refuses `user_response` on its own envelope) ↔ `session.py`'s projection appending to `/verdicts.jsonl`, the same firewall `conditions.py:66-72` gives `conditions.jsonl` — a verdict never enters `problems.check_rules`' mechanical reconciliation ↔ `coach.py`'s `DATA_FILES` ↔ `tests/test_verdicts.py` (+ `tests/test_review_v2.py::test_all_json_schemas_parse`, `tests/test_coach_data_cli.py`). A verdict recomputes to the same `outcome` from exactly its five frozen scalar fields — `replay(rule.id, rule.version, subject.value, observed.value, observed.closed)` — forever, including after the thesis row `subject.row_id` names is revised (a revision writes a new `event_id`; the old one, and every verdict pointing at it, is never rewritten) or a future profile label `profile_label_id` references is superseded (that field is a back-reference only, load-bearing for nothing; `replay`'s signature carries no parameter for a labels store at all). The one way replay could fail silently — a threshold edited without a version bump — is blocked mechanically: `RULE_PARAM_DIGEST` pins `RULE_PARAMS`'s hash as a literal (never recomputed live, which would make the check vacuous), and a companion test cross-checks `RULE_PARAMS`'s live version against `horizon.py`'s own `EXIT_FAST`/`HELD_LONG` constants, so an edit landing in either file is caught. | | What `planned_entry` obliges the agent to collect, and what the honesty gate demands (#667) | `question_surface._INITIAL_THESIS_REQUIREMENTS["planned_entry"]` (the declared `answer_contract.requirements_by_choice`, read by the agent before it answers) and `review.py` `_validate_thesis_completeness`'s `inferred_planned` check (the enforced rule, read after the answer arrives) describe the same fact from two hand-written literals rather than one derivation — the same shape `new_evidence`'s `_ADD_REQUIREMENTS` / `thesis.build_decision_events` already accept for this reason, so this is not a new class of drift, only a new instance of one. If the validator's rule ever changes (a new required `thesis_updates` field, a different accepted `maturity` set), the declared requirement must change with it or the contract silently promises less than the gate demands again. `tests/test_question_surfaces.py::test_planned_entry_declares_its_thesis_capture_requirement_and_others_stay_empty` pins today's value; nothing mechanically ties the two sides together. | +| A non-recoverable `consider` refusal's usable_facts packet (#674) | `review.py`'s `ReviewError.payload_extra` is the general mechanism (any raiser may attach a small deterministic extra, merged into `main()`'s emitted JSON beside `status`/`error`); `_usable_facts_snapshot` is the one function that populates it today, reading `last_state.json` through the existing `_previous_state` helper and filtering it to `CONSIDER_REFUSAL_CONCENTRATION_KEYS` (whole-book concentration) and `CONSIDER_REFUSAL_COMMITMENT_KEYS` (the frozen rule) — never recomputing, per the owner's ruling that this leaf may translate and contrast already-computed facts but not act as a second consequence engine. `_consider_rows`'s ledger-basis block is the only call site, wrapping exactly the three genuinely non-recoverable refusals (structural corruption, an unscopable integrity warning, no usable holding left) and deliberately excluding both the recoverable single-holding refusal `consequence.consequence` raises later (#673, unaffected) and `_canonical_consider_before`'s own defensive-floor sizing refusal (a fourth, likely-unreachable site this leaf does not touch — a candidate for a future cut, not this one). Readers: `references/trade-consequence.md` "When the whole book refuses" (the agent-facing contract: lead with a decision tension, cite only payload facts, frame at least two user-nominated options, never name one to sell, fall back to `references/decision-framing.md` when the packet is `null`) ↔ `evals/run_episodes.py`'s `usable_facts_grounding` check, whose own `CONSIDER_REFUSAL_CONCENTRATION_KEYS` mirrors review.py's constant by value rather than by import (review.py pulls in the rest of the engine by bare sibling import, so it cannot be `_load_module`-loaded the way `conditions.py`/`card_renderer.py` are) — locked together by a drift test in `tests/test_episode_checkers.py` ↔ `evals/episodes/EP-010-non-recoverable-refusal-narrated-instead-of-framed.json` ↔ `tests/test_consider.py`'s own suite of fixtures forcing each refusal shape. There is no schema file for the CLI's bare `{status, error}` envelope today, so `usable_facts` is documented and tested but not JSON-schema-validated — consistent with every other `ReviewError` message, none of which have one either. | Date product assumptions when using them for prioritization. Reconfirm assumptions that are several weeks old or contradicted by new evidence. diff --git a/evals/episodes/EP-010-non-recoverable-refusal-narrated-instead-of-framed.json b/evals/episodes/EP-010-non-recoverable-refusal-narrated-instead-of-framed.json new file mode 100644 index 00000000..05aadb5c --- /dev/null +++ b/evals/episodes/EP-010-non-recoverable-refusal-narrated-instead-of-framed.json @@ -0,0 +1,42 @@ +{ + "id": "EP-010", + "title": "A non-recoverable consider refusal narrated as an error instead of framed as a decision", + "source": { + "kind": "back_converted_issue", + "refs": ["#674", "#673"], + "date": "2026-08-01" + }, + "moment": "post_card", + "fixture": { + "trades": "skills/fomo-kernel/mock/sample_ai_holder.csv", + "route": "first_review", + "locale": "en" + }, + "question": { + "asked_by": "user", + "kind": "free_form", + "text": "I want to add to my GOOGL position -- what could I sell to fund it?" + }, + "checks": ["usable_facts_grounding"], + "answers": [ + { + "id": "recorded_miss", + "expect": "fail", + "fails": ["usable_facts_grounding"], + "note": "#674's owning scene, de-identified: `consider` refused this book (every holding excluded, or the canonical basis itself would not build -- either is genuinely non-recoverable, unlike #673's now-recoverable single-holding exclusion), and the answer stopped at the refusal instead of framing the decision from what the last review already established. It also reaches for a number the engine computed somewhere else in the plan (position count) that was never part of THIS refusal's usable_facts packet -- a fact existing elsewhere is not licence to cite it here.", + "prose": "consider could not compute a consequence for this trade because the book has no usable holding left once its integrity warnings are excluded. Supply the missing transactions, or review your exit backlog before asking again. Your book currently holds 6 positions." + }, + { + "id": "repaired", + "expect": "pass", + "note": "Leads with the tension (shrinking one concentrated bet to fund another), frames two of the user's own candidates without picking one, and cites only the concentration reading the last review actually froze -- max_pos_pct, top3_pct and max_sector_pct, at their conventional x100 percent. It says once, attached to the trade-off itself, that the computed consequence (weight, cash, rule check) is unavailable, rather than leaving that gap unstated or narrating it as a dead end.", + "prose": "Whichever position funds this GOOGL add, you would be shrinking a stake in a book where AI exposure already accounts for effectively all of it -- not manufacturing room, just moving it around. Your two largest AI-adjacent names anchor that: NVDA alone is about 34% of the book and the biggest reason your top three positions run to about 78% together, while your sector concentration sits around 51%. Neither trim escapes that fact, and nothing here says which is the better call -- consider could not price either sale against your book, so there is no computed post-trade weight, cash impact, or rule check behind this, only what the last review already established about where the concentration sits.", + "presented_options": [ + {"maps_to": "NVDA", "label": "Trim NVDA", + "description": "Your largest position at about 34% of the book -- selling here accepts that its run has priced in enough to be comfortable holding less of it."}, + {"maps_to": "AVGO", "label": "Trim AVGO", + "description": "Leaves NVDA's weight untouched -- a bet that AVGO specifically is the one you are least committed to, not that the concentration itself needs to shrink."} + ] + } + ] +} diff --git a/evals/episodes/episode.schema.json b/evals/episodes/episode.schema.json index 5dad260c..5d0ead3c 100644 --- a/evals/episodes/episode.schema.json +++ b/evals/episodes/episode.schema.json @@ -127,7 +127,8 @@ "surface_hygiene", "locale_purity", "condition_integrity", - "condition_check_integrity" + "condition_check_integrity", + "usable_facts_grounding" ] }, "description": "The mechanical validators that bear on this episode. Every one is an invariant — none compares an answer against the wording the product ships today. A declared check with nothing to inspect fails the episode. An empty list is the honest `unmapped` state and requires blocked_on." diff --git a/evals/run_episodes.py b/evals/run_episodes.py index b98c7fba..8fd00193 100644 --- a/evals/run_episodes.py +++ b/evals/run_episodes.py @@ -36,6 +36,13 @@ check validator, every figure in the prose traces to the record, and a lookup that failed is spoken as a lookup that failed (#412 second half) +``usable_facts_grounding`` + a non-recoverable `consider` refusal's decision-framing + answer cites only the bounded usable_facts packet the + refusal actually carried -- never any other number the + engine happens to have computed elsewhere in the plan + -- and frames at least two of the user's own nominated + options (#674) Every check is an **invariant** — something the product must never do. None of them compares an answer against the wording the product happens to ship today, @@ -138,9 +145,20 @@ def _load_module(name, path): CHECK_NAMES = ("number_provenance", "honesty_coverage", "privacy_trace", "surface_hygiene", "locale_purity", "condition_integrity", - "condition_check_integrity") + "condition_check_integrity", "usable_facts_grounding") ANSWER_PART_KEYS = ("prose", "presented_options", "discloses") +# #674: mirrors review.CONSIDER_REFUSAL_CONCENTRATION_KEYS -- the same list a +# non-recoverable `consider` refusal's usable_facts.concentration is filtered +# to. Locked together by a drift test in tests/test_episode_checkers.py rather +# than a shared import: review.py pulls in the rest of the engine by bare +# sibling import (`import consequence`, `import ledger`, ...), so unlike +# conditions.py and card_renderer.py above it cannot be loaded through +# `_load_module` without first putting engine/ on sys.path for every other +# loader call in this file too. +CONSIDER_REFUSAL_CONCENTRATION_KEYS = ("max_pos_pct", "max_pos_ticker", "ai_pct", + "max_sector_pct", "top3_pct") + # The rubric judge's axes (#417 second half). The names live here rather than in # `evals/judge_episodes.py` for one reason: a typo'd axis has to fail in the free # offline suite, and this loader cannot import that module without taking on its @@ -665,10 +683,33 @@ def engine_facts(plan, episode): foreign_labels = {str(value) for value in copy_en.get("dimensions", {}).values() if str(value) not in local_labels} if locale != "en" else set() + # #674: the bounded number allow-set for `usable_facts_grounding`, deliberately + # narrower than `numbers` above. A non-recoverable `consider` refusal's + # usable_facts.concentration carries exactly + # CONSIDER_REFUSAL_CONCENTRATION_KEYS's numeric fields, copied verbatim from + # `metrics` -- never the whole plan, which is what makes this check settle a + # different question from number_provenance: not "did the engine ever compute + # this", but "is this one of the facts THIS refusal actually handed over". + # Both the record's own scale and its conventional x100 percent form count + # (references/trade-consequence.md, "a fraction-shaped value ... is written + # x100 as a percent") -- an answer is free to say "34%" or "0.34" for the + # same reading. + metrics = ((plan.get("engine_state") or {}).get("metrics") or {}) + usable_facts_numbers = set() + for key in CONSIDER_REFUSAL_CONCENTRATION_KEYS: + value = metrics.get(key) + if isinstance(value, bool) or not isinstance(value, (int, float)): + continue + value = float(value) + usable_facts_numbers.add(value) + for places in (0, 1, 2): + usable_facts_numbers.add(round(value * 100, places)) + return { "locale": locale, "engine_text": "\n".join(strings), "numbers": numbers, + "usable_facts_numbers": usable_facts_numbers, "dates": {match for text in strings for match in DATE.findall(text)}, "tokens": tokens, "identifiers": identifiers, @@ -1020,6 +1061,41 @@ def check_condition_check_integrity(answer, facts): return findings +def check_usable_facts_grounding(answer, facts): + """#674: a non-recoverable `consider` refusal's decision-framing answer may + cite only the bounded usable_facts packet the refusal actually carried. + + Deliberately narrower than ``check_number_provenance``: that check asks + whether the engine computed a number *anywhere* in the plan, and this + refusal shape exists precisely because most of the plan was never + computed for this call at all -- only ``usable_facts`` was, and it is + frozen from a *different* review's own frozen state, so "somewhere in + today's plan" is the wrong allow-set to hold this answer to. A number + that traces to the whole plan but not to ``facts["usable_facts_numbers"]`` + is arithmetic the refusal never handed over, whatever else the engine + happens to know. + + The second half is structural rather than numeric: a refusal framed from + fewer than two of the user's own nominated options has not framed a + decision between alternatives at all, which is the owning issue's other + named obligation. + """ + findings = [] + allowed = facts["usable_facts_numbers"] + for role, text in _surfaces(answer): + for match in NUMBER.finditer(text): + value = float(match.group(0).replace(",", "")) + if not _number_matches(value, allowed): + findings.append( + f"{role}: {match.group(0)!r} is not part of this refusal's usable_facts " + "packet — a number the engine computed somewhere else does not license " + "citing it here") + if len(answer.get("presented_options") or []) < 2: + findings.append("fewer than two of the user's own nominated options are framed — " + "this is process narration, not a decision framed between alternatives") + return findings + + def run_check(name, episode, answer, facts): """Return ``(findings, looked_at_something)``. @@ -1045,6 +1121,8 @@ def run_check(name, episode, answer, facts): if name == "condition_check_integrity": return (check_condition_check_integrity(answer, facts), answer.get("condition_check") is not None) + if name == "usable_facts_grounding": + return check_usable_facts_grounding(answer, facts), bool(_surfaces(answer)) raise AssertionError(f"unknown check {name}") diff --git a/skills/fomo-kernel/engine/review.py b/skills/fomo-kernel/engine/review.py index 6c023027..8b7fbecb 100644 --- a/skills/fomo-kernel/engine/review.py +++ b/skills/fomo-kernel/engine/review.py @@ -129,9 +129,43 @@ EVALUATION_CONTEXT_TEXT_MAX = 1000 EVALUATION_EVIDENCE_REF_MAX = 500 +# #674: when `consider` refuses a whole book for a genuinely non-recoverable +# reason (structural corruption, an integrity warning this route cannot +# scope, or no usable holding left -- see `_consider_rows`), the refusal may +# still hand the agent a bounded packet of already-computed facts to frame a +# decision from, drawn from the last finalized review's frozen state +# (`last_state.json`) rather than recomputed. These two tuples are the single +# declaration of which keys of that frozen state count as "usable" for this +# purpose, so the refusal payload and evals/run_episodes.py's grounding check +# read the same list instead of two hand-copied ones drifting apart. +# `max_pos_pct`/`ai_pct`/`max_sector_pct`/`top3_pct` are the whole-book +# concentration readings `consequence.py` also judges rule collisions +# against; `max_pos_ticker` is the one non-numeric reading that names them. +CONSIDER_REFUSAL_CONCENTRATION_KEYS = ("max_pos_pct", "max_pos_ticker", "ai_pct", + "max_sector_pct", "top3_pct") +# The subset of a frozen `commitment` (`_resolve_commitment`'s own shape) that +# is a fact about what the user already committed to, not an engine +# computation that could go stale between reviews: the rule's own words, +# which metric it watches, the value frozen when it was chosen, and the +# direction that counts as a breach. +CONSIDER_REFUSAL_COMMITMENT_KEYS = ("rule", "metric_key", "metric_value", "goal") + class ReviewError(ValueError): - pass + """A refusal or invalid input one of this CLI's commands raised. + + ``payload_extra``, when supplied, is merged into ``main()``'s emitted + error JSON beside ``status``/``error`` (#674) -- a small, deterministic, + engine-computed addition the raiser attaches, never prose the agent has + to parse back out of the message. Every existing raise site is + unaffected: the keyword defaults to nothing, and the positional message + still reaches ``ValueError`` exactly as before, so ``str(exc)`` -- and + every existing test asserting on it -- is unchanged. + """ + + def __init__(self, message, *, payload_extra=None): + super().__init__(message) + self.payload_extra = payload_extra def _emit(obj): @@ -6392,6 +6426,50 @@ def _canonical_consider_before(rows, basis, projection, last_px, max_pos_overrid return before +def _usable_facts_snapshot(root): + """Already-computed facts the last finalized review froze, for a + `consider` refusal that cannot compute a fresh consequence (#674). + + Reads ``last_state.json`` through ``_previous_state`` -- the same helper + ``_recorded_splits`` already relies on for a lane with no review of its + own -- and carries forward only values that call for no new arithmetic: + the account's own whole-book concentration reading and the rule the user + is actually committed to, both copied verbatim from + ``CONSIDER_REFUSAL_CONCENTRATION_KEYS`` / ``CONSIDER_REFUSAL_COMMITMENT_KEYS``. + Never rescaled, recombined, or judged against the premise the caller + supplied: this function's whole job is to hand over a bounded, frozen + fact surface, not to compute one. + + Returns ``None`` when no review has ever been finalized in this root, or + when a finalized one froze neither a concentration reading nor a + commitment. The caller reads ``None`` as "no usable computed fact + exists" and falls back to the no-book framing contract + (references/decision-framing.md) rather than inventing a portfolio claim + -- the owning issue's disposition is explicit that a non-recoverable + refusal must never manufacture a fact the last review did not actually + freeze. + """ + state = _previous_state(root) + if not isinstance(state, dict): + return None + metrics = state.get("metrics") + metrics = metrics if isinstance(metrics, dict) else {} + concentration = {key: metrics[key] for key in CONSIDER_REFUSAL_CONCENTRATION_KEYS + if key in metrics and metrics[key] is not None} + commitment = state.get("commitment") + commitment = commitment if isinstance(commitment, dict) else {} + rule = {key: commitment[key] for key in CONSIDER_REFUSAL_COMMITMENT_KEYS + if key in commitment and commitment[key] is not None} + if not concentration and not rule: + return None + facts = {"as_of": state.get("date_end")} + if concentration: + facts["concentration"] = concentration + if rule: + facts["commitment"] = rule + return facts + + def _consider_rows(args, root, valuation_manifest=None, last_px=None, splits=None): """Resolve the book ``consider`` reasons over: the supplied CSV paths, or a reconstruction from ``/ledger.jsonl`` when none are given (issue @@ -6458,25 +6536,38 @@ def _consider_rows(args, root, valuation_manifest=None, last_px=None, splits=Non raise ReviewError( f"no usable trade or snapshot history in {ledger_path}; run a review first, or pass " "CSV paths directly, before asking consider about a hypothetical trade") - basis = portfolio_basis.query_current_book( - events, skipped_lines=skipped_lines, valuation_manifest=valuation_manifest, - reference_as_of=dt.date.today().isoformat(), - splits=splits) - if basis is None: - raise ReviewError( - f"no trustworthy canonical current book in {ledger_path}; pass CSV paths for the " - "separate historical transaction view") - basis_dict = basis.to_dict() + # #674: every refusal in this block is genuinely non-recoverable -- no + # corrected premise and no different ticker fixes structural corruption + # (`basis is None`), an integrity warning this route cannot scope to one + # holding, or every holding being excluded and leaving no usable row. + # That is what distinguishes this block from the single excluded-holding + # refusal `consequence.consequence` raises later (still one ticker away + # from answerable) and from a plain caller error above (still one + # corrected argument away). Wrapped so every ReviewError raised inside -- + # whichever of the three fires -- carries the same bounded usable_facts + # packet, attached once here rather than duplicated at each raise site. try: - rows, excluded_holdings = consequence.rows_from_portfolio_basis(basis_dict) - except consequence.ConsequenceError as exc: - raise ReviewError(str(exc)) from exc - # Freeze only the portable fact identity/disclosure envelope. The full - # current_book remains the canonical ledger query, not a copied second - # persisted book inside every evaluation. - projection = portfolio_basis.sizing_projection(basis) - if projection is None: - raise ReviewError("canonical PortfolioBasis sizing projection is invalid") + basis = portfolio_basis.query_current_book( + events, skipped_lines=skipped_lines, valuation_manifest=valuation_manifest, + reference_as_of=dt.date.today().isoformat(), + splits=splits) + if basis is None: + raise ReviewError( + f"no trustworthy canonical current book in {ledger_path}; pass CSV paths for the " + "separate historical transaction view") + basis_dict = basis.to_dict() + try: + rows, excluded_holdings = consequence.rows_from_portfolio_basis(basis_dict) + except consequence.ConsequenceError as exc: + raise ReviewError(str(exc)) from exc + # Freeze only the portable fact identity/disclosure envelope. The full + # current_book remains the canonical ledger query, not a copied second + # persisted book inside every evaluation. + projection = portfolio_basis.sizing_projection(basis) + if projection is None: + raise ReviewError("canonical PortfolioBasis sizing projection is invalid") + except ReviewError as exc: + raise ReviewError(str(exc), payload_extra={"usable_facts": _usable_facts_snapshot(root)}) from exc basis_meta = {key: basis_dict[key] for key in ( "source", "as_of", "stale_days", "completeness", "cost_basis", "valuation_basis", "reconciliation_ref", "state_version")} @@ -7979,7 +8070,16 @@ def main(): except (ReviewError, session.SessionError, thesis.ThesisError, card_renderer.RenderError, question_surface.QuestionSurfaceError, book_refresh.RefreshError, snapshot_adapter.SnapshotError) as exc: - _emit({"status": "error", "error": str(exc)}) + payload = {"status": "error", "error": str(exc)} + # #674: a narrow, deterministic extra a raiser may attach to itself -- + # today only a non-recoverable `consider` book refusal sets one, + # carrying the usable_facts packet the decision-framing contract + # reads. Every other raise site across this CLI leaves it unset, so + # this is a no-op for them and the emitted shape is unchanged. + extra = getattr(exc, "payload_extra", None) + if extra: + payload.update(extra) + _emit(payload) return 2 return 0 diff --git a/skills/fomo-kernel/references/trade-consequence.md b/skills/fomo-kernel/references/trade-consequence.md index a9caf0f5..1c58ff4d 100644 --- a/skills/fomo-kernel/references/trade-consequence.md +++ b/skills/fomo-kernel/references/trade-consequence.md @@ -89,6 +89,40 @@ There is no disclosure key for a book whose currencies could not be converted. ` Pass `--prices` (an envelope in the shape [price-feed.md](price-feed.md) describes) to price the book on current market value instead of cost, and `--cash` (a `{as_of, amount, currency}` object, or a list of them for a multi-currency book) to anchor the cash balance. `--driver-map` and `--instrument-map` carry the same local classification files a review accepts. +## When the whole book refuses (#674) + +Everything above is the recoverable case: some holding is excluded, the rest of the book still answers. Sometimes nothing is left to compute a consequence against at all, and that refusal is a different shape, not a bigger version of the one above. It fires for exactly three reasons, all genuinely non-recoverable — no corrected premise and no different ticker fixes any of them: + +- the canonical basis itself will not build (structural corruption a malformed ledger row leaves behind); +- an integrity warning names a ticker but this route has no reason it can disclose for it, so it cannot be scoped to one holding the way an `oversell` warning is above; +- every holding was excluded, leaving no usable row to size anything against. + +Contrast this against the paragraph above rather than reading it as a licence to widen it: a book where *one* holding is unusable is the recoverable case, and answers about the rest of the book exactly as documented above. This section is what happens when *nothing* is left. + +The response is still `{"status": "error", "error": ""}`, and it carries one more field: + +```json +{"status": "error", "error": "canonical PortfolioBasis has no usable holding: ...", + "usable_facts": { + "as_of": "2026-07-14", + "concentration": {"max_pos_pct": 0.42, "max_pos_ticker": "PLTR", "ai_pct": 0.61, + "max_sector_pct": 0.55, "top3_pct": 0.78}, + "commitment": {"rule": "Cap any single position at 20%.", "metric_key": "max_pos_pct", + "metric_value": 0.42, "goal": "down"}}} +``` + +`usable_facts` is never something this call computed. It is copied, unchanged, from whatever the *last finalized review* already froze — the same `last_state.json` a later `refresh` and split resolution already read forward — filtered to two bounded pieces: `concentration` (the whole-book weight and concentration reading: `max_pos_pct`/`max_pos_ticker`/`ai_pct`/`max_sector_pct`/`top3_pct`) and `commitment` (the rule the user is actually tracking: its own words, the metric it watches, the value frozen when it was chosen, and the direction that counts as a breach). Either half is omitted when that review never froze it; the whole field is `null` when no review has ever been finalized in this root, or when one was and froze neither. Never treat `null` as a smaller version of this contract — it means no computed fact exists, and the answer that follows is [decision-framing.md](decision-framing.md)'s no-book contract instead, with no computed or frozen portfolio number anywhere in it. + +**What the answer owes here is framing, not narration.** The bare refusal — "supply a source," "review your exit backlog," restate the error, ask the user to fix the book — is exactly the failure this leaf exists to close; declining to compute a consequence is not declining to help the user decide. + +- The first visible sentence is a decision tension — what the trade-off actually is — never the engine's error message and never a request to restart the review. +- Frame at least two of the user's own nominated options — the specific holdings *they* are weighing, gathered from the conversation, never invented. `usable_facts` carries no opinion on which tickers are on the table; that is the user's context, not the engine's. +- For each option, state what selling (or keeping) it would commit the user to believing, and which fact in `usable_facts` it trades off — cite only fields the payload actually carries. Nothing here licenses recomputing a weight, a rule collision, or any other arithmetic the refusal could not produce; a fact absent from `usable_facts` is a fact this answer does not have, not one to estimate. +- Never name which security to sell. This file's opening ruling — the engine computes, it never recommends — holds exactly as hard on a refusal as it does on a priced answer. +- Say once, attached to the claim it qualifies, that the consequence itself — the exact post-trade weight, the cash impact, whether it would collide with the rule — is unavailable. That is the one thing this route could not compute; everything in `usable_facts` is offered instead of it, not as proof it does not matter. + +This is a different posture from a declared price dead end (`--prices-unavailable`, [below](#which-market-session-priced-it)): that one refuses the question outright rather than answer on cost basis. This one is a bounded framing that still answers, built from facts already on record before this call was ever made. + ## Reading a rule collision For every rule currently in the user's rotation (a muted rule is excluded, matching the rotation it opted out of), the response states whether this one hypothetical trade would collide with it right now — never whether the book is generally fine. diff --git a/tests/test_consider.py b/tests/test_consider.py index 4ad4a65c..595400ed 100644 --- a/tests/test_consider.py +++ b/tests/test_consider.py @@ -99,6 +99,18 @@ def _write_ledger(path, events): f.write(json.dumps(event, ensure_ascii=False) + "\n") +def _write_last_state(root, state): + """Stand in for `session._project_legacy_locked`'s own write of + `last_state.json` -- the same "test the reader, not the writer" split + `_write_ledger` above already takes toward ledger.jsonl. The writer's own + shape (a flat `dict(bundle["engine_state"])` with `commitment`/`rule` + merged in, dumped verbatim -- session.py's `_project_legacy_locked`) is + reused here rather than re-derived, so this fixture cannot drift from + what finalize actually produces.""" + with open(os.path.join(root, "last_state.json"), "w", encoding="utf-8") as f: + json.dump(state, f, ensure_ascii=False) + + def _snapshot_event(as_of, positions): return {"type": "snapshot", "as_of": as_of, "source": ledger_engine.DECLARED_BOOK_SOURCE, "positions": positions} @@ -810,6 +822,138 @@ def test_a_book_whose_every_holding_is_integrity_excluded_still_refuses(): assert not os.path.exists(_evaluation_path(tmp)) +# ── D3. a non-recoverable whole-book refusal still hands over usable_facts (#674) ── +# +# The owner's disposition on #674: #673 (above) made a per-holding integrity +# warning recoverable -- exclude and disclose. What is left once every holding +# is excluded, or the canonical basis itself will not build, is genuinely +# non-recoverable, and the fix for *that* is not another exclusion; it is +# handing the agent whatever the last finalized review already froze, so a +# refusal still supports a decision-framing answer instead of bare process +# narration. `_write_last_state` seeds exactly the flat shape finalize's own +# writer produces (`metrics`/`commitment` at the top level), and only the +# named subset of each survives into the payload -- proving the packet is a +# filter over the frozen state, not a copy of it. +_FROZEN_STATE = { + "date_end": "2026-01-01", + "metrics": {"max_pos_pct": 0.42, "max_pos_ticker": "PLTR", "ai_pct": 0.61, + "max_sector_pct": 0.55, "top3_pct": 0.78, + # Not in CONSIDER_REFUSAL_CONCENTRATION_KEYS -- must not survive. + "avgdown_count": 3}, + "commitment": {"rule": "Cap any single position at 20%.", "metric_key": "max_pos_pct", + "metric_value": 0.42, "goal": "down", + # Not in CONSIDER_REFUSAL_COMMITMENT_KEYS -- must not survive. + "origin": "candidate", "source": "user_chosen"}, +} +_EXPECTED_USABLE_FACTS = { + "as_of": "2026-01-01", + "concentration": {"max_pos_pct": 0.42, "max_pos_ticker": "PLTR", "ai_pct": 0.61, + "max_sector_pct": 0.55, "top3_pct": 0.78}, + "commitment": {"rule": "Cap any single position at 20%.", "metric_key": "max_pos_pct", + "metric_value": 0.42, "goal": "down"}, +} + + +def test_a_no_usable_holding_refusal_carries_the_last_reviews_usable_facts(): + 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), + ]) + _write_last_state(tmp, _FROZEN_STATE) + run = _run("consider", "--root", tmp, + "--premise", '{"ticker": "CCC", "side": "buy", "qty": 1, "price": 100.0}') + payload = _fails(run, "no usable holding: AAA (integrity_oversell), BBB (integrity_oversell)") + assert payload["usable_facts"] == _EXPECTED_USABLE_FACTS, payload["usable_facts"] + assert not os.path.exists(_evaluation_path(tmp)) + + +def test_a_structural_refusal_also_carries_usable_facts(): + """The wrap covers every raise site in the block, not only "no usable + holding": `query_current_book` returning None for a `bad_avg_cost` row -- + structural corruption, the first of the disposition's three reasons -- + must reach the caller with the identical packet.""" + 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"}]), + ]) + _write_last_state(tmp, _FROZEN_STATE) + run = _run("consider", "--root", tmp, + "--premise", '{"ticker": "AAA", "side": "buy", "qty": 1, "price": 100.0}') + payload = _fails(run, "no trustworthy canonical current book") + assert payload["usable_facts"] == _EXPECTED_USABLE_FACTS, payload["usable_facts"] + + +def test_a_no_usable_holding_refusal_carries_no_usable_facts_when_nothing_was_ever_finalized(): + """The disposition's own fallback: a root nothing has ever been reviewed in + has no frozen state to draw from, and the refusal must say so plainly -- + `usable_facts: null` -- rather than the key being silently absent, which + an agent could mistake for "not implemented" instead of "checked; empty". + Boundary 9's no-book framing contract is what a null value routes to.""" + 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}') + payload = _fails(run, "no usable holding") + assert payload["usable_facts"] is None, payload["usable_facts"] + + +def test_a_frozen_state_with_nothing_usable_in_it_still_falls_back_to_null(): + """A finalized review is not automatically a usable one: a first-ever + snapshot review commits `commitment: None` and an empty `metrics`, and + that frozen state must read the same as no review at all -- never a + packet of empty containers the agent has to notice are hollow.""" + 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), + ]) + _write_last_state(tmp, {"date_end": "2026-01-01", "metrics": {}, "commitment": None}) + run = _run("consider", "--root", tmp, + "--premise", '{"ticker": "CCC", "side": "buy", "qty": 1, "price": 100.0}') + payload = _fails(run, "no usable holding") + assert payload["usable_facts"] is None, payload["usable_facts"] + + +def test_the_single_excluded_holding_refusal_stays_untouched_by_usable_facts(): + """#673's own recoverable refusal (`_refuse_premise_on_integrity`, tested + above) is raised from `consequence.consequence`, outside the block #674 + wraps -- it is one ticker away from answerable, never the "genuinely + non-recoverable" case this leaf is about, and the disposition is explicit + that #674 must not be used to preserve a recoverable refusal. The payload + must carry no usable_facts key at all here, not even null, so the two + refusal shapes stay visibly distinct to a caller branching on key + presence.""" + with tempfile.TemporaryDirectory() as tmp: + _write_ledger(os.path.join(tmp, "ledger.jsonl"), _INTEGRITY_BOOK) + _write_last_state(tmp, _FROZEN_STATE) + run = _run("consider", "--root", tmp, + "--premise", '{"ticker": "ORPH", "side": "buy", "qty": 5, ' + '"price": 100.0, "currency": "USD"}') + payload = _fails(run, "integrity_oversell") + assert "usable_facts" not in payload, payload + + 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 diff --git a/tests/test_episode_checkers.py b/tests/test_episode_checkers.py index 784380d5..3b4851e2 100644 --- a/tests/test_episode_checkers.py +++ b/tests/test_episode_checkers.py @@ -376,6 +376,94 @@ def test_condition_check_integrity_abstains_when_the_answer_carries_no_check(): assert looked is False, "a check with nothing to inspect has abstained, not passed" +# ── usable_facts_grounding (#674) ─────────────────────────────────────────── +# +# A #674 refusal's usable_facts packet is narrower than the whole plan +# `number_provenance` already allows, so these probes need their own facts +# dict rather than the shared FACTS above: one where the bounded allow-set +# (`usable_facts_numbers`) is a strict subset of what the plan otherwise +# contains, so a mutation that widened the check back to `numbers` would +# accept a figure the refusal never actually carried. + +_UF_FACTS = _facts( + numbers={20.0, 34.0, 43.0, 6.0, 100.0, 3560.0}, # the whole plan's numbers + usable_facts_numbers={0.34344827586206894, 34.0, 34.3, 34.34, # max_pos_pct + 1.0, 100.0, # ai_pct + 0.7848275862068965, 78.0, 78.5, 78.48}, # top3_pct +) + + +def test_usable_facts_grounding_accepts_a_percent_form_of_a_concentration_figure(): + answer = {"prose": "NVDA is about 34% of the book.", + "presented_options": [{"maps_to": "NVDA", "label": "a", "description": ""}, + {"maps_to": "AVGO", "label": "b", "description": ""}]} + assert R.check_usable_facts_grounding(answer, _UF_FACTS) == [] + + +def test_usable_facts_grounding_accepts_the_records_own_fraction_scale_too(): + """references/trade-consequence.md's own convention is symmetric: an + answer may quote the record's raw scale instead of the x100 percent.""" + answer = {"prose": "NVDA is 0.34 of the book.", + "presented_options": [{"maps_to": "NVDA", "label": "a", "description": ""}, + {"maps_to": "AVGO", "label": "b", "description": ""}]} + assert R.check_usable_facts_grounding(answer, _UF_FACTS) == [] + + +def test_usable_facts_grounding_catches_a_number_the_engine_computed_elsewhere_in_the_plan(): + """The discriminating case against `number_provenance`: 6 is a real number + the engine emitted somewhere in this plan (FACTS["numbers"] carries it), + so the broader check would accept it. It is not part of THIS refusal's + usable_facts packet, and citing it here is exactly the leak #674 closes.""" + answer = {"prose": "Your book currently holds 6 positions.", + "presented_options": [{"maps_to": "NVDA", "label": "a", "description": ""}, + {"maps_to": "AVGO", "label": "b", "description": ""}]} + findings = R.check_usable_facts_grounding(answer, _UF_FACTS) + assert findings and "'6'" in findings[0] and "usable_facts" in findings[0], findings + assert R.check_number_provenance(answer, _UF_FACTS) == [], ( + "sanity check: the plan-wide check must NOT catch this, or the two checks are not " + "actually testing different allow-sets") + + +def test_usable_facts_grounding_catches_fewer_than_two_presented_options(): + answer = {"prose": "NVDA is about 34% of the book.", + "presented_options": [{"maps_to": "NVDA", "label": "a", "description": ""}]} + findings = R.check_usable_facts_grounding(answer, _UF_FACTS) + assert findings and "fewer than two" in findings[0], findings + + +def test_usable_facts_grounding_catches_zero_presented_options(): + """The recorded-miss shape: pure process narration frames no alternative + at all, which is a sharper failure than framing only one.""" + answer = {"prose": "consider could not compute a consequence for this trade."} + findings = R.check_usable_facts_grounding(answer, _UF_FACTS) + assert findings and "fewer than two" in findings[0], findings + + +def test_usable_facts_grounding_abstains_when_the_answer_carries_no_surfaces(): + _findings, looked = R.run_check("usable_facts_grounding", {}, {}, _UF_FACTS) + assert looked is False, "a check with nothing to inspect has abstained, not passed" + + +def test_consider_refusal_concentration_keys_matches_reviews_own_constant(): + """#674's mirrored-but-not-imported constant (see run_episodes.py's own + comment on why review.py cannot be `_load_module`-loaded here): this is + the drift test that keeps the two declarations from silently diverging, + the same discipline test_exclusion_reasons_constant_is_exactly_what_ + both_schemas_declare already applies to EXCLUSION_REASONS.""" + import importlib.util + engine_dir = ROOT / "skills" / "fomo-kernel" / "engine" + spec = importlib.util.spec_from_file_location("episode_checkers_review", engine_dir / "review.py") + review_module = importlib.util.module_from_spec(spec) + sys.path.insert(0, str(engine_dir)) + try: + spec.loader.exec_module(review_module) + finally: + sys.path.remove(str(engine_dir)) + assert review_module.CONSIDER_REFUSAL_CONCENTRATION_KEYS == R.CONSIDER_REFUSAL_CONCENTRATION_KEYS, ( + "review.py and evals/run_episodes.py have drifted on which metrics keys count as " + "\"usable\" for a #674 refusal") + + # ── the interlocks ─────────────────────────────────────────────────────────── def test_a_declared_check_with_nothing_to_inspect_reports_no_data():