Skip to content

fix(consider): a trade that reduces an over-cap position is no longer delivered as a breach (refs #579, refs #674, refs #676) - #791

Merged
atomchung merged 1 commit into
mainfrom
claude/579-rule-effect
Aug 2, 2026
Merged

fix(consider): a trade that reduces an over-cap position is no longer delivered as a breach (refs #579, refs #674, refs #676)#791
atomchung merged 1 commit into
mainfrom
claude/579-rule-effect

Conversation

@atomchung

Copy link
Copy Markdown
Owner

Closes the deterministic half of #579 and the process-leakage half of #674/#676 in one cut, because both failed on the same consider challenge/projection surface (#713's scope guard forbids competing PRs over one user-visible outcome).

Branch cut from main@a33f315. Not merged — for review.

(a) What the failed surface actually emitted

Reproduced mechanically on a fictional book: a position at 80% of the book against a self-authored 20% cap, and a sell that takes it to 75%.

before.max_pct 0.80 / after.max_pct 0.75 / effective_oversize_trigger 0.20
rule_collision   -> {"state": "already_over", "worsens": false}
must_state       -> {"topic": "rule_collision", "value": "already_over",
                     "anchor": "rule_collisions.r1.state",
                     "detail": {"worsens": false, ...}}
required_coverage-> {"owes": "rule_collision", "key": "already_over"}

The engine emitted already_over + worsens:false; the direction arithmetic was correct. _worsened(0.80, 0.75) is false, as it should be. The defect was entirely in what the decision-facing surface presented: the absolute state was the headline value and the transaction's direction was a nullable boolean buried inside detail.

Two further mechanical findings turn this from "the agent composed it badly" into "the payload could not express it":

  1. already_over + worsens:false is emitted for both improved_but_still_over and unchanged_existing_breach_worsened is a strict >, so "reduced" and "did not move" are byte-identical payloads.
  2. clear is emitted for both compliant and resolved_existing_breach, and clear was not in _SPEAKING_STATES, so a trade that fully resolved a self-authored breach said nothing at all.

Two of the six required transitions were therefore not derivable from the frozen fields, whatever the prompt said. That is why PR #608's reference-only cut could not have held.

(b) rule_effect and the six transitions

consequence.classify_rule_effect(before_reading, after_reading, limit) is the single derivation. limit is a strict upper bound — the same > dim_size/dim_diversify already use, so the effect and the state beside it cannot disagree about which side of the line a reading is on — and movement uses _worsened's own epsilon, so one tolerance decides both fields.

effect condition
new_breach not over before, over after
worsened_existing_breach over both, reading rose
improved_but_still_over over both, reading fell
unchanged_existing_breach over both, reading did not move
resolved_existing_breach over before, not over after
compliant over neither
unjudged / unmapped carried through — a rule the engine never judged has no readings to classify, and an effect vocabulary that had to invent one would be the failure unmapped exists to prevent

Each collision branch classifies from the readings that branch judged: the premise ticker's own weight where causality was decided on it, the book's max_pct where it was not, this metric's own reading and line for the concentration trio, and a count of breaching average-down events against a limit of zero for avgdown_count (which is why that row carries no limit). Classifying every branch off one reading would produce the contradiction the pairing exists to avoid.

Two rows in the regression matrix are where state and rule_effect legitimately differ, and they are the proof the derivation is not a relabelled state:

  • a sell shrinks the denominator, so a different position crosses the cap on a trade whose own ticker did not. state calls that already_over even though the book was under the line before it; the effect says new_breach, which is the true reading.
  • clear covers both compliant and resolved_existing_breach.

A mutation that derives the effect from the state fails on exactly those rows.

limit and limit_source are new on the row for one reason: the correct sentence is "reduces the position from 80% to 75%, but it remains above your 20% rule", and nothing in the payload carried the 20%. limit_source is user_cap only when trade_recap.valid_position_cap accepted the user's own override — every other threshold is an engine constant, and an answer that calls a product default "your rule" is a claim the user never made.

(c) Which consumers changed

consumer before after
evaluation_challenge speaking set state in (would_breach, already_over, unjudged, unmapped) every effect except compliant — which is what makes resolved_existing_breach expressible at all
must_state rule-collision entry value: "already_over", anchor .state value: <effect>, anchor .rule_effect; state and worsens demoted into detail
evaluation_challenge (new) rule_effects, the product-safe projection
answer_provenance.required_coverage _COVERED_STATES _COVERED_EFFECTS — the same four rows, keyed on what the trade did
answer_provenance case 7 claim declares worsens (2-way) claim declares rule_effect (6-way); worsens survives as the legacy arm
answer_provenance (new) a delivered claim may not narrate a snake_case engine token

opening_value is on #579's list and is untouched here: card_renderer.build_opening_value is the review-card lane and reads the honesty ledger, not rule_collisions. When it does project rule material it reads rule_effect; the mirrored-surfaces row records that.

The gate's width is deliberately unchanged: _COVERED_EFFECTS is exactly the old (would_breach, already_over) set re-expressed as transitions, so no case that was submittable becomes unsubmittable and vice versa. resolved_existing_breach is stated but not required — silence about good news cannot hide a risk, and the floor is allowed to be wider than the gate.

(d) Diagnostics vs user projection (#713 step 4)

state and worsens stay complete on the stored evaluation row, which is what QA and a later replay read. The rule_effects block carries the effect, the line, whose line it is, and semantic slots — no state, no worsens, no retrieval/recovery/validator chronology. A schema-level additionalProperties: false plus a test on the real emission enforce that.

The slots are meanings, not wording, and the negative half is the load-bearing one:

{"rule_id": "r1", "text": "Cap any single position at 20%.",
 "effect": "improved_but_still_over", "limit": 0.20, "limit_source": "user_cap",
 "must_convey": ["over_before_this_trade", "moved_toward_the_line", "over_after"],
 "must_not_convey": ["crossed_by_this_trade", "moved_further_over"]}

It is a fixed six-row table over one route's rule effect — not a generic response framework, not a second rules engine.

The #674/#676 half on the same surface is mechanical rather than prose: a claim whose text contains a snake_case token out of the engine's own enums (already_over, improved_but_still_over, cost_basis, partial_book, unusable_shares, …) is refused before the row is written. The mechanism and the snake_case-only discipline are question_surface._assert_no_internal_leak's (#305), applied to the answer surface that never had it, and the vocabulary is read off consequence's constants so a new disclosure key is covered the day it is added. clear/unjudged/unmapped are single ordinary words and are excluded by that same rule, not by an exception — "computed on cost basis" passes, cost_basis does not.

Where the two vocabularies can contradict each other, the row is refused. consequence.effect_disagrees_with_state is one table read by both the challenge surface and the provenance gate, so neither believes whichever field it read first. That is #579's "validators must reject disagreement with the recomputed effect".

Compatibility

rule_effect, limit and limit_source are optional on trade-evaluation.schema.json's collision row, so a row already on a user's trade_evaluations.jsonl still validates. A legacy row also still speaks (_LEGACY_SPEAKING_STATES), still obliges a claim (_LEGACY_COVERED_STATES), and still has its direction checked through the worsens arm — the floor and the gate stay one list on an old row too, because a challenge that dropped what the gate still requires would make every case on such a book unsubmittable. test_a_legacy_row_still_states_what_the_gate_still_requires and test_a_legacy_row_still_has_its_direction_checked pin both halves.

Evaluation ids move for a book with tracked rules, because _evaluation_id seeds on rule_collisions and the answer genuinely gained a computed fact — which is that function's own stated contract ("any input that changes the answer necessarily changes what gets hashed"). Existing rows keep their stored ids, --resolve and reconciliation are unaffected, and retries remain idempotent within this version. The pinned-seed regression is unchanged.

(e) Mutation verification

cp backup and restore (never git checkout), __pycache__ cleared before every run, verdict read from the exit code (never grepped for FAIL), baseline and post-restore both re-verified green. 12/12 turned their named suite red; no survivors.

# mutation red in
M1 classify_rule_effect reads only "is it over now" (status-only) consequence, consider
M2 improvement and stall collapse into one effect consequence
M3 must_state states the absolute state again challenge, consider
M4 improved_but_still_over loses crossed_by_this_trade from must_not_convey challenge, consider
M5 the projection carries state/worsens (raw diagnostic narration) challenge
M6 the internal-token leak check becomes a no-op provenance, consider
M7 a reversed transition is accepted provenance, consider
M8 the gate stops requiring an improving collision to be cited challenge, consider
M9 an effect contradicting its own state is believed consequence, challenge, provenance
M10 the line becomes a non-strict bound consequence
M11 every threshold is attributed to the user consequence
M12 a legacy row silently stops speaking challenge

Assertion discipline followed deliberately: each of the six transitions is asserted on its own matrix row and separately by name in a negatively-phrased test, so a mutation returning a plausible-but-wrong member of the vocabulary is caught rather than passing a table lookup; no assertion sits inside a branch a mutation can delete; and the improved_but_still_over fixture is checked to produce the same state/worsens payload as the unchanged_existing_breach one, so the fixture can tell right from wrong instead of passing by coincidence.

(f) Tests

  • python3 tests/run_all.py55/55 suites on 3.11.9, 3.12.4 and 3.14.5 (local python3). CI runs 3.11/3.12.
  • New coverage: tests/test_consequence.py section H (transition matrix + endpoints + attribution + disagreement), tests/test_evaluation_challenge.py section 6 (projection, slots, diagnostics separation, legacy), tests/test_answer_provenance.py section 7/7b (reversed and omitted transition, leak, legacy arm), tests/test_consider.py (the owner scenario end-to-end on the real CLI, plus both refusals).
  • Mirrored surfaces updated together: engine ↔ three schemas ↔ references/trade-consequence.mdqa/SKILL.md's challenge example ↔ evals/trade_answers/TA-001tests/test_interaction_trajectory.py's synthetic challenge ↔ a new docs/maintainer-guide.md row.

Parallel-session note

PR #789 also touches consequence.py, review.py, references/trade-consequence.md, tests/test_consequence.py and tests/test_consider.py, but in the cash-flow/anchor region (portfolio_state, consequence) rather than the rule-collision region. No semantic overlap; whichever lands second should still be re-run merged rather than trusting per-PR CI.

Not in this PR

Owner-live acceptance. #713 step 7 (freeze a new exact main@<sha> and restart the three-journey count from zero) is the owner's, after this merges. This is steps 1–6.

🤖 Generated with Claude Code

… delivered as a breach (refs #579, refs #674, refs #676)

Owner-live acceptance failed on 2026-08-02 because `already_over` — where the
book stands — was read as the transaction's verdict. A sell taking a position
from 80% to 75% against a self-authored 20% cap emitted
`already_over + worsens:false`, which is arithmetically correct, and the
answer built from it told the owner the sell broke their own rule.

`consequence.classify_rule_effect` is now the single derivation over before,
after and the line, and `rule_collision` stamps `rule_effect` on every row.
Two of the six transitions were not expressible in the old pair at all:
`_worsened` is a strict `>`, so an improvement and a stall are the same
`false`, and `clear` cannot say whether the line was crossed before — which
is why a prompt-only cut could not have held whatever it said.

Every decision-facing consumer reads the effect and nothing re-derives it:
the challenge's `must_state` value and anchor, the new product-safe
`rule_effects` projection, `required_coverage`, and the provenance gate,
where a claim must now declare the frozen transition as a six-way assertion
instead of a two-way boolean. `state`/`worsens` stay on the stored row as
machine diagnostics for QA and replay, and a row whose two vocabularies
contradict each other is refused by both readers through one table.

The same cut closes the other half of that walk on the same surface: a
delivered claim that narrates a snake_case engine token is refused, reusing
the question surface's own rule (#305) on the answer surface that lacked it.

`rule_effect`/`limit`/`limit_source` are optional on a stored row, so a row
recorded before them still validates, still speaks, and still has its
direction checked through the pre-#579 `worsens` arm.

Twelve mutations — status-only interpretation restored, the improving effect
losing its prohibition, the projection leaking diagnostics, a reversed
transition accepted, raw diagnostic narration allowed, the legacy row going
silent — each turned its named suite red. Full offline suite green on Python
3.11, 3.12 and 3.14.

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

atomchung commented Aug 2, 2026

Copy link
Copy Markdown
Owner Author

Verified, and one of the two "needs an owner ruling" items is already ruled

(Reposted — the first version of this comment lost four identifiers to shell substitution. Content below is the intended text.)

The mechanical conclusion checks out. _worsened on main is (after_value - before_value) > _EPSILON — strict — so a reading that fell and a reading that did not move are indistinguishable at the payload level. Confirmed by reading the pre-fix source, not by taking the report. That is what makes this a payload-expressiveness defect rather than a prose defect, and it is the record #713 step 1 asked for. Posted there.

Combined with current main@bd2d9f9 (which now also carries #789's consequence.py changes and the parallel session's #785): merged locally, six files auto-merged clean, full suite 56/56 green.


Ruling 2 — limit_source — already decided, and this implements it

You flagged limit_source (user_cap when the user really set the cap, engine_default otherwise) as needing confirmation. It does not: the owner ruled on exactly this on 2026-08-02, and this PR is the mechanical hook that ruling asked for.

The ruling came from reading three parallel lanes side by side. The product was storing source: "user_chosen" in rules.jsonl for a threshold that was a hardcoded engine constant the user had never set, and then telling the user "the rule you just wrote yourself". The owner's own words on seeing it: that 20% is our system's default, the user didn't choose it, right? He was right.

AGENTS.md boundary 2 already forbids the mirror image — relabelling the user's own words as an outside source. The product was shipping the reflection of that: engine sentence plus engine threshold, wearing the user's name.

So limit_source is not a new attribution decision to confirm. It is the field that makes the existing ruling enforceable in code instead of by copy discipline. Keep it exactly as built.

Ruling 1 — evaluation ids move — taking the call

Your reading is right and I am not sending it up. _evaluation_id seeds on the content that produced the answer; the answer legitimately gained a computed fact, so a fresh id for a re-ask across the upgrade is the id contract working, not drifting. Stored rows keep their ids, --resolve and reconciliation are unaffected, and the blast radius is one migration-boundary re-ask converging to a new row instead of an old one. Keeping the effect off the stored row — the alternative you rejected — would have meant the id no longer covers everything the answer asserts, which is the worse of the two.

Merging.

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.

1 participant