Test guard - #455
Closed
iamcristi wants to merge 60 commits into
Closed
Conversation
_run_install tests that mock _install_hooks were running the real post-install serversDiscovered send: actual machine discovery plus a hook-script invocation on a mock path (~1.2s per test, environment- dependent). Patch _send_servers_discovered_event autouse in the three affected classes, and pin that clients with only unparseable configs still emit an empty-servers entry.
_copy_hook_script returned was_updated=False whenever the forwarding script was already current, so an install that only restored or updated snyk-agent-guard-discover.sh printed 'hook integration up to date'. Track the discovery-script write and include it in the returned flag.
- send hook events through the shared platform session (certifi plus the CA certs load_extra_ca_certs picks up from the environment), so Agent Guard delivery has the same trust posture as the analysis path; retries stay opt-in so session-start discovery keeps a single attempt - request only servers for the install-time discovery event, and honor discovery_scope in the well-known-client path so a servers-only request no longer walks every skills dir - let the Windows discovery trampoline hand its stdin to the child like the POSIX one does, instead of a blocking ReadToEnd outside the try/catch, and cover the script with executing tests for the first time - catch ValueError in safe_resolve: Path.resolve raises it for a NUL byte, and target folders arrive from untrusted hook payloads - resolve the active subparser from the parsed namespace instead of re-walking argv, dropping the hand-rolled option-width heuristic - drop the two path helpers left with no callers, and repoint the tests and docstrings at _discovery_paths_with_ancestors
safe_resolve was extracted in 4cbf8f3 from two try/excepts that were already written inline, and its single "return the literal path on failure" contract does not fit all three of its callers. Inline the handling at each site instead and delete the helper. _scans_own_home goes back to failing closed, as it did before the extraction: routing it through safe_resolve let each side degrade to its literal path and keep comparing, so it could report own-home where it previously did not. That gate gets to decide whether this process's CLAUDE_CONFIG_DIR / VSCODE_PORTABLE apply to the home being scanned, so an unresolvable path must not count as proof. It still catches more than OSError, since everything now maps to the same safe answer and letting anything escape would abort discovery outright. The alias filter in _all_discovery_folders and the target-folder dedup in discover_clients_to_inspect keep the literal-path fallback unchanged, so unresolvable paths stay distinct rather than collapsing into one another. The ValueError arm stays earned on the pipelines side, where the paths come straight from hook payloads and a NUL byte raises it.
guard install accepted --control-identifier as an alias for --machine-id, which made that option string mean control_identifier on scan/inspect/evo and machine_id here. explicitly_provided_dests keys a flat option-string to dest map, so the collision resolved to whichever action the walk visited last -- scan --config-file c.yaml --control-server URL --control-identifier ID stopped registering control_identifier as explicit and let the config file's control_servers override the command line. Keep --machine-id only. --control-identifier is already deprecated in favour of it, and guard install could never warn about the spelling because its dest is machine_id. explicitly_provided_dests goes back to walking every action, and a test over the real parser now asserts no option string maps to two dests.
The `--` break and its test are unrelated to guard server discovery. The argv scan mistaking a post-`--` positional for a flag predates this branch, so the fix belongs on its own change against main rather than riding along here.
Reverts wording churn and an invariant note that duplicated the enforcing test's own docstring. The function now matches main verbatim.
The install failure path snapshotted each hook script's bytes and mode before the copy and wrote them back when the test event failed, covering scripts that merely got overwritten. main only deletes a script the install had just created, so this widened the abort contract well past the PR's scope. Back to main's rule, applied to both scripts: a newly created forwarder or discovery trampoline is unlinked on abort, a pre-existing one is left alone. Removes _HookScriptBackup, _snapshot_hook_script, _restore_hook_script and _hook_script_path, whose only purpose was the snapshot.
The Python delivery path claimed to be snyk-agent-guard.sh/.ps1, so backend telemetry could not tell shell-script traffic apart from in-process sends. Identify the actual sender instead.
_agent_scan_bin guessed the CLI location from sys.frozen, argv[0] and the interpreter's sibling console script when AGENT_SCAN_BIN was unset. The caller supplies the path instead, so the guessing is gone: the value is read from the environment at install time and baked into the hook command as before, and an unset variable leaves it out so the trampolines fall back to PATH.
The four command builders were a platform x variant cross-product, each re-implementing the same platform logic, and _invoke_hook_script described the same contract a fifth time as an argv list plus env dict with no shared code. Adding a field meant four coordinated edits and a fifth in a subprocess path; tenant_id had already drifted, accepted by all four builders but emitted by one. A single _HookInvocation now holds the raw values and three renderers turn it into a POSIX shell string, a PowerShell string, or an argv/env pair. Skipping empty optional fields is what lets one code path reproduce both variants, so the strings written into agent config files are unchanged: verified byte-identical across 5832 input combinations covering both platforms and embedded apostrophes. That matters because the emitted command is parsed back by _DETECTION_RE, _extract_env_from_cmd and the push-key redaction patterns, all of which depend on argument order and single-quoting. Two per-variant quirks stay encoded in the spec rather than normalised: the main forwarder leaves --client unquoted where discovery quotes it, and TENANT_ID is emitted only on the POSIX main path, where _parse_command_info reads it back as install-time metadata. _build_discover_hook_command_powershell is gone. Routing the Windows discovery path straight at the shared renderer left it unreferenced, and its output is reproduced exactly by the surviving path. _build_hook_command_powershell stays, since the live subprocess round-trip calls it directly on any platform. Tests pin the exact output of every builder per platform, so a reordering that _DETECTION_RE still happens to match can no longer slip through, and cover the discovery-shaped argv on both platforms.
An unset variable leaves the value out of the hook command so the trampolines fall back to PATH, and a bare executable name resolves through PATH even when the variable is set, so the trampoline does not bypass the lookup.
_uninstall_hooks took a missing_label argument used only in the "Nothing to uninstall" message. path.name yields the same string for every caller and is accurate for managed and --file paths, where the hardcoded label named a file that was never checked.
The posix _render_argv tests force IS_WINDOWS=False but built the script path with Path(), whose flavour follows the host. On the Windows runner that stringifies with backslashes, so the hardcoded POSIX expectation failed. Compare against str(script_path) instead, matching the sibling Windows test.
PR Summary by QodoReport MCP server discovery through Agent Guard hooks
AI Description
Diagram
High-Level Assessment
Files changed (27)
|
Code Review by Qodo
1. Failed upgrade breaks hooks
|
![qodo-merge-etso[bot]](https://avatars.githubusercontent.com/u/12959162?s=60&v=4){kind=small}
Comment on lines
+402
to
+403
| except Exception: | ||
| pass |
There was a problem hiding this comment.
1. Hook payload errors swallowed 📘 Rule violation ☼ Reliability
_run_discover catches every payload parsing exception and silently continues with empty discovery context. Malformed hook input can therefore produce an apparently valid event with missing target folders and session identity instead of an explicit failure.
Agent Prompt
## Issue description
Hook payload parsing failures are swallowed, so discovery continues with incomplete context.
## Issue Context
Differentiate absent optional input from malformed or unreadable hook input, and report a safe, actionable error without exposing exception internals.
## Fix Focus Areas
- src/agent_scan/guard.py[362-415]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+48
to
+49
| except Exception as error: | ||
| return False, str(error) |
There was a problem hiding this comment.
2. Exception details exposed to users 📘 Rule violation ☼ Reliability
Hook delivery and discovery failures are converted with str(error) and printed directly to CLI users. Transport, subprocess, filesystem, or runtime exceptions may reveal internal paths, host details, or other implementation information instead of a generic message.
Agent Prompt
## Issue description
Raw exception strings flow into user-visible guard failure messages.
## Issue Context
Keep detailed exceptions in internal logs with safe context, while returning stable generic messages to CLI callers.
## Fix Focus Areas
- src/agent_scan/hook_events.py[32-50]
- src/agent_scan/guard.py[1228-1243]
- src/agent_scan/guard.py[1289-1301]
- src/agent_scan/guard.py[1324-1346]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+352
to
+356
| thread = threading.Thread(target=run, daemon=True) | ||
| thread.start() | ||
| thread.join(timeout) | ||
| if thread.is_alive(): | ||
| raise TimeoutError(f"timed out after {timeout:g}s") |
There was a problem hiding this comment.
3. Discovery worker never drains 📘 Rule violation ☼ Reliability
_run_with_timeout launches discovery on a daemon thread and deliberately abandons it after the deadline. Timed-out filesystem and subprocess work can continue in the background without cancellation or draining until the process exits.
Agent Prompt
## Issue description
Timed-out discovery workers are abandoned as live daemon threads.
## Issue Context
Use cancellable operations or managed worker processes and ensure timed-out work is terminated and joined before shutdown.
## Fix Focus Areas
- src/agent_scan/guard.py[333-359]
- src/agent_scan/guard.py[1180-1201]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+530
to
+531
| main_script = _copy_hook_script(_forwarder_script_path(config_path)) | ||
| discover_script = _copy_hook_script(discover_script_path) if install_discovery else None |
There was a problem hiding this comment.
5. Failed upgrade breaks hooks 🐞 Bug ☼ Reliability
_install_hooks overwrites an existing forwarding script before the test event, but on test failure only deletes newly created scripts and never restores overwritten content. During an upgrade from the previous hook, the retained config does not supply MACHINE_ID while the newly copied script now requires it, so a transient test-event failure leaves every existing Guard hook failing.
Agent Prompt
## Issue description
A failed Guard upgrade leaves overwritten hook scripts in place while retaining the old config, which is incompatible with the new `MACHINE_ID` requirement.
## Issue Context
Capture existing script contents or defer replacement until validation succeeds. On every failure path, restore pre-existing forwarding and discovery scripts, while still removing scripts that were newly created.
## Fix Focus Areas
- src/agent_scan/guard.py[527-585]
- src/agent_scan/hooks/snyk-agent-guard.sh[88-91]
- src/agent_scan/hooks/snyk-agent-guard.ps1[49-56]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+995
to
+999
| guard_discover_parser.add_argument( | ||
| "--scope", | ||
| choices=[scope.value for scope in DiscoveryScope], | ||
| default=DiscoveryScope.ALL.value, | ||
| help="Discovery data to collect (default: all)", |
There was a problem hiding this comment.
6. Skills scope drops results 🐞 Bug ≡ Correctness
The new guard discover --scope skills option disables server discovery and discovers skills, but _servers_discovered_entries serializes only mcp_configs and always leaves skills empty. The command therefore reports success while sending none of the data requested by the advertised skills or skill-bearing all scopes.
Agent Prompt
## Issue description
`guard discover` accepts `skills` and `all` scopes but drops discovered `skills_dirs` when constructing the event payload.
## Issue Context
Use the existing skill inspection/serialization path so skill results and skill discovery errors populate each `InspectedPath.skills`, or restrict the command to the only supported `servers` scope if skill delivery is not part of the event contract.
## Fix Focus Areas
- src/agent_scan/cli.py[995-999]
- src/agent_scan/guard.py[1142-1201]
- src/agent_scan/guard.py[1304-1339]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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.
No description provided.