Skip to content

fix(aws-startup-advisor): conflict options apply the Sonnet 5.5 yield and exact pin matching in agent-advisor - #620

Merged
herosjourney merged 2 commits into
mainfrom
fix/sonnet-yield-decision-options
Nov 12, 2026
Merged

herosjourney merged 2 commits into
mainfrom
fix/sonnet-yield-decision-options

Conversation

@kapurshr

@kapurshr kapurshr commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Follow-up to #607 (stage 2 of #566). Gen's last review on #607 found one P2 that was not fixed before #607 merged.

When Messages continuity (preserve_messages_api: true) conflicts with runtime-only governance such as Guardrails, the Anthropic module returns decision_required with one option per API path. _decision_options() offered each path's top-ranked candidate. It skipped the Sonnet 5.5 capability yield and the inference_profile_id pin, both of which the main path applies. So with thinking_enabled: false or detected Message Batches, the runtime Converse option named Sonnet 5.5, but choosing it and rerunning selected Sonnet 5. The model shown was not the model the choice produces.

Fixing that exposed a second, older bug in _honor_explicit_profile. Pin matching used substring checks, so the Sonnet 5.5 profile global.anthropic.claude-sonnet-5-5 also matched Sonnet 5's id. On a path without inference profiles (Mantle, where Sonnet 5.5 is not offered), a Sonnet 5.5 pin pulled Sonnet 5 ahead of the ranked choice. For example, at quality priority it replaced Opus 5.5.

Fix

  • _decision_options() now picks each option the same way the main path does: _yield_to_capable_alternative, then _honor_explicit_profile, on that path's candidates. Each option now names the model a rerun on that path selects.
  • A new helper, _pin_names_model(), replaces the substring checks. The pin's last path segment must equal the model id or end in .<model_id>. _explicit_profile_matches() already used that rule for the later prefix and geography checks.

Scope

  • skills/agent-advisor/scripts/anthropic_model_recommendation.py and its tests in test_model_refresh_a2.py. No catalog, registry, or OpenAI module changes.
  • Golden comparison against main (56,160 cases): no changes.
  • Targeted matrix (25,920 Anthropic cases varying thinking, Batch detection in both encodings, pins, path and priority):
    • 5,472 cases change only decision_options.
    • 1,008 cases change the primary selection, all through pin matching:
      • 360 cases, no path constraint: a Sonnet 5.5 runtime profile pin now selects runtime_converse with Sonnet 5.5 and the pinned profile. main dropped the pin and returned Mantle with Sonnet 5.
      • 648 cases, Mantle preferred: a Sonnet 5.5 pin no longer selects Sonnet 5, and the ranked choice wins: Haiku 4.5 at speed priority, Opus 5.5 at quality.
    • No errors or other fields change. An earlier version of this description said only decision_options change; that matrix had been run before the pin fix was added.
  • Option vs rerun check (2,880 conflict options): for each option, resolve the conflict toward that option's path and rerun. On main, 1,356 options name a different model than the rerun. With this change, 0 do.
  • Visible shift, unconstrained path: with no path constraint, a runtime profile pin now selects the runtime path (runtime_converse) and the pinned model, where main returned Mantle. A team that pinned a runtime profile and was getting a Mantle recommendation now gets a different integration path, which matches the pin.
  • Visible shift, Mantle: a versioned runtime Haiku 4.5 pin (us.anthropic.claude-haiku-4-5-20251001-v1:0) no longer selects Haiku 4.5 on Mantle, whose model id is unversioned (anthropic.claude-haiku-4-5). That is the same prefix match this change removes, and a runtime profile pin has no effect on Mantle. The pin still selects Haiku 4.5 on the runtime path, and a test covers it.

Tests

  • Disabled thinking, Batch in the detected_features array, and Batch in feature_status: the runtime option and the rerun both name Sonnet 5.
  • No unmet need, an explicit Sonnet 5.5 pin, and the quality tier: the option is unchanged and still matches the rerun.
  • A Mantle option honors the pin its rerun honors.
  • A Sonnet 5.5 pin does not select Sonnet 5 on Mantle. Profile, ARN, and bare-id pins for Sonnet 5 still select it.
  • Versioned Haiku 4.5 profile and ARN pins still select Haiku 4.5 on the runtime path.
  • With no path constraint, Sonnet 5.5 and versioned Haiku 4.5 runtime pins select runtime_converse and the pinned model. This is Logan's test; it fails on main.
  • Reverting each fix fails its tests: the three yield cases for options, and the prefix case for pin matching.

Type of Change

Select at least one.

  • New skill
  • Bug fix
  • Documentation update
  • CI/CD change
  • Other

Startup Advisor review checklist

By submitting this PR, I confirm that:

  • I have reviewed the full diff and understand all changes I am submitting.
  • I have tested my changes and confirmed they work as intended.
  • I have run mise -C plugins/aws-startup-advisor run ci and all checks pass.
  • I have disclosed any known issues, limitations, or untested scenarios.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

… exact pin matching

Follow-up to #607. _decision_options now applies _yield_to_capable_alternative and
_honor_explicit_profile like the main path, so each option names the model a rerun on
that path selects. Pin matching uses exact id segments, so a Sonnet 5.5 pin no longer
names Sonnet 5 by prefix.
@kapurshr
kapurshr requested review from a team as code owners November 12, 2026 00:07
@kapurshr

Copy link
Copy Markdown
Contributor Author

Pre-PR review

An independent read-only review (a Sonnet subagent) checked this diff before the PR was opened. Its findings and my responses:

  1. A versioned runtime Haiku 4.5 pin no longer selects Haiku 4.5 on Mantle (low). Agreed; this is intended. The Mantle id is unversioned (anthropic.claude-haiku-4-5), and the old match succeeded only through the prefix check this PR removes. A runtime profile pin has no effect on Mantle. The PR description now discloses it, and a new test covers the same pin on the runtime path.
  2. The option vs rerun tests cover only runtime_converse (low). Agreed. Added test_mantle_option_honors_a_pin_the_rerun_honors.
  3. No test for versioned Haiku pins (low). Agreed. Added profile and ARN cases for the runtime path.

The review also checked these and found them unchanged: application-inference-profile ARNs (they never name a model and fall back to the ranked choice), foundation-model/ ARNs (rejected by _resolve_invocation_model_id), and the prefix and geography validation in _explicit_profile_matches.

@github-actions

github-actions Bot commented Nov 12, 2026 •

Copy link
Copy Markdown

Problem, Fix, and Scope are filled in. A change type is selected and all four Startup Advisor author confirmations are complete.

The author checklist check now passes. CI and reviewers still need to verify the change.

@herosjourney herosjourney left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings: 0 Blocking, 1 Should-fix, 0 Nits. Heads-up: 1.
Reviewed: head 9ff076e (base 09af68c, merges cleanly into main), 2 files (+101/−4) — agent-advisor (anthropic_model_recommendation.py, test_model_refresh_a2.py), one plugin tree.
Ran: mise -C plugins/aws-startup-advisor run ci → exit 0 (test 1717 passed, test:telemetry 311 passed, the agent-advisor task collects the changed test file); test_model_refresh_a2.py directly → 50 passed; four mutations of the head, each caught by a new test (revert the _decision_options change → 4 fail; revert only the pin matcher → 1 test fails; options without the pin → 2 fail; options without the yield → 3 fail); my own option-vs-rerun sweep, 7,168 options across 4 regions, 4 residency settings, 4 priorities, thinking on/off, Batch on/off, 7 pin shapes, Converse and Invoke paths → 0 mismatches at head, 3,310 on main; old-vs-new pin matcher over every catalog model id and 8 prefixes × 3 ARN/bare forms → only the two disclosed pin shapes change; full recommend() output diff main vs head over 32,256 inputs (details under the Should-fix).
Did not run: the author's 56,160-case golden comparison (I ran my own matrix above instead); no live AWS calls. The Startup Advisor author checklist check fails with "PR checklist incomplete". Its summary does not name the item that trips it, so I have not diagnosed it. It reruns when the description is edited.

Change narrative: conflict options offered each path's top-ranked candidate, skipping the Sonnet 5.5 capability yield and the inference_profile_id pin the main path applies → _decision_options now runs _yield_to_capable_alternative then _honor_explicit_profile, in the order recommend_anthropic_workload does at line 983–984; _pin_names_model replaces three substring checks, so global.anthropic.claude-sonnet-5-5 no longer names anthropic.claude-sonnet-5 → one engine file plus its tests. Body vs diff: matches on the code and the tests; the "Visible shift" paragraph describes one of the two ways selections move (see the Should-fix).

Axis Stance One line
Parsimony clear 14 changed lines of engine code and one helper; the 87 test lines are proportionate to two bugs
Completeness finding The most common input shape that changes (a runtime pin, no path constraint) has no test and no mention in the body
Security clear scanners: CodeQL and dependency-review pass; contracts touched: none; the helper only parses a string
Alternatives clear none worth raising
Contradictions clear model-recommend.md Step 6 and its pin-validation paragraph describe the behavior at head
Overlap advisory #588 edits _decision_options and the same file; see Heads-up
Cross-plugin parity n/a one plugin tree
Propagation & data clear SSOT: the model catalog; sites swept: _pin_names_model has 1 caller (_honor_explicit_profile); stale forms found: 0

Should-fix

  1. [Completeness] A runtime pin on the unconstrained default path now flips api_path and the model, and no test or body text covers it — plugins/aws-startup-advisor/skills/agent-advisor/scripts/anthropic_model_recommendation.py:503

    if not _pin_names_model(explicit, path_config["model_id"]):
    Consequence: for {priority: balanced, data_residency: global_allowed, inference_profile_id: "global.anthropic.claude-sonnet-5-5"} with no governance and no preferred_api_path, main returns mantle_messages / anthropic.claude-sonnet-5 (the pin dropped, a different model), and head returns runtime_converse / anthropic.claude-sonnet-5-5 / global.anthropic.claude-sonnet-5-5. The versioned Haiku 4.5 runtime pin moves the same way, mantle_messages → runtime_converse. Both are the correct results. In my 32,256-input diff, 240 inputs flip api_path this way (all with no governance, no native payload, no preferred path), 704 more change primary_model on Mantle, and 11,775 change only decision_options. The body says only decision_options change in the targeted matrix and describes the shift as "no longer selects Haiku 4.5 on Mantle". A team that pinned a runtime profile and had been getting a Mantle recommendation will now get a different integration path. The new tests pin the Mantle and the option outcomes. None pins this one (every _mantle_pinned test sets preferred_api_path).
    Fix: add a sentence to the body's "Visible shift" paragraph: "With no path constraint, a runtime profile pin now selects the runtime path (runtime_converse) and the pinned model, where main returned Mantle." Add the test below. Regression: it fails on main and passes at head (I ran both).

    @pytest.mark.parametrize(("pin", "model"), [
        ("global.anthropic.claude-sonnet-5-5", "anthropic.claude-sonnet-5-5"),
        ("us.anthropic.claude-haiku-4-5-20251001-v1:0", "anthropic.claude-haiku-4-5-20251001-v1:0"),
    ])
    def test_a_runtime_pin_selects_the_runtime_path_when_no_path_is_constrained(pin, model):
        rec = _rec("anthropic", "claude-3-7-sonnet-latest", "messages", priority="balanced",
                   inference_profile_id=pin)
        assert (rec["api_path"], rec["primary_model"], rec["invocation_model_id"]) == ("runtime_converse", model, pin)
    CI check: none.

Heads-up (out of diff, pre-existing — not blocking)

  • #588 (approved, +4,051/−426, 10 commits behind main) already conflicts with main in this file and model-selection.md, and its _decision_options still ends in option = _candidate_summary(candidates[0], requirements, tradeoff, region) at its line 600, with a new runtime_messages option path and a mantle_selector argument. Whichever PR lands second has to carry this PR's yield-then-pin selection into that function, including the runtime_messages option. I can file a note on #588 to say so.

Verdict

Comment — nothing blocks the merge. The engine change is correct against every probe I ran. The one Should-fix is a disclosure and one regression test. The reviewing account is not the author's, and a Should-fix is open, so I do not approve.

@leon1418 leon1418 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[🤖 AI review 🤖] Reviewed both changed files at 9ff076ea196f41ff33b80214ae42b800c7e8623d against base 09af68caa86c8913e60c4a66290b52c82e901d42, with OCR-assisted repository review, a fresh independent reviewer, and producer/consumer contract verification. No substantive findings remain; approval is recommended for the user to submit.

Local validation at this unchanged head: 653 agent-advisor tests and 415 llm-to-bedrock tests passed in the earlier attempt; this retry independently passed 286 focused tests, 1,440 parent and 2,440 independent option/rerun comparisons, and verification-retention/invalidation checks. Exact runtime pins and the unconstrained Sonnet 5.5 runtime-path selection were checked. No live AWS calls were made.

The existing nonblocking request for an unconstrained runtime-pin regression test and description clarification remains open for its original reviewer/author to address. This review does not resolve that conversation or establish merge readiness.

…no path is constrained

From Logan's review on #620: with no path constraint, a runtime profile pin now
selects runtime_converse and the pinned model, where main returned Mantle.

@herosjourney herosjourney left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings: 0 Blocking, 0 Should-fix open, 0 Nits. Heads-up: 1.
Re-review at 6321520 (previous: 9ff076e). Delta: 1 commit, 1 file (+12/−0, test_model_refresh_a2.py).

Prior finding Status at head
[Should-fix] Unconstrained runtime pin changes the path, untested and undisclosed (anthropic_model_recommendation.py:503) Fixed at 6321520 — re-probed: the new parametrized test passes at head (2 passed) and fails on upstream/main (2 failed) with the test file copied across. The PR body's "Visible shift, unconstrained path" paragraph now states the change, and the matrix summary is corrected to 1,008 primary changes (360 unconstrained, 648 Mantle).

New in delta: none. The delta touches only the test file, and the new test asserts the behavior the Should-fix named. Sweep for stale forms: not applicable, no value changed.

Ran: at 6321520, uv run python -m pytest -q test_model_refresh_a2.py → 52 passed (50 prior + 2 new). The new test against upstream/main → 2 failed. The PR body re-read for the disclosure and the corrected numbers.

Did not run: the 25,920-case matrix behind the corrected 1,008 figure. I re-read the description's numbers but did not re-derive them. The full mise -C plugins/aws-startup-advisor run ci was not re-run for this push; the prior ci exit 0 was at 9ff076e. No live AWS calls.

Heads-up (unchanged, not blocking): #588 still conflicts with this PR in the same engine file and has the older _decision_options. Whichever PR lands second has to carry the yield-then-pin selection into that function.

Verdict: Comment — the Should-fix is closed at 6321520, and nothing else is open on this PR. This supersedes my 2026-11-12 review (pullrequestreview-5867426850), which raised the Should-fix. The reviewing account is not the author's, so no identity line applies.

@herosjourney herosjourney left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving at 6321520. The Should-fix from 9ff076e is closed (re-review: pullrequestreview-5869207860), and no review thread is unresolved.

Ran: test_model_refresh_a2.py at 6321520 → 52 passed. The new unconstrained-pin test → fails on upstream/main, passes at head. CI at head: build, startup-advisor, checklist-test, CodeQL, dependency-review, pr-title and the author checklist all SUCCESS.

Not re-run by me at this head: the full mise -C plugins/aws-startup-advisor run ci (the build and startup-advisor checks on GitHub passed for 6321520), and the 25,920-case matrix behind the corrected description numbers.

Heads-up, not blocking: #588 conflicts with this change in anthropic_model_recommendation.py and carries the older _decision_options. Whichever PR lands second has to apply the yield-then-pin selection to that function.

@herosjourney
herosjourney added this pull request to the merge queue Nov 12, 2026
Merged via the queue into main with commit 7a652e5 Nov 12, 2026
21 of 23 checks passed
@herosjourney
herosjourney deleted the fix/sonnet-yield-decision-options branch November 12, 2026 04:42
herosjourney pushed a commit to herosjourney/agent-toolkit-for-aws that referenced this pull request Nov 12, 2026
Keep aws#588's _decision_options (runtime paths, mantle_selector) and apply aws#620's
yield-then-pin selection to each option's candidates. Adapt aws#620's Mantle/default-path
tests to the bedrock-runtime default (Mantle via mantle_selector; Messages source
lands on runtime_messages) and add a runtime_messages option test.
Verified: mise run ci in plugins/aws-startup-advisor passed (1857 tests).
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.

3 participants