Conversation
There was a problem hiding this comment.
Pull request overview
This PR standardizes the default sandbox variant naming to "eval" across the codebase and improves CLI usability for the GitHub skill scripts by providing clearer guidance when invalid arguments are supplied.
Changes:
- Rename/standardize sandbox variant references from
"local-eval"to"eval"(config default, tests, examples, docs, README, executor comments). - Add a custom
ArgumentParsersubclass to GitHub skill scripts to show more helpful usage/parameter guidance on invalid args. - Configure pytest to discover tests from the
tests/directory viatestpaths.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_executor_async.py | Updates sandbox creation to use variant="eval". |
| tests/test_direct_execution.py | Updates CodeModeConfig.sandbox_variant to "eval". |
| tests/test_agent_minimal.py | Updates CodeModeConfig.sandbox_variant to "eval". |
| skills/github/scripts/search_repos.py | Adds custom argparse error handler with parameter guidance. |
| skills/github/scripts/list_repos.py | Adds custom argparse error handler with parameter guidance. |
| skills/github/scripts/list_prs.py | Adds custom argparse error handler with parameter guidance. |
| skills/github/scripts/list_issues.py | Adds custom argparse error handler with parameter guidance. |
| skills/github/scripts/get_repo.py | Adds custom argparse error handler with parameter guidance. |
| pyproject.toml | Sets pytest testpaths to ["tests"]. |
| examples/simple/agent_cli.py | Updates LocalEvalSandbox import path. |
| examples/patterns/codemode_example.py | Updates sandbox_variant example to "eval". |
| docs/docs/skills/index.mdx | Updates LocalEvalSandbox import path in docs snippet. |
| agent_codemode/types.py | Changes default sandbox_variant to "eval". |
| agent_codemode/composition/executor.py | Updates terminology in comments/docstrings from local-eval to eval. |
| README.md | Updates documented default sandbox_variant to "eval". |
Comments suppressed due to low confidence (1)
agent_codemode/composition/executor.py:140
- This method name (
_is_local_eval_sandbox) and local variable (is_local_eval) still refer to “local_eval”, but the updated docstring/comments now call the same concept “eval”. This makes the branching logic harder to follow; consider renaming the method/variable (or updating the docstring) for consistency. Also, the docstring should use “an eval type”, not “a eval type”.
def _is_local_eval_sandbox(self) -> bool:
"""Check if the sandbox is a eval type (has in-memory namespaces).
This checks the actual sandbox instance, not the config, to handle
cases where an external sandbox is passed that differs from config.
"""
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| params_help = ( | ||
| "\nValid parameters for list_repos:\n" | ||
| " --visibility all | public | private (default: all)\n" | ||
| " --sort updated | created | pushed | full_name (default: updated)\n" | ||
| " --format table | json (default: table)\n" | ||
| " --limit integer — max repos to return\n" | ||
| "\nDo NOT pass 'per_page' — use 'limit' instead.\n" | ||
| ) | ||
| self.print_usage(sys.stderr) | ||
| print(f"\nerror: {message}", file=sys.stderr) | ||
| print(params_help, file=sys.stderr) |
There was a problem hiding this comment.
The docstring says this parser “prints full help on invalid arguments”, but error() prints usage plus a custom parameters block rather than argparse’s full help text. Consider calling self.print_help(sys.stderr) (or adjusting the docstring) to keep behavior and documentation aligned.
| params_help = ( | |
| "\nValid parameters for list_repos:\n" | |
| " --visibility all | public | private (default: all)\n" | |
| " --sort updated | created | pushed | full_name (default: updated)\n" | |
| " --format table | json (default: table)\n" | |
| " --limit integer — max repos to return\n" | |
| "\nDo NOT pass 'per_page' — use 'limit' instead.\n" | |
| ) | |
| self.print_usage(sys.stderr) | |
| print(f"\nerror: {message}", file=sys.stderr) | |
| print(params_help, file=sys.stderr) | |
| print(f"\nerror: {message}", file=sys.stderr) | |
| print("", file=sys.stderr) | |
| self.print_help(sys.stderr) |
|
|
||
|
|
||
| class _HelpOnErrorParser(argparse.ArgumentParser): | ||
| """ArgumentParser that prints full help on invalid arguments.""" |
There was a problem hiding this comment.
The class docstring says this parser “prints full help on invalid arguments”, but error() only prints usage + a custom parameter summary. To avoid misleading users, either print the full argparse help (self.print_help(sys.stderr)) or update the docstring/wording accordingly.
| """ArgumentParser that prints full help on invalid arguments.""" | |
| """ArgumentParser that prints usage and a parameter summary on invalid arguments.""" |
|
|
||
|
|
||
| class _HelpOnErrorParser(argparse.ArgumentParser): | ||
| """ArgumentParser that prints full help on invalid arguments.""" |
There was a problem hiding this comment.
The docstring claims “prints full help on invalid arguments”, but error() prints usage plus a hand-written parameter list. Either include self.print_help(sys.stderr) to truly show full help, or adjust the docstring to reflect what’s printed.
| """ArgumentParser that prints full help on invalid arguments.""" | |
| """ArgumentParser that prints usage and parameter guidance on invalid arguments.""" |
|
|
||
|
|
||
| class _HelpOnErrorParser(argparse.ArgumentParser): | ||
| """ArgumentParser that prints full help on invalid arguments.""" |
There was a problem hiding this comment.
The docstring says this parser prints “full help” on errors, but error() prints usage and a custom parameter summary rather than argparse’s full help output. Consider printing self.print_help(sys.stderr) (or rewording the docstring) so expectations match behavior.
| """ArgumentParser that prints full help on invalid arguments.""" | |
| """ArgumentParser that prints usage and a concise parameter summary on invalid arguments.""" |
| class _HelpOnErrorParser(argparse.ArgumentParser): | ||
| """ArgumentParser that prints full help on invalid arguments.""" | ||
|
|
||
| def error(self, message: str) -> None: # noqa: D401 | ||
| params_help = ( |
There was a problem hiding this comment.
The docstring says this parser “prints full help on invalid arguments”, but error() only prints usage plus a custom parameters block. Either call self.print_help(sys.stderr)/self.format_help() (and optionally append the custom notes), or reword the docstring to match the actual behavior so it’s not misleading.
No description provided.