feat: send discovered servers during guard install - #443
Merged
Conversation
PR Summary by QodoReport discovered MCP servers during Guard install and session start
AI Description
Diagram
High-Level Assessment
Files changed (27)
|
iamcristi
marked this pull request as draft
August 20, 2026 09:51
Code Review by Qodo
1.
|
![qodo-merge-etso[bot]](https://avatars.githubusercontent.com/u/12959162?s=60&v=4){kind=small}
_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.
iamcristi
marked this pull request as ready for review
August 27, 2026 06:18
|
Code review by qodo was updated up to the latest commit 4c32484 |
Contributor
|
Consider omitted protocol in the url, use http:// if omitted. |
eugene-eko2000
approved these changes
Aug 27, 2026
hemang-snyk
approved these changes
Aug 28, 2026
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.
iamcristi
disabled auto-merge
August 28, 2026 07:25
iamcristi
force-pushed
the
feat/guard-serversdiscovered-event
branch
from
August 28, 2026 07:26
70eead6 to
3ef9db1
Compare
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
3 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
Agent Guard now reports the MCP servers it can see at install time and, when
session-start discovery is enabled, at every session start. Hook events identify the
reporting machine by an explicit machine ID rather than by hostname.
Breaking changes
guard installnow requires--machine-id(orMACHINE_ID) and exits 1 withoutit. Existing invocations must be updated.
--control-identifieris not accepted byguard install; it remains available only for the legacy control-server blocks usedby scan, inspect, and evo commands.
X-User.identifieron hook events is now the machine ID, not the hostname. ThePOSIX forwarder requires
MACHINE_ID; the PowerShell forwarder requires either-MachineIdorMACHINE_ID. A forwarder from this branch will therefore not runagainst config written by an earlier version unless a machine ID is supplied
externally.
New behavior
guard discover --client {claude-code,cursor,codex} [--scope {servers,skills,all}], withsnyk-agent-guard-discover.sh/.ps1trampolines and a new
hook_events.pythat POSTs hook events directly from Python(its own User-Agent,
backend_client_session, transport-only retries).guard installsends one best-efforthooksConfiguredServerDiscoveryevent throughthe first installed client's endpoint.
sessionStartServerDiscoveryevent persession (
async: truefor Claude and Codex; Cursor'ssessionStartisfire-and-forget by design). It is installed only when
AGENT_SCAN_COMMANDis setat install time; otherwise install prints a warning and writes no discovery hook.
DiscoveryScope(servers/skills/all) gates both theAgentDiscovererimplementations and the well-known-client path.
target_foldersfrom hook payloads;_project_paths_with_ancestorsbecomes_discovery_paths_with_ancestorsand mergesrecorded project history with the folders named in the request.
Payload / wire changes
hooksConfigured.hooks_scriptgainsdiscover_current_checksumanddiscover_new_checksum.hooksConfiguredServerDiscoveryandsessionStartServerDiscoverypayloads carrydiscovery_duration_ms.snyk-agent-guard.shsends the body via--data-binary @-on stdin instead of on theargv.
Fixes
get_relative_pathhome matching is now boundary-aware, so/home/alicexno longercollapses to
~x; matching is case-insensitive on Windows.scanwithout--skillsskips the skills glob entirely instead of globbing anddiscarding the result.
control characters.
Refactors (no behavior change)
_write_claude/cursor/codex_configshare_write_config;_uninstall_claude/cursor/codexshare_uninstall_hooks; the three client-specificdetection wrappers share
_detect_install;_run_statusis table-driven._HookInvocationspec._analysis_client_sessionis now the publicbackend_client_session;_RETRYABLE_TRANSPORT_EXCEPTIONSis public.managed-settings.json).Verification
Known issues / follow-ups
Open on this branch:
_compute_hooks_diffnormalizes push keys only, so an upgrade that addsMACHINE_ID=to the command reports every event as
modified-- i.e. as customer hook tampering._servers_discovered_entriesnever serializesskills, so--scope skillsposts nodiscovered skill data and the CLI default
allpays for a skills sweep whose resultis discarded.
--scopehas three defaults:allin the CLI,serversin the PowerShell trampoline,and
servershard-coded in the installed hook._build_discover_hook_commandacceptstenant_idand drops it.Pre-existing, surfaced by this branch's refactor:
_render_powershell_commandnever emitsTENANT_ID, so_parse_command_infocannotrecover the tenant on Windows and
guard uninstallnever revokes the push key there.The new
_HookInvocationalready carries the field.Cross-repo:
hooksConfiguredServerDiscoveryandsessionStartServerDiscoveryevent names.of the new discovery-script checksums.
agent-monitor.
has no session-state effects beyond expectation pairing.