Handle boolean flag parameters correctly in CommandExecutor.run_topp()#350
Open
t0mdavid-m wants to merge 2 commits intomainfrom
Open
Handle boolean flag parameters correctly in CommandExecutor.run_topp()#350t0mdavid-m wants to merge 2 commits intomainfrom
t0mdavid-m wants to merge 2 commits intomainfrom
Conversation
TOPP tools have two boolean parameter styles: flag-style (registerFlag_) where presence means enabled, and string-style (registerStringOption_ with valid_strings=["true","false"]) where explicit values are required. Add isinstance(v, bool) check in the params[tool] loop so that: - Python bool True emits only the flag (e.g., -flag) - Python bool False omits the flag entirely - String "true"/"false" continues to emit -param true / -param false This fixes tools like FLASHDeconv and FLASHTnT that expect POSIX-style boolean flags. https://claude.ai/code/session_016pdqzsAKEB6UDBcGM82T6F
📝 WalkthroughWalkthroughThe pull request modifies Changes
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can customize the tone of the review comments and chat replies.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_boolean_flag_params.py (1)
71-91: Add one regression test for boolean flags passed viacustom_params.Current tests validate boolean handling from
params.json, but not from runtimecustom_params. A dedicated test here would lock behavior across both input paths.🧪 Suggested test addition
@@ class TestBooleanFlagParams: @@ def test_command_starts_with_tool(self, captured_command): """Command should start with the tool name.""" assert captured_command[0] == "FakeTool" + + def test_custom_params_bool_flags(self, workflow_env): + """Boolean custom_params should follow flag semantics.""" + captured = {} + + pm = ParameterManager(workflow_env) + logger = MagicMock(spec=Logger) + executor = CommandExecutor(workflow_env, logger, pm) + + def fake_run_command(cmd): + captured["command"] = cmd + return True + + executor.run_command = fake_run_command + executor.run_topp( + "FakeTool", + {"in": ["input.mzML"]}, + custom_params={"custom_on": True, "custom_off": False}, + ) + + cmd = captured["command"] + assert "-custom_on" in cmd + assert "-custom_off" not in cmd + idx = cmd.index("-custom_on") + assert cmd[idx + 1].startswith("-")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_boolean_flag_params.py` around lines 71 - 91, Add a regression test that verifies boolean flags passed via runtime custom_params are handled the same as those from params.json by reusing the captured_command helper: instantiate a workflow_env with custom_params containing a boolean flag, call captured_command(workflow_env) (which constructs ParameterManager and CommandExecutor and intercepts run_command), then assert the returned command list includes or omits the flag appropriately; reference the captured_command function and the use of ParameterManager, CommandExecutor.run_topp and the fake_run_command replacement to locate where to plug the new test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/workflow/CommandExecutor.py`:
- Around line 287-292: The boolean flag handling for custom_params is
inconsistent: update the loop that serializes custom_params so booleans follow
the same semantics as the earlier params[tool] block — if a value is a bool and
True, append only the short flag token (e.g., command += [f"-{k}"]); if False,
skip adding anything; otherwise serialize non-bool values as before. Locate the
custom_params serialization loop (the block that iterates over custom_params,
referencing variables like k, v, and command) and apply the same isinstance(v,
bool) check and conditional logic used in the params[tool] boolean handling to
ensure consistent CLI output.
---
Nitpick comments:
In `@tests/test_boolean_flag_params.py`:
- Around line 71-91: Add a regression test that verifies boolean flags passed
via runtime custom_params are handled the same as those from params.json by
reusing the captured_command helper: instantiate a workflow_env with
custom_params containing a boolean flag, call captured_command(workflow_env)
(which constructs ParameterManager and CommandExecutor and intercepts
run_command), then assert the returned command list includes or omits the flag
appropriately; reference the captured_command function and the use of
ParameterManager, CommandExecutor.run_topp and the fake_run_command replacement
to locate where to plug the new test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3a9f3a47-fbc9-48f0-9e97-97519674a69d
📒 Files selected for processing (2)
src/workflow/CommandExecutor.pytests/test_boolean_flag_params.py
Comment on lines
+287
to
+292
| # Boolean flag handling (from registerFlag_ params): | ||
| # True -> emit flag only, False -> skip entirely | ||
| if isinstance(v, bool): | ||
| if v: | ||
| command += [f"-{k}"] | ||
| continue |
There was a problem hiding this comment.
Boolean flag handling is incomplete for custom_params.
Line 287-Line 292 correctly applies flag semantics for params[tool], but Line 302-Line 310 still serializes booleans in custom_params as values (-flag True / -flag False). This creates inconsistent CLI behavior for the same option source.
🔧 Suggested fix
@@
# Add custom parameters
for k, v in custom_params.items():
+ # Keep boolean handling consistent with params[tool]
+ if isinstance(v, bool):
+ if v:
+ command += [f"-{k}"]
+ continue
command += [f"-{k}"]
# Skip only empty strings (pass flag with no value)
# Note: 0 and 0.0 are valid values, so use explicit check
if v != "" and v is not None:
if isinstance(v, list):
command += [str(x) for x in v]
else:
command += [str(v)]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/workflow/CommandExecutor.py` around lines 287 - 292, The boolean flag
handling for custom_params is inconsistent: update the loop that serializes
custom_params so booleans follow the same semantics as the earlier params[tool]
block — if a value is a bool and True, append only the short flag token (e.g.,
command += [f"-{k}"]); if False, skip adding anything; otherwise serialize
non-bool values as before. Locate the custom_params serialization loop (the
block that iterates over custom_params, referencing variables like k, v, and
command) and apply the same isinstance(v, bool) check and conditional logic used
in the params[tool] boolean handling to ensure consistent CLI output.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds proper handling for boolean flag parameters in the
CommandExecutor.run_topp()method. Boolean parameters are now correctly converted to TOPP command-line flags:Truevalues emit only the flag (no value), whileFalsevalues omit the flag entirely. This distinguishes them from string-based boolean parameters ("true"/"false") which continue to emit explicit values.Changes
CommandExecutor.py: Added boolean parameter detection and handling logic
Trueboolean values now emit only the flag name (e.g.,-enable_feature)Falseboolean values are skipped entirely (flag not emitted)test_boolean_flag_params.py: Added comprehensive test suite
bool Truebool FalseImplementation Details
The implementation uses a simple
isinstance(v, bool)check before the existing parameter processing logic. When a boolean is detected:True: append the flag andcontinueto skip the normal value-appending logicFalse:continueto skip the flag entirelyThis approach maintains backward compatibility while adding proper support for registerFlag_ style parameters in TOPP tools.
https://claude.ai/code/session_016pdqzsAKEB6UDBcGM82T6F
Summary by CodeRabbit