Add semantic policy injection#261
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds PreToolUse semantic policy injection for Git mutation and Python file targets, merges the new hook output into runner aggregation, updates push handling constants, and replaces several fixed cache timestamps with current UTC time in tests. ChangesSemantic Policy Injection via PreToolUse Hook
Cache Time Sources
Sequence Diagram(s)sequenceDiagram
participant Agent
participant HookRunner
participant SemanticPolicyInjection
participant ProviderEncoder
Agent->>HookRunner: PreToolUse tool call
HookRunner->>SemanticPolicyInjection: event, bundle
SemanticPolicyInjection-->>HookRunner: HookSpecificOutput or nil
HookRunner->>HookRunner: merge advisory outputs
HookRunner->>ProviderEncoder: EncodeProviderResult(merged output)
ProviderEncoder-->>Agent: injected AdditionalContext
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linter] gci: create analyzer: finding local modules for Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces just-in-time semantic policy injection during the PreToolUse hook. It detects mutating Git commands and Python file targets in incoming tool calls to inject relevant skill pointers and policy guidance instead of front-loading them at startup. It also includes documentation updates, helper functions to merge advisory hook outputs, and comprehensive tests. Feedback on the changes suggests improving Git mutation detection in shellCommandMutatesGit to handle global flags (e.g., git -C <path> commit) by scanning the arguments slice using slices.ContainsFunc, and simplifying the allocation of merged in mergeAdvisoryHookOutputs to avoid taking the address of a block-local variable.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| func shellCommandMutatesGit(command shellparse.Command) bool { | ||
| args := command.Argv | ||
| if len(args) == 0 { | ||
| return false | ||
| } | ||
|
|
||
| switch shellCommandName(command) { | ||
| case tokenGit: | ||
| return len(args) > 1 && semanticGitMutation(args[1]) | ||
| case wrapperRunnerName: | ||
| return len(args) > 2 && | ||
| args[1] == "policy-git" && | ||
| semanticGitMutation(args[2]) | ||
| case "coding-ethos-git": | ||
| return len(args) > 1 && semanticGitMutation(args[1]) | ||
| } | ||
|
|
||
| return false | ||
| } |
There was a problem hiding this comment.
The current implementation of shellCommandMutatesGit only checks args[1] (or args[2] for policy-git) to detect mutating Git subcommands. If global flags (such as git -C <path> commit or git -c <config> commit) are used, the mutating subcommand is shifted to a later index, causing the detection to fail and skipping the semantic policy injection.
Using slices.ContainsFunc to scan the arguments slice is much more robust and handles global flags gracefully.
| func shellCommandMutatesGit(command shellparse.Command) bool { | |
| args := command.Argv | |
| if len(args) == 0 { | |
| return false | |
| } | |
| switch shellCommandName(command) { | |
| case tokenGit: | |
| return len(args) > 1 && semanticGitMutation(args[1]) | |
| case wrapperRunnerName: | |
| return len(args) > 2 && | |
| args[1] == "policy-git" && | |
| semanticGitMutation(args[2]) | |
| case "coding-ethos-git": | |
| return len(args) > 1 && semanticGitMutation(args[1]) | |
| } | |
| return false | |
| } | |
| func shellCommandMutatesGit(command shellparse.Command) bool { | |
| args := command.Argv | |
| if len(args) == 0 { | |
| return false | |
| } | |
| switch shellCommandName(command) { | |
| case tokenGit: | |
| return slices.ContainsFunc(args[1:], semanticGitMutation) | |
| case wrapperRunnerName: | |
| return len(args) > 2 && | |
| args[1] == "policy-git" && | |
| slices.ContainsFunc(args[2:], semanticGitMutation) | |
| case "coding-ethos-git": | |
| return slices.ContainsFunc(args[1:], semanticGitMutation) | |
| } | |
| return false | |
| } |
| if merged == nil { | ||
| next := *output | ||
| next.AdditionalContext = "" | ||
| merged = &next | ||
| } |
There was a problem hiding this comment.
Taking the address of a block-local variable next scoped inside the if block is a Go code smell and can lead to unnecessary heap allocations. Since we already ensure that PermissionDecision, PermissionDecisionReason, and UpdatedInput are empty before this block, we can simplify this by directly allocating a new HookSpecificOutput with the HookEventName copied over.
| if merged == nil { | |
| next := *output | |
| next.AdditionalContext = "" | |
| merged = &next | |
| } | |
| if merged == nil { | |
| merged = &HookSpecificOutput{ | |
| HookEventName: output.HookEventName, | |
| } | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@go/internal/hooks/runner_test.go`:
- Around line 4587-4622: The test TestEncodeGeminiSemanticPolicyInjection only
validates that expected substrings exist in the encoded output using
strings.Contains, but does not verify that the encoded JSON is well-formed.
After calling EncodeProviderResult and obtaining the encoded string, parse it as
JSON into a struct or map to validate the JSON structure is valid, and then
verify the expected fields and values exist in the parsed JSON object rather
than just checking for substring presence. This ensures both that the JSON is
properly formatted and that the expected data is in the correct structure.
In `@go/internal/hooks/semantic_policy_injection.go`:
- Around line 103-155: The shellCommandMutatesGit function only inspects fixed
argument positions (args[1] for tokenGit and "coding-ethos-git", args[2] for
wrapperRunnerName) without accounting for leading global options that start with
a dash. This causes commands like "git -C /repo commit" to be missed because
"-C" is at args[1] instead of the actual "commit" subcommand. Update
shellCommandMutatesGit to scan past leading options (tokens starting with "-")
in the argument list for each case statement (tokenGit, wrapperRunnerName, and
"coding-ethos-git") to find the first non-option token, then pass that token to
semanticGitMutation instead of checking fixed indices.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: fdd7b685-bfda-4f94-b3c9-b2069cffeef2
📒 Files selected for processing (6)
README.mddocs/AGENT_PROXY.mdgo/internal/hooks/git_wrapper_enforcement.gogo/internal/hooks/runner.gogo/internal/hooks/runner_test.gogo/internal/hooks/semantic_policy_injection.go
📜 Review details
⏰ Context from checks skipped due to timeout. (8)
- GitHub Check: Coding Ethos SARIF Gate / Coding Ethos SARIF Gate
- GitHub Check: Test (Python 3.11)
- GitHub Check: Test (Python 3.13)
- GitHub Check: Go coverage
- GitHub Check: Unified lint
- GitHub Check: Validate GitHub workflows
- GitHub Check: CodeQL (go)
- GitHub Check: Go fuzz smoke
🧰 Additional context used
📓 Path-based instructions (1)
**/{hooks,policies,lint-capture,runtime}/**/*.{go,sh,py}
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer compiled Go for hook, policy, lint-capture, and runtime glue; treat shell and Python as transitional unless they are clearly the right interface
Files:
go/internal/hooks/runner_test.gogo/internal/hooks/semantic_policy_injection.gogo/internal/hooks/git_wrapper_enforcement.gogo/internal/hooks/runner.go
🧠 Learnings (1)
📚 Learning: 2026-06-23T12:26:47.373Z
Learnt from: paudley
Repo: paudley/coding-ethos PR: 258
File: go/internal/geminiprompts/render.go:125-132
Timestamp: 2026-06-23T12:26:47.373Z
Learning: In the paudley/coding-ethos repo, the documented guideline “Gemini prompt pack should be generated in go/internal/geminiprompts/” refers to the generator implementation location (Go source), not the location of the generated prompt-pack artifact. Always treat PromptPackPath as the enforced output artifact path `.coding-ethos/gemini/prompt-pack.json` (e.g., render_test.go asserts PromptPackPath, and compiler_policies.go / hookrunner lookup / README document the expected path). Do not flag a code review issue just because PromptPackPath points to `.coding-ethos/gemini/prompt-pack.json`; only flag violations if the artifact output path logic breaks that enforced contract.
Applied to files:
go/internal/hooks/runner_test.gogo/internal/hooks/semantic_policy_injection.gogo/internal/hooks/git_wrapper_enforcement.gogo/internal/hooks/runner.go
🔇 Additional comments (9)
go/internal/hooks/runner_test.go (2)
7-7: LGTM!
4503-4585: LGTM!README.md (1)
673-679: LGTM!docs/AGENT_PROXY.md (1)
362-380: LGTM!go/internal/hooks/runner.go (3)
36-36: LGTM!Also applies to: 525-537, 547-556
293-301: LGTM!
316-350: 🎯 Functional CorrectnessAdvisory outputs never populate decision fields, so the early return never occurs.
All five advisory output functions passed to
mergeAdvisoryHookOutputs(semanticPolicyInjectionOutput, sessionMemoryImportOutput, continuationOutput, lifecycleOutput, postEditOutput) populate onlyAdditionalContextand never setPermissionDecision,PermissionDecisionReason, orUpdatedInput. The early return condition inmergeAdvisoryHookOutputscannot be triggered by these functions, so accumulated context is not discarded in practice.> Likely an incorrect or invalid review comment.go/internal/hooks/git_wrapper_enforcement.go (1)
151-165: LGTM!Also applies to: 590-614
go/internal/hooks/semantic_policy_injection.go (1)
26-101: 🩺 Stability & AvailabilityAll referenced symbols are defined and resolve correctly in the package.
wrapperRunnerPathandwrapperRunnerNameare defined ingit_wrapper_enforcement.go,tokenGitis defined ingit_wrapper_enforcement.go,toonCellis a function inoutput_format.go, andEvent.Command()andEvent.Files()are methods on the Event type inevent.go. The code compiles without cross-file resolution issues.> Likely an incorrect or invalid review comment.
|
Stage 2 remediation is pushed. Commit: 4adf67b fix(proxy): handle semantic policy review feedback Feedback addressed:
Validation:
Current status:
|
|
Stage 2 final status. Pushed commits:
Validation and CI:
Merge status:
No admin override was used. |
Closes #56
Summary
Validation