feat: interactive asset selection and list-typed target handling#204
Open
DS-KoolAid wants to merge 2 commits intomainfrom
Open
feat: interactive asset selection and list-typed target handling#204DS-KoolAid wants to merge 2 commits intomainfrom
DS-KoolAid wants to merge 2 commits intomainfrom
Conversation
Add interactive asset picker for job/schedule commands so users can select existing assets or enter new network targets (IP/CIDR) instead of defaulting to the agent hostname. Handle target field being a list (ENG-1908) via normalize_target helper.
Drop normalize_target in favor of main's extract_target_type helper for handling list-typed capability targets (ENG-1908).
Contributor
WalkthroughNon-domain asset handling in job and schedule interactive commands now uses interactive selection via new helpers instead of constructing fixed asset keys. Added helpers: ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
praetorian_cli/sdk/test/ui/test_aegis_job.py (1)
49-51: Add one non-mocked test for the new target-handling path.These monkeypatches keep the existing job-flow tests stable, but they also bypass all of the new behavior from this PR. There’s still no automated coverage for list-backed
targetvalues, manual IP/CIDR entry, or the409conflict path inselect_asset().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@praetorian_cli/sdk/test/ui/test_aegis_job.py` around lines 49 - 51, Add a new test in praetorian_cli/sdk/test/ui/test_aegis_job.py that does not monkeypatch Confirm.ask or _select_asset so the real target-handling logic and select_asset() are exercised: construct a job invocation that provides a list-backed target value, simulate manual IP/CIDR input (e.g., by supplying input to the prompt mechanism used by Confirm.ask or the prompt toolkit), and stub only the network/backend call that can return a 409 to trigger the conflict path in select_asset(); assert the correct branch behavior (selected target, conflict handling, and resulting job payload) to cover list-backed targets, manual entry, and the 409 conflict branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@praetorian_cli/ui/aegis/commands/job_helpers.py`:
- Around line 381-390: Remove the unnecessary f-string prefixes on the two
menu.console.print calls that do not interpolate values: change the call
printing "\n Select target asset:" and the call printing " 0. Enter target
manually (IP, CIDR, or hostname)" to plain string literals (leave the loop's
menu.console.print(f" {i:2d}. {display}") intact since it interpolates i and
display). This fixes the F541 lint errors while keeping the enumerated line that
uses {i:2d} and {display} unchanged.
- Around line 367-370: The asset lookup uses key_prefix=f"#asset#{hostname}"
which can match hostnames with the same leading text (e.g. "agent01-old");
update the key_prefix passed to menu.sdk.assets.list to delimit the hostname
segment (e.g. append the segment separator used in your key schema after
{hostname}) so only exact-host matches are returned; change the call in
job_helpers.py where menu.sdk.assets.list(...) is invoked to use the delimited
prefix (keeping hostname variable and assets, _ = menu.sdk.assets.list(...)
unchanged).
- Around line 341-349: The current asset-creation try/except logs non-409 errors
but still returns a fabricated target_key; change the block so you call
menu.sdk.assets.add(...) and capture its return (use the key/value it returns to
set target_key/target_display on success), and in the except block if '409' is
not in str(e) log the error and return (None, None) immediately so callers
(run_job(), add_schedule()) don't proceed with a missing asset; if the exception
contains '409' treat it as a benign conflict and continue returning the existing
target_key/target_display as before.
---
Nitpick comments:
In `@praetorian_cli/sdk/test/ui/test_aegis_job.py`:
- Around line 49-51: Add a new test in
praetorian_cli/sdk/test/ui/test_aegis_job.py that does not monkeypatch
Confirm.ask or _select_asset so the real target-handling logic and
select_asset() are exercised: construct a job invocation that provides a
list-backed target value, simulate manual IP/CIDR input (e.g., by supplying
input to the prompt mechanism used by Confirm.ask or the prompt toolkit), and
stub only the network/backend call that can return a 409 to trigger the conflict
path in select_asset(); assert the correct branch behavior (selected target,
conflict handling, and resulting job payload) to cover list-backed targets,
manual entry, and the 409 conflict branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a0e61681-37ca-41fd-90c8-e513328bccc4
📒 Files selected for processing (5)
praetorian_cli/sdk/test/ui/test_aegis_job.pypraetorian_cli/sdk/test/ui/test_aegis_schedule.pypraetorian_cli/ui/aegis/commands/job.pypraetorian_cli/ui/aegis/commands/job_helpers.pypraetorian_cli/ui/aegis/commands/schedule.py
Comment on lines
+341
to
+349
| try: | ||
| menu.console.print(f" [{colors['dim']}]Creating asset {target_input}...[/{colors['dim']}]") | ||
| menu.sdk.assets.add(group=hostname, identifier=target_input) | ||
| except Exception as e: | ||
| # Asset may already exist — 409 conflict is fine | ||
| if '409' not in str(e): | ||
| menu.console.print(f" [{colors['dim']}]Note: could not create asset: {e}[/{colors['dim']}]") | ||
|
|
||
| return target_key, target_display |
Contributor
There was a problem hiding this comment.
Fail the flow when asset creation fails.
Non-409 errors are only logged here, but the helper still returns a fabricated target_key. That lets run_job() and add_schedule() continue against a target that was never created. Return (None, None) for non-conflict failures, and prefer the key returned by assets.add() on success.
Suggested fix
def _create_asset_target(menu, hostname, target_input, colors):
target_key = f"#asset#{hostname}#{target_input}"
target_display = f"asset {target_input}"
try:
menu.console.print(f" [{colors['dim']}]Creating asset {target_input}...[/{colors['dim']}]")
- menu.sdk.assets.add(group=hostname, identifier=target_input)
+ asset = menu.sdk.assets.add(group=hostname, identifier=target_input)
+ target_key = asset.get('key', target_key)
except Exception as e:
# Asset may already exist — 409 conflict is fine
if '409' not in str(e):
menu.console.print(f" [{colors['dim']}]Note: could not create asset: {e}[/{colors['dim']}]")
+ return None, None
return target_key, target_display🧰 Tools
🪛 Ruff (0.15.9)
[warning] 344-344: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@praetorian_cli/ui/aegis/commands/job_helpers.py` around lines 341 - 349, The
current asset-creation try/except logs non-409 errors but still returns a
fabricated target_key; change the block so you call menu.sdk.assets.add(...) and
capture its return (use the key/value it returns to set
target_key/target_display on success), and in the except block if '409' is not
in str(e) log the error and return (None, None) immediately so callers
(run_job(), add_schedule()) don't proceed with a missing asset; if the exception
contains '409' treat it as a benign conflict and continue returning the existing
target_key/target_display as before.
Comment on lines
+367
to
+370
| menu.console.print(f" [{colors['dim']}]Fetching assets for {hostname}...[/{colors['dim']}]") | ||
|
|
||
| # Fetch assets associated with this agent's hostname | ||
| assets, _ = menu.sdk.assets.list(key_prefix=f"#asset#{hostname}", pages=1) |
Contributor
There was a problem hiding this comment.
Delimit the hostname segment in the asset lookup prefix.
key_prefix=f"#asset#{hostname}" also matches groups like agent01-old or agent010. That can surface the wrong assets in the picker. Use the full segment prefix instead.
Suggested fix
- assets, _ = menu.sdk.assets.list(key_prefix=f"#asset#{hostname}", pages=1)
+ assets, _ = menu.sdk.assets.list(key_prefix=f"#asset#{hostname}#", pages=1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| menu.console.print(f" [{colors['dim']}]Fetching assets for {hostname}...[/{colors['dim']}]") | |
| # Fetch assets associated with this agent's hostname | |
| assets, _ = menu.sdk.assets.list(key_prefix=f"#asset#{hostname}", pages=1) | |
| menu.console.print(f" [{colors['dim']}]Fetching assets for {hostname}...[/{colors['dim']}]") | |
| # Fetch assets associated with this agent's hostname | |
| assets, _ = menu.sdk.assets.list(key_prefix=f"#asset#{hostname}#", pages=1) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@praetorian_cli/ui/aegis/commands/job_helpers.py` around lines 367 - 370, The
asset lookup uses key_prefix=f"#asset#{hostname}" which can match hostnames with
the same leading text (e.g. "agent01-old"); update the key_prefix passed to
menu.sdk.assets.list to delimit the hostname segment (e.g. append the segment
separator used in your key schema after {hostname}) so only exact-host matches
are returned; change the call in job_helpers.py where menu.sdk.assets.list(...)
is invoked to use the delimited prefix (keeping hostname variable and assets, _
= menu.sdk.assets.list(...) unchanged).
Comment on lines
+381
to
+390
| menu.console.print(f"\n Select target asset:") | ||
|
|
||
| for i, c in enumerate(choices, 1): | ||
| display = c['name'] | ||
| if c['dns'] and c['dns'] != c['name']: | ||
| display = f"{c['dns']} / {c['name']}" | ||
| menu.console.print(f" {i:2d}. {display}") | ||
|
|
||
| # Option to enter manually | ||
| menu.console.print(f" 0. Enter target manually (IP, CIDR, or hostname)") |
Contributor
There was a problem hiding this comment.
Remove the unused f prefixes here.
Ruff is already flagging both of these strings as F541, so this helper will stay red on lint until they become plain literals.
Suggested fix
- menu.console.print(f"\n Select target asset:")
+ menu.console.print("\n Select target asset:")
@@
- menu.console.print(f" 0. Enter target manually (IP, CIDR, or hostname)")
+ menu.console.print(" 0. Enter target manually (IP, CIDR, or hostname)")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| menu.console.print(f"\n Select target asset:") | |
| for i, c in enumerate(choices, 1): | |
| display = c['name'] | |
| if c['dns'] and c['dns'] != c['name']: | |
| display = f"{c['dns']} / {c['name']}" | |
| menu.console.print(f" {i:2d}. {display}") | |
| # Option to enter manually | |
| menu.console.print(f" 0. Enter target manually (IP, CIDR, or hostname)") | |
| menu.console.print("\n Select target asset:") | |
| for i, c in enumerate(choices, 1): | |
| display = c['name'] | |
| if c['dns'] and c['dns'] != c['name']: | |
| display = f"{c['dns']} / {c['name']}" | |
| menu.console.print(f" {i:2d}. {display}") | |
| # Option to enter manually | |
| menu.console.print(" 0. Enter target manually (IP, CIDR, or hostname)") |
🧰 Tools
🪛 Ruff (0.15.9)
[error] 381-381: f-string without any placeholders
Remove extraneous f prefix
(F541)
[error] 390-390: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@praetorian_cli/ui/aegis/commands/job_helpers.py` around lines 381 - 390,
Remove the unnecessary f-string prefixes on the two menu.console.print calls
that do not interpolate values: change the call printing "\n Select target
asset:" and the call printing " 0. Enter target manually (IP, CIDR, or
hostname)" to plain string literals (leave the loop's menu.console.print(f"
{i:2d}. {display}") intact since it interpolates i and display). This fixes the
F541 lint errors while keeping the enumerated line that uses {i:2d} and
{display} unchanged.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
praetorian_cli/ui/aegis/commands/job_helpers.py (3)
345-353:⚠️ Potential issue | 🟠 MajorStill returns a fake asset key after failed creation.
On Lines 347-352, non-
409failures only log and then still return a truthytarget_key. Callers treat that as success, so the flow can continue against an asset that was never created. Also,assets.add()already returns the canonical asset record, so use its key on success.Proposed fix
try: menu.console.print(f" [{colors['dim']}]Creating asset {target_input}...[/{colors['dim']}]") - menu.sdk.assets.add(group=hostname, identifier=target_input) + asset = menu.sdk.assets.add(group=hostname, identifier=target_input) + target_key = asset.get('key', target_key) except Exception as e: # Asset may already exist — 409 conflict is fine if '409' not in str(e): menu.console.print(f" [{colors['dim']}]Note: could not create asset: {e}[/{colors['dim']}]") + return None, None return target_key, target_display🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@praetorian_cli/ui/aegis/commands/job_helpers.py` around lines 345 - 353, The code currently logs failures from menu.sdk.assets.add but still returns the provisional target_key; instead, when calling menu.sdk.assets.add(group=hostname, identifier=target_input) capture its return value (the canonical asset record) and set target_key from that record on success; on a 409 conflict call menu.sdk.assets.get(group=hostname, identifier=target_input) to retrieve the existing asset's canonical key and set target_key accordingly; on any non-409 error do not return the fake key (set target_key to None or propagate the error) and only return a truthy target_key when you have the actual key from the add/get calls. Ensure you reference menu.sdk.assets.add, menu.sdk.assets.get, hostname, target_input, target_key, and target_display when making the changes.
385-394:⚠️ Potential issue | 🟡 MinorStill tripping Ruff with literal
f-strings.These two prints do not interpolate anything, so Ruff will keep failing with
F541until they become plain string literals.Proposed fix
- menu.console.print(f"\n Select target asset:") + menu.console.print("\n Select target asset:") @@ - menu.console.print(f" 0. Enter target manually (IP, CIDR, or hostname)") + menu.console.print(" 0. Enter target manually (IP, CIDR, or hostname)")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@praetorian_cli/ui/aegis/commands/job_helpers.py` around lines 385 - 394, The two menu.console.print calls in job_helpers.py (the literal prints in the selection UI: the string "\n Select target asset:" and the option " 0. Enter target manually (IP, CIDR, or hostname)") are using f-strings without interpolation which triggers Ruff F541; remove the f prefix so they become plain string literals (e.g., menu.console.print("\n Select target asset:") and menu.console.print(" 0. Enter target manually (IP, CIDR, or hostname)")), leaving other f-strings like the enumerated display formatting unchanged.
374-374:⚠️ Potential issue | 🟠 MajorStill using an undelimited asset prefix.
Line 374 uses
#asset#{hostname}as the prefix, which also matches groups likeagent01-oldoragent010. Add the segment delimiter so the picker only shows exact-host assets.Proposed fix
- assets, _ = menu.sdk.assets.list(key_prefix=f"#asset#{hostname}", pages=1) + assets, _ = menu.sdk.assets.list(key_prefix=f"#asset#{hostname}#", pages=1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@praetorian_cli/ui/aegis/commands/job_helpers.py` at line 374, Replace the undelimited prefix passed to menu.sdk.assets.list so it matches only exact-host assets: change the key_prefix argument used where assets, _ = menu.sdk.assets.list(...) is called (currently f"#asset#{hostname}") to include a segment delimiter before (and ideally after) the hostname (for example use a delimiter like "/" or "-" so the prefix becomes "#asset#/<hostname>/" or similar) so the picker only returns assets for the exact hostname rather than partial matches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@praetorian_cli/ui/aegis/commands/job_helpers.py`:
- Around line 398-406: After reading the user's choice from Prompt.ask and
calling .strip(), explicitly reject empty string inputs before attempting
int(choice) so we don't treat blank input as a direct target; update the logic
in the block around Prompt.ask/choice handling in job_helpers.py (where
variables menu, hostname, choice and function _create_asset_target are used) to
check if choice == "" and if so re-prompt (or loop back to Prompt.ask) instead
of falling through to the non-numeric branch that calls _create_asset_target
with an empty identifier.
---
Duplicate comments:
In `@praetorian_cli/ui/aegis/commands/job_helpers.py`:
- Around line 345-353: The code currently logs failures from menu.sdk.assets.add
but still returns the provisional target_key; instead, when calling
menu.sdk.assets.add(group=hostname, identifier=target_input) capture its return
value (the canonical asset record) and set target_key from that record on
success; on a 409 conflict call menu.sdk.assets.get(group=hostname,
identifier=target_input) to retrieve the existing asset's canonical key and set
target_key accordingly; on any non-409 error do not return the fake key (set
target_key to None or propagate the error) and only return a truthy target_key
when you have the actual key from the add/get calls. Ensure you reference
menu.sdk.assets.add, menu.sdk.assets.get, hostname, target_input, target_key,
and target_display when making the changes.
- Around line 385-394: The two menu.console.print calls in job_helpers.py (the
literal prints in the selection UI: the string "\n Select target asset:" and
the option " 0. Enter target manually (IP, CIDR, or hostname)") are using
f-strings without interpolation which triggers Ruff F541; remove the f prefix so
they become plain string literals (e.g., menu.console.print("\n Select target
asset:") and menu.console.print(" 0. Enter target manually (IP, CIDR, or
hostname)")), leaving other f-strings like the enumerated display formatting
unchanged.
- Line 374: Replace the undelimited prefix passed to menu.sdk.assets.list so it
matches only exact-host assets: change the key_prefix argument used where
assets, _ = menu.sdk.assets.list(...) is called (currently f"#asset#{hostname}")
to include a segment delimiter before (and ideally after) the hostname (for
example use a delimiter like "/" or "-" so the prefix becomes
"#asset#/<hostname>/" or similar) so the picker only returns assets for the
exact hostname rather than partial matches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5d91d47a-96ab-49df-99d1-f7c53477a5b9
📒 Files selected for processing (3)
praetorian_cli/ui/aegis/commands/job.pypraetorian_cli/ui/aegis/commands/job_helpers.pypraetorian_cli/ui/aegis/commands/schedule.py
🚧 Files skipped from review as they are similar to previous changes (2)
- praetorian_cli/ui/aegis/commands/schedule.py
- praetorian_cli/ui/aegis/commands/job.py
Comment on lines
+398
to
+406
| default = "1" if choices else "0" | ||
| choice = Prompt.ask(" Choose target", default=default).strip() | ||
|
|
||
| # Allow direct target input (IP, CIDR, hostname) instead of a number | ||
| try: | ||
| choice_num = int(choice) | ||
| except ValueError: | ||
| # Treat non-numeric input as a direct target | ||
| return _create_asset_target(menu, hostname, choice, colors) |
Contributor
There was a problem hiding this comment.
Reject blank direct input before creating an asset.
After .strip(), a whitespace-only response becomes '', falls into the non-numeric branch, and calls _create_asset_target(...) with an empty identifier. Handle the blank case before int(choice) and re-prompt.
Proposed fix
default = "1" if choices else "0"
choice = Prompt.ask(" Choose target", default=default).strip()
+ if not choice:
+ menu.console.print(" Please enter a number or target value")
+ continue
# Allow direct target input (IP, CIDR, hostname) instead of a number
try:
choice_num = int(choice)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@praetorian_cli/ui/aegis/commands/job_helpers.py` around lines 398 - 406,
After reading the user's choice from Prompt.ask and calling .strip(), explicitly
reject empty string inputs before attempting int(choice) so we don't treat blank
input as a direct target; update the logic in the block around Prompt.ask/choice
handling in job_helpers.py (where variables menu, hostname, choice and function
_create_asset_target are used) to check if choice == "" and if so re-prompt (or
loop back to Prompt.ask) instead of falling through to the non-numeric branch
that calls _create_asset_target with an empty identifier.
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
1 participant
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
targetfield being a list (ENG-1908) via newnormalize_targethelper injob_helpers.pyjob.pyandschedule.pyTest plan
test_aegis_job.py— monkeypatch_select_assetin 5 test casestest_aegis_schedule.py— mockselect_assetfor non-AD capabilitiesruncommand with an AD-domain capability (should skip asset selection)runcommand with a non-AD capability shows asset picker