diff --git a/docs/maintainer-guide.md b/docs/maintainer-guide.md index 63a88a9e..a50bb797 100644 --- a/docs/maintainer-guide.md +++ b/docs/maintainer-guide.md @@ -134,7 +134,7 @@ When multiple sessions are active: | HTML card design system | `card-template.html` is generated by `tools/gen_card_template.py` from `card-template.src.html` plus the runtime's own `_HTML_SHIM_CSS` / `_HTML_WIDGET_CSS` literals in `card_renderer.render_html()` (#401) — it is not hand-mirrored, so it cannot disagree with the runtime on palette, dark-mode rules, or layout constraints; `tests/test_card_html.py::test_card_template_matches_its_generator` fails the suite if the committed file and a fresh regeneration disagree. Design-provenance prose and template-only illustrative CSS (a few icons, one accessibility idea the runtime does not implement yet) are authored in `card-template.src.html`, not in the generated file. Before adding or reshaping a visual element, read [design-guidelines.md](design-guidelines.md): it defines the module slot contracts that keep one element's change from forcing its neighbours to be re-tuned. | | Card copy on the branches no persona reaches | `skills/fomo-kernel/copy/*.json` ↔ `tests/golden/copy-corpus.txt`, generated by `tests/copy_corpus.py --update` from real `render_private`/`render_html`/`render_public` output (#402 knife 5). Never hand-edit the golden, and never pin a catalog sentence as a literal inside a test again: add a scene instead. `tests/copy_corpus.py` also fails when a key in a claimed register reaches no surface, so dead copy and a renderer that silently stopped emitting a sentence both surface as red rather than as a passing suite. | | Design-bundle preview CSS | `tools/design_bundle.py` `TOKENS`/`CARD` (`.rc2`/`--` aliases) are derived at build time from `_HTML_WIDGET_CSS` / `_HTML_SHIM_CSS`, not hand-copied. Rerun `python3 skills/fomo-kernel/tools/design_bundle.py` after a runtime CSS change to refresh `ds-bundle/`. | -| Coach-root persistence registry (#452) | `coach.py`'s `DATA_FILES` (name, kind, user-facing description) is the only registry — `data-status`/`data-export`/`data-reset` all read it through `_scan_root(root)`, never a second list. Completeness is generated, not hand-verified: `tests/test_coach_data_cli.py`'s `test_data_files_registry_covers_every_engine_written_path` parses every non-test module under `skills/fomo-kernel/engine/` for the `os.path.join(, "")` idiom every writer already uses, and fails if a literal it finds is not registered — `condition_checks.jsonl` was exactly this gap (shipped in #412's second half, never registered, nothing failed until this check existed). `review.py`'s `_engine_version()` is named `repo_root` rather than `root` specifically so the scan needs no hand-written exclusion for the skill's own checkout path. Two already-registered entries the scan cannot see — `profile.md` (the agent writes it directly per `SKILL.md`) and `ux/` (`tools/ux_receipt.py`'s pathlib writer, outside `engine/`) — are covered instead by their own dedicated tests in the same file, not by this one. | +| 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` 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. Those two are unseen for **different reasons, and only one of them has a writer**: `ux/` is written from outside `engine/`, while `profile.md` is written from nowhere at all. No `SKILL.md`, `flows/*.md`, or `references/*.md` text instructs any agent to create it (the only `profile` in `references/` is `profile.json`, a different file holding `max_position_pct` and `muted_rules`), and no engine module does either. It is a vestige of an earlier design, still registered so `data-status`/`data-export`/`data-reset` keep seeing a file an older root may hold, and still advertised by `README.md` as a review baseline. Do not read its registry entry, or its test, as evidence that a writer exists — this row asserted "the agent writes it directly per `SKILL.md`" for a release (inherited from the test comment below, not invented here), and that sentence is exactly the false conclusion a later audit would draw from a registered file. Whether it is revived, re-homed into an append-only projection, or retired is #58's open call; #597 separately ruled it may not become a second canonical truth for provisional principles. This statement is mirrored in `tests/test_coach_data_cli.py`'s scope comment above `_dotted_call_name`, which is where it was written first: correct both or neither. | | 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. | | 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. | | 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. **`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. | diff --git a/tests/test_coach_data_cli.py b/tests/test_coach_data_cli.py index af4be9ad..f111fd9a 100644 --- a/tests/test_coach_data_cli.py +++ b/tests/test_coach_data_cli.py @@ -299,12 +299,30 @@ def test_condition_checks_projection_is_status_export_and_reset_managed(): # than teaching the checker to special-case it. # # Scope, named rather than silently assumed: this sees skills/fomo-kernel/ -# engine/*.py only. Two already-registered entries are written from outside -# that scope and this check cannot see either: profile.md (written directly -# by the agent per SKILL.md, never by engine code) and ux/ (written by -# skills/fomo-kernel/tools/ux_receipt.py's pathlib `root / "ux" / ...`, a -# different shape than os.path.join). Both stay correct because a dedicated -# test exercises each -- profile.md via _seed()'s tests above, +# engine/*.py only. Two already-registered entries fall outside it and this +# check cannot see either -- for different reasons, and only one of them has +# a writer at all. +# +# ux/ is written, just not from engine/: skills/fomo-kernel/tools/ +# ux_receipt.py's pathlib `root / "ux" / ...`, a different shape than +# os.path.join. +# profile.md is written from nowhere. No SKILL.md, flows/*.md or +# references/*.md text instructs any agent to create it (the only +# `profile` in references/ is profile.json, a different file holding +# max_position_pct and muted_rules), and no engine module does either. +# It is a vestige of an earlier design, still registered so data-status +# /data-export/data-reset keep seeing a file an older root may hold. +# +# This comment said "written directly by the agent per SKILL.md" for a +# release, and docs/maintainer-guide.md's registry row inherited that sentence +# from here -- a writer nobody could find, asserted in the two places a reader +# checks. Whether profile.md is revived, re-homed into an append-only +# projection, or retired is issue #58's open call; the pair of statements is +# mirrored, so correct both or neither. +# +# Both entries stay correct because a dedicated test exercises each -- +# profile.md via _seed()'s tests above (which prove the registry entry is +# reachable by status/export/reset, never that anything produces the file), # test_reset_and_status_cover_ux_trace_dir for ux/ -- not because this check # reaches them. What this check adds is the direction #452 actually broke on: # a literal constructed inside engine/ with no test written for it at all.