diff --git a/docs/decisions/ADR-0174-held-hypothesis-comprehension.md b/docs/decisions/ADR-0174-held-hypothesis-comprehension.md index 763a235c..88a5270e 100644 --- a/docs/decisions/ADR-0174-held-hypothesis-comprehension.md +++ b/docs/decisions/ADR-0174-held-hypothesis-comprehension.md @@ -292,3 +292,130 @@ This ADR moves to **Accepted** when: - **Thesis anchor**: [[thesis-decoding-not-generating]] — every change in this ADR must pass the "teach the engine to find, not store another found thing" gate. - **HITL corridor preserved**: ADR-0150 (contemplation), ADR-0152 (proposal), ADR-0155 (review), ADR-0161 (HITL queue), ADR-0172 (math-corpus decomposition). - **Anti-overfitting obligations**: ADR-0114a — held-hypothesis reads are evaluated against the same obligations as the existing pipeline; perturbation, OOD ratio, depth curve, and adversarial axes all apply. + +--- + +## Implementation Notes (added 2026-05-28 after Phase 1-3a lookback review) + +The 2026-05-28 lookback review (per CLAUDE.md §Lookback Review Discipline) +surfaced drift between this ADR's spec text and what Phases 1-3a +actually shipped. Documenting honestly here so Phase 4-5 implementers +work from accurate ground truth. + +### `reevaluate` signature — implementation differs from spec + +**ADR text (§Decision §2):** +```text +reevaluate(hypotheses, new_token, position) -> (refined_hypotheses, eliminations) +``` + +**As shipped (Phase 3a, PR #423):** +```python +reevaluate(hypothesis: Hypothesis, refinement: Refinement) -> ReevaluateResult +``` + +Differences and rationale: + +- Single hypothesis + refinement object, not hypothesis set + token+position. + More composable: refinement objects (`PronounResolution` etc.) are + reusable; the caller decides which hypotheses to apply to which + refinements. +- Returns a single `ReevaluateResult` (refined-or-None plus + bookkeeping), not a (refined_hypotheses, eliminations) tuple. + Bulk-eliminate semantics belong on a higher-level orchestrator (not + yet built — Phase 4 work). + +The shipped design is preferred and the ADR text should be considered +amended. Phase 4 implementers should follow the shipped signature. + +### Phase 2 is per-candidate, not per-token + +**ADR text (§Decision §3):** +> Move them inside the reader so they fire per-token: After every EMIT, +> run the in-flight constraint check against the partial hypothesis. + +**As shipped (Phase 2, PR #420):** +Constraint propagation runs at the `math_candidate_graph` recognizer- +injection site (per-candidate, after `inject_from_match` returns), not +inside `lifecycle.apply_word` (per-token, during reading). + +This is a real substrate-vs-active-wiring gap. The check_constraints +primitive is available; the per-token integration is Phase 5 work +(legacy parser removal + apply_word refactor). Phase 2 is more +honestly described as "constraint propagation substrate ready for +per-token wiring in Phase 5." + +### Lookback recompute scope is candidate-level, not token-level + +**ADR text (§Decision §2):** +> For each hypothesis, recomputes the category assignment of any *prior* +> token whose role depended on the now-resolved ambiguity. + +**As shipped (Phase 3a, PR #423):** +`PronounResolution` appends one `(0, "pronoun_resolved", pronoun)` +entry to `Hypothesis.category_assignments` and rewrites the candidate's +semantic actor field. It does not walk back through per-token +assignments because Phase 1-3a's `category_assignments` is +candidate-level, not token-level. Per-token category assignment +becomes meaningful in Phase 5 (apply_word refactor). + +Phase 3b will widen this when compound-clause refinement enters +(multiple per-clause assignments do need recompute walks). + +### Hypothesis.constraint_state is never populated by Phase 2 + +The Phase 1 substrate carries `Hypothesis.constraint_state: tuple[tuple[str, str], ...]` +for recording predicate outcomes. Phase 2's `check_constraints` +populates `ConstraintResult.predicates_run` but does NOT copy that +into `Hypothesis.constraint_state` on the survivors. Survivors carry +forward their original (empty) constraint_state. + +Phase 4 (in-loop contemplation) may want to consult +`constraint_state` to decide which evidence to seek. If so, Phase 4 +must wire the population step explicitly. Not a Phase 2 defect — it's +a Phase 2 scope limit. + +### Multi-actor pronoun wrong=0 hazard defense (Phase 3a follow-up) + +The 2026-05-28 review surfaced a real wrong=0 hazard in Phase 3a's +`PronounResolution` wiring: the `_discourse_prior_subjects` lookup is +gender-blind and stores only most-recent-prior subject. In multi-actor +problems ("Alice has 5. Bob has 3. She buys 2."), this could resolve +"She" to "Bob" and produce wrong attribution. + +Fix landed in the same Phase 3a PR (PR #423): defensive +`no_antecedent_ambiguous` refusal when more than one distinct +proper-noun subject appears in prior context. Refusal-preferring +discipline preserves `wrong = 0`. + +This is the prototype for refinement-quality gating that Phase 4 (in- +loop contemplation) inherits: ambiguity that resolution cannot +disambiguate is a refusal, not a guess. + +### LOC accounting + +The ADR §"What this collapses" projects "Net ~1,900 lines removed" +once Phase 5 retires the legacy parser. Honest current state: + +- Phase 1 added ~243 lines (`state.py`) +- Phase 2 added ~726 lines (`constraint_propagation.py` new module) + + ~50 lines `math_candidate_graph.py` wiring +- Phase 3a added ~387 lines (`lookback.py` new module) + ~125 lines + `math_candidate_graph.py` wiring + ~15 lines `recognizer_match.py` + +**Phases 1-3a net: +1,500 lines added.** Phase 5 will remove +math_parser.py (~1,100 lines) + per-category dispatch (~400 lines) + +duplicate per-sentence-choice scaffolding (~300 lines) to reach the +projected net removal. + +The substrate is correctly load-bearing; the line-count payoff is in +Phase 5. + +### Test coverage backfill + +The lookback review found 13 of 17 `VALID_PREDICATE_NAMES` lacked +direct predicate-name assertions in tests, and all 4 +`_check_composed_initial` sub-checks were untested (parity verified +manually). Backfill landed in the same Phase 3a PR (10 new tests). + +--- diff --git a/generate/comprehension/lookback.py b/generate/comprehension/lookback.py index 4dffe3d4..87a0cbc6 100644 --- a/generate/comprehension/lookback.py +++ b/generate/comprehension/lookback.py @@ -292,9 +292,13 @@ def reevaluate( hypothesis so trace serialisation can record the before/after pair. """ # Dispatch on refinement kind. Phase 3a knows pronoun_resolution. + # The defensive raise below is unreachable today (the Refinement + # Union has one member), but it is correct future-proofing for + # Phase 3b's CompoundClauseExpansion and other refinement types — + # if a caller passes a non-Union type by accident, fail loudly. if isinstance(refinement, PronounResolution): return _apply_pronoun_resolution(hypothesis, refinement) - raise ComprehensionStateError( + raise ComprehensionStateError( # type: ignore[unreachable] f"reevaluate: unsupported refinement type {type(refinement).__name__}" ) diff --git a/generate/math_candidate_graph.py b/generate/math_candidate_graph.py index f06c2760..02ba546d 100644 --- a/generate/math_candidate_graph.py +++ b/generate/math_candidate_graph.py @@ -876,6 +876,29 @@ def parse_and_solve( break _antecedent = _effective_prior + # ADR-0174 Phase 3a — multi-actor pronoun + # ambiguity defense. When the problem has + # more than one distinct proper-noun subject + # in prior context, picking the most-recent + # is a guess (e.g. "Alice has 5. Bob has 3. + # She buys 2." — "She" should bind to Alice + # by gender but the discourse map returns + # Bob). No safety net downstream would catch + # a wrong attribution (single-binding + # emission → no multi-branch disagreement; + # verifier re-derives the same wrong graph). + # Refuse rather than guess. Surfaced by + # 2026-05-28 Phase 1-3a lookback review; + # see project-adr-0174-multi-actor-pronoun-hazard + # memory and CLAUDE.md §Lookback Review + # Discipline. + _distinct_priors = { + v for v in _discourse_prior_subjects.values() + if v is not None + } + if _prior_subject is not None: + _distinct_priors.add(_prior_subject) + _multi_actor_ambiguous = len(_distinct_priors) > 1 if _held_pronoun is None or not _antecedent: # No resolution path available — drop the # held candidates and log the lookback @@ -888,6 +911,21 @@ def parse_and_solve( "sentence_index": s_idx, }, sort_keys=True)) injected = () + elif _multi_actor_ambiguous: + # Refusal-preferring discipline: multiple + # distinct proper-noun subjects in prior + # context means the resolver would be + # guessing. Drop the held candidates, + # log the ambiguous-antecedent event. + _statement_trace.append(json.dumps({ + "layer": "lookback", + "phase": 3, + "outcome": "no_antecedent_ambiguous", + "pronoun": _held_pronoun, + "candidate_antecedents": sorted(_distinct_priors), + "sentence_index": s_idx, + }, sort_keys=True)) + injected = () else: _refinement = PronounResolution( pronoun=_held_pronoun, @@ -1013,11 +1051,17 @@ def parse_and_solve( f"(category={recognizer_match.category.value})" ), branches_enumerated=0, branches_admissible=0, + # ADR-0174 Phase 3a — preserve statement-stage + # trace events on early refusal so consumers see + # WHY admission failed (lookback no_antecedent, + # constraint_propagation eliminations, etc.). + reader_trace=tuple(_statement_trace), ) return CandidateGraphResult( answer=None, selected_graph=None, refusal_reason=f"no admissible candidate for statement: {s!r}", branches_enumerated=0, branches_admissible=0, + reader_trace=tuple(_statement_trace), ) per_sentence_choices.append(_collapse_per_sentence_ties(choices)) # ME-2 — update prior_subject AFTER this sentence is processed. diff --git a/tests/test_adr_0174_phase2_constraint_propagation.py b/tests/test_adr_0174_phase2_constraint_propagation.py index b091c383..8cddad4d 100644 --- a/tests/test_adr_0174_phase2_constraint_propagation.py +++ b/tests/test_adr_0174_phase2_constraint_propagation.py @@ -248,6 +248,188 @@ class TestCheckConstraintsOperationParity: assert roundtrip_admissible(op) is False +class TestCheckConstraintsInitialPredicateNames: + """Predicate-name assertions for every initial.* failure path. + Surfaced by 2026-05-28 lookback review — 13 of 17 predicates in + VALID_PREDICATE_NAMES lacked direct elimination-reason assertions.""" + + def test_initial_value_grounds_predicate_name(self) -> None: + ic = _initial(matched_value_token="99") # source has "3" + result = check_constraints(hypothesis_from_initial(ic, 0)) + assert result.admitted is False + first_fail = next( + (p for p, o in result.predicates_run if o == "fail"), None + ) + assert first_fail == "initial.value_grounds" + + def test_initial_unit_grounds_predicate_name(self) -> None: + ic = _initial(matched_unit_token="oranges") # source has "apples" + result = check_constraints(hypothesis_from_initial(ic, 0)) + assert result.admitted is False + first_fail = next( + (p for p, o in result.predicates_run if o == "fail"), None + ) + assert first_fail == "initial.unit_grounds" + + def test_initial_entity_grounds_predicate_name(self) -> None: + ic = _initial(matched_entity_token="Tom") # source has "Sam" + result = check_constraints(hypothesis_from_initial(ic, 0)) + assert result.admitted is False + first_fail = next( + (p for p, o in result.predicates_run if o == "fail"), None + ) + assert first_fail == "initial.entity_grounds" + + +class TestCheckConstraintsOperationPredicateNames: + """Predicate-name assertions for every operation.* failure path.""" + + def test_operation_verb_grounds_predicate_name(self) -> None: + # Verb registered for kind but not in source span — distinct + # from verb_registered failure. + op = CandidateOperation( + op=Operation(actor="Sam", kind="add", + operand=Quantity(value=5, unit="apples")), + source_span="Sam now owns 5 apples.", # 'buys' not in source + matched_verb="buys", + matched_value_token="5", + matched_unit_token="apples", + matched_actor_token="Sam", + ) + result = check_constraints(hypothesis_from_operation(op, 0)) + assert result.admitted is False + first_fail = next( + (p for p, o in result.predicates_run if o == "fail"), None + ) + assert first_fail == "operation.verb_grounds" + + def test_operation_value_grounds_predicate_name(self) -> None: + op = _operation_add( + matched_value_token="99", # source has "5" + source_span="Sam buys 5 apples.", + ) + result = check_constraints(hypothesis_from_operation(op, 0)) + assert result.admitted is False + first_fail = next( + (p for p, o in result.predicates_run if o == "fail"), None + ) + assert first_fail == "operation.value_grounds" + + def test_operation_unit_grounds_predicate_name(self) -> None: + op = _operation_add( + matched_unit_token="oranges", # source has "apples" + source_span="Sam buys 5 apples.", + ) + result = check_constraints(hypothesis_from_operation(op, 0)) + assert result.admitted is False + first_fail = next( + (p for p, o in result.predicates_run if o == "fail"), None + ) + assert first_fail == "operation.unit_grounds" + + +class TestCheckConstraintsComposedInitialPath: + """The RAT-1 composed_initial path was completely untested before + 2026-05-28 lookback review — parity was manually verified but no + automated test asserted the 4 sub-checks fire correctly.""" + + def _composed(self, **ev_overrides: str) -> CandidateInitial: + from generate.math_problem_graph import InitialPossession, Quantity + ev: dict[str, str] = { + "composition_shape": "bound(count) × bound(unit_cost)", + "input_tokens": "3|400", + "entity_source": "prior_sentence", + "currency_symbol": "$", + } + ev.update(ev_overrides) + return CandidateInitial( + initial=InitialPossession( + entity="John", quantity=Quantity(value=1200, unit="dollars"), + ), + source_span="3 vet appointments at $400 each", + matched_anchor="has", + matched_value_token="1200", + matched_unit_token="dollars", + matched_entity_token="John", + composition_evidence=ev, + ) + + def test_well_formed_composed_initial_admits(self) -> None: + ic = self._composed() + result = check_constraints(hypothesis_from_initial(ic, 0)) + assert result.admitted is True + # All 4 sub-checks run (some may skip). + predicate_names = {p for p, _ in result.predicates_run} + assert "composed_initial.evidence_complete" in predicate_names + assert "composed_initial.input_tokens_ground" in predicate_names + assert "composed_initial.entity_token_present" in predicate_names + + def test_composed_initial_missing_evidence_key_eliminated(self) -> None: + # Construct via dict mutation since the field is a Mapping + ev_partial = { + "composition_shape": "shape", + "input_tokens": "3|400", + # entity_source missing + } + ic = self._composed() + # Replace composition_evidence via dataclasses.replace would + # break frozen; build a new candidate directly. + from generate.math_problem_graph import InitialPossession, Quantity + ic2 = CandidateInitial( + initial=InitialPossession( + entity="John", quantity=Quantity(value=1200, unit="dollars"), + ), + source_span=ic.source_span, + matched_anchor=ic.matched_anchor, + matched_value_token=ic.matched_value_token, + matched_unit_token=ic.matched_unit_token, + matched_entity_token=ic.matched_entity_token, + composition_evidence=ev_partial, + ) + result = check_constraints(hypothesis_from_initial(ic2, 0)) + assert result.admitted is False + first_fail = next( + (p for p, o in result.predicates_run if o == "fail"), None + ) + assert first_fail == "composed_initial.evidence_complete" + + def test_composed_initial_input_token_missing_eliminated(self) -> None: + ic = self._composed(input_tokens="999|400") # 999 not in source + result = check_constraints(hypothesis_from_initial(ic, 0)) + assert result.admitted is False + first_fail = next( + (p for p, o in result.predicates_run if o == "fail"), None + ) + assert first_fail == "composed_initial.input_tokens_ground" + + def test_composed_initial_currency_symbol_missing_eliminated(self) -> None: + # Build a candidate whose source span has no $ but evidence + # claims currency_symbol="$". + from generate.math_problem_graph import InitialPossession, Quantity + ic = CandidateInitial( + initial=InitialPossession( + entity="John", quantity=Quantity(value=1200, unit="dollars"), + ), + source_span="3 vet appointments at 400 each", # no $ + matched_anchor="has", + matched_value_token="1200", + matched_unit_token="dollars", + matched_entity_token="John", + composition_evidence={ + "composition_shape": "shape", + "input_tokens": "3|400", + "entity_source": "prior_sentence", + "currency_symbol": "$", + }, + ) + result = check_constraints(hypothesis_from_initial(ic, 0)) + assert result.admitted is False + first_fail = next( + (p for p, o in result.predicates_run if o == "fail"), None + ) + assert first_fail == "composed_initial.currency_symbol_present" + + class TestCheckConstraintsResultShape: def test_predicates_run_only_uses_known_predicate_names(self) -> None: ic = _initial() diff --git a/tests/test_adr_0174_phase3_lookback.py b/tests/test_adr_0174_phase3_lookback.py index 8494d05a..108f1d9f 100644 --- a/tests/test_adr_0174_phase3_lookback.py +++ b/tests/test_adr_0174_phase3_lookback.py @@ -347,6 +347,74 @@ class TestPhase3WiringEndToEnd: for ev in lookback_events ), f"expected lookback admitted event; trace={lookback_events}" + def test_multi_actor_ambiguous_refuses_with_no_antecedent_ambiguous(self) -> None: + """ADR-0174 Phase 3a wrong=0 hazard defense — surfaced by + 2026-05-28 lookback review. + + When a problem has more than one distinct proper-noun subject + in prior context, the _discourse_prior_subjects lookup is + gender-blind and would silently pick the most-recent-prior as + the antecedent. In 'Alice has 5. Bob has 3. She buys 2.', + this would resolve 'She' to 'Bob' and attribute Alice's + purchase to Bob — wrong attribution with no downstream safety + net. + + Defense: refuse with no_antecedent_ambiguous trace event when + multiple distinct proper-noun subjects appear in prior + context. Refusal-preferring discipline preserves wrong=0. + """ + from generate.math_candidate_graph import parse_and_solve + text = ( + "Alice has 5 Pokemon cards. " + "Bob has 3 Pokemon cards. " + "She collected 2 Pokemon cards. " + "How many Pokemon cards does Bob have?" + ) + r = parse_and_solve(text) + # MUST refuse — wrong attribution is the hazard. + assert r.answer is None + lookback_events = [ + json.loads(ev) for ev in r.reader_trace + if json.loads(ev).get("layer") == "lookback" + ] + ambig = [ + ev for ev in lookback_events + if ev.get("outcome") == "no_antecedent_ambiguous" + ] + assert ambig, ( + f"expected no_antecedent_ambiguous event; trace={lookback_events}" + ) + ev = ambig[0] + assert "Alice" in ev["candidate_antecedents"] + assert "Bob" in ev["candidate_antecedents"] + assert ev["pronoun"] == "She" + + def test_single_actor_pronoun_still_resolves(self) -> None: + """Counter-test: when there's only ONE distinct prior subject, + the defense MUST NOT fire — pronoun resolution proceeds.""" + from generate.math_candidate_graph import parse_and_solve + text = ( + "Bob has 10 Pokemon cards. " + "He collected 5 Pokemon cards. " + "How many Pokemon cards does Bob have?" + ) + r = parse_and_solve(text) + lookback_events = [ + json.loads(ev) for ev in r.reader_trace + if json.loads(ev).get("layer") == "lookback" + ] + assert not any( + ev.get("outcome") == "no_antecedent_ambiguous" + for ev in lookback_events + ), ( + f"single-actor case must not trigger ambiguity defense; " + f"trace={lookback_events}" + ) + assert any( + ev.get("outcome") == "admitted" and ev.get("resolved_to") == "Bob" + for ev in lookback_events + ) + def test_no_antecedent_emits_no_antecedent_trace_event(self) -> None: from generate.math_candidate_graph import parse_and_solve # No proper-noun antecedent before the held pronoun sentence. @@ -359,20 +427,11 @@ class TestPhase3WiringEndToEnd: json.loads(ev) for ev in r.reader_trace if json.loads(ev).get("layer") == "lookback" ] - # Either we emitted a no_antecedent event, OR the sentence - # refused before reaching the lookback path. Both preserve - # wrong=0; the assertion captures the intent. - no_antecedent = any( - ev.get("outcome") == "no_antecedent" - for ev in lookback_events - ) - # The problem should refuse cleanly regardless. assert r.refusal_reason is not None or r.answer is None - # If lookback fired at all, it must have been no_antecedent. for ev in lookback_events: - assert ev.get("outcome") in ("no_antecedent", "eliminated"), ( - f"unexpected lookback outcome on no-antecedent input: {ev}" - ) + assert ev.get("outcome") in ( + "no_antecedent", "no_antecedent_ambiguous", "eliminated" + ), f"unexpected lookback outcome on no-antecedent input: {ev}" # ---------------------------------------------------------------------------