feat(agent-advisor): Model Recommend phase (Anthropic + OpenAI→Bedrock) + seeded runs

[leon1418]

Problem

agent-advisor picked a Bedrock model as a side effect of runtime scoring: scoring.py
emitted a model_recommendation field alongside the compute verdict. That conflated two
independent decisions and left no place for the things model selection actually needs —
the Bedrock API path (Mantle Messages / Mantle OpenAI Responses / Converse / Invoke),
per-model capability evidence, version-hop analysis, or live verification against a target
account. An OpenAI-sourced workload had no path rules at all.

Separately, a non-interactive run (benchmark seed, CI regression) could not be made
reproducible: Clarify had no machine-readable input, so values were re-derived from prose
each run and provenance was not recorded.

Solution

A first-class Model Recommend phase between Clarify and Confirm, owning model + API
path per model-bearing workload. Runtime scoring is decoupled — the legacy
model_recommendation field is removed from scoring.py and the scoring-result schema
(runtime scoring owns compute only).

• Anthropic engine — dated path-aware catalog, joint (model, api_path) filtering and
  ranking, CRIS resolution without guessing geography, Claude version-hop analysis,
  structured compatibility/architecture findings.
• OpenAI→Bedrock engine (new) — source-family detection (reasoning / legacy / unknown;
  opaque deployments stay unknown); path rules (Responses + continuity →
  mantle_openai_responses; Chat Completions → Responses reshape; runtime governance →
  runtime_converse; continuity + runtime-only → decision_required); capability-aware
  fail-closed selection; separate-modality targets (embeddings/images/audio) emitted as
  explicitly unresolved contracts rather than invented model ids.
• verify_model_path.py — optional live probe of the selected (model, path) in the
  target account. With no account, the run stays provisional: POC never emits a runnable
  model id without passed verification.
• Seeded runs — scripts/schemas/seed.json supplies the dimensions Clarify would
  otherwise ask a human for, so the deterministic engine gets byte-identical input every
  run. Clarify Step 2.5 resolves each dimension by an explicit precedence ladder and
  records provenance (seed|detected|asked|inherited|adapter|interview|assumed); a seeded
  value is copied byte-equal (never reshaped), a malformed seed is a hard error rather than
  a silent fall-through, and every assumed dimension must appear in UNANSWERED.md.

Downstream Confirm / Design / Generate / POC carry the selected contract verbatim;
Migration Plan validates the advisor's (model, api_path) contract instead of reselecting.

Both dated catalogs state their evidence and keep numeric limits "unknown" unless
sourced — the engines only rank where there is evidence, and otherwise return an explicit
decision or a provisional limitation rather than inventing a comparative order.

Note: this touches skills/agent-advisor/SKILL.md (registering the new phase in the
backbone + Files table), which CODEOWNERS routes to @awslabs/startups-admins.

Type of Change

• Enhancement to existing content

Team Folder

• migrate/

Checklist

• I have read the CONTRIBUTING.md guidelines
• My changes do not include hardcoded secrets, credentials, or internal-only content
• I have run mise run build locally and it passes
• I have updated documentation if needed
• My changes are scoped to my team's folder only

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

[leon1418]

[🤖 AI review 🤖]

Reviewed all 34 changed files: the new Model Recommend phase (Anthropic + OpenAI→Bedrock engines, orchestrator, verifier, seeded runs), both dated catalogs, all downstream phase wiring, and all 4 new test suites (197 tests passing). CI clean: build, gitleaks, bandit, semgrep, checkov all GREEN.

Scope reviewed: Design, functionality, security, tests, consistency across the markdown specification and the deterministic Python engines.

Summary: This is a large, well-structured decomposition that factually improves code health by separating model selection from runtime scoring, adding provenance tracking, and making the engine deterministic and testable. The dual-catalog approach with explicit evidence citations and honest unknown limits is a sound design. The OpenAI→Bedrock path rules (Responses-only for GPT-5.x, tier-mapped Converse fallback) are correct per the stated evidence.

Two inline findings below — one nit and one FYI. No blocking issues found.

Merge recommendation: MERGEABLE. Note: open PR #167 (feat(migrate): recommend Claude Sonnet 5 / Opus 4.8 as Anthropic defaults) touches scoring.py's model_recommendation field and model-selection.md — both modified here. A merge conflict is expected; this PR supersedes #167's model selection approach.

[leon1418]

[🤖 AI review 🤖]

Nit: PROVIDER_MODULES maps "none" and "unknown" to "anthropic", which means a workload with no detected provider silently gets Anthropic catalog treatment. The behavior is arguably correct (Anthropic is the default pool), but the intent would be clearer with an inline comment explaining that none/unknown sources fall through to the Anthropic engine as the "generic Bedrock-native" path — especially since recommend_anthropic_workload already emits a provider_module_pending block for truly non-Anthropic sources.

[leon1418]

Investigated — the behaviour you describe is real, but the dict wasn't what produced it, and there was a bigger problem right next to it. Fixed in 4f085d3.

The routing was never a four-way table. The lookup is PROVIDER_MODULES.get(provider, "generic") followed by if module == "openai" / else, so everything non-OpenAI reached the Anthropic module regardless of the dict. That made the none and unknown rows dead weight, and it meant azure_openai / google_genai / bedrock — absent from the dict entirely — took the same path without the dict saying so:

provider	in the dict	actually routed to
anthropic	anthropic	anthropic engine
openai	openai	openai engine
none, unknown	anthropic	anthropic engine
azure_openai, google_genai, bedrock	absent	anthropic engine

So a comment explaining the two rows would have documented a decision that wasn't being made there.

The real issue: the actual classification lived twice — as those rows, and again inline in anthropic_model_recommendation.py as provider in {"anthropic", "none", "unknown"}. Two copies of one rule with nothing holding them together.

The fix names it once as anthropic_model_recommendation.ANTHROPIC_POOL, where the module that owns the decision can be read next to the provider_module_pending finding it drives, and replaces the dict with an explicit two-way _provider_module() carrying the reasoning. Behaviour is unchanged — every provider routes exactly where it did before.

test_every_schema_provider_routes_and_is_labelled_honestly pins it. Worth noting how the first draft of that test was wrong: it derived its expectation from ANTHROPIC_POOL, so it passed while I gutted that constant — a test that reads its expectation out of the thing it's checking cannot catch that thing moving. The intended treatment of all seven schema providers is now written in the test instead. Verified it fails on each of the three ways this can drift: editing ANTHROPIC_POOL, editing the orchestrator's router, and adding a provider to the schema without deciding its treatment.

[leon1418]

[🤖 AI review 🤖]

FYI: _default_openai_responses_client imports aws_bedrock_token_generator.provide_token which is a relatively uncommon dependency (not in standard boto3/openai installs). The main() CLI path validates schemas via jsonschema but doesn't check for this optional dep upfront — if a user runs the verifier on an OpenAI workload without the package, they get a RuntimeError mid-execution after probing other workloads successfully. Not blocking since the function is only reached for mantle_openai_responses paths, but a pre-flight check in main() or a note in the phase doc about uv run --with aws-bedrock-token-generator could save confusion.

[leon1418]

Checked this one and it doesn't reproduce — the failure is already handled, and the suggested fix would be a regression. Documented the real gap instead, in 4f085d3.

verify_workload wraps the client factory in try / except Exception (the factory(result["region"]) call is inside the try), records the workload as status: "failed" with a typed error, and the loop in verify_recommendation continues to the next one. Ran it on an interpreter without the package to be sure:

aws_bedrock_token_generator absent: good, this is the reviewed scenario

  a: status=failed  error=RuntimeError: Mantle Responses verification requires the openai package
  b: status=failed  error=RuntimeError: no creds in this test

workloads recorded: 2 -> the run did NOT abort; both results are present

main() still writes model-verification.json and returns exit 2. Nothing is lost and nothing aborts mid-execution — a missing dependency is a recorded per-workload outcome, which is the right design for a probe that hits several independent paths.

A pre-flight check in main() would make it worse: it would abort the whole run over a dependency only one workload needs, discarding the other workloads' probe results. Failing at the point of use and recording it is what you want here.

The grain of truth is in the documented command, which names boto3 and anthropic only — so the copy-pasteable line in Step 7 does not work for a mantle_openai_responses workload. That's now stated: which paths need --with openai --with aws-bedrock-token-generator, plus a warning that a failed status here can mean the dependency was absent rather than the model being unavailable, so read the error type before treating it as a capability finding.

[leon1418]

[🤖 AI review 🤖]

Re-review of commit 4f085d3 (follow-up to prior review at 520ca2c).

Changed scope: One commit (4f085d3a) addressing both prior findings — single-sources the provider classification and documents the verifier's per-path deps.

Verification of prior findings:

1. Provider fallback (Nit) — Fully resolved. The PROVIDER_MODULES dict is replaced by a two-way _provider_module() function with a thorough comment explaining the design intent. The actual classification now lives once as anthropic_model_recommendation.ANTHROPIC_POOL, and test_every_schema_provider_routes_and_is_labelled_honestly pins all seven schema providers against an independently-written expectation table — verified it catches drift in ANTHROPIC_POOL, the orchestrator router, and schema additions.

2. aws-bedrock-token-generator optional dep (FYI) — Addressed via documentation, not a pre-flight check (correctly so). The author demonstrated that verify_workload's try/except Exception already records a per-workload failed status with the typed error without aborting the entire run. A pre-flight would be a regression (aborts probing of unrelated workloads). The model-recommend.md Step 7 now documents which paths need --with openai --with aws-bedrock-token-generator and warns that failed can mean missing-dep rather than model-unavailable.

Test results (302 tests, all pass): test_model_recommendation 24✅, test_openai_model_recommendation 50✅, test_scoring 45✅, test_unit_grouping 69✅, test_verify_model_path 10✅, and 4 other suites 104✅.

Mutation coverage verified: ANTHROPIC_POOL reduction → test catches it. Router misroute → test catches it. Missing-dep path → graceful per-workload failure confirmed empirically.

CI: All 8 checks GREEN (build, gitleaks, bandit, semgrep, checkov).

PR #167 interaction:

• 7 files overlap: scoring.py, test_scoring.py, test_unit_grouping.py, model-selection.md, generate.md, migration-plan.md, poc.md.
• #167 bumps model names in _MODEL_PRIORITY/_FEATURE_OVERRIDE/_MIGRATE_FAMILY (Sonnet 4.6→5, Opus 4.7→4.8). #195 deletes that entire section (~90 lines), replacing it with the new model_recommendation.py orchestrator. The changes are non-additive — #195 supersedes #167's scoring.py work entirely.
• The markdown overlaps (model-selection.md, generate.md, etc.) are similarly superseded: #195 replaces the content #167 modifies.
• Recommended merge order: #195 first. Merging #167 first would introduce dead code that #195 immediately removes, causing a needless conflict. Merging #195 first means #167's scoring.py and model-selection.md hunks simply have no target (already deleted) — trivial to resolve by dropping those hunks. #167's other hunks (fixture HTML, lifecycle docs with model name bumps) may still be independently valuable.
• #167 is NOT fully superseded as a PR — it carries non-overlapping value in llm-to-bedrock (pricing.py, test_bedrock_pricing.py, test_validate_result.py), design refs (ai-anthropic/gemini/openai-to-bedrock.md), and shared references. After #195 merges, #167 should rebase, drop the conflicting scoring/model-selection hunks, and land the remaining independently-valuable changes.

Merge recommendation: Clean. No actionable findings. Both prior comments substantively addressed with code + documentation + mutation-guarded test coverage. Recommend merge (after rebase to resolve BEHIND state).

[leon1418]

Two more commits, both found by benchmarking the standalone bundle rather than by reading the code.

9b8db5b — the two AI-review findings, answered inline above. One was real but misdiagnosed (the provider classification lived in two places, and the dict rows the review pointed at were dead code); the other did not reproduce (a missing optional dependency is already recorded as a per-workload failed, and the suggested pre-flight check would have aborted unrelated probes).

a17b2ee — two rules the Generate phase states but never checked. A benchmark run scored 9/10 on two scorers and both deductions had the same shape:

• The template says "INSERT the Mermaid block + ASCII fallback" into Section 4; the run wrote "See diagram.md". An earlier run over the same repository pasted it. Nothing was broken — the rule was simply unenforced, so which behaviour you got was luck.
• recommendation.md reported "scored 40/56". scoring-result.json publishes per-runtime scores and no maximum; 56 was the agent's own arithmetic. Presenting an engine number as a fraction claims a precision the engine does not.

Both now have postconditions, and the template and Step 2 say what "embed" means.

Worth naming the pattern, because it is now three for three in this branch: a rule written down but never checked holds only by luck. The seed-verbatim postcondition named only $RUN_DIR/seed.json while the fixture's seed sits at the run root, so it read as inapplicable and a run reshaped a seeded object. The benchmark's own seed-fidelity check was sys.exit(bad and print(...) or 0) — always 0 — and passed a run that provably violated it. And these two. Each was caught by something outside the rule itself: a repeated run, a deliberately-bad input, an independent scorer.

302 tests passing, mise run build clean, no conflict with current main.

[ayn-builds]

Read through the whole thing, mostly by running the engines against the real catalogs rather than reading them. The design holds up well - provider-module split, dated catalogs with honest "unknown" limits, fail-closed CRIS resolution, fail-closed POC gate. Both of your self-review dismissals check out: the dead PROVIDER_MODULES dict, and the pre-flight dependency check that would have been a regression, since the client factory being inside the try means a missing dep is a recorded per-workload failed rather than a crash. That's the better behaviour. test_every_schema_provider_routes_and_is_labelled_honestly is load-bearing - I mutated it two ways and it caught both.

Four things I'd want addressed before merge, left inline. Short version: the CRIS resolver is duplicated and the OpenAI copy has no test coverage at all; preferred_api_path and requires_native_payload are silently dropped on the OpenAI path; Azure/google_genai sources lose most of their feature scan and get a native claim the catalog doesn't back; and SKILL.md never declares model-recommend even though intake.md writes it into .phase-status.json. Five mediums also inline.

One that has no line to attach to, since the file isn't in the diff: references/handoff/handoff-migration.md:14 still says "the source model from the user's current_model answer". This PR removes current_model from scoring.py DEFAULTS, so that handoff reference is now dangling.

[ayn-builds]

Blocking. This function is byte-identical to anthropic_model_recommendation.py:345. Of the 9 functions defined in both modules it's the only byte-identical one, which makes it copy-paste rather than parallel evolution.

The copy here has no test coverage. I replaced the whole body with return model_id - deleting the fail-closed path entirely - and the suite still reported 302 passed. grep -cE "cris|CRIS|geo_required|global_allowed" test_openai_model_recommendation.py returns 0.

The behavioural difference isn't subtle: with residency unresolved, invocation_model_id goes from None to anthropic.claude-sonnet-5, which flips a needs_resolution into a live probe against an ID that can't be invoked in the account. This is the function that stops verify_model_path.py from probing an unresolvable model, so it's the highest-consequence code in the PR and it's unguarded on one of the two paths.

Either extract it to a shared module both import, or add the CRIS matrix to the OpenAI tests. I'd prefer extract - one implementation can't drift.

[ayn-builds]

Blocking. Path selection here is a bare two-way branch on runtime_required, so preferred_api_path and requires_native_payload never enter the decision. Both are schema-legal input and the Anthropic module honours them.

Probed it: preferred_api_path: "runtime_converse" on an OpenAI source returns mantle_openai_responses with decision_status: recommended and no finding of any kind. Same for requires_native_payload: True. grep -c "preferred_api_path|requires_native_payload" openai_model_recommendation.py finds neither field read anywhere in the module.

What makes this worse than a gap is that references/phases/model-recommend/model-recommend.md:101 instructs the agent to record preferred_api_path when an explicit preference exists, with no provider caveat. So the phase spec tells the agent to capture a user's stated preference that this engine then drops silently. Either honour it, or emit a finding saying it was ignored and why.

[ayn-builds]

Medium. priority is never read in this module - all four values return openai.gpt-5.6-sol on the Mantle path. It works correctly on Converse via _converse_tier_for_source. Fine if Mantle genuinely has one Responses-capable model today, but then the rationale should say the priority was noted and had no effect, otherwise a user who asked for cost-optimised gets the same answer as one who asked for max capability with nothing telling them why.

[ayn-builds]

Blocking. native is derived from a hardcoded name set rather than from the selected model's catalog capabilities, so it can assert support the catalog doesn't back.

Concrete case: an azure_openai source with streaming detected gets compatibility.native: ["streaming"], but claude_sonnet_5's catalog capabilities are ['tool_use', 'extended_thinking', 'vision', 'long_context']. streaming isn't among them. The output is making a claim about the target model from a list that has no connection to the target model.

Suggest intersecting against catalog["models"][model_key]["capabilities"] so the claim is always catalog-derived, keeping the prompt_caching/mantle_messages special case below as-is.

[ayn-builds]

Blocking. The reasoning here is sound for provider classification but doesn't cover feature surface, and the gap is measurable.

I fed an azure_openai source the 13 OpenAI feature codes that model-recommend.md Step 3 itself tells you to scan for. Findings came back for 3. The other 10 - tool_or_function_calling, structured_output_json, web_search, vector_stores, assistants_threads, embeddings, images, audio_modality, reasoning, sampling_params - produced no finding at all, not even informational. The provider_module_pending [BLOCKS] finding does fire, so nothing ships silently, and I'd agree that's the important guarantee.

But the routing isn't decision-neutral either: Azure + api_continuity: required + governance returns recommended here, where the OpenAI engine on byte-identical input returns decision_required. So the fall-through isn't just "provisional pending a module" - it produces a materially different decision than the module that actually understands the source shape.

Given azure_openai is an OpenAI-API-shaped source, routing it to the OpenAI module with the pending-block attached would preserve both the block and the feature scan. If that's out of scope, worth saying so in this comment, since as written it reads as "the fall-through is safe" and the feature-scan loss isn't mentioned.

[ayn-builds]

Medium. critical_features.items is {"type": "string"} while detected_features.items is {"$ref": "#/$defs/featureName"}. So critical_features: ["totally-made-up"] validates and is then silently dropped, but the identical string in detected_features is correctly rejected. Since critical features are the ones that gate a decision, that's the more dangerous of the two to leave open.

Separately, agentic, long_context, extended_thinking, multimodal, rag, embedding, image_generation, and speech are all absent from the featureName enum, though model-selection.md and the phase spec all use them. Either add them or $ref the enum from critical_features and see what breaks.

[ayn-builds]

Medium. No seed or non-interactive story in this phase: zero mentions of seed, non-interactive, or AskUserQuestion, despite instructing up to 8 batched clarification questions. Clarify's new Step 2.5 establishes the full precedence ladder (seed > detected > prose > asked > adapter > inherited > assumed, with assumed owing an UNANSWERED.md entry), so a seeded run reaching this phase has no defined behaviour - it either re-asks what the seed already answered or stalls.

Related: scripts/schemas/seed.json defines probe and poc_mode, and neither has a consumer anywhere in references/ (poc_mode has zero occurrences; probe only appears as catalog prose). Worth either wiring them or dropping them from the schema.

[ayn-builds]

Medium. This claim doesn't hold on the Anthropic path. I probed embedding, image_generation, speech, and rag individually - each returned decision_status: recommended with additional_targets absent and zero blocking findings. grep -c additional_targets anthropic_model_recommendation.py returns 0, the engine never emits the key, and it isn't in the output schema's required list so validation doesn't catch the omission.

The spellings also don't match the OpenAI engine, which keys _SEPARATE_MODALITY_TARGETS on embeddings / images / audio_modality. And line 8 still describes the tables as "a coarse compatibility hint for old scoring-result consumers" although no family table survives this diff.

[ayn-builds]

Medium. An Anthropic workload carrying a schema-legal OpenAI preferred_api_path (e.g. mantle_openai_responses) reaches this raise instead of the unsupported preferred_api_path message at line 213. The input is plausible - a mixed-provider run with a copy-paste slip - and the error points at the catalog rather than at the field that's actually wrong.

[ayn-builds]

Blocking. grep -n "model-recommend|Model Recommend" SKILL.md returns nothing, so the file that declares the skill's phases never declares this one. Four specific spots:

• line 45-47, backbone list still intake -> discover -> clarify -> confirm -> design -> ...
• line 118-123, phase gate checks only clarify + confirm
• line 127-145, .phase-status.json example omits it
• line 161-184, Files table omits every new file in this PR

The asymmetry is what makes it load-bearing rather than cosmetic: intake.md was updated and now writes the name into state - "Set all later phases (clarify, model-recommend, confirm, design, estimate, generate) to pending" - while INTERPRETER.md validation rule 3 halts on "Unrecognized phase name in phases (not a phase the skill declares)". One half of that contract moved.

(Leaving this at file level because the four lines are outside the diff hunks, so GitHub won't take line comments on them.)