🧹 [code health improvement] Refactor argparse commands into handlers#179
🧹 [code health improvement] Refactor argparse commands into handlers#179badMade wants to merge 3 commits into
Conversation
- Replaces long `if-elif` chain in `main()` with standalone handlers mapped via `set_defaults`. - Scopes instantiation of `build_port_manifest()` to only the commands that need it. - Preserves identical functionality. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
Pull request overview
Refactors src/main.py’s CLI dispatch from a long if args.command == ... chain into per-command handler functions wired via argparse’s set_defaults(func=...), and avoids building the port manifest for commands that don’t need it.
Changes:
- Extracted each subcommand’s logic into top-level
handle_*functions. - Updated
build_parser()to attach the handler viaparser.set_defaults(func=...)for each subparser. - Simplified
main()to invoke the handler function rather than branching acrossargs.command.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
| if hasattr(args, 'func'): | ||
| return args.func(args) | ||
|
|
||
| parser.error(f'unknown command: {args.command}') |
| def handle_summary(args: argparse.Namespace) -> int: | ||
| manifest = build_port_manifest() | ||
| print(QueryEnginePort(manifest).render_summary()) | ||
| return 0 | ||
|
|
||
| def handle_manifest(args: argparse.Namespace) -> int: | ||
| manifest = build_port_manifest() | ||
| print(manifest.to_markdown()) |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the CLI command dispatch logic in src/main.py by extracting command-specific implementations into dedicated handler functions and utilizing argparse's set_defaults for cleaner execution. Feedback was provided regarding handle_load_session, specifically recommending the addition of input validation for session_id and error handling for file I/O to improve robustness against missing or malformed session files.
| session = load_session(args.session_id) | ||
| print(f'{session.session_id}\n{len(session.messages)} messages\nin={session.input_tokens} out={session.output_tokens}') | ||
| return 0 |
There was a problem hiding this comment.
The session_id identifier is used to construct a file path for loading session data. According to the general rules, identifiers used in file paths should be explicitly checked for empty strings to prevent unexpected file access (e.g., attempting to read .json). Additionally, since load_session performs file I/O and JSON parsing, it should be wrapped in a try-except block to handle missing files or corrupted data gracefully, providing a clean error message instead of a traceback in the CLI.
| session = load_session(args.session_id) | |
| print(f'{session.session_id}\n{len(session.messages)} messages\nin={session.input_tokens} out={session.output_tokens}') | |
| return 0 | |
| if not args.session_id: | |
| print('Error: session_id cannot be empty') | |
| return 1 | |
| try: | |
| session = load_session(args.session_id) | |
| except FileNotFoundError: | |
| print(f'Session not found: {args.session_id}') | |
| return 1 | |
| except Exception as e: | |
| print(f'Error loading session: {e}') | |
| return 1 | |
| print(f'{session.session_id}\n{len(session.messages)} messages\nin={session.input_tokens} out={session.output_tokens}') | |
| return 0 |
References
- When validating identifiers that are used as part of a filename, explicitly reject empty strings to prevent the creation of files that only consist of an extension (e.g., .jsonl).
- To prevent path traversal vulnerabilities, ensure that validation for identifiers used in file paths is applied on all access paths, including creation, resolution, and loading, not just on creation.
- Replaces long `if-elif` chain in `main()` with standalone handlers mapped via `set_defaults`. - Scopes instantiation of `build_port_manifest()` to only the commands that need it. - Preserves identical functionality. - Fixed syntax error in CI auto-merge action. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
@claude code review |
|
@copilot code review |
| def handle_summary(args: argparse.Namespace) -> int: | ||
| manifest = build_port_manifest() | ||
| print(QueryEnginePort(manifest).render_summary()) | ||
| return 0 | ||
|
|
||
| def handle_manifest(args: argparse.Namespace) -> int: | ||
| manifest = build_port_manifest() | ||
| print(manifest.to_markdown()) | ||
| return 0 |
| // Check if there are any non-successful checks | ||
| // Skip the 'auto-merge' check itself | ||
| const relevantChecks = checkRuns.check_runs.filter(c => c.name !== 'auto-merge'); | ||
|
|
||
| const allCompleted = relevantChecks.every(c => c.status === 'completed'); | ||
| const allSuccess = relevantChecks.every(c => c.conclusion === 'success' || c.conclusion === 'neutral' || c.conclusion === 'skipped'); | ||
|
|
||
| if (allCompleted) { |
- Reduce 4 blank lines between imports and first handler to 2 - Add two blank lines between every top-level handler function - Remove unreachable hasattr(args, 'func') guard; call args.func(args) directly Agent-Logs-Url: https://github.com/badMade/claw-code/sessions/8172ee42-861d-4edb-9371-d2578d5d2d68 Co-authored-by: badMade <106821302+badMade@users.noreply.github.com>
Addressed all three issues from the review in commit b4ed04e:
|
🎯 What: The complex, monolithic
if args.command == ...chain insrc/main.py:main()was refactored. The logic for each of the 24 commands was extracted into individual, top-level handler functions (handle_summary,handle_route, etc.). These handlers are mapped natively to their subparsers usingargparse'sparser.set_defaults(func=...)method. Themain()function was simplified to unconditionally executeargs.func(args).💡 Why: The previous pattern was difficult to read, maintain, and test, growing linearly with every newly supported command. The new approach is a standard
argparsepattern that heavily improves separation of concerns. Additionally, it addresses a minor performance inefficiency wherebuild_port_manifest()was being called unconditionally for all commands, even though only three handlers required it.✅ Verification:
python3 -m unittest discover tests) ran and passed (31/31).python3 -m src.main summaryandpython3 -m src.main --help).✨ Result: The
main.pyfile is significantly more readable and maintainable. The monolithic branch dispatch is gone, ensuring future commands can be added elegantly using localized handlers.PR created automatically by Jules for task 12626777393880839229 started by @badMade