From 8b67bba31111e654beb5e4e2ed9569ee4e0453b5 Mon Sep 17 00:00:00 2001 From: test Date: Fri, 31 Jul 2026 10:36:42 +0800 Subject: [PATCH 1/2] feat(review): the entry-thesis question quotes what the user already said, instead of asking them to remember it (refs #636) A first review asked every un-thesised holding to reconstruct its entry motive from memory, offering five buckets. For a position the user had already discussed through `consider`, their exact `reason` was sitting in `trade_evaluations.jsonl` the whole time -- stored verbatim by schemas/decision-context.schema.json and read by nothing outside `consider` itself. The stem now quotes that statement, dated, and asks whether it was the thesis they entered on. Same question kind, same choices, same event, same slot in the #291 density band: the question is asked better, not asked twice. Recall deliberately does not route through `_evaluation_reconciliation`. That surface is open-evaluations-only, and a resolved evaluation is still something the user said; its schema also declares itself a supply-side fact surface and not a question. `_evaluation_recall` reads the store directly and ignores `decision` entirely. Three boundaries, each locked by a test: - a statement recorded after the cycle's entry is a different decision than the one this question asks about, so it is never quoted; - the latest qualifying statement wins, because an earlier one it superseded is not the user's account of why they entered; - an evaluation carrying no `reason` or `why_now` is dropped rather than quoted empty -- `consider` runs fine with no `--decision-context`, and a recalled blank is worse than the canned question it replaces. This states a fact and never a cause. The stem says the user said this on this date; it never says the position was opened because of it. `initial_thesis` stays in `evals/run_episodes.py`'s `KNOWN_UNWIRED`. This improves the question; the answer still has no consumer, and #429 is not fixed by it. Co-Authored-By: Claude Opus 5 --- skills/fomo-kernel/engine/review.py | 142 ++++++++++++++++++++++++++-- tests/test_review_v2.py | 83 ++++++++++++++++ 2 files changed, 218 insertions(+), 7 deletions(-) diff --git a/skills/fomo-kernel/engine/review.py b/skills/fomo-kernel/engine/review.py index ce23f2e9..a8e478f5 100644 --- a/skills/fomo-kernel/engine/review.py +++ b/skills/fomo-kernel/engine/review.py @@ -2864,26 +2864,49 @@ def _initial_thesis_id(cycle_id): return "initial_thesis_" + hashlib.sha256(str(cycle_id).encode("utf-8")).hexdigest()[:12] -def _initial_thesis_question(ticker, pos, cost, card, state, language): +def _initial_thesis_question(ticker, pos, cost, card, state, language, recalled=None): """One first-review entry-thesis capture (#291) grounded in ticker + cost. The stem cites the engine-owned cost basis (the deterministic per-position magnitude the engine stores; live-price weights are not persisted). Both the stem number and the stored `cost_basis` come from the same value so the card context and the recorded event cannot drift. + + ``recalled`` is a ``_recalled_entry_statement`` result: the user's own words + about this ticker, recorded through ``consider`` no later than this cycle's + entry. When one exists the stem stops asking the user to reconstruct a + motive from memory and shows them what they actually said, dated, so the + question becomes confirm-or-correct (#636). The words are inserted verbatim + and never truncated, translated or paraphrased — the same rule + schemas/decision-context.schema.json puts on storing them. + + The choice set, the event and the question budget are deliberately + unchanged: this is the same question asked better in the same slot, not an + extra one. What a different answer branches to is unchanged too, so the + recall cannot turn into the #429 shape of a question nothing consumes. """ cycle_id = pos.get("cycle_id") currency = str(pos.get("currency") or "USD") amount = _format_notional(cost, currency) importance, basis = _ticker_importance(card, state, ticker) because = _asked_because(basis, language) + said = (recalled or {}).get("reason") or (recalled or {}).get("why_now") if str(language).lower().startswith("en"): - stem = (f"You are holding {ticker} at a cost basis of about {amount}. " - "When you first entered this position, what was your thesis?") + stem = f"You are holding {ticker} at a cost basis of about {amount}. " + if said: + stem += (f"On {recalled['created'].isoformat()} you said: “{said}” " + "Was that the thesis you entered on?") + else: + stem += "When you first entered this position, what was your thesis?" if because: stem += f" (Asked because {because}.)" else: - stem = f"你持有 {ticker}，成本約 {amount}。當初第一次進場時，你的論點是什麼？" + stem = f"你持有 {ticker}，成本約 {amount}。" + if said: + stem += (f"{recalled['created'].isoformat()} 你說：「{said}」" + "當初進場的論點就是這個嗎？") + else: + stem += "當初第一次進場時，你的論點是什麼？" if because: stem += f"（問這題是因為{because}）" row = { @@ -2893,6 +2916,15 @@ def _initial_thesis_question(ticker, pos, cost, card, state, language): "cost_basis": cost, "currency": currency, "_importance": importance, "_importance_basis": basis, "_tie": 1, } + if said: + # Provenance for the agent and the receipt: which stored statement the + # stem quoted, so a reader can check the quote against the source + # rather than trusting the rendered stem. + row["recalled_statement"] = { + "evaluation_id": recalled.get("evaluation_id"), + "created": recalled["created"].isoformat(), + "quoted": said, + } if because: row["asked_because"] = because row["question_opportunity"] = question_surface.build_opportunity(row, language) @@ -2915,7 +2947,7 @@ def _rejection(id_, kind, reason, cycle_id=None): def _question_queue(card, state, active, previous_state, language, recent_exits=None, thesis_states=None, due_revisits=None, problem_stats=None, rule_history=None, horizon_markers=None, route=None, missing_thesis_positions=None, tier=None, - condition_questions=None): + condition_questions=None, evaluation_recall=None): """Return (queue, selection_report). The report states, plan-internally, how the route's density band was filled: the eligible/selected counts, why the queue fell short of the route minimum, and every candidate rejected with its @@ -3056,7 +3088,9 @@ def _question_queue(card, state, active, previous_state, language, recent_exits= cost = card_renderer._finite_number(pos.get("cost")) if cost is None or cost <= 0: continue # cannot ground the stem in a concrete magnitude - initial_candidates.append(_initial_thesis_question(ticker, pos, cost, card, state, language)) + initial_candidates.append(_initial_thesis_question( + ticker, pos, cost, card, state, language, + recalled=_recalled_entry_statement(evaluation_recall, ticker, cycle_id))) initial_candidates.sort(key=lambda row: (-float(row.get("_importance") or 0), str(row.get("id")))) candidates.extend(initial_candidates[:INITIAL_THESIS_LIMIT]) initial_overflow = initial_candidates[INITIAL_THESIS_LIMIT:] @@ -3878,7 +3912,8 @@ def _build_plan(card, state, engine_meta, root, paths, route, language, fingerpr card, state, active, previous, language, recent_exits, by_cycle, due_revisits, problem_stats, rule_history, horizon_markers, route=route, missing_thesis_positions=missing, tier=review_tier["tier"], - condition_questions=condition_questions) + condition_questions=condition_questions, + evaluation_recall=_evaluation_recall(root)) question_selection["rejected"].extend(condition_deferred) candidate_rules = _candidate_rules(card, state, language) plan = { @@ -6416,6 +6451,99 @@ def _fold_evaluations(rows): return latest +def _evaluation_recall(root): + """What the user already told us, in their own words, about a ticker. + + ``consider`` stores the user's exact ``reason`` and ``why_now`` on + ``trade_evaluations.jsonl`` (schemas/decision-context.schema.json: stored + verbatim, never rewritten or translated). Nothing outside ``consider`` ever + read that text, so a first review asked its entry-motive question from + scratch — offering five buckets to a user who had already answered in their + own language, and storing a choice ``evals/run_episodes.py`` still lists in + ``KNOWN_UNWIRED``. This returns the text so ``_question_queue`` can show it + back instead of asking someone to reconstruct it from memory (#636). + + Returns ``{ticker: [statement, ...]}``, each list oldest ``created`` first. + Cycle matching belongs to the caller, which is the layer that knows which + cycle a ticker is currently in. + + Two scope choices worth stating, because both differ from + ``_evaluation_reconciliation`` directly below: + + * ``decision`` is not read at all. A resolved evaluation is still something + the user said, so that function's open-only filter is the wrong one here. + * A statement with neither ``reason`` nor ``why_now`` is dropped rather than + returned empty. ``consider`` may run with no ``--decision-context``, and a + recalled blank is worse than the canned question it would replace. + + Like that function this states a fact — "you said this, on this date" — and + never a cause. Nothing here claims a position was opened *because* of a + statement; ``premise`` is what the user contemplated, not what they did. + """ + recall = {} + for row in _fold_evaluations(thesis.read_jsonl(_evaluation_path(root))).values(): + premise = row.get("premise") or {} + ticker = premise.get("ticker") + context = row.get("context") or {} + reason = context.get("reason") + why_now = context.get("why_now") + if not ticker or not (reason or why_now): + continue + try: + created = dt.date.fromisoformat(str(row.get("created"))) + except (TypeError, ValueError): + continue + recall.setdefault(str(ticker), []).append({ + "evaluation_id": row.get("evaluation_id"), + "created": created, + "reason": reason, + "why_now": why_now, + }) + for statements in recall.values(): + statements.sort(key=lambda item: (item["created"], str(item.get("evaluation_id") or ""))) + return recall + + +def _cycle_start_date(cycle_id): + """The entry date encoded in a ``trade_recap`` cycle id, or None. + + The contract is ``trade_recap.py``'s: ``"{ticker}#{start}#{seq}"``, with + ``"{ticker}#unknown"`` for a position whose opening trade is not in the + supplied history. The unknown form returns None rather than guessing — a + recalled statement must not attach to a cycle whose start nobody knows. + """ + parts = str(cycle_id or "").split("#") + if len(parts) < 2: + return None + try: + return dt.date.fromisoformat(parts[1]) + except ValueError: + return None + + +def _recalled_entry_statement(recall, ticker, cycle_id): + """The user's latest own-words statement recorded no later than this + cycle's entry, or None. + + "No later than the entry" is what makes this an *entry* thesis rather than + a later add: an evaluation recorded after the position opened describes a + decision the entry question is not asking about. Same-day counts — the + contemplation and the fill routinely land on one date. + + The latest qualifying statement wins. An earlier one it superseded is not + the user's current account of why they entered, and showing the oldest + would be the same memory-reconstruction problem in a new place. + """ + start = _cycle_start_date(cycle_id) + if start is None: + return None + eligible = [item for item in (recall or {}).get(str(ticker), []) + if item["created"] <= start] + if not eligible: + return None + return eligible[-1] + + def _evaluation_reconciliation(root, rows, date_end): """Reconcile every unsettled ``consider`` evaluation against the transaction record, for ``_build_plan`` (#317; #429's rule one layer up — diff --git a/tests/test_review_v2.py b/tests/test_review_v2.py index 3acceb66..3b709ee7 100644 --- a/tests/test_review_v2.py +++ b/tests/test_review_v2.py @@ -9301,6 +9301,89 @@ def test_initial_thesis_dedup_skips_a_position_with_an_existing_thesis(): for q in plan["question_queue"]), "the un-thesised holding is still asked" +def _evaluation_row(evaluation_id, ticker, created, reason=None, decision="open"): + """A trade_evaluations.jsonl row shaped like consider's own writer. + + `context` is omitted rather than nulled when there is no reason: review's + identity seed keys on that presence test, so a stored `context: null` is a + different row than a row with no context at all.""" + row = {"evaluation_id": evaluation_id, "created": created, + "premise": {"ticker": ticker, "side": "buy", "qty": 10.0, "price": 100.0, + "date": created, "currency": "USD"}, + "basis": {"state_version": "csv-v1:seed"}, "consequence": {}, "rule_collisions": [], + "decision": decision, "decided_on": created} + if reason is not None: + row["context"] = {"reason": reason} + return row + + +def test_initial_thesis_recalls_what_the_user_already_said_before_entering(): + """#636: a holding the user discussed through `consider` before entering is + not asked to reconstruct a motive from memory. The stem quotes their own + stored words verbatim, dated, and carries the provenance to check the quote + against. A statement recorded *after* the entry is not an entry thesis and + must not be quoted.""" + with tempfile.TemporaryDirectory() as tmp: + root = pathlib.Path(tmp) / "coach" + root.mkdir(parents=True) + early = "Order visibility runs into the next build cycle." + latest = "Guidance was cut but the multiple already prices it." + after = "Chasing the breakout after the gap." + rows = [ + # AAA: two statements before the 2026-01-01 entry. The later one is + # the user's current account of why they entered; `decision: acted` + # proves recall does not inherit the reconciliation's open-only filter. + _evaluation_row("eval-aaa-early", "AAA", "2025-12-20", early), + _evaluation_row("eval-aaa-latest", "AAA", "2025-12-28", latest, decision="acted"), + # BBB: recorded two months *after* its entry — a different decision + # than the one this question asks about. + _evaluation_row("eval-bbb-after", "BBB", "2026-03-01", after), + ] + (root / "trade_evaluations.jsonl").write_text( + "".join(json.dumps(row, ensure_ascii=False) + "\n" for row in rows), encoding="utf-8") + positions = {"AAA": _pos("AAA", 5000), "BBB": _pos("BBB", 4000)} + card, state = _density_artifacts(tmp, "recall", positions, thesis_questions=[]) + run = _run("prepare", "--root", root, "--language", "en", "--route", "first_review", + "--card-json", card, "--state-json", state) + assert run.returncode == 0, run.stdout + run.stderr + plan = _pending_plan(root, run.stdout) + asked = {q.get("ticker"): q for q in plan["question_queue"] + if q.get("kind") == "initial_thesis"} + assert set(asked) == {"AAA", "BBB"}, "both holdings are still asked exactly once" + + aaa = asked["AAA"] + assert latest in aaa["question"], "the stem quotes the user's own words verbatim" + assert early not in aaa["question"], \ + "a superseded earlier statement is not the account of why they entered" + assert "2025-12-28" in aaa["question"], "the quote is dated, so it reads as a record" + assert aaa["recalled_statement"] == { + "evaluation_id": "eval-aaa-latest", "created": "2025-12-28", "quoted": latest} + + bbb = asked["BBB"] + assert after not in bbb["question"], \ + "a statement recorded after the entry is not the entry thesis" + assert "recalled_statement" not in bbb + assert "what was your thesis?" in bbb["question"], \ + "with nothing to recall the question falls back unchanged" + + # The recall replaces the wording of an existing question; it never adds + # one, so the route's #291 density band is untouched. + assert len(plan["question_queue"]) <= review_engine.QUESTION_POLICY["first_review"]["max"] + + +def test_initial_thesis_recall_ignores_a_statement_with_no_words(): + """#636: `consider` runs fine with no --decision-context, so an evaluation + may carry no reason at all. A recalled blank is worse than the canned + question it would replace, so it is dropped rather than quoted empty.""" + with tempfile.TemporaryDirectory() as tmp: + root = pathlib.Path(tmp) / "coach" + root.mkdir(parents=True) + (root / "trade_evaluations.jsonl").write_text( + json.dumps(_evaluation_row("eval-bare", "AAA", "2025-12-20")) + "\n", encoding="utf-8") + recall = review_engine._evaluation_recall(str(root)) + assert recall == {}, "an evaluation with no stored words is not a recallable statement" + + def test_initial_thesis_consumption_maturity_gate_and_idempotency(): """#291: planned_entry forces a real captured thesis; other answers keep the inferred record legal; the classification projects; finalize stays idempotent.""" From 0243bd54e7f95ee52650f2098f08e3399b62cac1 Mon Sep 17 00:00:00 2001 From: test Date: Fri, 31 Jul 2026 11:12:49 +0800 Subject: [PATCH 2/2] fix(review): the recall never speaks for a position it was not about, and the field it adds is declared where readers look (refs #636) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent review of the first cut found two blockers and four smaller defects. Every one is a claim the offline suite could not make false, which is the point of the review. **A re-entered position quoted the previous one's reason.** A cycle id carries no lower bound, so every statement made before a ticker's *first* entry also satisfies `created <= start` for its second. The owner's ruling on #636 is per cycle: a ticker fully exited and re-entered is a new position with its own reason. Bounding the window properly needs the prior cycle's exit date, which this layer does not have, so a re-entry now recalls nothing rather than attributing the old reason to the new position. **The new `recalled_statement` made the emitted plan invalid.** A question row is `additionalProperties: false` in `review-plan.schema.json`, and the field was not declared. Nothing failed: no offline test fed a first_review question queue through that schema. The field is declared now, and the recall test pins every emitted row against the schema's own property set, so the next undeclared field fails here instead of in a reader. **`_cycle_start_date` did not enforce the contract it cited.** It split on `#` and parsed the second segment, which accepts shapes `trade_recap` can never produce. It is now `_cycle_entry`, matching `trade_recap.CYCLE_ID_RE` — the single source of truth — and returning the sequence the re-entry rule needs. The first version of the test did not catch this: `int()` and `fromisoformat()` already reject the malformed cases it listed, so the strictness was decorative until the test grew the shapes only the regex rejects (a ticker with whitespace, an empty ticker, a trailing space). Three corrections to claims that were wrong rather than incomplete: - the docstring called an earlier statement "superseded". It is not: `_evaluation_id` seeds on `context`, so the same premise re-asked with a different `why_now` mints a *distinct* evaluation. Taking the latest is a choice of the statement closest to the entry, not a supersede semantic. - a comment offered `recalled_statement` to "the receipt". `ux_receipt`'s `question_presented` accepts only source and digest and rejects content. - the test fixture wrote a context with `reason` alone, a shape `decision-context.schema.json` forbids — it requires `why_now` too, so the core test was asserting against a record `consider` cannot write. `docs/maintainer-guide.md`'s mirrored-surfaces row still said `_evaluation_reconciliation` was the only reader of `trade_evaluations.jsonl` outside `consider`. It now names both readers and states why their filters differ, so a third one has to choose deliberately. Co-Authored-By: Claude Opus 5 --- docs/maintainer-guide.md | 2 +- skills/fomo-kernel/engine/review.py | 74 +++++++++++------- .../schemas/review-plan.schema.json | 19 +++++ tests/test_review_v2.py | 76 ++++++++++++++++++- 4 files changed, 139 insertions(+), 32 deletions(-) diff --git a/docs/maintainer-guide.md b/docs/maintainer-guide.md index 096dc026..c25e8725 100644 --- a/docs/maintainer-guide.md +++ b/docs/maintainer-guide.md @@ -137,7 +137,7 @@ When multiple sessions are active: | 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 → review reconciliation | `review.py`'s `_evaluation_reconciliation` (the single reader; 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); nothing else outside `consider` reads it. | +| 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` — #485 Slice C, #531) | `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.build_adoption` 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. 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. | | Freeform informational answer shape (#543, second cut 2026-07-29) | `skills/fomo-kernel/SKILL.md` rule 8 (`## Non-negotiable rules`) ↔ `AGENTS.md` boundary 8 (`## Non-negotiable boundaries`) — the two guaranteed-delivery entry points (`docs/development-guide.md` §6) — ↔ `skills/fomo-kernel/references/freeform-answers.md` (the full rationale and the named chart set) ↔ cross-references in `references/interaction-delivery.md` and `references/trade-consequence.md` ↔ `tests/test_doc_language.py`'s `test_freeform_answer_shape_is_a_boundary_in_both_entry_points` and its paired mutation test. Three rules, not one. **The default is text, not a picture**: an ad hoc question, including a `consider` call, gets a quick, direct answer with no chart, rendered artifact, or multi-tool production unless the user asks for more — the 34-turn dogfood session that motivated this rule is the counter-example the rule forbids, not a hypothetical. **A chart is named in advance, never improvised**: the owner reversed the first cut's empty set (PR #554) into two named entries — the review card itself, reachable in freeform conversation only through the existing `card-delivery.md` contract (never a fresh rendering, and the card's own sparkline stays scoped to the card rather than becoming independently reachable), and a Positions view, at the time a fixed ungraphical table (`Ticker | Shares | Avg cost | Weight of book`, sorted by weight descending) that `freeform-answers.md` stated plainly had no dedicated no-side-effect `engine/review.py` data outlet yet — `consider` requires a real premise and durably records an evaluation, `refresh` requires a newly supplied broker snapshot, and neither is a passive-lookup shortcut. A third, owner-approved entry is still a name written into the reference file, not a shape invented inline. The gap closed in #561 (owner ruling 2026-07-30): the shape widened into the richer, already-demoed per-position diagnosis (README.md's "What it looks like") — ticker, shares, avg cost, value, $ P&L, and the sizing/averaging-down/exit-discipline/hold-consistency tags, sorted by size — and `engine/review.py positions` is the outlet, reconstructing the book from `ledger.jsonl` alone through the exact FIFO pipeline (`round_trips` → `fifo_held` → `classify_adds` → `dim_size` → `ticker_diagnosis`) `trade_recap.py`'s own CSV review already runs, deliberately not the canonical average-cost book `consider`'s ledger route uses (`ticker_diagnosis` sums FIFO-realized and unrealized P&L into one figure, and mixing that with an average-cost remaining balance misstates the sum on a multi-lot partial sell — `engine/review.py`'s `_positions_diagnosis` docstring). Whitelisted alongside `resolve-market-data` as writing no session, ledger, or evaluation row. **The set bounds the agent, not the user**: both rules above constrain what the agent decides to produce unprompted; neither is a ceiling on an explicit user request, and every other non-negotiable rule still governs however that request gets answered. Only the rule's *presence in both entry points* is mechanically gated — the shared literal phrase check catches one entry point silently dropping the rule while the other keeps it, but nothing offline can observe whether a live host conversation actually stayed brief, the same instruction-only footing `docs/development-guide.md` §4 already admits for the recommendation ban. Boundary with #525: this is an effort/scope ceiling on production cost, not a disclosure-content contract — whether a freeform answer states every material limitation as rigorously as the card's `honesty_ledger` is #525's separate, still-open question, not settled here. **That boundary is itself a pinned phrase in both entry points**, not only in the soft-routed reference: the rule names `consider`, which is exactly the surface #479 delivers a visible two-sided challenge on, so an entry point that says "answered briefly" without `Brevity bounds what an answer produces, never which facts it owes` invites compressing away the disclosures that surface exists to make. The user-override clause is pinned the same way — `bounds what the agent decides to produce on its own initiative, never what the user explicitly asks for` appears verbatim in both entry points, so one cannot silently narrow back to "not even if the user asks" while the other still permits it. **The rule now carries exactly one exception, pinned the same way** (#629, owner ruling 2026-07-30): price recovery *is* multi-tool production by the letter of the rule, so an entry point carrying only the rule tells a host not to do what the ruling requires — and the failure is silent, because the host then relays a concentration answer weighted on cost. Five further phrases are therefore pinned in both entry points — the exception itself (`recovering a price the engine could not retrieve is completing the input, not production`), its narrowness (`It licenses no chart, artifact, or other multi-tool work`), its bound (`transcription, not analysis`, `at most twenty instruments`, `delegated to whatever faster tier the host has`), and what it degrades to (`refused rather than answered on cost basis`). The delegation clause names no model, tier, or subagent mechanism (owner ruling 2026-07-26): this is a cross-host public skill, so the contract states the outcome and the host decides how. The row below owns the engine half. | diff --git a/skills/fomo-kernel/engine/review.py b/skills/fomo-kernel/engine/review.py index a8e478f5..75885ee8 100644 --- a/skills/fomo-kernel/engine/review.py +++ b/skills/fomo-kernel/engine/review.py @@ -2917,9 +2917,11 @@ def _initial_thesis_question(ticker, pos, cost, card, state, language, recalled= "_importance": importance, "_importance_basis": basis, "_tie": 1, } if said: - # Provenance for the agent and the receipt: which stored statement the - # stem quoted, so a reader can check the quote against the source - # rather than trusting the rendered stem. + # Provenance for the agent and for anything reading the plan: which + # stored statement the stem quoted, so a reader can check the quote + # against its source rather than trusting the rendered stem. Not the + # receipt — `question_presented` accepts only source and digest, never + # content, and this must not be read as travelling there. row["recalled_statement"] = { "evaluation_id": recalled.get("evaluation_id"), "created": recalled["created"].isoformat(), @@ -6504,38 +6506,56 @@ def _evaluation_recall(root): return recall -def _cycle_start_date(cycle_id): - """The entry date encoded in a ``trade_recap`` cycle id, or None. +def _cycle_entry(cycle_id): + """``(entry_date, sequence)`` for a canonical cycle id, else ``(None, None)``. - The contract is ``trade_recap.py``'s: ``"{ticker}#{start}#{seq}"``, with - ``"{ticker}#unknown"`` for a position whose opening trade is not in the - supplied history. The unknown form returns None rather than guessing — a - recalled statement must not attach to a cycle whose start nobody knows. + ``trade_recap.CYCLE_ID_RE`` is the single source of truth for the shape + (``"{ticker}#{start}#{seq}"``), with ``CYCLE_ID_UNKNOWN_RE`` covering the + two-segment ``"{ticker}#unknown"`` a CSV without opening holdings produces. + Matching against that regex rather than splitting on ``#`` matters: a split + accepts ``"AAA#2026-01-01#garbage"`` and fails open into a valid-looking + entry date, and every caller here is deciding whether to attribute the + user's own words to a position. """ - parts = str(cycle_id or "").split("#") - if len(parts) < 2: - return None + if not trade_recap.CYCLE_ID_RE.match(str(cycle_id or "")): + return None, None + _ticker, start, seq = str(cycle_id).split("#") try: - return dt.date.fromisoformat(parts[1]) + return dt.date.fromisoformat(start), int(seq) except ValueError: - return None + return None, None def _recalled_entry_statement(recall, ticker, cycle_id): - """The user's latest own-words statement recorded no later than this - cycle's entry, or None. - - "No later than the entry" is what makes this an *entry* thesis rather than - a later add: an evaluation recorded after the position opened describes a - decision the entry question is not asking about. Same-day counts — the - contemplation and the fill routinely land on one date. - - The latest qualifying statement wins. An earlier one it superseded is not - the user's current account of why they entered, and showing the oldest - would be the same memory-reconstruction problem in a new place. + """The user's latest own-words statement recorded before this cycle opened, + or None. + + Two bounds, and the repository has a rule behind each. + + *Upper*: no later than the cycle's entry. That is what makes this an + *entry* thesis rather than a later add — an evaluation recorded after the + position opened describes a decision this question is not asking about. + Same-day counts, because the contemplation and the fill routinely land on + one date. + + *Lower*: only the position's **first** cycle. A ticker fully exited and + re-entered is a new position with its own reason (the owner's per-cycle + ruling on #636), and a cycle id carries no lower bound — every statement + made before the *first* entry also satisfies ``created <= start`` for the + second. Rather than quote the previous position's reason as this one's, + a re-entry recalls nothing. Bounding it properly needs the prior cycle's + exit date, which this layer does not have; failing closed is the honest + version until it does. + + Among the eligible, the most recent is used. Note what that is *not*: two + evaluations for one ticker are two distinct decisions, not a revision + chain — ``_evaluation_id`` seeds on ``context``, so re-asking the same + premise with a different ``why_now`` mints a new evaluation rather than + superseding the old one. Taking the latest is a choice of the closest + statement to the entry, not a supersede semantic. """ - start = _cycle_start_date(cycle_id) - if start is None: + start, seq = _cycle_entry(cycle_id) + if start is None or seq != 1: return None eligible = [item for item in (recall or {}).get(str(ticker), []) if item["created"] <= start] diff --git a/skills/fomo-kernel/schemas/review-plan.schema.json b/skills/fomo-kernel/schemas/review-plan.schema.json index 87752861..5d57dd6a 100644 --- a/skills/fomo-kernel/schemas/review-plan.schema.json +++ b/skills/fomo-kernel/schemas/review-plan.schema.json @@ -431,6 +431,25 @@ "basis_note": { "type": "string", "description": "condition_basis: why the agent believes the measurement basis changed underneath the frozen threshold." + }, + "recalled_statement": { + "type": "object", + "additionalProperties": false, + "required": ["evaluation_id", "created", "quoted"], + "description": "initial_thesis only, and only when the user discussed this ticker through `consider` before entering it (#636): the stored statement this question's stem quotes back at them, instead of asking them to reconstruct an entry motive from memory. Provenance, not a second copy of the record -- `quoted` is the exact text already on that trade_evaluations.jsonl row, present so a reader can check the rendered stem against its source. States a fact and never a cause: the user said this, on this date. It is not a claim that the position was opened because of it, the same boundary `evaluation_reconciliation` draws around `status: matched`. Absent whenever nothing qualified, which includes a re-entry cycle -- review.py's _recalled_entry_statement fails closed there rather than attributing the previous position's reason to this one.", + "properties": { + "evaluation_id": {"type": "string", "minLength": 1}, + "created": { + "type": "string", + "pattern": "^[0-9]{4}-[0-9]{2}-[0-9]{2}$", + "description": "The evaluation's own `created` date, which the stem shows so the quote reads as a dated record rather than a present-tense claim." + }, + "quoted": { + "type": "string", + "minLength": 1, + "description": "The user's words, verbatim from schemas/decision-context.schema.json's `reason` (or `why_now` when only that is present). Never truncated, translated or paraphrased -- the same rule that schema puts on storing them." + } + } } }, "additionalProperties": false diff --git a/tests/test_review_v2.py b/tests/test_review_v2.py index 3b709ee7..48fcbc12 100644 --- a/tests/test_review_v2.py +++ b/tests/test_review_v2.py @@ -9304,16 +9304,19 @@ def test_initial_thesis_dedup_skips_a_position_with_an_existing_thesis(): def _evaluation_row(evaluation_id, ticker, created, reason=None, decision="open"): """A trade_evaluations.jsonl row shaped like consider's own writer. - `context` is omitted rather than nulled when there is no reason: review's - identity seed keys on that presence test, so a stored `context: null` is a - different row than a row with no context at all.""" + When a context is present it carries both `reason` and `why_now`, because + schemas/decision-context.schema.json requires both -- a row with only one + is a shape `consider` cannot write, and a fixture that uses it would be + testing against an impossible record. When there is no context the key is + omitted rather than nulled: review's identity seed keys on that presence + test, so a stored `context: null` is a different row than no context.""" row = {"evaluation_id": evaluation_id, "created": created, "premise": {"ticker": ticker, "side": "buy", "qty": 10.0, "price": 100.0, "date": created, "currency": "USD"}, "basis": {"state_version": "csv-v1:seed"}, "consequence": {}, "rule_collisions": [], "decision": decision, "decided_on": created} if reason is not None: - row["context"] = {"reason": reason} + row["context"] = {"reason": reason, "why_now": f"why-now for {evaluation_id}"} return row @@ -9370,6 +9373,71 @@ def test_initial_thesis_recalls_what_the_user_already_said_before_entering(): # one, so the route's #291 density band is untouched. assert len(plan["question_queue"]) <= review_engine.QUESTION_POLICY["first_review"]["max"] + # A question row is `additionalProperties: false`, so a new field that + # is not declared makes the emitted plan invalid against the published + # schema -- silently, because nothing else in the offline suite feeds a + # first_review question queue through it. Pinned here with the same + # manual idiom test_consider.py uses (the suite carries no jsonschema + # dependency). + item_schema = json.loads( + (pathlib.Path(review_engine.__file__).resolve().parent.parent + / "schemas" / "review-plan.schema.json").read_text(encoding="utf-8") + )["properties"]["question_queue"]["items"] + assert item_schema["additionalProperties"] is False + allowed = set(item_schema["properties"]) + for row in plan["question_queue"]: + assert set(row) <= allowed, f"undeclared question fields: {set(row) - allowed}" + recalled_schema = item_schema["properties"]["recalled_statement"] + assert set(recalled_schema["required"]) <= set(aaa["recalled_statement"]) + assert set(aaa["recalled_statement"]) <= set(recalled_schema["properties"]) + + +def test_initial_thesis_recall_fails_closed_on_a_re_entry_cycle(): + """#636: a ticker fully exited and re-entered is a new position with its own + reason (owner ruling: per cycle, not per ticker). A cycle id carries no + lower bound, so every statement made before the *first* entry also satisfies + `created <= start` for the second. Rather than attribute the previous + position's reason to this one, a re-entry recalls nothing.""" + with tempfile.TemporaryDirectory() as tmp: + root = pathlib.Path(tmp) / "coach" + root.mkdir(parents=True) + first_cycle_reason = "Bought the first time for the backlog." + (root / "trade_evaluations.jsonl").write_text( + json.dumps(_evaluation_row("eval-old-cycle", "AAA", "2025-11-01", + first_cycle_reason)) + "\n", encoding="utf-8") + positions = {"AAA": dict(_pos("AAA", 5000), cycle_id="AAA#2026-01-01#2")} + card, state = _density_artifacts(tmp, "reentry", positions, thesis_questions=[]) + run = _run("prepare", "--root", root, "--language", "en", "--route", "first_review", + "--card-json", card, "--state-json", state) + assert run.returncode == 0, run.stdout + run.stderr + plan = _pending_plan(root, run.stdout) + asked = [q for q in plan["question_queue"] + if q.get("kind") == "initial_thesis" and q.get("ticker") == "AAA"] + assert asked, "the re-entered holding is still asked its entry thesis" + assert first_cycle_reason not in asked[0]["question"], \ + "the previous position's reason is not this position's entry thesis" + assert "recalled_statement" not in asked[0] + + +def test_cycle_entry_rejects_a_non_canonical_cycle_id(): + """#636: trade_recap.CYCLE_ID_RE is the shape's single source of truth. A + `split("#")` would accept `AAA#2026-01-01#garbage` and fail open into a + valid-looking entry date -- on a path that decides whether to attribute the + user's own words to a position, fail-open is the wrong direction.""" + assert review_engine._cycle_entry("AAA#2026-01-01#1") == (dt.date(2026, 1, 1), 1) + # The shapes a permissive `split("#")` would also reject, because int() or + # fromisoformat() raises on them anyway. + for bad in ("AAA#2026-01-01#garbage", "AAA#unknown", "AAA", "", None, + "AAA#2026-13-01#1", "AAA#2026-01-01"): + assert review_engine._cycle_entry(bad) == (None, None), bad + # The shapes only the regex rejects. Without these the strictness is + # decorative: a split-based helper parses each of them into a real date + # and a real sequence, and the caller would attribute the user's words to + # a cycle id trade_recap can never have produced. + for forged in ("A A#2026-01-01#1", "#2026-01-01#1", " AAA#2026-01-01#1", + "AAA#2026-01-01#1 "): + assert review_engine._cycle_entry(forged) == (None, None), forged + def test_initial_thesis_recall_ignores_a_statement_with_no_words(): """#636: `consider` runs fine with no --decision-context, so an evaluation