Skip to content

refactor: dedupe inner-tool model routing into a single helper#10

Merged
adamlu123 merged 1 commit into
microsoft:mainfrom
adamlu123:refactor/inner-tool-routing-cleanup
May 27, 2026
Merged

refactor: dedupe inner-tool model routing into a single helper#10
adamlu123 merged 1 commit into
microsoft:mainfrom
adamlu123:refactor/inner-tool-routing-cleanup

Conversation

@adamlu123
Copy link
Copy Markdown
Contributor

@adamlu123 adamlu123 commented May 27, 2026

Follow-up to #5. Same behavior (Anthropic-only runs work), but smaller surface.

What changed

  • New tools/_model_config.py consolidates the model-config helpers that
    PR fix: route image_qa and self_reflection through the configured model (closes #3) #5 added verbatim to both image_qa.py and self_reflection.py.
    Exposes load_tool_model, resolve_model_config_path, and
    _extract_model_block.
  • Path resolution is now two-step: explicit --model-config arg, then
    <workspace_dir>/config_snapshot/merged_config.yaml (already written
    by cli.py). Dropped the WEBWRIGHT_TOOL_MODEL_CONFIG env var and the
    tool_model_config.json fallback — neither was produced by anything
    in the repo.
  • Removed the legacy OpenAI direct-HTTP code paths from both tools
    (_call_openai, _openai_config, _post_with_retry, _sleep_backoff,
    _RETRYABLE_STATUS_CODES) and the CLI flags they relied on
    (--api-key, --endpoint, --model, --max-attempts,
    --retry-base-delay). The model client owns HTTP retries via
    models/base.py:_post_with_retries.
  • model_claude.yaml no longer declares the &model YAML anchor or the
    tools.image_qa.model / tools.self_reflection.model block — the
    tools read the top-level model: block directly.

Tests

tests/unit/test_tool_model_routing.py now covers:

  • model_claude.yaml sets a top-level Anthropic model
  • model_claude.yaml has no tools: block
  • _extract_model_block reads the top-level model / raises if missing
  • resolve_model_config_path honors an explicit arg, falls back to the
    snapshot, and raises FileNotFoundError when neither exists

All 7 pass.

Behavior note

The --api-key/--endpoint/--model direct CLI invocation paths are
gone. Callers must either pass --model-config <path> or run via the
agent (which writes config_snapshot/merged_config.yaml).

Net: -583 / +106 lines vs. the state after #5 merged.

Following up on microsoft#5: consolidate the model-config resolution helpers that
were duplicated verbatim between image_qa.py and self_reflection.py into
a shared `tools/_model_config.py` and drop the legacy OpenAI direct-HTTP
code paths that the new model-routed path made redundant.

- New `tools/_model_config.py` exposes `load_tool_model` plus
  `resolve_model_config_path` + `_extract_model_block`. Path resolution
  is now two-step: an explicit `--model-config` arg, then
  `<workspace_dir>/config_snapshot/merged_config.yaml` (already written
  by `cli.py`).
- Drop the `WEBWRIGHT_TOOL_MODEL_CONFIG` env var and the
  `tool_model_config.json` fallback (neither was produced by anything in
  the repo).
- Drop the `--api-key` / `--endpoint` / `--model` / `--max-attempts` /
  `--retry-base-delay` CLI flags from both tools; the model client owns
  HTTP retries via `models/base.py:_post_with_retries`.
- `model_claude.yaml` no longer needs the `&model` anchor or the
  `tools.image_qa.model` / `tools.self_reflection.model` block; tools
  read the top-level `model:` block directly.

Tests updated to cover the new resolver (explicit arg, snapshot
fallback, missing-config error) plus a sanity check that
`model_claude.yaml` has no `tools:` block.

Net: -583 / +106 lines.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@adamlu123 adamlu123 merged commit 0be73c1 into microsoft:main May 27, 2026
1 check was pending
@adamlu123 adamlu123 deleted the refactor/inner-tool-routing-cleanup branch May 27, 2026 01:48
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