Skip to content

feat(zerocommands): add typed MCP, hooks, and plugins snapshots - #90

Merged
Vasanthdev2004 merged 2 commits into
mainfrom
feat/zerocommands-mcp-hooks-plugins-snapshots
Jun 6, 2026
Merged

feat(zerocommands): add typed MCP, hooks, and plugins snapshots#90
Vasanthdev2004 merged 2 commits into
mainfrom
feat/zerocommands-mcp-hooks-plugins-snapshots

Conversation

@gnanam1990

@gnanam1990 gnanam1990 commented Jun 6, 2026

Copy link
Copy Markdown
Collaborator

Summary

The merged permission and verification event contracts added typed snapshots for config, providers, models, sessions, and the sandbox grant store, but the MCP server config, hooks config, and loaded plugin manifests still expose only the raw backend structs. The TUI render path, the headless zero mcp, zero hooks, and zero plugins commands, and PR/CI automation all need typed snapshots so they do not hand-roll map[string]any payloads and do not accidentally leak the secret material that lives in MCP server Env and Headers.

This commit adds three typed snapshot constructors plus a bundled backend lifecycle snapshot to internal/zerocommands.

MCPServerSnapshot

  • name, type, identity, url, command are copied verbatim and trimmed. They are non-secret metadata the operator needs to see when triaging an MCP failure.
  • argCount, envKeyCount, headerCount replace the slices and maps with counts. Secret material in Env and Headers is never serialized.
  • toolCount, allowGranted, denyGranted are optional runtime counts that the snapshot can carry when the caller has a live tool registry and a live permission store. A nil counts struct leaves the snapshot at zero.

HookSnapshot

  • id, name, event, matcher, command, args, enabled, source.
  • command and args are run through the standard redaction pipeline. Whitespace-only args are dropped so JSON output is tight.

PluginSnapshot

  • id, name, version, description, enabled, source, root, pluginDir, manifestPath are copied verbatim and trimmed so the operator can see where the manifest came from.
  • toolCount, promptCount, skillCount, hookCount replace the full extension slices with counts so the headless JSON output stays small.

BackendLifecycleSnapshot

  • Bundles the three slices into a single payload so zero doctor and zero config can return one typed result that describes the full extensibility surface.
  • All three inner slices are always non-nil, so JSON output is always [] and never null.

Tests

  • TestMCPServerSnapshotFromServerStripsSecretsAndCountsMaps: verifies env and header values never appear in the JSON output, and that the count fields record the maps' sizes.
  • TestMCPServerSnapshotWithCountsMergesRuntimeCounts: verifies the optional counts bundle is merged and that a nil bundle leaves the snapshot at zero.
  • TestMCPServerSnapshotsSortsAndReturnsEmptySliceForEmptyInput: verifies alphabetical sort and that nil input returns a non-nil empty slice whose JSON encoding is [].
  • TestHookSnapshotFromDefinitionRedactsCommandAndTrimsFields: verifies command and args are redacted, and the surrounding fields are trimmed.
  • TestHookSnapshotsSortsByIDAndEvent: verifies the deterministic (id, event) ordering.
  • TestHookSnapshotsWithSourceTagsEverySnapshot: verifies the helper that tags every snapshot with the same source string.
  • TestPluginSnapshotFromPluginCollapsesSlicesToCounts: verifies the path fields are preserved verbatim and the count fields record the slices' sizes.
  • TestPluginSnapshotsSortsByIDAndReturnsEmptySliceForEmptyInput: verifies id-based sort and empty-slice stability.
  • TestNewBackendLifecycleSnapshotBundlesAndDefaultsNilsToEmpty: verifies the bundled payload, that nil inputs produce empty slices in the JSON, and that a populated input round-trips.

Verification

  • go build ./...
  • go vet ./...
  • go test -count=1 -p 1 ./internal/zerocommands/... ./internal/mcp/... ./internal/hooks/... ./internal/plugins/... ./internal/redaction/...

DRI

Gnanam (runtime core owner per docs/WORK_SPLIT_PRD.md §4). Closes the typed-snapshot gap for the three backend surfaces in §7.E that the TUI render path, the headless commands, and PR/CI automation consume.

Out of scope

This PR only adds the typed contracts and the constructors. The TUI render path, the zero mcp / zero hooks / zero plugins headless commands, and the PR/CI automation consumer are follow-up work in their respective owners' lanes.

Summary by CodeRabbit

  • New Features
    • Structured backend snapshots for servers, hooks, and plugins with trimmed fields, sensitive-value redaction, deterministic ordering, plugin content summarized as counts, optional server permission totals, and hook snapshots tagged with their source.
  • Bug Fixes
    • Server URLs now strip embedded credentials while preserving unparseable URLs; hook arg redaction preserves positional ordering and omits empty arg lists.
  • Tests
    • Expanded coverage validating trimming, redaction, counts, sorting, URL handling, tagging, and nil defaults.

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Zero automated PR review

Verdict: No blockers found

Blockers

  • None found.

Validation

  • [pass] Diff hygiene: git diff --check
  • [pass] Typecheck: bun run typecheck
  • [pass] Tests: bun run test
  • [pass] Build: bun run build
  • [pass] Smoke build: bun run smoke:build

Scope

Head: 08cf126fe4e7
Changed files (2): internal/zerocommands/backend_snapshots.go, internal/zerocommands/backend_snapshots_test.go

This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality.

@coderabbitai

coderabbitai Bot commented Jun 6, 2026

Copy link
Copy Markdown

Wondering what really moved? Review this PR in Change Stack to inspect semantic changes, definitions, and references.

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: d0c87f32-3df6-41d7-8b7d-d5c746e3c2e6

📥 Commits

Reviewing files that changed from the base of the PR and between 1440273 and 08cf126.

📒 Files selected for processing (2)
  • internal/zerocommands/backend_snapshots.go
  • internal/zerocommands/backend_snapshots_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • internal/zerocommands/backend_snapshots.go

Walkthrough

Transforms runtime MCP servers, hook definitions, and loaded plugins into deterministically ordered, trimmed, redacted, count-oriented JSON snapshots and bundles them into a nil-safe BackendLifecycleSnapshot.

Changes

Backend Snapshot Transformations

Layer / File(s) Summary
Snapshot data models and imports
internal/zerocommands/backend_snapshots.go
Introduces snapshot types for MCP servers, hooks, plugins, and a combined lifecycle bundle with JSON field shapes and non-nil slice expectations.
MCP server snapshot conversion
internal/zerocommands/backend_snapshots.go (lines 90–154), internal/zerocommands/backend_snapshots_test.go (lines 1–116)
Trims server fields, strips embedded URL credentials, converts maps/lists to count fields, overlays optional runtime counts, and sorts snapshots by Name. Tests validate redaction, trimming, counts merging, deterministic sorting, and nil-safe empty slice JSON marshaling.
Hook snapshot conversion with redaction
internal/zerocommands/backend_snapshots.go (lines 155–202), internal/zerocommands/backend_snapshots_test.go (lines 118–184)
Maps hook definitions to snapshots with trimmed identifiers, redacted command text and args (positional order preserved; empty arg slices omitted), source tagging, and deterministic sorting by ID then Event. Tests cover trimming, redaction behavior, positional preservation, sorting, and per-snapshot source tagging.
Plugin snapshot conversion
internal/zerocommands/backend_snapshots.go (lines 203–242), internal/zerocommands/backend_snapshots_test.go (lines 268–336)
Trims plugin metadata, preserves manifest/path fields, collapses extension slices into count fields (tools/prompts/skills/hooks), and sorts by plugin ID. Tests validate trimming, count collapsing, sorting, and nil-safe empty slice marshaling.
Lifecycle bundling and utilities
internal/zerocommands/backend_snapshots.go (lines 243–301), internal/zerocommands/backend_snapshots_test.go (lines 338–365)
Bundles MCP/hook/plugin snapshots into BackendLifecycleSnapshot with nil-to-empty defaults; adds redactStringSlice and stripURLCredentialsFromURL utilities (trimming, redaction, and tolerant URL userinfo stripping). Tests verify bundling, defaults, redaction trimming, and URL sanitization behavior.

Suggested reviewers

  • anandh8x
  • Vasanthdev2004

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 44.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title clearly and directly describes the main change: adding typed snapshot models (MCP, hooks, and plugins) to the zerocommands package.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/zerocommands-mcp-hooks-plugins-snapshots

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
internal/zerocommands/backend_snapshots_test.go (1)

118-154: 💤 Low value

Consider testing that redacted args are preserved, not dropped.

The test verifies that secrets don't leak (lines 149-153), but doesn't verify that the arg count is preserved. If the implementation drops fully-redacted args (as discussed in my earlier comment on redactStringSlice), this test wouldn't catch it.

Consider adding an assertion:

if len(snapshot.Args) != len(def.Args) {
	t.Fatalf("expected %d args after redaction, got %d", len(def.Args), len(snapshot.Args))
}

Note: This assumes redacted args should be preserved. If the design intentionally drops them, this suggestion can be ignored.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/zerocommands/backend_snapshots_test.go` around lines 118 - 154, The
test TestHookSnapshotFromDefinitionRedactsCommandAndTrimsFields currently checks
that secrets are redacted but not that arg counts are preserved; update the test
to assert len(snapshot.Args) == len(def.Args) to ensure redacted args are kept
rather than dropped (this targets behavior in HookSnapshotFromDefinition which
uses redactStringSlice); if redactStringSlice is meant to preserve
fully-redacted entries, add the length assertion in the test so regressions are
caught, otherwise document the intended behavior and adjust the test
accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/zerocommands/backend_snapshots_test.go`:
- Around line 13-67: Add a new unit test
TestMCPServerSnapshotStripsURLCredentials that creates an mcp.Server with URL
"https://admin:secret123@api.example.com/v1", calls MCPServerSnapshotFromServer,
and asserts the returned snapshot.URL does not contain "admin" or "secret123"
and equals "https://api.example.com/v1"; if that fails the test should t.Fatalf
with the URL. Also update MCPServerSnapshotFromServer to sanitize server.URL by
parsing it with net/url, removing/clearing the User (userinfo) before formatting
(e.g., u.User = nil or u.User = url.UserPassword("", "") then u.String()), then
TrimSpace and set snapshot.URL so credentials never leak into serialized
snapshots.

In `@internal/zerocommands/backend_snapshots.go`:
- Around line 90-104: MCPServerSnapshotFromServer currently preserves server.URL
after trimming, which can leak embedded credentials; parse the trimmed URL (use
net/url.Parse) inside MCPServerSnapshotFromServer, clear any User info
(username/password) from the parsed URL, then assign the sanitized URL string
(e.g., parsedURL.Scheme + "://" + host + path + (rawquery/fragment as needed) or
parsedURL.String() after removing User) to the URL field of the returned
MCPServerSnapshot so snapshots never include embedded credentials.
- Around line 251-270: redactStringSlice currently trims and drops elements when
redaction returns "" which causes intentionally-empty or whitespace-only args
and fully-redacted non-empty args to vanish; update redactStringSlice so it
preserves the position of each input element: for each value compute trimmed :=
strings.TrimSpace(value) and if trimmed=="" append an empty string to out (to
preserve whitespace-only inputs), otherwise compute redacted :=
redaction.RedactString(trimmed, redaction.Options{}) and append redacted to out
regardless of whether it is empty (so fully-redacted non-empty args don't get
dropped); keep the initial nil-on-empty-input behavior (return nil if
len(values)==0) but do not filter out elements based solely on redacted == "".

---

Nitpick comments:
In `@internal/zerocommands/backend_snapshots_test.go`:
- Around line 118-154: The test
TestHookSnapshotFromDefinitionRedactsCommandAndTrimsFields currently checks that
secrets are redacted but not that arg counts are preserved; update the test to
assert len(snapshot.Args) == len(def.Args) to ensure redacted args are kept
rather than dropped (this targets behavior in HookSnapshotFromDefinition which
uses redactStringSlice); if redactStringSlice is meant to preserve
fully-redacted entries, add the length assertion in the test so regressions are
caught, otherwise document the intended behavior and adjust the test
accordingly.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 0734b93b-9de8-4d86-b650-34bd1aa450e7

📥 Commits

Reviewing files that changed from the base of the PR and between b70fea7 and 1440273.

📒 Files selected for processing (2)
  • internal/zerocommands/backend_snapshots.go
  • internal/zerocommands/backend_snapshots_test.go

Comment thread internal/zerocommands/backend_snapshots_test.go
Comment thread internal/zerocommands/backend_snapshots.go
Comment thread internal/zerocommands/backend_snapshots.go
Gnanam added 2 commits June 6, 2026 13:41
The merged permission and verification event contracts added typed
snapshots for config, providers, models, sessions, and the sandbox
grant store, but the MCP server config, hooks config, and loaded
plugin manifests still expose only the raw backend structs. The TUI
render path, the headless 'zero mcp', 'zero hooks', and
'zero plugins' commands, and PR/CI automation all need typed
snapshots so they do not hand-roll map[string]any payloads and do
not accidentally leak the secret material that lives in MCP
server Env and Headers.

This commit adds three typed snapshot constructors plus a bundled
backend lifecycle snapshot to internal/zerocommands.

MCPServerSnapshot
  - name, type, identity, url, command are copied verbatim and
    trimmed. They are non-secret metadata the operator needs to
    see when triaging an MCP failure.
  - argCount, envKeyCount, headerCount replace the slices and maps
    with counts. Secret material in Env and Headers is never
    serialized.
  - toolCount, allowGranted, denyGranted are optional runtime
    counts that the snapshot can carry when the caller has a live
    tool registry and a live permission store. A nil counts
    struct leaves the snapshot at zero.

HookSnapshot
  - id, name, event, matcher, command, args, enabled, source.
  - command and args are run through the standard redaction
    pipeline. Whitespace-only args are dropped so JSON output is
    tight.

PluginSnapshot
  - id, name, version, description, enabled, source, root,
    pluginDir, manifestPath are copied verbatim and trimmed so the
    operator can see where the manifest came from.
  - toolCount, promptCount, skillCount, hookCount replace the
    full extension slices with counts so the headless JSON output
    stays small.

BackendLifecycleSnapshot
  - bundles the three slices into a single payload so 'zero
    doctor' and 'zero config' can return one typed result that
    describes the full extensibility surface.
  - all three inner slices are always non-nil, so JSON output is
    always '[]' and never 'null'.

Tests
  - TestMCPServerSnapshotFromServerStripsSecretsAndCountsMaps:
    verifies env and header values never appear in the JSON
    output, and that the count fields record the maps' sizes.
  - TestMCPServerSnapshotWithCountsMergesRuntimeCounts: verifies
    the optional counts bundle is merged and that a nil bundle
    leaves the snapshot at zero.
  - TestMCPServerSnapshotsSortsAndReturnsEmptySliceForEmptyInput:
    verifies alphabetical sort and that nil input returns a
    non-nil empty slice whose JSON encoding is '[]'.
  - TestHookSnapshotFromDefinitionRedactsCommandAndTrimsFields:
    verifies command and args are redacted, and the surrounding
    fields are trimmed.
  - TestHookSnapshotsSortsByIDAndEvent: verifies the deterministic
    (id, event) ordering.
  - TestHookSnapshotsWithSourceTagsEverySnapshot: verifies the
    helper that tags every snapshot with the same source string.
  - TestPluginSnapshotFromPluginCollapsesSlicesToCounts: verifies
    the path fields are preserved verbatim and the count fields
    record the slices' sizes.
  - TestPluginSnapshotsSortsByIDAndReturnsEmptySliceForEmptyInput:
    verifies id-based sort and empty-slice stability.
  - TestNewBackendLifecycleSnapshotBundlesAndDefaultsNilsToEmpty:
    verifies the bundled payload, that nil inputs produce empty
    slices in the JSON, and that a populated input round-trips.

Verification
  - go build ./...
  - go vet ./...
  - go test -count=1 -p 1 ./internal/zerocommands/... \
           ./internal/mcp/... ./internal/hooks/... \
           ./internal/plugins/... ./internal/redaction/...

DRI: Gnanam (runtime core owner per WORK_SPLIT_PRD.md §4). Closes
the typed-snapshot gap for the three backend surfaces in §7.E
that the TUI render path, the headless commands, and PR/CI
automation consume.
…sition

Addresses the three actionable review items left on PR #90 by
CodeRabbit.

1. MCPServerSnapshotFromServer now strips userinfo from the URL
   field so credentials embedded in an MCP server URL
   (https://user:token@host) never reach the headless JSON output.
   The new stripURLCredentialsFromURL helper uses net/url.Parse to
   remove the User field, then returns the sanitized URL. A URL
   that fails to parse is returned trimmed (not empty) so the
   operator still sees the configured endpoint when triaging a
   malformed MCP configuration. A blank input returns blank.

2. redactStringSlice now preserves position. Previously the helper
   appended to the output slice and skipped any element whose
   redacted value was empty, which meant a fully-redacted secret
   in the middle of a hook command line would shift every
   following argument by one position. The helper now allocates
   the output slice to the input length and assigns each element
   by index, so a fully-redacted element becomes an empty string
   in the output rather than disappearing. Whitespace-only inputs
   also become empty strings by index, which matches the previous
   drop behaviour but without shifting positions. A nil or
   empty input still returns nil so the JSON output omits the
   field entirely.

3. Test coverage is extended for both changes:
   - TestHookSnapshotFromDefinitionRedactsCommandAndTrimsFields
     now asserts len(snapshot.Args) == len(def.Args) so a future
     regression that drops or shifts redacted args is caught.
   - TestHookSnapshotFromDefinitionPreservesPositionForRedactedArgs
     verifies that a secret in the middle of an arg list ends up
     as an empty-string element at the same index, while the
     surrounding non-secret args round-trip verbatim.
   - TestMCPServerSnapshotStripsURLCredentials verifies that an
     https://admin:secret123@host URL becomes the credential-free
     form and that the username, password, and the '@' separator
     do not appear anywhere in the snapshot.
   - TestMCPServerSnapshotPreservesURLWithoutCredentials verifies
     that a URL without userinfo passes through unchanged.
   - TestMCPServerSnapshotKeepsUnparseableURLInsteadOfEmpty
     verifies the tolerance contract: a malformed URL is returned
     trimmed rather than empty, so the operator still sees the
     configured endpoint when triaging a broken configuration.

Verification
  - go build ./...
  - go vet ./...
  - go test -count=1 -p 1 ./internal/zerocommands/... \
           ./internal/mcp/... ./internal/hooks/... \
           ./internal/plugins/... ./internal/redaction/...

DRI: Gnanam.
@gnanam1990
gnanam1990 force-pushed the feat/zerocommands-mcp-hooks-plugins-snapshots branch from 1440273 to 08cf126 Compare June 6, 2026 08:15
@gnanam1990

Copy link
Copy Markdown
Collaborator Author

Addressed the three actionable review items from CodeRabbit:

  1. URL credential leak in MCPServerSnapshot.URL — fixed. Added stripURLCredentialsFromURL that parses the URL with net/url and clears the User field. The helper is tolerant: a URL that fails to parse is returned trimmed (not empty) so the operator still sees the configured endpoint when triaging a broken configuration.

  2. redactStringSlice dropped fully-redacted elements — fixed. The helper now preserves position: it pre-allocates the output slice to the input length and assigns each element by index. A fully-redacted element becomes an empty string in the output rather than disappearing, and whitespace-only inputs become empty strings by index. A nil or empty input still returns nil so the JSON output omits the field entirely.

  3. Arg-length preservation test coverage — added. TestHookSnapshotFromDefinitionRedactsCommandAndTrimsFields now asserts len(snapshot.Args) == len(def.Args). Two new tests cover the URL sanitization: TestMCPServerSnapshotStripsURLCredentials verifies the username, password, and @ separator do not appear anywhere in the snapshot, and TestMCPServerSnapshotKeepsUnparseableURLInsteadOfEmpty verifies the malformed-URL tolerance contract.

Branch rebased onto current main (post-#89 merge) before the fixes landed. go build, go vet, and the affected test packages all pass. Ready for re-review.

@anandh8x anandh8x left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's good

  • The internal/zerocommands package is the right home for typed snapshot contracts. It already hosts the config, provider, model, and session snapshots (contracts.go), and adding the three backend snapshots here keeps the surface consistent. The package is consumer-only: it converts backend structs into stable, trimmed, redacted, sorted views. No runtime behavior changes, and no consumer is forced to migrate in this PR.
  • MCP Env and Headers are correctly reduced to counts. EnvKeyCount and HeaderCount preserve the operator's diagnostic value (the size of the secret-bearing maps) without ever serializing the values. TestMCPServerSnapshotFromServerStripsSecretsAndCountsMaps exercises a realistic MCP_AUTH_TOKEN and Authorization: Bearer ... case and asserts that neither the secret nor the literal "Bearer" nor the key name appears in the JSON. That is the right contract for a snapshot that lands in TUI, headless JSON, audit log, and PR/CI automation.
  • MCPServerSnapshotWithCounts is a thoughtful addition. The base snapshot records config-time fields; the WithCounts variant lets a caller that has a live tools.Registry and a live mcp.PermissionStore attach ToolCount, AllowGranted, DenyGranted so the headless zero mcp command can answer both "what is configured?" and "what is live?" in one typed payload. The nil counts contract is explicit and tested.
  • Stable ordering across all three snapshot kinds. MCPServerSnapshots sorts by Name; HookSnapshots sorts by (ID, Event) so duplicate-ID hooks get a deterministic event order; PluginSnapshots sorts by ID. sort.SliceStable ensures the input order is the tiebreak, so two snapshots with the same key are not randomly swapped.
  • Empty-slice stability is enforced and tested. Every FooSnapshots(nil) returns a non-nil empty slice whose JSON encoding is exactly []. The test TestMCPServerSnapshotsSortsAndReturnsEmptySliceForEmptyInput and TestPluginSnapshotsSortsByIDAndReturnsEmptySliceForEmptyInput both assert string(encoded) != "[]" would fail. That is the right invariant for a JSON consumer that expects arrays, not null.
  • Hook command and args go through the standard redaction pipeline. HookSnapshotFromDefinition runs def.Command and every entry of def.Args through redaction.RedactString, and the test feeds sk-proj-... through an arg to confirm the snapshot does not carry the secret. That is consistent with how other CLI/audit surfaces treat hook command lines.
  • HookSnapshotsWithSource is a small but useful escape hatch. A caller that has a single hooks.LoadResult can tag every snapshot with the same ConfigSource in one pass instead of mapping over the result and threading the source through. The helper is tiny and the test pins the contract.
  • BackendLifecycleSnapshot is the right shape for the zero doctor and zero config consumers. One payload that describes the full extensibility surface (MCP servers, hooks, plugins) with non-nil slices is much easier to render than three separate snapshots. The bundled JSON shape is asserted by TestNewBackendLifecycleSnapshotBundlesAndDefaultsNilsToEmpty.
  • Plugin path fields are preserved verbatim. Root, PluginDir, and ManifestPath are the operator's primary tool for triaging a load failure (e.g., "did the manifest come from the user config or a project-local override?"). Trimming is applied, but the path itself is copied.
  • Slices and maps are not aliased. The snapshot constructors copy via MCPServerSnapshots and PluginSnapshots; HookSnapshots builds a new slice; redactStringSlice returns a fresh slice. A consumer that mutates the snapshot cannot accidentally mutate the underlying mcp.Server / hooks.Definition / plugins.LoadedPlugin.

Observations (non-blocking)

  1. MCPServerSnapshot.URL is preserved verbatim, including any embedded userinfo. A configured mcp.Server of the form {"url": "https://admin:secret123@api.example.com/v1"} will serialize as "url": "https://admin:secret123@api.example.com/v1" in the snapshot. The comment block on MCPServerSnapshot says "Env and Headers never leak" but does not address URL credentials. The fix is small: parse the URL with net/url, clear u.User, then re-encode. A unit test that constructs a server with a credentialed URL and asserts the snapshot URL contains no : after https:// would lock the contract. CodeRabbit's review surfaced this; it is worth a follow-up commit, but the slice as designed is not regressing anything that was not already in the mcp.Server struct.

  2. MCPServerSnapshot.ArgCount is a regression in triage usefulness compared to the source struct. The doc comment on MCPServerSnapshot says "Command arguments are preserved because the tool surface (path, flags) is the part a maintainer needs to see when triaging an MCP failure", but the actual struct has ArgCount int and no Args field. The PR description is consistent with the struct ("argCount, envKeyCount, headerCount replace the slices and maps with counts"). The doc comment on the type is misleading. Either add Args []string to the struct (redacted, with whitespace-only entries dropped) and update the test, or fix the comment to say "args are reduced to a count to keep the snapshot small; use the headless zero mcp command with --verbose to see the full arg list".

  3. redactStringSlice drops empty results, which loses positional fidelity. If an arg like (whitespace only) or a fully-redacted secret is passed in, the returned slice has one fewer element. For a hook command of ["sh", "-c", "auth token sk-proj-...", "---", "next"], the redacted output is ["sh", "-c", "---", "next"] (the secret is replaced with [REDACTED], not dropped, so this specific case is fine). But the helper also drops out = append(out, redacted) when redacted == "" even if the input was non-empty. A future caller that relies on arg position will be surprised. Two options: (a) preserve the position by always appending redacted (even when empty), or (b) document the dropping behavior in the helper's doc comment so the next caller knows. Right now the behavior is silent.

  4. redactStringSlice is not unit-tested directly. The hook snapshot test covers a happy-path arg with a secret, but it does not cover the all-redacted case (where Args would be nil because every element was dropped). A focused test would lock in the behavior and make observation 3 actionable.

  5. MCPServerSnapshots sorts by Name. If two servers share a name, the order between them is determined by input order (via sort.SliceStable). That is fine for a config that does not allow duplicate names, but mcp.NormalizeConfig could in principle produce duplicates. A follow-up test that feeds two servers with the same name would confirm the stable-tiebreak behavior, or a sort.Slice with a secondary key on the original index would make the order fully deterministic regardless of input.

  6. MCPServerSnapshotWithCounts is a good pattern but the live path is not exercised. The test only constructs a counts struct directly. A future zero mcp --json consumer should be the place where the registry + permission store are wired in, and that consumer should add its own integration test. The slice as designed is the right contract for that consumer.

  7. PluginSnapshot.Root, PluginDir, ManifestPath may contain the user's home directory. This is arguably PII, but the source plugins.LoadedPlugin already carries these strings, so the snapshot is no leakier than the loaded plugin. A future redaction pass that replaces $HOME with ~ would help when the snapshot is sent to a PR comment or audit log. Not blocking; the snapshot is consistent with the source.

  8. HookSnapshot.Matcher is trimmed but not redacted. A matcher like write_file.* --token=sk-proj-... would have the secret in the matcher. Right now Matcher is a free-form string from the user's config; the snapshot copies it verbatim. If the matcher can carry secrets, the snapshot should redact it. A test that feeds a matcher with a secret would surface the gap.

  9. No tests for redactStringSlice ordering with mixed secrets. A test like redactStringSlice([]string{"", "-c", " "}) would assert whether the helper preserves position or drops. Locking that in would make observation 3 a one-line fix.

  10. The package has no top-level doc.go or package comment. Other packages (internal/sandbox, internal/zerocommands itself via contracts.go) carry a brief package comment explaining the pattern. A one-liner like // Package zerocommands defines typed snapshot contracts for the headless, TUI, audit, and PR/CI consumers. The constructors in this package convert backend structs into stable, trimmed, redacted, sorted views and are the only sanctioned way for those consumers to render backend state. would help new readers.

  11. Cross-PR consistency with PR #92. PR #92 adds the sandbox snapshot constructors in the same package; the contract here uses redactStringSlice while PR #92 uses inline redaction.RedactString in HookSnapshotFromDefinition. The two patterns are equivalent but slightly different (slice-aware vs. string-only). If the team wants one helper for the pattern, redactStringSlice could be exported as RedactStringSlice and reused in #92's SandboxViolationSnapshot (or wherever a list of strings needs redaction). Cosmetic.

  12. BackendLifecycleSnapshot is never nil but never tested for a populated case where the slices share items with the input. The bundled test only checks that one of each kind round-trips, not that the input slices are not aliased. A test that mutates the input slice after construction and asserts the snapshot slice is unchanged would lock the no-aliasing contract. Same applies to the per-type FooSnapshots helpers.

  13. The MCPServerSnapshot.Identity field is new relative to what other surfaces expose. It is a per-server identity string from mcp.Server.Identity that the runtime uses for permission keys. Including it in the snapshot is correct (it is non-secret metadata), but no test in this PR asserts what the field is supposed to be. A follow-up test that constructs two servers with different identities and asserts the snapshot preserves them would be cheap.

  14. mcp.Server.URL is JSON-omitted when empty (via ,omitempty on the source struct), so the snapshot's URL field correctly serializes only when set. The snapshot type itself does not have omitempty on URL, which is consistent with the source.

  15. HookSnapshot.Event is string (not hooks.Event). That is a deliberate widening so the snapshot is JSON-stable even when a new event kind is added. The trade-off is that the snapshot accepts any string; a future consumer that wants to validate the event kind would have to map against hooks.Event constants. Acceptable.

  16. PluginSnapshot.Source is string (not plugins.Source). Same widening as Event. Consistent.

  17. The PR description says "Closes the typed-snapshot gap for the three backend surfaces in §7.E that the TUI render path, the headless commands, and PR/CI automation consume." A consumer PR that uses these snapshots is not in this PR. The slice is the contract; follow-up slices are the consumers. That is the right sequencing for typed contracts.

  18. No benchmark. The snapshot helpers are called in render paths (TUI) and on every audit log entry. A small benchmark in backend_snapshots_test.go that times 1000 snapshot conversions would catch a future regression where someone re-aliases the input or adds an O(n) string allocation per call. Not blocking; performance is fine for the current scale.

  19. MCPServerSnapshot.Command is preserved verbatim. For a stdio server, the command (e.g., npx, python3) is the operator's primary tool for understanding the server. For an http server, Command is empty and URL is the operator's tool. The current shape covers both.

  20. mcp.Server.Args for an http server is typically empty. ArgCount will be 0 for an http snapshot, which is correct but slightly noisy. A typed mcp.Server could split into StdioServer and HTTPServer variants in a future PR, but that is out of scope for this slice.

No blockers. The slice is well-scoped, well-tested, and closes the typed-snapshot gap for the MCP, hooks, and plugins surfaces without changing runtime behavior. The three follow-ups I would prioritize next: (1) strip URL userinfo in MCPServerSnapshotFromServer so credentialed URLs never leak, (2) reconcile the doc comment on MCPServerSnapshot with the actual ArgCount field, and (3) add a focused test for redactStringSlice that covers the all-redacted and whitespace-only cases.

@gnanam1990

Copy link
Copy Markdown
Collaborator Author

@CodeRabbit review

@coderabbitai

coderabbitai Bot commented Jun 6, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@Vasanthdev2004 Vasanthdev2004 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blockers

  • None found.

Non-Blocking

  • None.

Looks Good

  • The current head 08cf126fe4e76ba4a7e75ce3a9b261f2e8d6f974 fixes the earlier snapshot safety issues: MCP URLs with embedded userinfo are sanitized before JSON serialization, and hook arg snapshots preserve argument positions while redacting secret-looking values.
  • The MCP/hook/plugin backend snapshots keep the public diagnostic payload typed and compact: env/header values are counted rather than serialized, hook commands/args go through the standard redaction pipeline, plugin extension slices are collapsed to counts, and all snapshot slices stay non-nil for stable JSON output.
  • Deterministic sorting and nil/empty-slice behavior are covered well, including the URL credential and arg-position regressions.

Validation run locally on the PR head and on a latest-main test merge:

  • go test -count=1 ./internal/zerocommands -run 'TestPR90ReviewProbe' -v — passed; verified URL credentials are stripped and hook arg positions are preserved.
  • go test -count=1 ./internal/zerocommands ./internal/mcp ./internal/hooks ./internal/plugins ./internal/redaction — passed.
  • go test -count=1 -p 1 ./... — passed.
  • git diff --check — passed.
  • Latest-main test merge onto b70fea7d6ffd4ef085aef71cb95997ce960d86e9 merged cleanly and repeated the targeted/full Go validation successfully.

@Vasanthdev2004

Copy link
Copy Markdown
Collaborator

@coderabbitai approve

@coderabbitai

coderabbitai Bot commented Jun 6, 2026

Copy link
Copy Markdown
✅ Action performed

Comments resolved and changes approved.

@Vasanthdev2004
Vasanthdev2004 merged commit 1ddbe9d into main Jun 6, 2026
6 checks passed
@Vasanthdev2004
Vasanthdev2004 deleted the feat/zerocommands-mcp-hooks-plugins-snapshots branch June 28, 2026 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants