Skip to content

fix(card): a review with no persisted rule never claims one is standing - #644

Merged
atomchung merged 1 commit into
mainfrom
claude/issue-546-first-review-preview-copy
Jul 30, 2026
Merged

fix(card): a review with no persisted rule never claims one is standing#644
atomchung merged 1 commit into
mainfrom
claude/issue-546-first-review-preview-copy

Conversation

@atomchung

Copy link
Copy Markdown
Owner

The false claim, and where it came from

On a fresh root with no prior finalized review, the pre-commitment preview rendered:

This period keeps the rule unchanged; the standing rule remains: "…"

That claim is false: nothing was ever committed, so there is no standing rule to keep. skills/fomo-kernel/engine/card_renderer.py's _next_block, no-commitment branch (previously lines 3435-3447), resolved standing from state["rule_dim"]this run's engine prescription — and formatted block_missing.rule_standing without first checking whether a prior commitment was ever persisted. Because cmd_preview always drafts with require_commitment=False, that branch fires on every preview before the user has chosen a rule, not only a first-ever review — so a returning user whose most recent finalize offered no candidate rule (real shape, confirmed against the rotator mock persona below) hit it too.

The persisted fact was already being read two thousand lines above in the same file, at _reconciliation_lines (#292's breach path): bundle["review_plan"]["state_snapshot"]["prior_commitment"]. The predicate existed; it was simply not consulted here.

The fix

Gate the wrapper on that same predicate. With a real prior commitment on record, the standing-rule copy is unchanged. With none, a new copy key states the same {rule} as a recommendation awaiting the user's choice, rather than a thing that "remains":

prior_commitment = (((bundle.get("review_plan") or {}).get("state_snapshot") or {})
                    .get("prior_commitment") or {})
standing_key = "rule_standing" if prior_commitment.get("rule") else "rule_pending"
text = None
if standing and missing.get(standing_key):
    try:
        text = missing[standing_key].format(rule=standing)
    except (KeyError, IndexError, ValueError):
        text = None

The issue's own alternative — the existing generic no-commitment fallback (block_missing.rule, "No new rule commitment this time; you can pick one at the next review.") — is not used for this branch, per the issue's own reasoning: this preview shows before the choice, so telling the user the choice already didn't happen would be a second false claim. That generic fallback still applies unchanged when state.rule_dim has nothing resolvable at all (no recommendation to state either way) — untouched by this PR.

New copy key: block_missing.rule_pending

One key, added to all three locales, one-line rationale each:

Locale Text Why this wording
en No rule chosen yet this period; the recommendation is: "{rule}" Mirrors rule_standing's two-clause shape (fact, then the quoted rule) so the two wrappers read as siblings, but states absence-of-choice instead of continuity — no "remain"/"keep"/"standing".
zh-TW 這期還沒選規矩;建議是:「{rule}」 Phrased natively (not translated word-for-word from the English): "還沒選" ("not chosen yet") is the direct negation of a choice event, which "維持" ("remains/is maintained") is not — swapping only that word would still assert continuity.
zh-CN 这期还没选规矩;建议是:“{rule}” Same native phrasing as zh-TW, converted to Simplified characters and the locale's existing curly-quote convention (“…” vs zh-TW's 「…」), matching how rule_standing itself differs only in glyph set and quote style between the two.

All three keep the same {rule} interpolation rule_standing already uses (localized_rule(state.rule_dim, ...)), so a standing single-position-cap override still threads through correctly (test_standing_rule_placeholder_carries_the_user_cap_override, unmodified and still green).

skills/fomo-kernel/copy/en.json, zh-TW.json, zh-CN.json key-parity check (en vs zh-TW, the block_missing register): 0 diff before and after. block_missing remains an unclaimed register in tests/copy_corpus.py (17 → 18 keys, none exercised by a scene), so the copy-corpus golden needed no regeneration — confirmed by running it before and after (PASS copy corpus: 438 rendered surfaces match the golden, byte-identical).

Rendered evidence — both states, all three locales, both renderers

All renders below are from the committed fictional sample_ai_holder persona (skills/fomo-kernel/mock/sample_ai_holder.csv) in isolated roots (tests/persona_sweep.py for en/zh-TW, a direct engine/review.py prepare/preview CLI walk in a scratch root for zh-CN, since the sweep only covers en/zh-TW). No real trade data anywhere below.

State (a): fresh root, first-ever review, preview before choice

State-snapshot fact for this state: prior_commitment is absent (None) — confirmed by reading review_plan.state_snapshot.prior_commitment directly off the plan.

Markdown, Next step block:

  • en: [*] Change only this next time: No rule chosen yet this period; the recommendation is: "Cap any single position at 20%. Trim if it goes over, and do not add."
  • zh-TW: [*] 下次只改這一件:這期還沒選規矩;建議是:「單筆部位上限定死 20%;超過就減,不新增。」
  • zh-CN: [*] 下次只改这一件:这期还没选规矩;建议是:“单笔部位上限定死 20%;超过就减,不新增。”

HTML, .sec.keystep rule panel:

  • en: <p class="rmain">No rule chosen yet this period; the recommendation is: &quot;Cap any single position at 20%. Trim if it goes over, and do not add.&quot;</p>
  • zh-TW: <p class="rmain">這期還沒選規矩;建議是:「單筆部位上限定死 20%;超過就減,不新增。」</p>
  • zh-CN: <p class="rmain">这期还没选规矩;建议是:“单笔部位上限定死 20%;超过就减,不新增。”</p>

None of the six lines above contain "remain", "keep", "standing", or "維持"/"维持".

State (b): returning user, real prior commitment, preview before this period's choice

Seeded by actually finalizing a week-1 slice of the same persona's history first (so state_snapshot.prior_commitment is a real persisted fact, not a synthetic one), then previewing week 2. State-snapshot fact for this state: prior_commitment = {"rule": "Cap any single position at 20%. Trim if it goes over, and do not add.", "origin": "candidate", "source": "user_chosen", ...}.

Markdown, Next step block:

  • en: [*] Change only this next time: No rule change this period; the standing rule remains: "Cap any single position at 20%. Trim if it goes over, and do not add."
  • zh-TW: [*] 下次只改這一件:這期不改規矩;現行規矩維持:「單筆部位上限定死 20%;超過就減,不新增。」
  • zh-CN: [*] 下次只改这一件:这期不改规矩;现行规矩维持:“单笔部位上限定死 20%;超过就减,不新增。”

HTML, .sec.keystep rule panel:

  • en: <p class="rmain">No rule change this period; the standing rule remains: &quot;Cap any single position at 20%. Trim if it goes over, and do not add.&quot;</p>
  • zh-TW: <p class="rmain">這期不改規矩;現行規矩維持:「單筆部位上限定死 20%;超過就減,不新增。」</p>
  • zh-CN: <p class="rmain">这期不改规矩;现行规矩维持:“单笔部位上限定死 20%;超过就减,不新增。”</p>

This state renders byte-identically to main before this PR — the continuity copy is preserved exactly where it is true.

Net line count, state (a)

  1. This is a straight substitution inside one already-existing paragraph line, not an added sentence — ai_holder-en.first.preview.private.md is 48 lines both before and after (wc -l), byte-identical everywhere else on the card. Checked programmatically (not just this one file): every one of the 12 markdown cards the persona sweep found drifted changes exactly 2 removed + 2 added lines (the "Next rule" text-first-scan quote plus the "Next step" block itself, both reading the same panel) — a pure substitution, never a growth, on every affected card.

Cross-renderer / cross-locale proof: tests/persona_sweep.py --baseline

Ran the sweep with --engine pointed at this branch and --baseline at a pristine origin/main@6723db1 checkout. Every mock persona × locale × review (first/second) × variant (preview/committed/skip), 156 Markdown + 156 HTML cards, byte-compared:

FAIL  markdown drift vs baseline: ai_holder-en.first.preview.private.md
FAIL  markdown drift vs baseline: ai_holder-zh-TW.first.preview.private.md
FAIL  markdown drift vs baseline: momentum-en.first.preview.private.md
FAIL  markdown drift vs baseline: momentum-zh-TW.first.preview.private.md
FAIL  markdown drift vs baseline: panic_seller-en.first.preview.private.md
FAIL  markdown drift vs baseline: panic_seller-zh-TW.first.preview.private.md
FAIL  markdown drift vs baseline: pyramid-en.first.preview.private.md
FAIL  markdown drift vs baseline: pyramid-zh-TW.first.preview.private.md
FAIL  markdown drift vs baseline: rotator-en.first.preview.private.md
FAIL  markdown drift vs baseline: rotator-en.second.preview.private.md
FAIL  markdown drift vs baseline: rotator-zh-TW.first.preview.private.md
FAIL  markdown drift vs baseline: rotator-zh-TW.second.preview.private.md

persona sweep: 156 bundles x 3 surfaces rendered -- FAIL: 12 failure(s)

(--baseline treats any drift as the tool's own FAIL — that's the tool doing its job of forcing a human read, not a suite regression; tests/run_all.py does not run this comparison.) 144/156 Markdown and 144/156 HTML byte-identical; the other 12+12 diffed line-by-line and every single one is exactly the rule_standingrule_pending wrapper swap, nothing else — verified programmatically (diffed every drifted file, asserted every changed line matches the wrapper-swap pattern) rather than eyeballed. rotator's second review is in the drift list too: its own week-1 review offered no candidate rule, so its week-2 preview also has no persisted prior_commitment — real evidence the predicate is "has a real prior commitment", not "is this literally review #1".

Mutation table

Both directions of the new predicate, on the new focused test (tests/test_card_html.py::test_first_review_preview_states_the_recommendation_as_pending_not_standing), backed up/restored with cp (never git checkout), __pycache__ cleared between runs, exit codes read directly (not grepped):

# Mutation What it does Result
1 standing_key = "rule_standing" (predicate deleted, old unconditional behaviour restored) Every preview claims continuity, even a fresh root Reddened: AssertionError: zh-TW: fresh-root Markdown preview must not claim rule continuity ('維持' found in Next step), exit code 1. The pre-existing #356 test (test_standing_rule_placeholder_resolves_copy_not_the_v1_literal) stayed green under this mutation — it only checks the {rule} substring, not the wrapper — confirming the new test is the one actually covering this regression.
2 standing_key = "rule_pending" (predicate inverted to always-false/pending) Every preview claims pending, even a returning user with a real prior commitment Reddened: AssertionError: zh-TW: a returning user with a real prior commitment must keep the standing-rule continuity copy in Markdown, exit code 1 — the complementary assertion, confirming the test is not decorative in either direction.
Restored (cp from backup) Green, exit code 0, both times (tests/test_card_html.py: 62/62; confirmed again in the full suite).

Verification

  • python3 tests/run_all.py: PASS: all 49 suites passed (tail pasted below), run twice — once mid-flight, once as the final gate after fixing a test_doc_language.py catch (my first draft of the changelog row above embedded literal zh-TW characters in docs/output-contract.md, which is implementation-doc-English-only territory; fixed by paraphrasing instead of quoting).
  • No locale mixing: scanned every rendered card above and the full diff for CJK-in-English or the reverse; none found. test_standing_rule_placeholder_resolves_copy_not_the_v1_literal (the exact fix(card): pre-commitment preview leaks a hardcoded Chinese rule string into English cards #356 regression test for "Chinese leaking into an English card") stays green, untouched in its core assertions.
  PASS  Documentation and agent workflow boundaries  (tests/test_doc_language.py)
  PASS  Copy ratchet (#368 Phase 1)  (tests/test_copy_ratchet.py)
  PASS  Copy corpus golden (#402 knife 5)  (tests/copy_corpus.py)
  PASS  Episode checker probes (#417)  (tests/test_episode_checkers.py)
  PASS  Question-episode bank, mechanical half (#417)  (evals/run_episodes.py)
  PASS  Trigger matrix corpus and scorer (#458)  (tests/test_triggers.py)
  PASS  Persona sweep (personas x locales x reviews x variants)  (tests/persona_sweep.py)

PASS: all 49 suites passed.

Scope

Touches only skills/fomo-kernel/engine/card_renderer.py, skills/fomo-kernel/copy/{en,zh-TW,zh-CN}.json, docs/output-contract.md, and tests/test_card_html.py. Does not touch evaluation_challenge.py, review.py's consider path, or references/trade-consequence.md (PR #643 / #618's surface) — confirmed by diffing file lists, no overlap. Does not touch candidate-rule generation, prescribe(), the final committed card, or the Next-step block's layout.

closes #546

🤖 Generated with Claude Code

…ng (closes #546)

The pre-commitment preview's no-commitment branch (card_renderer.py's
_next_block) resolved state.rule_dim -- this period's fresh prescription --
into block_missing.rule_standing ("the standing rule remains: ...") without
checking whether a prior commitment was ever recorded. cmd_preview always
drafts with require_commitment=False, so this branch fires on every preview
before the user has chosen a rule, not only a first-ever review; on a fresh
root it told the user continuity the product never earned.

Gate the wrapper on the same review_plan.state_snapshot.prior_commitment.rule
predicate _reconciliation_lines already reads for #292: with a real prior
commitment on record, the standing-rule copy is unchanged; with none, a new
key (block_missing.rule_pending, synchronized across en/zh-TW/zh-CN) states
the same {rule} as a recommendation awaiting the user's choice, replacing
rather than adding a sentence. Both renderers share the branch through
_card_structure, including Markdown's compact text-first scan.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@atomchung

Copy link
Copy Markdown
Owner Author

Review — verified by rendering, merging. One residual found and filed separately.

Rendered both states myself through _next_block rather than reading the test names.

The fix works

Same prescription (position_sizing) in both, only the persisted prior commitment differs:

A) no prior commitment on record
   "No rule chosen yet this period; the recommendation is:
    \"Cap any single position at 20%. Trim if it goes over, and do not add.\""

B) a prior commitment on record
   "No rule change this period; the standing rule remains:
    \"Cap any single position at 20%. Trim if it goes over, and do not add.\""

The false continuity claim is gone from a first review, the returning-user path keeps its copy, and the substitution is one line for one line — the card does not grow.

The copy is right. 這期還沒選規矩;建議是:「…」 and 这期还没选规矩;建议是:“…” are natively phrased rather than translated, they negate a choice event rather than describing a state, and each keeps its locale's own quote convention. "還沒選" is the correct register for a sentence the user reads immediately before being asked to choose.

Your broader finding is the more valuable half of this PR. cmd_preview always drafts with require_commitment=False, so this branch fired on every preview before a choice, not only a first-ever review — the issue described the symptom it was reported from, not the trigger. Saying so rather than quietly fixing the wider case is what makes the 12/156 sweep drift readable.

docs/output-contract.md was correctly in scope, not scope creep: docs/maintainer-guide.md's mirrored-surfaces table names it as paired authority with card_renderer.py, and its Block-4 row (falls back to restating the standing rule when the engine proposes no change) became factually wrong the moment this landed. Leaving it would have been the defect this repository ships most often.

Residual, out of this issue's acceptance — filed as its own issue

standing is still resolved from state["rule_dim"] in both branches, and rule_dim = actionable[0]["dim"] (trade_recap.py:2018) has no input from prior_commitment. So on the returning-user path the continuity sentence names this period's prescription, not the rule the user actually chose. Reproduced:

prior_commitment.rule = "Hold any new position at least 30 days before selling"
rendered              = "the standing rule remains:
                         \"Cap any single position at 20%. Trim if it goes over, and do not add.\""

A rule the user never chose, presented as the one that "remains". Same falsehood as #546, moved from first-time users to returning ones — and it only becomes visible once #546's fix routes returning users into that branch deliberately rather than everyone.

This is explicitly permitted by #546's own acceptance ("Returning-user preview with a real prior commitment may use the standing-rule continuity copy") and past its stop condition, so it is not blocking here. Opening it separately.

@atomchung
atomchung merged commit ad4289e into main Jul 30, 2026
3 checks passed
@atomchung
atomchung deleted the claude/issue-546-first-review-preview-copy branch July 30, 2026 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(card): pre-commitment preview falsely implies a standing rule exists on a user's first-ever review

1 participant