Conversation
m-y-mo
requested review from
JarLob,
Kwstubbs,
anticomputer,
kevinbackhouse,
p- and
sylwia-budzynska
as code owners
There was a problem hiding this comment.
Pull request overview
Adds support for specifying a model configuration reference via a CLI option when running taskflows, allowing the CLI-provided value to override the taskflow’s embedded model_config_ref.
Changes:
- Add
--model-config/-moption to the Typer CLI. - Thread the CLI model config through
run_main(...). - Override
taskflow_doc.model_config_refwith the CLI-provided value during taskflow execution.
Show a summary per file
| File | Description |
|---|---|
src/seclab_taskflow_agent/cli.py |
Adds --model-config option and forwards it into run_main. |
src/seclab_taskflow_agent/runner.py |
Accepts cli_model_config and uses it to override the taskflow’s model config reference. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 4
| run_main( | ||
| available_tools, personality, effective_taskflow, | ||
| cli_globals, user_prompt, resume_session_id=resume, | ||
| cli_globals, user_prompt, cli_model_config = model_config, resume_session_id=resume, |
There was a problem hiding this comment.
The keyword argument is written with spaces around the '=' ("cli_model_config = model_config"), which violates Ruff/pycodestyle E251 and will fail linting. Remove the spaces (use "cli_model_config=model_config").
| cli_globals, user_prompt, cli_model_config = model_config, resume_session_id=resume, | |
| cli_globals, user_prompt, cli_model_config=model_config, resume_session_id=resume, |
| async def run_main( | ||
| available_tools: AvailableTools, | ||
| personality_path: str | None, | ||
| taskflow_path: str | None, | ||
| cli_globals: dict[str, str], | ||
| prompt: str | None, | ||
| cli_model_config: str | None, | ||
| resume_session_id: str | None = None, | ||
| ) -> None: |
There was a problem hiding this comment.
run_main now takes a new required positional parameter (cli_model_config) inserted before resume_session_id, which is a breaking API change for any external callers passing resume_session_id positionally (and run_main is exported via __all__). To preserve compatibility, consider making cli_model_config optional with a default and/or moving it after resume_session_id (e.g., add it at the end as a keyword-only option).
| # Resolve model config | ||
| model_config_ref = taskflow_doc.model_config_ref | ||
| if cli_model_config: | ||
| model_config_ref = cli_model_config |
There was a problem hiding this comment.
New behavior allows overriding the taskflow’s model_config_ref via cli_model_config, but there are existing tests for runner logic (e.g., tests/test_runner.py) and none cover this override path. Please add a unit test that verifies the CLI-provided model config takes precedence over the taskflow document value.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
Allow model config file to be passed via cli argument when running taskflows