fix(specialist): stop rejecting our own proposals on a format rule we ship broken

[zoroyihan7]

Refs #1143.

Deliberately not Closes: #1143's agreed fix had two halves, and only the first
is here. (a) strip the forbidden fields at source instead of only auditing them —
done. (b) pass approved_variant_names so a partial reject stops discarding
every variant — untouched, and _handle_review_verdict's collapse still ranks
reject above advise, so a multi-variant proposal with one genuinely rejected
variant still loses the set. The reported failure no longer produces a reject
at all, but that general problem outlives this PR and the issue should stay open
for it.

The Critic rejected its own scouts' proposals for carrying a self-reported gain
field, so the run's only novel optimization ideas never reached a benchmark and
there was no budget left to resubmit. The cause is entirely in this repo — no
run artifacts were needed to reproduce the reasoning.

What was actually wrong

The system detected the violation, declined to fix it, then punished itself for it.
scan_quantitative_claims finds the forbidden fields, records them in an audit
note, and forwards the payload unchanged; the Critic is then instructed to reject
the whole proposal_set over them.

We also instructed the specialist to commit the violation. The cold-start
directive told it to flag each fallback proposal confidence: low — a field in
FORBIDDEN_PROPOSAL_FIELDS. A compliant specialist therefore tripped the guard
on exactly the round where it was the only source of ideas, which is the reported
scenario.

The guard's scope contradicted itself. The rule the Critic is given covers
proposal_set[*], but the scan also applied it to the payload top level, where
confidence is the round-level self-assessment our own output schema asks for
and two audit writers record. That is not a per-proposal gain claim and cannot
bias which variant gets benched.

The ban was prose only, so the Critic generalised it. specialist_quantitative_claim_violation
appears nowhere in the codebase except the Critic prompt — nothing produces,
consumes or tests it. The verdict that lost the run named predicted_gain_pct,
which is not on the enforced list, and is a required field on the
propose_action channel taught by our own prompt template. No output could
satisfy both readings of that field name.

Changes

Commit 1 — fix(specialist): stop shipping a proposal schema that fails its own guard

• The runner strips the forbidden fields before writing specialist_done.json.
  The claim is worthless either way (measured gain is the Coordinator's), so
  dropping it makes the verdict unreachable rather than merely audited; the audit
  note still records what was there.
• Cold start no longer asks for confidence.
• Guard scope split so the top-level round assessment survives.

Commit 2 — fix(critic): make a proposal format slip advisory instead of fatal

• review_constraints now carries a rule descriptor generated from
  FORBIDDEN_PROPOSAL_FIELDS, so the Critic's list cannot drift from the
  enforced one. Uses the mechanism the cross-domain rules already use rather than
  templating a verbatim-loaded prompt.
• The verdict for this class becomes advise. Detection breadth is deliberately
  kept — a gain claim smuggled under an unlisted name should still be noticed —
  but noticing it must not reject the set.

Commit 3 — fix(loop): hold a reject to the verdict its own rule declared

Commit 2 left the behavioural requirement as prose, same class as the defect
it fixes: nothing stopped the Critic rejecting anyway, and the reason code it
cites had no production consumer, so a repeat would lose the round undetected.

• advisory_only_reason_codes() derives the code set from the descriptors the
  Critic is handed — the quantitative-claim rule plus every cross-domain rule
  declaring advise — rather than restating them, so a rule that changes its
  verdict cannot leave a stale entry behind.
• A reject citing one of those codes is held to advise in the verdict path,
  logged, and recorded as an observation, so prompt drift stays visible instead
  of being silently corrected.
• The hold runs per verdict_map entry before the collapse, since the collapse
  ranks reject above advise: one variant rejected on an advisory-only rule
  would otherwise still discard its siblings' advice. This narrows the blast
  radius for this class only; half (b) above is still the general fix.
• advise is already permissive for the integrate_patch gate, so the downgrade
  lands the proposal exactly where an obedient Critic would have.

Kept as separate commits so the prompt-behaviour change can be reverted on its own.

Test plan

• test_specialist_patch_safety_unit.py — strip semantics, non-dict payloads,
  round-level confidence preserved, descriptor matches the enforced list and
  carries advise, and the advisory-code set covers every rule that declared
  advise while leaving a rule that kept reject out of it
• test_critic_verdict_map.py — a reject citing an advisory-only rule is held
  to advise and materialises; the downgrade leaves a log line and an
  observation; a cross-domain hint reject is held too; a reject with no code or
  a code outside the set still rejects; one held variant does not out-rank its
  siblings, and a map rejected only on advisory grounds survives
• test_specialist_runner_helpers_unit.py — _finalize writes a stripped
  artifact and still emits the audit note
• test_specialist_prompt_builder_coverage_unit.py — cold start no longer
  asks for a forbidden field
• test_critic_agent_backend.py — the bundle delivered to the model carries
  the rule
• 1544 passed across the specialist / critic / explore / writeback / prompt
  suites; the only 2 failures are pre-existing (claude-agent-sdk not
  installed in this environment, identical on the base commit)

Made with Cursor

[github-actions]

CI E2E report — ✅ Succeeded

item	value
result	✅ Succeeded
model	Qwen/Qwen3-0.6B (dense)
resources	1× GPU, TP=1
PR branch	fix/1143-proposal-schema
commit	c14ee71cbd849e6f86fb7880e083089242534e66
session_id	66d75126-eb9b-414e-8c37-c41bdfb1575e
queue → dispatch	0s
run time	160m 17s
total	160m 17s

details

[chaojhou]

The diagnosis in this PR is better than the issue it closes. Two root causes here were not in #1143 at all, and both sit closer to the real defect than the field-name confusion the issue led with:

• We instructed the specialist to commit the violation. The cold-start directive asked it to flag each fallback proposal confidence: low, and confidence is in FORBIDDEN_PROPOSAL_FIELDS. A compliant specialist therefore tripped the guard on exactly the round where it was the only source of ideas.
• The guard's scope contradicted itself. The rule handed to the Critic covers proposal_set[*], but the scan also applied it to the payload top level, where confidence is the round-level self-assessment our own output schema asks for and two audit writers record.

I checked the implementation against main rather than taking it on trust, and these hold:

• The strip runs before self._write_specialist_done(...), and the ordering is scan-then-strip, so the audit note keeps the evidence while the artifact ships clean.
• The drift is structurally gone: quantitative_claim_rule_descriptor() derives forbidden_proposal_fields from sorted(FORBIDDEN_PROPOSAL_FIELDS) and critic.md no longer hardcodes the six names. Worth noting the prompt copy had already drifted — it was missing expected_gain_pct.
• advise genuinely lets the proposal through: intent_router.py materializes on verdict in ("approve", "advise"), so switching the verdict is the real fix and not just a softer label.
• The framework_agent carve-out for predicted_gain_pct / prior_score / prior_rank is preserved and clarified. That matters, because the verdict that lost the run named predicted_gain_pct and anyone fixing this from the issue text alone would plausibly have deleted a field that propose_action, the Critic's own verdict schema and the gain-calibration path all depend on.

Two things I would still change.

1. The remedy is prose-only, same class as the diagnosis (can be a follow-up)

The PR's own finding is "the ban was prose only, so the Critic generalised it". The rule is now delivered as data, which fixes the drift — but the behavioural requirement ("emit advise, never reject") is still an instruction to a model with no code backstop.

QUANTITATIVE_CLAIM_REASON_CODE has zero production consumers: the constant, the descriptor field, and one test asserting the descriptor carries it. So if the Critic still emits reject with this reason, the round is lost exactly as before and nothing detects it.

Now that failure_reason_code is a named constant travelling in the bundle, a deterministic backstop is cheap: in the verdict path, when verdict == "reject" and the reason matches QUANTITATIVE_CLAIM_REASON_CODE, downgrade to advise and log it. That turns this from "we asked nicely" into "it cannot happen". Given the cost of the failure is an entire round's novel ideas, I think it is worth having — but I am fine with it landing separately.

2. Closes #1143 overstates the scope (worth changing now)

#1143's agreed fix had two halves: (a) strip the forbidden fields at source instead of only auditing them, and (b) pass approved_variant_names so a partial reject stops discarding every variant.

(a) is done thoroughly here. (b) is untouched, and the collapse in intent_router.py still ranks reject above advise:

verdict = ("approve" if "approve" in sub_verdicts
           else "reject" if "reject" in sub_verdicts
           else "advise" if "advise" in sub_verdicts
           else "needs_review")

So a multi-variant proposal with one genuinely rejected variant still discards the whole set, including the ones that should have been advised through, and approved_variant_names is still never passed on the live path.

Since this class no longer produces a reject, the reported failure is fixed — but the general "one bad variant kills the set" problem remains. I would either do (b) here or retitle to Refs #1143 and keep the issue open for it. My preference is the retitle: if the issue closes on merge, (b) is the half that quietly gets forgotten.

Nit

strip_forbidden_proposal_fields returns the removed field names but the production call site ignores them (the audit uses the scan's list). Not a defect, just noting the return value exists for the tests.

Splitting the prompt-behaviour change into its own commit so it can be reverted alone is the right instinct for a change like this.

[zoroyihan7]

Thanks — both changes are in, as be260e19, plus a sync with main now that #1162 has landed.

1. The remedy is no longer prose-only

Took this in this PR rather than as a follow-up: the cost of the failure is a
round's novel ideas, and the constant having zero production consumers was the
same defect class the PR is about.

Generalised it one step beyond QUANTITATIVE_CLAIM_REASON_CODE, because the
three CROSS_DOMAIN_RULES declare advise for the same reason and were equally
unenforced. advisory_only_reason_codes() derives the code set from the
descriptors the Critic is actually handed, so a rule that changes its
failure_verdict cannot leave a stale entry behind, and a typo in one rule's
verdict literal cannot silently drop it (hence ADVISE_VERDICT spelled once and
read by the table).

Two details worth flagging:

• The hold runs per verdict_map entry, before the collapse. With it applied
  only to the collapsed verdict, a single variant rejected on an advisory-only
  rule would still out-rank its siblings' advise and discard the set — the
  ranking you quote in point 2. So the entry-level hold is what makes the
  downgrade actually reach the multi-variant case.
• advise is already in INTEGRATE_PATCH_PERMISSIVE_VERDICTS, so a
  downgraded verdict lands a specialist patch exactly where an obedient Critic
  would have, rather than into a state the gate has no rule for.

The downgrade is logged and recorded as a verdict_downgraded_to_rule_verdict
observation. Silently correcting the model would have hidden exactly the drift
that cost the run, so it is detectable now — which was the substance of your
objection.

I left critic.md alone deliberately: telling the model a backstop exists
invites it to lean on the backstop.

2. Closes → Refs

Agreed, and retitled to Refs #1143 with the reason stated in the description:
half (b) (approved_variant_names on the live path, and the collapse ranking
reject above advise) is untouched, so a multi-variant proposal with one
genuinely rejected variant still loses the set. The entry-level hold above
narrows that for this one class only; the general problem outlives this PR and
should keep the issue open. Your reasoning for the retitle over doing (b) here is
also mine — (b) needs the _materialize_approved_proposal filter wired to the
live path and deserves its own review, not a rider on a prompt fix.

3. Nit

Correct, and left as-is. The production call site audits from the scan's list
because the scan runs first and sees the pre-strip payload; the return value is
what lets the tests assert which fields the strip removed without re-deriving
it. Changing the call site to consume it would make the audit depend on strip
order for no gain.

New tests: nine, split between the code-set derivation
(test_specialist_patch_safety_unit.py) and the verdict path
(test_critic_verdict_map.py) — including that a reject with no code, or a code
no rule declares as advisory, still rejects.

[zoroyihan7]

Pushed ea3eab2e1, which closes a gap the field data on #1146 exposed in be260e193.

The downgrade keyed on an entry-level failure_reason_code. That field is an input descriptor — prompts/critic.md and specialists/patch_safety.py hand the code to the model, but nothing on the output side validates or requires it back. The verdict that motivated this branch does not carry it:

{
  "verdict": "reject",
  "reasoning": "specialist_quantitative_claim_violation: the proposal payload carries the forbidden predicted_gain_pct field.",
  "packet_evidence": ["payload.predicted_gain_pct"],
  "notes": ["Resubmit without predicted_gain_pct or other prohibited quantitative ranking fields."]
}

The code is in reasoning. So replaying the exact payload this PR exists to fix would have left the reject standing, and the FRAMEWORK_AGENT phase entry would still have been lost.

The commit adds cited_advisory_reason_code(): the explicit field first, then a word-bounded scan of the verdict's own prose (reasoning, notes). Two properties worth checking in review:

• An explicit code still wins. A verdict declaring failure_reason_code: "specialist_patch_not_grounded" while mentioning an advisory code in passing stays a reject — prose cannot soften a rule the Critic cited at reject.
• The scan is word-bounded. x_specialist_quantitative_claim_violation_v2 in a log key is not a citation.

Tests: the field payload's exact shape, an ordinary reject whose prose happens to mention predicted_gain_pct without citing a rule, the explicit-code-outranks-prose case, and the substring guard.

The pre-existing no_reason_code parametrisation of test_a_substantive_reject_still_rejects still passes unchanged, which is the property that matters most here: a reject citing nothing at all is still a reject.

[zoroyihan7]

/retest

The e2e run for ea3eab2e1 was cancelled 16s after it started, by the comment posted alongside the push. Same defect as the earlier cancellation on this PR: ci-e2e answers issue_comment, its concurrency group is keyed on the PR number with cancel-in-progress, and GitHub resolves the group when the run is created — before resolve.if can decline the work. #1173 fixes it; until that lands, a comment on a PR kills that PR's in-flight run.

Verified ea3eab2e1 locally against the field payload: it downgrades to advise citing specialist_quantitative_claim_violation, x_specialist_quantitative_claim_violation_v2 in prose does not fire, an explicit non-advisory failure_reason_code keeps the reject even with an advisory code in the prose, and a plain reject is untouched. 82 tests green across the two suites; ruff clean.

[chaojhou]

Following up on my earlier review — the deterministic downgrade I asked for is here and built the right way. advisory_only_reason_codes() derives the set from the descriptors the Critic is handed rather than restating it (patch_safety.py:279), so a rule that changes its failure_verdict cannot leave a stale entry behind; the hold runs per verdict_map entry before the collapse (intent_router.py:216), so an advisory reject cannot out-rank its siblings; and the move is recorded as an observation instead of being silently corrected. test_a_rule_asking_for_a_reject_is_left_alone pins the derivation. That part I have no further comments on.

One blocking item, in the commit added after that review.

The prose scan downgrades on a mention, not on a citation

ea3eab2e1 extends the lookup to the verdict's own prose. Its premise is that failure_reason_code is an input descriptor that nothing on the output side requires back — which is correct, and which also means the prose scan is not a fallback: it is the primary path for most real verdicts.

    text = "\n".join(prose)
    # Sorted so a verdict citing several advisory rules still records one
    # deterministic code; every candidate is advisory, so the choice is only
    # about which one the audit trail names.
    cited = sorted(code for code in advisory if re.search(rf"(?<![\w-]){re.escape(code)}(?![\w-])", text))
    return cited[0] if cited else ""

The scan requires only that the code appear as a whole word somewhere in reasoning or notes. It does not require the code to be the reason for the verdict. So a Critic that rejects for an unrelated, genuine problem and merely names the rule in passing gets its reject downgraded:

Checked specialist_quantitative_claim_violation: clean. Rejecting because the patch rewrites a kernel with no before/after benchmark to stand on.

That has no failure_reason_code (the observed shape), contains the code word-bounded, and so returns advise. By the PR's own tests a downgrade means the proposal is materialised — test_a_rule_named_only_in_prose_still_holds_the_verdict asserts len(coord._materialise_calls) == 1 where the reject case asserts []. A proposal the Critic meant to block executes.

notes widens this further: they are remediation text, the place a model is most likely to write "this is not a <code> problem". The observed shape needs only reasoning.

The negative test aims at the adjacent case rather than this one. test_prose_that_cites_no_rule_leaves_the_reject_alone uses prose mentioning the field name predicted_gain_pct, not the reason code, so it passes without exercising the risk.

Suggested shape. Anchor to the citation form the field run actually produced — from your own commit message, "reasoning": "specialist_quantitative_claim_violation: the proposal payload carries the forbidden predicted_gain_pct field." — i.e. require ^\s*<code>\s*: rather than a free-floating match, and drop notes from _VERDICT_PROSE_KEYS. That still passes test_a_rule_named_only_in_prose_still_holds_the_verdict unchanged, since its fixture puts the code first with a colon, and it removes the whole mentioned-in-passing class. A test whose prose cites the code while rejecting for another reason is the one to add.

The precedence design already in place is the right instinct and worth keeping: an explicit non-advisory failure_reason_code blocks the prose scan entirely (coordinator_helpers.py:572-574, pinned by test_a_declared_reject_code_outranks_an_advisory_one_in_prose), so any Critic that does emit the field is unaffected either way.

---

One non-blocking note, since it touches the same seam. The downgrade is applied in the live verdict path, but the bus event keeps the raw payload, and the report renders straight from it:

        summary = f"verdict={payload.get('verdict')} reason={(payload.get('reasoning') or '')[:60]}"

So a downgraded reject still shows as verdict=reject in the final report's highlights while the proposal in fact ran, and reconstructing what happened means joining that against the separate observation record. That is the same class of mismatch #1178 is cleaning up, so it may be worth landing there rather than here.

[zoroyihan7]

Both items handled at 4fb6b6cf3. The blocking one was right, and the non-blocking one turned out not to be a defect — evidence below, since it is the kind of thing worth being sure about rather than fixing twice.

The prose scan now reads a citation, not a mention

Fixed exactly as suggested, in 4fb6b6cf3. The match is anchored to the citation form the field run produced:

_CITATION_OPENER = r"[\s>*\-–—`\"'(\[]*"
...
if re.search(rf"^{_CITATION_OPENER}{re.escape(code)}`?\s*:", text, re.MULTILINE):

notes is out of _VERDICT_PROSE_KEYS; only reasoning is scanned. The opener tolerates the wrappers a model actually emits around the code — leading whitespace, a bullet, a blockquote marker, backticks — while still requiring the code to open its own clause and be followed by a colon. MULTILINE is there because the citation is often on the second line after a preamble.

Your example is now the test. test_the_rule_the_critic_cleared_is_not_the_grounds_for_its_reject uses your exact prose — clearing specialist_quantitative_claim_violation and rejecting over a missing benchmark — and asserts the reject stands and nothing is materialised. It fails against the previous commit. test_a_rule_named_only_in_prose_still_holds_the_verdict passes unchanged, as you predicted.

The weak negative test you flagged is replaced rather than kept: it asserted on prose naming the field predicted_gain_pct, which never exercised the risk.

Three more that came out of working through it

• bd77bf225 — failure_reason_code never left the Critic runtime. build_review_verdict_intent() dropped it, so on the live path the declared-code branch you pinned with test_a_declared_reject_code_outranks_an_advisory_one_in_prose was unreachable and every verdict fell through to the prose scan. The precedence design was correct; it just had no input. Now threaded into both the intent payload and the session-memory decision record.
• 109589533 — the hold now requires the cited rule to be the whole reject. A reject resting on several grounds, or one still asking for evidence, is no longer downgraded on the strength of one advisory rule among them.
• 56cffd512 — a held reject was becoming a patch-landing permit. The downgraded advise was mirrored into specialist_patch_verdicts, where advise is an approval vector for integrate_patch. So holding a reject to a formatting rule did not just save the round's ideas, it let the patch the Critic refused land on the next tick. What is mirrored is now the authored verdict, never what the hold made of it; a held proposal that deserves to land gets there through a fresh verdict.

The report already renders the effective verdict

I checked report.py:1160 against the bus and it reads the held verdict, not the raw one. _handle_review_verdict applies the hold at intent_router.py:216, before _handle_single_verdict is called; the only review_verdict message on the bus is the rebroadcast that method builds, and it is constructed from the post-hold verdict variable. The Critic's raw intent is never appended. So there is no verdict=reject in the highlights for a proposal that ran, and nothing for #1178 to clean up here.

I also confirmed pending.verdict, which the same line assigns the held value to, has no reader anywhere in orchestrator/ — so the hold cannot leak into a permit by that route either.

[ZhengGong-amd]

Approved