Hooks management command + build & supply-chain hardening - #193
Merged
Conversation
go.mod pinned toolchain go1.24.13, whose bundled standard library has 9 govulncheck-reachable CVEs (crypto/tls KeyUpdate DoS, net/http2 loop, crypto/x509 x3, net, net/url, os, net/textproto). CI and release build via go-version-file: go.mod, so shipped binaries inherited them. Bumping the toolchain directive to go1.26.4 clears all nine — govulncheck ./... now reports 0 reachable (minimum that clears them is go1.25.11; go1.26.4 matches the team's local Go). The go directive stays at 1.24.2 (minimum language version). Verified: go build/vet/gofmt clean, full go test ./... green.
WalkthroughGo toolchain upgraded from ChangesToolchain and CI Infrastructure Updates
Hooks Management CLI Feature
Sequence Diagram(s)sequenceDiagram
participant User
participant runHooks
participant Handler
participant ConfigStore
User->>runHooks: zero hooks add/remove/enable/disable
runHooks->>Handler: dispatch to handler function
Handler->>ConfigStore: Upsert/Remove/SetEnabled
ConfigStore->>Handler: result (hook config or removal status)
Handler->>User: JSON or text output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
- ci.yml: new 'security' job runs govulncheck as a hard gate (fails on a reachable vulnerability; passes now that the toolchain is bumped) plus an advisory deadcode step that surfaces dormant code without blocking. - Pin every GitHub Action to a full commit SHA (checkout, upload-artifact, github-script) with a version comment, across all four workflows — previously only setup-go was pinned, leaving the mutable v4/v7 tags as a supply-chain risk.
gnanam1990
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.github/workflows/ci.yml:
- Around line 101-109: The govulncheck and deadcode commands in the CI workflow
are using `@latest` version specifiers, which creates non-deterministic builds and
supply-chain risks. Replace the `@latest` versions with explicit pinned versions:
use govulncheck@v1.3.0 in the govulncheck step and deadcode@v0.45.0 in the
deadcode step. Consider using a locked tool module (such as tools.go) or
environment variables for reproducibility and centralized version management
across your CI pipeline.
- Around line 23-24: Add `persist-credentials: false` to the `with:` section of
each `actions/checkout` step to prevent credential persistence in local git
config. This change is needed at three locations in .github/workflows/ci.yml:
the smoke job at lines 23-24, the performance job at lines 61-62, and the
security job at lines 88-89. For each `actions/checkout` step, add a new line
`persist-credentials: false` under the `with:` configuration block.
🪄 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: 5d90afdd-6614-4fcc-ad15-55f6920c76c5
📒 Files selected for processing (4)
.github/workflows/ci.yml.github/workflows/pr-auto-review.yml.github/workflows/release-artifacts.yml.github/workflows/zero-action-smoke.yml
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/zero-action-smoke.yml
Wires the previously-dormant hooks.ConfigStore (10 unreachable funcs -> 0) into the CLI: zero hooks add <id> --event <event> --command <cmd> [--name --description --matcher --arg --user --json] zero hooks remove|enable|disable <id> [--user --json] Writes the project hook config by default (<cwd>/.zero/hooks.json) or the user config with --user, reusing the store's locking + atomic writes + validation (normalizeDefinition). New hooks are enabled; state is managed via enable/disable. JSON output is secret-scrubbed via redaction. Mirrors the existing 'zero mcp add/remove/enable/disable' shape. Covered by add/remove/toggle round-trip, validation, unknown-event, and JSON-redaction tests.
gnanam1990
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/cli/hooks_manage_test.go (1)
77-85: ⚡ Quick winTighten the unknown-event test to assert usage-path behavior explicitly.
This currently accepts any non-success exit, so a crash-path regression would still pass. Please assert the specific usage-error contract (exit code and/or usage-style stderr message) for invalid
--eventinputs.🤖 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/cli/hooks_manage_test.go` around lines 77 - 85, The test TestRunHooksAddRejectsUnknownEvent currently only verifies that runHooksAdd returns a non-success exit code when given an invalid event, which would pass even if the command crashes instead of properly reporting a usage error. Strengthen this test by explicitly asserting the specific usage-error contract: check that the exit code matches the expected usage-error exit code (not just any non-success code) and verify that the stderr output contains a usage-style error message (such as text indicating invalid event or similar usage guidance) that confirms this is a proper usage error rather than an unexpected crash or other failure.
🤖 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/cli/hooks_manage.go`:
- Around line 295-300: The validation for `options.def.Event` in the argument
parsing section only checks for non-empty values but does not validate that the
event is one of the allowed/valid event types. Add validation in the same block
where the empty check for `options.def.Event` occurs to verify that the event
string matches one of the allowed event values, and if not, return an
execUsageError indicating an invalid event was provided, similar to the
structure of the existing validation errors for missing options.
---
Nitpick comments:
In `@internal/cli/hooks_manage_test.go`:
- Around line 77-85: The test TestRunHooksAddRejectsUnknownEvent currently only
verifies that runHooksAdd returns a non-success exit code when given an invalid
event, which would pass even if the command crashes instead of properly
reporting a usage error. Strengthen this test by explicitly asserting the
specific usage-error contract: check that the exit code matches the expected
usage-error exit code (not just any non-success code) and verify that the stderr
output contains a usage-style error message (such as text indicating invalid
event or similar usage guidance) that confirms this is a proper usage error
rather than an unexpected crash or other failure.
🪄 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: fa006616-a7a0-44a7-9b71-085148d424b3
📒 Files selected for processing (3)
internal/cli/extensions.gointernal/cli/hooks_manage.gointernal/cli/hooks_manage_test.go
go run govulncheck@latest selected the toolchain from govulncheck's own go.mod (downgrading to go1.25.11), which then could not load our go1.26-requiring packages (fips140only_go1.26.go), failing the security job. Resolve GOTOOLCHAIN from go.mod's toolchain line so govulncheck and deadcode both run under go1.26.4.
Collaborator
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
Collaborator
Author
|
@CodeRabbit approvable ? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
REPO="Gitlawb/zero"
PR=193
echo "=== PR State ==="
gh pr view $PR --repo $REPO --json state,reviewDecision,mergeable,statusCheckRollup \
| jq '{state, reviewDecision, mergeable, checks: [.statusCheckRollup[]? | {name, conclusion, status}]}'
echo ""
echo "=== CodeRabbit Review Comments (open/unresolved) ==="
gh api graphql \
-F owner="Gitlawb" \
-F name="zero" \
-F pr=$PR \
-f query='
query($owner: String!, $name: String!, $pr: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $pr) {
reviewThreads(first: 50) {
nodes {
isResolved
isOutdated
comments(first: 1) {
nodes {
author { login }
body
}
}
}
}
}
}
}' | jq '
.data.repository.pullRequest.reviewThreads.nodes
| map(select(.isResolved == false and .isOutdated == false))
| map({
author: .comments.nodes[0].author.login,
body: (.comments.nodes[0].body | .[0:200])
})'Length of output: 2902
✅ CI checks: All green (Smoke on Ubuntu/macOS/Windows, Security & code health, Performance Smoke, Validate action.yml). ❌ Review decision:
The two Major issues need to be addressed (or explicitly acknowledged as non-issues with an explanation) before I can approve. Once those are resolved, the PR looks solid overall — the supply-chain hardening and hooks CLI feature are well-structured, tests cover the key flows, and all checks pass. |
…event CodeRabbit findings on #193: - ci.yml: persist-credentials: false on all checkout steps (smoke/performance/security) — these jobs run repo code and never push, so don't keep the token in git config. - ci.yml: pin govulncheck@v1.3.0 and deadcode@v0.46.0 instead of @latest, so the security gate is deterministic and not subject to supply-chain drift. - hooks: add exported IsValidEvent/KnownEvents (single source); parseEvent reuses it, and 'zero hooks add' now rejects an invalid --event with a usage error at parse time instead of falling through to the Upsert app-error crash path.
Collaborator
Author
|
All three addressed in a5eda64:
Verified: build/vet/gofmt clean, @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
REPO="Gitlawb/zero"
PR=193
echo "=== Commit a5eda64 details ==="
gh api repos/$REPO/commits/a5eda64 --jq '{sha, message: .commit.message}'
echo ""
echo "=== Files changed in a5eda64 ==="
gh api repos/$REPO/commits/a5eda64 --jq '[.files[]| {filename, additions, deletions, status}]'Length of output: 1434 🏁 Script executed: #!/bin/bash
set -euo pipefail
REPO="Gitlawb/zero"
PR=193
echo "=== ci.yml current state (relevant sections) ==="
gh api repos/$REPO/contents/.github/workflows/ci.yml --jq '.content' | base64 -d | grep -n "persist-credentials\|govulncheck\|deadcode\|checkout" | head -60
echo ""
echo "=== hooks_manage.go IsValidEvent / parseEvent ==="
gh api "repos/$REPO/git/trees/a5eda64?recursive=1" --jq '[.tree[] | .path] | map(select(test("hooks")))'Length of output: 867 🏁 Script executed: #!/bin/bash
set -euo pipefail
REPO="Gitlawb/zero"
COMMIT="a5eda64e587fe57746775a04649acdf1da10c068"
echo "=== ci.yml at commit ==="
gh api "repos/$REPO/contents/.github/workflows/ci.yml?ref=$COMMIT" --jq '.content' | base64 -d | grep -n "persist-credentials\|govulncheck\|deadcode\|checkout\|latest"
echo ""
echo "=== hooks.go at commit ==="
gh api "repos/$REPO/contents/internal/hooks/hooks.go?ref=$COMMIT" --jq '.content' | base64 -d | grep -n "IsValidEvent\|KnownEvents\|parseEvent\|ValidEvent"
echo ""
echo "=== hooks_manage.go at commit (parseEvent / event validation) ==="
gh api "repos/$REPO/contents/internal/cli/hooks_manage.go?ref=$COMMIT" --jq '.content' | base64 -d | grep -n "IsValidEvent\|parseEvent\|event\|Event"Length of output: 3369
Everything looks solid. Approving. [approve] |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/hooks/hooks.go (1)
826-829: ⚡ Quick winConsider generating the error message from
KnownEvents()for maintainability.The validation now uses
IsValidEvent(), but the error message on line 827 still hardcodes the event names. If a new event is added, you'd need to update the const declaration,KnownEvents(), and this error message. Generating the message fromKnownEvents()would eliminate that third update point.♻️ Example refactor
func parseEvent(raw any, field string) (Event, error) { text, ok := raw.(string) if !ok || strings.TrimSpace(text) == "" { return "", manifestError{fieldPath: field, message: "Expected a hook event."} } event := Event(strings.TrimSpace(text)) if !IsValidEvent(event) { - return "", manifestError{fieldPath: field, message: "Expected beforeTool, afterTool, sessionStart, sessionEnd, specialistStart, or specialistStop."} + known := KnownEvents() + names := make([]string, len(known)) + for i, e := range known { + names[i] = string(e) + } + return "", manifestError{fieldPath: field, message: fmt.Sprintf("Expected %s.", strings.Join(names, ", "))} } return event, nil }🤖 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/hooks/hooks.go` around lines 826 - 829, The error message in the validation block for IsValidEvent() hardcodes the event names, creating a maintenance burden when new events are added. Instead of manually listing the event names in the error message, generate the message dynamically from the KnownEvents() function so that adding a new event only requires updates in the const declaration and KnownEvents(), not in the error message itself.
🤖 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.
Nitpick comments:
In `@internal/hooks/hooks.go`:
- Around line 826-829: The error message in the validation block for
IsValidEvent() hardcodes the event names, creating a maintenance burden when new
events are added. Instead of manually listing the event names in the error
message, generate the message dynamically from the KnownEvents() function so
that adding a new event only requires updates in the const declaration and
KnownEvents(), not in the error message itself.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c3aafc8c-3288-4ae1-9c8d-5b8b9759e083
📒 Files selected for processing (3)
.github/workflows/ci.ymlinternal/cli/hooks_manage.gointernal/hooks/hooks.go
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/ci.yml
- internal/cli/hooks_manage.go
Vasanthdev2004
approved these changes
Jun 14, 2026
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
Collaborator
There was a problem hiding this comment.
Verdict: Approved
No blocking issues found in this rereview.
The hooks management command is scoped and covered, event validation now happens before persistence, workflow checkout credentials are disabled, Actions/tool versions are pinned, and the security job is deterministic with the GOTOOLCHAIN value from go.mod.
Non-blocking nit: CodeRabbit's suggestion to generate the invalid-event error message from KnownEvents() is valid maintenance polish, but I would not block this PR on it.
Validation run locally on PR head a5eda64:
gofmt -l internal\\cli internal\\hooks-> cleango test ./internal/cli ./internal/hooks -run Hooks|Hook|KnownEvents|IsValidEvent -count=1-> passgo test ./... -timeout 300s-> passgo vet ./...-> passgo build ./...-> passgo run ./cmd/zero-release build-> pass, builtzero.exego run ./cmd/zero-release smoke-> pass$env:GOTOOLCHAIN='go1.26.4'; go run golang.org/x/vuln/cmd/govulncheck@v1.3.0 ./...-> no called vulnerabilities found
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
Two things in one PR — a new CLI feature plus build/supply-chain hardening.
1. Hooks management command (feature)
Wires the previously-dormant
hooks.ConfigStore(10 unreachable funcs → 0) into the CLI,so hooks can be managed from the command line, not just listed:
<cwd>/.zero/hooks.json) or the userconfig with
--user, reusing the store's existing locking + atomic writes + validation(
normalizeDefinition).enable/disable.redaction); mirrors the existingzero mcp add/remove/enable/disablecommand shape.validation, and JSON secret redaction.
2. Build & supply-chain hardening
go.modtoolchain go1.24.13 → go1.26.4, clearing 9govulncheck-reachable stdlib CVEs (CI/release build via
go-version-file: go.mod, soshipped binaries inherited them).
govulncheckCI gate — a newsecurityjob fails on a reachable vulnerability (hardgate); plus an advisory
deadcodestep.across all four workflows (previously only
setup-gowas pinned).CVEs cleared
Verification
go build ./...,go vet ./...,gofmt -lclean; fullgo test ./...green.govulncheck ./...→ 0 reachable (was 9 under go1.24.13).internal/hooksis now fully reachable (10 → 0 unreachable funcs); total prod-unreachabledown to 114.
Notes
govulncheckgate may flag a newly published advisory on an unrelated PR — intentional(don't ship known-reachable vulns); fix is a toolchain bump.
deadcodeis advisory.godirective stays at1.24.2; only thetoolchaindirective moves.Summary by CodeRabbit
zero hookswithadd,remove,enable, anddisablecommands, including optional JSON output and--uservs project scope.