Feat: Agent model settings with three quality presets#27
Feat: Agent model settings with three quality presets#27sea212 wants to merge 20 commits intojosstei:mainfrom
Conversation
josstei
left a comment
There was a problem hiding this comment.
Review Summary
Nice feature — the preset system is well-structured and the cross-file synchronization is thorough. Requesting changes for one bug and a few model assignment refinements.
Requesting changes
- [Bug] Settings merge in
setup-models.js— Current code stomps existing agent override properties. Must shallow-merge to preserve non-modelConfigfields. - [Quality preset] —
coderandrefactorshould use Pro. These are the core code-producing agents; users choosing "quality" expect top-tier models here. - [Economic preset] —
architectandproduct_managershould use Flash. These are planning agents whose outputs are user-reviewed — Flash is sufficient. - [Tests] — Add test coverage for
handleSetupModelsfollowing existing patterns intests/transforms/. - [Nit] — Validate agent names against
KNOWN_AGENTSat load time to prevent config drift.
What looks good
- All 4
orchestrate.tomlcopies and all 3orchestration-steps.mdcopies are correctly synchronized - All 40+ internal step references correctly renumbered
- Handler follows existing codebase conventions (signature, error handling, atomic writes, JSDoc)
- Defense-in-depth input validation (JSON schema enum + handler-level check)
skipoption for graceful opt-out
Generated by Claude Code
josstei
left a comment
There was a problem hiding this comment.
All 5 review items have been addressed:
- Bug fix — Agent overrides now shallow-merge, preserving existing non-
modelConfigproperties ✓ - Quality preset —
coderandrefactorupgraded to Pro ✓ - Economic preset —
architectandproduct_managerdowngraded to Flash ✓ - Tests — Full coverage added in
mcp-setup-models.test.jscovering all 6 requested scenarios (skip, invalid mode, fresh file, merge preservation, corrupted file, missing config) ✓ - Agent validation — Test-time validation against
KNOWN_AGENTSadded;codebase_investigatorgap resolved ✓
Generated by Claude Code
|
@josstei Applied all suggestions, PR is ready for review. |
josstei
left a comment
There was a problem hiding this comment.
Reopening review — the check-architecture workflow is failing on two issues:
1. Adapter drift (4 files out of sync)
node scripts/generate.js produces changes that aren't committed. The following generated files need to be regenerated and committed:
claude/src/core/agent-registry.jsclaude/src/mcp/tool-packs/workspace/index.jsplugins/maestro/src/core/agent-registry.jsplugins/maestro/src/mcp/tool-packs/workspace/index.js
Fix: run node scripts/generate.js and commit the output.
2. Two existing tests don't account for setup_models (190 pass, 2 fail)
The new setup_models tool was registered in the workspace pack but two existing test files still have hardcoded expected tool lists that don't include it:
tests/transforms/mcp-workspace-pack.test.js— expects[initialize_workspace, assess_task_complexity, validate_plan, resolve_settings], actual now includessetup_modelstests/transforms/mcp-pack-composition.test.js— same issue with the full tool list assertion
Fix: add 'setup_models' to the expected arrays in both test files.
The prior review items (shallow-merge bug, preset adjustments, new test coverage) all look good. Just these CI fixes remaining.
Generated by Claude Code
josstei
left a comment
There was a problem hiding this comment.
Dismissing prior request — the CI failures are minor integration oversights (2 test fixtures missing setup_models in expected arrays + node scripts/generate.js needs a rerun). All substantive review items from the original review have been addressed.
Generated by Claude Code
josstei
left a comment
There was a problem hiding this comment.
Feature looks good and all review items are addressed. Deferring merge until next release to align with upcoming architectural shifts. The platform layer is changing significantly so this will likely need a rebase before it can land.
josstei
left a comment
There was a problem hiding this comment.
Requesting changes for the critical registry architecture conflict — the PR patches a hardcoded KNOWN_AGENTS array that main has replaced with auto-generated registries (src/generated/agent-registry.json). This needs a rebase and strategy change before it can merge.
See the detailed comment above for the full analysis, including the codebase_investigator design question and agents.overrides schema verification.
Generated by Claude Code
Analysis: Rebase Requirements and Open Design Questions1. Registry Architecture Conflict (Critical — blocks merge)
// src/core/agent-registry.js (main, lines 1-8)
const agentRegistryData = require('../generated/agent-registry.json');
const KNOWN_AGENTS = Object.freeze(
agentRegistryData.map((entry) => toSnakeCase(entry.name))
);The registry is now built at generate-time by The PR's changes to Resolution path: On rebase, drop the 2.
|
| Issue | Severity | Blocks Merge? |
|---|---|---|
| Registry architecture conflict | Critical | Yes — requires rebase + strategy change |
codebase_investigator identity |
Significant | Needs design decision |
agents.overrides schema unverified |
Significant | Should confirm before merge |
| Cross-runtime artifact cleanup | Low | No |
| Restart UX / step placement | Low | No |
Generated by Claude Code
|
@josstei Before beginning to work, please review the approach:
as a side note, I think trying to create the golden hen that works in every agent runtime is destined to fail. From my experience from countless other tools, it just massively bloats the codebase and doesn't work properly for runtimes other than the one it was originally designed for. |
Related to #13
Description
This pull request introduces a system for configuring subagent models based on user-selectable model quality presets (
quality,balanced,economic). The orchestrator now asks the user about the model quality preset and applies the model quality preset config in a project-specific settings.json via a newly introducedsetup-modelsmcp. This allows for granular control over model selection for specialized subagents, enabling users to optimize for output quality or token efficiency.Key Changes
🛠️ Subagent Model Mapping System
src/config/agent-modes.jsonwhich defines model mappings for all specialized agents across three modes:quality: Prioritizes high-tier models.balanced: A mix of Pro and Flash models.economic: Maximizes usage of lightweight Flash models.setup_models(src/mcp/handlers/setup-models.js) to apply these mappings to the project-local.gemini/settings.json.🔄 Orchestration Workflow Enhancements
src/references/orchestration-steps.mdand related platform variants to maintain sequential accuracy:orchestrate.tomlacross all variants to prompt for quality mode selection and trigger the model setup.📁 Infrastructure & Setup
.gitignore: Added.worktreesto the exclusion list.src/,claude/, andplugins/maestro/to ensure consistent behavior across all Maestro entry points.📒 Final Notes
BeforeModelhook system. Currently blocked by Hooks: Attach agent information google-gemini/gemini-cli#21615agent-modes.json(global) to specify their own agent to model mappings. Orchestrator model-per-phase logic still tbd.zero_diff.test.jsdemands thatsetup-models.jsandagent-modes.jsonexist identically in claude and plugins/maestro directory. This is nonsensical, as those files are gemini-specific. However, inorchestrate.toml, Claude and Codex are instructed to do nothing when setting up agents, effectively rendering those nonsensical files useless in those cases.Summary table of changes
src/config/agent-modes.json,src/mcp/handlers/setup-models.jssrc/references/orchestration-steps.md,claude/src/references/orchestration-steps.md,plugins/maestro/src/references/orchestration-steps.mdsrc/platforms/gemini/commands/maestro/orchestrate.toml,commands/maestro/orchestrate.toml(and others).gitignore,src/mcp/tool-packs/workspace/index.js