Add provider-neutral supervisor hook integration - #264
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📜 Recent review details🧰 Additional context used🧠 Learnings (1)📚 Learning: 2026-06-23T12:26:47.373ZApplied to files:
🔇 Additional comments (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds Kimi provider support, a provider-neutral hook contract, capability discovery, split repository/state-root routing, private durable storage for code intelligence and memory, runtime-policy commands, configurable hook timeouts, and related sandbox, subprocess, CLI, documentation, and test coverage. ChangesRuntime and hook behavior
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Probe delegated cgroup FD start support before managed tool launch and record advisory evidence when the host cannot attach at start. Add Linux sandbox regression coverage for namespace-isolated skips and unavailable start attachment cleanup.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Upgrade vulnerable Go modules reported by the PR OSV scan: github.com/google/cel-go, github.com/klauspost/compress, and golang.org/x/text. Keep transitive x/* module versions consistent with go get and go mod tidy.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 @.coding-ethos/.gitignore:
- Around line 3-10: Fix the ignore patterns in .coding-ethos/.gitignore by
making them relative to that directory: remove the repeated .coding-ethos/
prefix so cache, code-intel.duckdb and its WAL, events, hook-runs, lint-runs,
prune-runs, and state paths match the repository-root .coding-ethos contents.
In `@go/cmd/coding-ethos-run/dispatch.go`:
- Around line 983-1001: Update runAgentHooksCommand so CODE_ETHOS_CONSUMER_ROOT
uses the same root precedence as withCommandRoots: resolve --repo-root first,
then --root, then the existing paths.Root value. Do not fall back to
settingsRoot when exporting CODE_ETHOS_CONSUMER_ROOT; preserve the separate
settings-root fallback for envStateRoot.
In `@go/cmd/coding-ethos-run/parent_workflow.go`:
- Around line 213-224: Update cleanOptionalRoot to avoid embedding the
state-root-specific error text: include the rejected root value in the
absolute-path validation error and make the message generic enough for any
caller. Preserve the existing empty-root behavior and cleaned absolute-path
return path.
In `@go/internal/agenthooks/settings_test.go`:
- Around line 903-1050: Extract the shared expected verification-check count
into a single test constant near the related tests, then replace the hardcoded
17 values in all three report assertions and their failure messages with that
constant. Keep the existing validation behavior and test names unchanged.
In `@go/internal/agenthooks/settings.go`:
- Around line 1543-1588: Add a TimeoutSeconds field to kimiHook and update
buildKimiSettings to accept or access SettingsOptions.HookTimeoutSeconds,
assigning that configured value to every generated hook, including spec hooks,
provider alias hooks, and observation event hooks. Preserve the existing
command, event, matcher, and filtering behavior.
In `@go/internal/agenthooks/state_artifacts.go`:
- Around line 178-185: Update agentHookStateArtifactInputs to accept a
providerStateContent struct with named fields instead of multiple positional
string parameters. Define fields for claude, claudeMCP, codex, gemini,
kimiConfig, and kimiMCP, then update callers and internal references to use the
struct fields while preserving the existing hashing behavior.
In `@go/internal/agenthookscli/main.go`:
- Around line 114-138: Remove the unused `--json` flag registration from
`capabilities`, since this subcommand already always writes JSON through
`feedback.WriteJSON`. Keep the existing flag parsing and capability output
behavior unchanged.
In `@go/internal/hooks/contract_v1.go`:
- Around line 186-207: Update validateHookContractProvider to validate the
resolved value from event.Provider(), not only event.ProviderHint. Explicitly
reject an empty unresolved provider fallback, and ensure the resolved provider
is contained in hookContractV1Providers() while preserving the existing contract
error behavior.
In `@tests/test_lint_capture_lifecycle.py`:
- Around line 25-27: Add a concise Google-style docstring to the public test
function test_subprocess_environment_drops_inherited_runtime_roots describing
that it verifies subprocess environments remove inherited runtime roots.
🪄 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: aca8577c-de1a-4fda-b32a-1939451556aa
⛔ Files ignored due to path filters (2)
pre-commit/hooks/npm-locks/eslint-10.3.0/package-lock.jsonis excluded by!**/package-lock.jsonuv.lockis excluded by!**/*.lock
📒 Files selected for processing (62)
.coding-ethos/.gitignore.gitattributes.gitignoreMakefileREADME.mddocs/HOOK_CONTRACT_V1.mddocs/PROVIDER_CAPABILITY_MATRIX.mdgo/cmd/coding-ethos-run/args.gogo/cmd/coding-ethos-run/dispatch.gogo/cmd/coding-ethos-run/hook_policy_paths.gogo/cmd/coding-ethos-run/main.gogo/cmd/coding-ethos-run/main_test.gogo/cmd/coding-ethos-run/parent_workflow.gogo/internal/agenthooks/codex_trust.gogo/internal/agenthooks/doctor_contract_test.gogo/internal/agenthooks/provider_capabilities.gogo/internal/agenthooks/settings.gogo/internal/agenthooks/settings_probe.gogo/internal/agenthooks/settings_test.gogo/internal/agenthooks/spec.gogo/internal/agenthooks/state_artifacts.gogo/internal/agenthookscli/main.gogo/internal/agenthookscli/main_internal_test.gogo/internal/e2e/mcp_test.gogo/internal/e2e/sandbox_workflow_test.gogo/internal/e2e/scenario.gogo/internal/e2e/scenario_internal_test.gogo/internal/hookcli/main.gogo/internal/hookcli/main_internal_test.gogo/internal/hookrunnercli/main.gogo/internal/hookrunnercli/main_internal_test.gogo/internal/hookrunnercli/toolchain_groups.gogo/internal/hookrunnercli/toolchain_groups_internal_test.gogo/internal/hooks/contract_v1.gogo/internal/hooks/contract_v1_test.gogo/internal/hooks/event.gogo/internal/hooks/inspection.gogo/internal/hooks/json.gogo/internal/hooks/provider_output.gogo/internal/hooks/provider_output_test.gogo/internal/hooks/result.gogo/internal/hooks/runner.gogo/internal/hooks/testdata/neutral_v1_allowed.jsongo/internal/hooks/testdata/neutral_v1_blocked.jsongo/internal/managedcapture/capture.gogo/internal/managedcapture/capture_test.gogo/internal/managedcapture/writable_paths.gogo/internal/mcp/code_intel.gogo/internal/mcp/code_intel_workspace.gogo/internal/mcp/server.gogo/internal/mcp/server_test.gogo/internal/mcpcli/main.gogo/internal/memories/memory.gogo/internal/memories/memory_test.gogo/internal/sandbox/cgroup_linux.gogo/internal/sandbox/cgroup_linux_test.gogo/internal/syncstate/state.gogo/internal/toolaliases/aliases.gogo/internal/toolaliases/aliases_test.gopyproject.tomltests/lint_capture_support.pytests/test_lint_capture_lifecycle.py
📜 Review details
⏰ Context from checks skipped due to timeout. (9)
- GitHub Check: Coding Ethos SARIF Gate / Coding Ethos SARIF Gate
- GitHub Check: Validate GitHub workflows
- GitHub Check: Test (Python 3.11)
- GitHub Check: Go coverage
- GitHub Check: Unified lint
- GitHub Check: Test (Python 3.13)
- GitHub Check: OSV dependency scan
- GitHub Check: CodeQL (go)
- GitHub Check: Go fuzz smoke
🧰 Additional context used
📓 Path-based instructions (5)
{.python-version,pyproject.toml,mypy.ini,pyrightconfig.json,ruff.toml,.golangci.yml}
📄 CodeRabbit inference engine (AGENTS.md)
Python version is the single authority across generated tool configs, pyupgrade autofix pass, and repo-root consistency checks for .python-version, pyproject.toml, mypy.ini, pyrightconfig.json, ruff.toml, and .golangci.yml's lll line-length setting
Files:
pyproject.toml
Makefile
📄 CodeRabbit inference engine (AGENTS.md)
Makefile: Usemake installto install dependencies
Usemake checkas the canonical automated verification gate; usemake testoruv run pytestonly as focused Python-test helpers
Files:
Makefile
**/{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/inspection.gogo/internal/hooks/provider_output_test.gogo/internal/hooks/event.gogo/internal/hooks/runner.gogo/internal/hooks/contract_v1_test.gogo/internal/hooks/json.gogo/internal/hooks/provider_output.gogo/internal/hooks/result.gogo/internal/hooks/contract_v1.go
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Crash early on ambiguous startup and configuration states instead of degrading silently
Treat required imports as hard dependencies and fail immediately if they are missing (no soft dependency pattern)
Make ruff and mypy blocking quality gates rather than advisory tools
Model required dependencies as non-optional and default to full-strength behavior (no Optional types for required dependencies)
Run required validation unconditionally; a missing component is itself a failure
Validate required capabilities at startup instead of probing for them at runtime
Validate configuration, schema, and extensions during bootstrap rather than on first use
Route configuration through validated bootstrap paths instead of inline shell environment variables
Log important decisions with context and instrument the system with metrics (Radical Visibility)
Define and verify interfaces before writing or referencing implementations (Protocol-First Design)
Resolve lint findings with structural fixes; suppress only with documented necessity
Use Python's functional tools (itertools, functools) when they make code clearer and more local
Keep public behavior documented as part of the interface contract; every public function must have a Google-style docstring
Use precise exception types and actionable, context-rich error messages; all application exceptions should inherit from a base exception with structured context
Files:
tests/lint_capture_support.pytests/test_lint_capture_lifecycle.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.py: Treat tests as executable behavioral contracts and update them with code changes (Testing as Specification)
Prove critical behavior with real functional workflows before relying on unit tests or mocks (Functional Testing Is the Proof)
Files:
tests/lint_capture_support.pytests/test_lint_capture_lifecycle.py
🧠 Learnings (2)
📚 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/toolaliases/aliases_test.gogo/internal/e2e/scenario_internal_test.gogo/internal/agenthooks/spec.gogo/internal/hookrunnercli/toolchain_groups.gogo/internal/managedcapture/writable_paths.gogo/internal/hooks/inspection.gogo/internal/e2e/mcp_test.gogo/internal/mcp/server.gogo/internal/mcpcli/main.gogo/internal/managedcapture/capture_test.gogo/internal/syncstate/state.gogo/internal/hooks/provider_output_test.gogo/internal/mcp/server_test.gogo/internal/hookrunnercli/toolchain_groups_internal_test.gogo/internal/hooks/event.gogo/internal/memories/memory_test.gogo/internal/managedcapture/capture.gogo/internal/hookrunnercli/main_internal_test.gogo/internal/toolaliases/aliases.gogo/internal/agenthooks/doctor_contract_test.gogo/internal/hookrunnercli/main.gogo/internal/hooks/runner.gogo/internal/hooks/contract_v1_test.gogo/internal/sandbox/cgroup_linux_test.gogo/internal/hooks/json.gogo/internal/hookcli/main_internal_test.gogo/internal/agenthooks/codex_trust.gogo/internal/hooks/provider_output.gogo/internal/mcp/code_intel.gogo/internal/agenthooks/provider_capabilities.gogo/internal/hookcli/main.gogo/internal/hooks/result.gogo/internal/e2e/scenario.gogo/internal/e2e/sandbox_workflow_test.gogo/internal/agenthookscli/main_internal_test.gogo/internal/hooks/contract_v1.gogo/internal/sandbox/cgroup_linux.gogo/internal/agenthooks/settings_probe.gogo/internal/agenthooks/state_artifacts.gogo/internal/memories/memory.gogo/internal/mcp/code_intel_workspace.gogo/internal/agenthookscli/main.gogo/internal/agenthooks/settings_test.gogo/internal/agenthooks/settings.go
📚 Learning: 2026-06-07T21:57:34.433Z
Learnt from: paudley
Repo: paudley/coding-ethos PR: 237
File: .gitignore:62-62
Timestamp: 2026-06-07T21:57:34.433Z
Learning: When reviewing this repo’s .gitignore files, do NOT flag the ignore entry that uses a dash form for the DuckDB shm sidecar as incorrect. Specifically, the pattern `.coding-ethos/code-intel.duckdb-shm` (and its recursive equivalent `**/.coding-ethos/code-intel.duckdb-shm`) must remain exactly as-is; the dot form `.coding-ethos/code-intel.duckdb.shm` will trip the repo’s blocking `repo.required_ignores` policy/lint stage.
Applied to files:
.gitignore
🪛 LanguageTool
docs/HOOK_CONTRACT_V1.md
[style] ~185-~185: Consider an alternative for the overused word “exactly”.
Context: ... are rejected. The explicit MCP form is exactly an absolute coding-ethos-run mcp; omi...
(EXACTLY_PRECISELY)
🔇 Additional comments (83)
.gitattributes (1)
7-7: LGTM!.gitignore (1)
102-102: LGTM!Makefile (1)
34-36: LGTM!README.md (6)
494-505: LGTM!
1581-1581: LGTM!
1590-1661: LGTM!
1673-1677: LGTM!
1688-1705: LGTM!
1764-1786: LGTM!pyproject.toml (1)
90-90: LGTM!docs/HOOK_CONTRACT_V1.md (1)
1-200: LGTM!go/internal/hooks/contract_v1.go (3)
18-56: LGTM!
58-152: LGTM!
209-433: LGTM!go/internal/hooks/contract_v1_test.go (1)
14-172: LGTM!go/internal/hooks/json.go (2)
7-69: LGTM!
89-131: LGTM!go/internal/hookcli/main.go (3)
30-34: LGTM!Also applies to: 46-56, 70-133
91-94: LGTM!
135-204: LGTM!go/internal/hookcli/main_internal_test.go (1)
166-340: LGTM!go/internal/hooks/provider_output_test.go (1)
143-181: LGTM!go/internal/hooks/event.go (1)
17-17: LGTM!Also applies to: 32-33, 51-52, 65-66, 86-87
go/internal/hooks/inspection.go (1)
197-201: LGTM!go/internal/hooks/provider_output.go (1)
20-20: LGTM!Also applies to: 62-62, 84-110, 213-226
go/internal/hooks/result.go (1)
12-28: LGTM!Also applies to: 30-33
go/internal/hooks/runner.go (1)
101-105: LGTM!Also applies to: 118-122, 249-250, 271-277, 1030-1033
go/internal/hooks/testdata/neutral_v1_allowed.json (1)
1-19: LGTM!go/internal/hooks/testdata/neutral_v1_blocked.json (1)
1-25: 🎯 Functional CorrectnessVerify the blocked fixture’s tracking identifier.
Blocked results receive
Result.TrackingID, and the neutral encoder serializes it. This fixture omits that field; unlessassertJSONMatchesFixtureintentionally strips dynamic tracking identifiers, the test will fail or fail to verify part of the response contract. Confirm the helper’s normalization and either include the expected identifier or document the omission.docs/PROVIDER_CAPABILITY_MATRIX.md (1)
19-19: LGTM!Also applies to: 209-284
go/internal/agenthooks/provider_capabilities.go (2)
12-13: LGTM!Also applies to: 30-72
227-299: LGTM!go/internal/agenthooks/spec.go (1)
16-17: LGTM!go/internal/agenthooks/codex_trust.go (1)
40-56: LGTM!Also applies to: 76-92, 185-189
go/internal/agenthooks/settings_test.go (6)
77-129: LGTM!
131-144: LGTM!Also applies to: 146-150, 182-187, 223-225
349-350: LGTM!Also applies to: 436-440, 442-499
632-634: LGTM!Also applies to: 846-847, 887-888
1052-1132: LGTM!
1607-1612: LGTM!Also applies to: 1645-1691, 1693-1729
go/internal/agenthooks/state_artifacts.go (1)
13-92: LGTM!Also applies to: 94-99, 137-176, 218-231
go/internal/agenthookscli/main.go (5)
12-12: LGTM!Also applies to: 29-69
140-175: LGTM!
246-298: LGTM!Also applies to: 311-339, 354-387
476-515: LGTM!Also applies to: 541-544
229-243: 🗄️ Data Integrity & IntegrationNo change needed. Artifact rendering and write rendering use the same post-existence path for Codex/Kimi text settings, so the non-overlay path does not record stale post-apply
expected_sha256values.> Likely an incorrect or invalid review comment.go/cmd/coding-ethos-run/parent_workflow.go (3)
52-52: LGTM!Also applies to: 118-153
164-168: LGTM!Also applies to: 183-187, 206-211
383-383: LGTM!Also applies to: 402-405, 953-962
go/internal/hookrunnercli/main.go (1)
445-451: LGTM!go/internal/hookrunnercli/main_internal_test.go (1)
116-125: LGTM!go/internal/toolaliases/aliases.go (1)
19-19: LGTM!Also applies to: 42-59
go/internal/toolaliases/aliases_test.go (1)
67-73: LGTM!go/internal/agenthooks/settings.go (1)
186-209: LGTM!Also applies to: 225-278, 1609-1679
go/internal/agenthooks/settings_probe.go (1)
25-57: LGTM!Also applies to: 189-211, 240-302, 320-369
go/internal/agenthooks/doctor_contract_test.go (1)
42-45: LGTM!go/cmd/coding-ethos-run/args.go (1)
13-17: LGTM!Also applies to: 36-58, 153-164, 176-222, 224-275
go/cmd/coding-ethos-run/dispatch.go (1)
109-109: LGTM!Also applies to: 161-164, 815-820, 969-981, 1015-1029
go/cmd/coding-ethos-run/hook_policy_paths.go (1)
17-26: LGTM!go/cmd/coding-ethos-run/main.go (1)
27-27: LGTM!Also applies to: 39-39, 76-87, 157-157, 185-185, 203-203, 228-236, 396-396, 489-489
go/cmd/coding-ethos-run/main_test.go (1)
64-74: LGTM!Also applies to: 186-190, 201-363, 543-557
go/internal/agenthookscli/main_internal_test.go (1)
14-14: LGTM!Also applies to: 76-107, 161-162, 202-257, 259-328
go/internal/mcp/code_intel.go (1)
592-600: LGTM!Also applies to: 1037-1047, 1908-1931
go/internal/mcp/code_intel_workspace.go (1)
303-303: LGTM!Also applies to: 340-344, 370-372, 392-392
go/internal/mcp/server.go (1)
61-61: LGTM!go/internal/mcp/server_test.go (1)
2553-2597: LGTM!go/internal/mcpcli/main.go (1)
29-33: LGTM!Also applies to: 63-69
go/internal/memories/memory.go (1)
24-25: LGTM!Also applies to: 60-60, 191-222, 232-267, 325-344, 353-412
go/internal/memories/memory_test.go (1)
7-7: LGTM!Also applies to: 147-193
go/internal/syncstate/state.go (1)
766-769: LGTM!go/internal/hookrunnercli/toolchain_groups_internal_test.go (1)
137-151: LGTM!tests/lint_capture_support.py (1)
21-48: LGTM!go/internal/managedcapture/capture.go (1)
696-732: LGTM!go/internal/managedcapture/capture_test.go (1)
37-69: LGTM!go/internal/managedcapture/writable_paths.go (1)
99-114: LGTM!go/internal/sandbox/cgroup_linux.go (1)
7-14: LGTM!Also applies to: 23-24, 34-52, 85-123
go/internal/sandbox/cgroup_linux_test.go (1)
9-13: LGTM!Also applies to: 43-112
go/internal/e2e/scenario.go (2)
207-213: LGTM!Also applies to: 299-325
398-398: 🎯 Functional CorrectnessNo change needed. The inherited-runtime-context strip list covers the Coding Ethos root-/state/routing envs set by the runtime path/export flow.
go/internal/e2e/scenario_internal_test.go (1)
1-33: LGTM!go/internal/e2e/sandbox_workflow_test.go (1)
95-108: LGTM!Also applies to: 332-390
go/internal/e2e/mcp_test.go (1)
34-85: LGTM!go/internal/hookrunnercli/toolchain_groups.go (1)
897-921: LGTM!
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
README.md (1)
1696-1705: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDisambiguate Kimi denial exit codes.
The bullet combines exit-2 policy denial with structured-deny Stop continuation, but
go/internal/agenthooks/settings_probe.go:664-707requires the Stop-continuation probe to exit with code0. Split these behaviors and document both exit codes explicitly so external supervisors do not treat structured continuation as an exit-2 failure.Proposed documentation fix
-- Kimi exit-2 policy denial with a stderr reason and structured-deny Stop - continuation +- Kimi policy denial exits with code 2 and writes a reason to stderr. +- Kimi Stop continuation exits with code 0 and returns a structured deny + payload.🤖 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 `@README.md` around lines 1696 - 1705, Update the Kimi probe bullets in the README description to separate exit-2 policy denial from structured-deny Stop continuation. Document that policy denial exits with code 2 and the structured-deny Stop continuation exits with code 0, so their distinct supervisor behavior is explicit.go/internal/codeintelcli/main_internal_test.go (1)
639-649: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDrain stdout while the command runs.
captureStdoutcallsrunCommandbefore readingreader; if the command writes more than the OS pipe buffer, it blocks before returning and can hang the test. Startio.ReadAllin a goroutine beforerunCommand, close the writer afterward, then await the reader.Proposed fix
+ var ( + output []byte + readErr error + ) + readDone := make(chan struct{}) + go func() { + output, readErr = io.ReadAll(reader) + close(readDone) + }() + runCommand() os.Stdout = original if err := writer.Close(); err != nil { t.Fatalf("close stdout writer: %v", err) } writerClosed = true - output, err := io.ReadAll(reader) - if err != nil { - t.Fatalf("read stdout pipe: %v", err) + <-readDone + if readErr != nil { + t.Fatalf("read stdout pipe: %v", readErr) }🤖 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 `@go/internal/codeintelcli/main_internal_test.go` around lines 639 - 649, Update captureStdout around runCommand so io.ReadAll(reader) starts in a goroutine before the command executes, allowing stdout to drain continuously. After runCommand returns, restore os.Stdout, close writer, then await the goroutine’s output and error while preserving the existing failure handling.docs/HOOK_CONTRACT_V1.md (1)
192-196: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUpdate the Kimi timeout contract wording.
Kimi
[[hooks]]configs support atimeoutfield and read it from generated TOML, so this doc should not say Kimi lacks per-hook timeouts. Also add that Kimi limitstimeoutto 1–600 seconds, while--hook-timeout-secondsis still validated to 1–3600 before rendering.🤖 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 `@docs/HOOK_CONTRACT_V1.md` around lines 192 - 196, Update the timeout contract paragraph to state that generated Kimi TOML includes the per-hook timeout field, with Kimi enforcing a 1–600 second limit. Clarify that --hook-timeout-seconds remains validated to 1–3600 seconds before rendering, and remove the claim that Kimi lacks per-hook timeout support.
🤖 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.
Outside diff comments:
In `@docs/HOOK_CONTRACT_V1.md`:
- Around line 192-196: Update the timeout contract paragraph to state that
generated Kimi TOML includes the per-hook timeout field, with Kimi enforcing a
1–600 second limit. Clarify that --hook-timeout-seconds remains validated to
1–3600 seconds before rendering, and remove the claim that Kimi lacks per-hook
timeout support.
In `@go/internal/codeintelcli/main_internal_test.go`:
- Around line 639-649: Update captureStdout around runCommand so
io.ReadAll(reader) starts in a goroutine before the command executes, allowing
stdout to drain continuously. After runCommand returns, restore os.Stdout, close
writer, then await the goroutine’s output and error while preserving the
existing failure handling.
In `@README.md`:
- Around line 1696-1705: Update the Kimi probe bullets in the README description
to separate exit-2 policy denial from structured-deny Stop continuation.
Document that policy denial exits with code 2 and the structured-deny Stop
continuation exits with code 0, so their distinct supervisor behavior is
explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e6613fb3-17b7-4284-b257-e3e1495fd216
⛔ Files ignored due to path filters (1)
go/go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
.coding-ethos/.gitignoreREADME.mddocs/HOOK_CONTRACT_V1.mdgo/cmd/coding-ethos-run/dispatch.gogo/cmd/coding-ethos-run/main_test.gogo/cmd/coding-ethos-run/parent_workflow.gogo/go.modgo/internal/agenthooks/kimi_settings.gogo/internal/agenthooks/settings.gogo/internal/agenthooks/settings_test.gogo/internal/agenthooks/state_artifacts.gogo/internal/agenthookscli/main.gogo/internal/agenthookscli/main_internal_test.gogo/internal/codeintelcli/main_internal_test.gogo/internal/hooks/contract_v1.gogo/internal/hooks/contract_v1_test.gotests/test_lint_capture_lifecycle.py
💤 Files with no reviewable changes (1)
- go/internal/agenthookscli/main.go
📜 Review details
⏰ Context from checks skipped due to timeout. (8)
- GitHub Check: Coding Ethos SARIF Gate / Coding Ethos SARIF Gate
- GitHub Check: Unified lint
- GitHub Check: Go coverage
- GitHub Check: Test (Python 3.13)
- GitHub Check: Test (Python 3.11)
- GitHub Check: Validate GitHub workflows
- GitHub Check: CodeQL (go)
- GitHub Check: Go fuzz smoke
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Crash early on ambiguous startup and configuration states instead of degrading silently
Treat required imports as hard dependencies and fail immediately if they are missing (no soft dependency pattern)
Make ruff and mypy blocking quality gates rather than advisory tools
Model required dependencies as non-optional and default to full-strength behavior (no Optional types for required dependencies)
Run required validation unconditionally; a missing component is itself a failure
Validate required capabilities at startup instead of probing for them at runtime
Validate configuration, schema, and extensions during bootstrap rather than on first use
Route configuration through validated bootstrap paths instead of inline shell environment variables
Log important decisions with context and instrument the system with metrics (Radical Visibility)
Define and verify interfaces before writing or referencing implementations (Protocol-First Design)
Resolve lint findings with structural fixes; suppress only with documented necessity
Use Python's functional tools (itertools, functools) when they make code clearer and more local
Keep public behavior documented as part of the interface contract; every public function must have a Google-style docstring
Use precise exception types and actionable, context-rich error messages; all application exceptions should inherit from a base exception with structured context
Files:
tests/test_lint_capture_lifecycle.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.py: Treat tests as executable behavioral contracts and update them with code changes (Testing as Specification)
Prove critical behavior with real functional workflows before relying on unit tests or mocks (Functional Testing Is the Proof)
Files:
tests/test_lint_capture_lifecycle.py
**/{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/contract_v1_test.gogo/internal/hooks/contract_v1.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/agenthooks/kimi_settings.gogo/internal/hooks/contract_v1_test.gogo/internal/agenthookscli/main_internal_test.gogo/internal/agenthooks/state_artifacts.gogo/internal/codeintelcli/main_internal_test.gogo/internal/hooks/contract_v1.gogo/internal/agenthooks/settings_test.gogo/internal/agenthooks/settings.go
🔇 Additional comments (18)
tests/test_lint_capture_lifecycle.py (1)
13-22: LGTM!Also applies to: 25-45
.coding-ethos/.gitignore (1)
3-10: LGTM!go/go.mod (1)
24-24: LGTM!Also applies to: 39-60
README.md (1)
1581-1581: LGTM!Also applies to: 1590-1661, 1673-1677, 1688-1694
go/internal/codeintelcli/main_internal_test.go (1)
27-27: LGTM!Also applies to: 39-43, 138-138, 465-465, 523-527, 577-581, 676-676, 785-789, 808-808, 817-820, 825-828, 833-836, 889-892, 897-900, 905-908, 913-917, 1340-1344, 1349-1353, 1358-1359, 1413-1413, 1418-1419
go/internal/hooks/contract_v1.go (2)
186-230: Past-flagged provider validation gap is fixed.The new
hookContractV1ProviderFromHintresolves strictly from the declaredproviderhint (no source/environment fallback) andvalidateHookContractProvidernow rejects both an unresolvable hint and one that maps outsidehookContractV1Providers(). This correctly addresses the earlier concern about acceptingevent.ProviderHintas-is.
337-399: LGTM!go/internal/hooks/contract_v1_test.go (1)
117-126: LGTM!docs/HOOK_CONTRACT_V1.md (2)
1-123: LGTM!
124-191: LGTM!Also applies to: 197-199
go/internal/agenthooks/kimi_settings.go (1)
16-174: LGTM!go/internal/agenthooks/settings.go (1)
88-88: LGTM!Also applies to: 110-110, 147-147, 275-276, 1493-1534
go/internal/agenthooks/settings_test.go (1)
77-137: LGTM!Also applies to: 874-875, 897-904, 918-1065
go/internal/agenthooks/state_artifacts.go (1)
94-153: LGTM!Also applies to: 180-239
go/internal/agenthookscli/main_internal_test.go (1)
76-107: LGTM!go/cmd/coding-ethos-run/dispatch.go (1)
983-1002: Past root-precedence issue resolved.
CODE_ETHOS_CONSUMER_ROOTis now sourced from--repo-root(falling back topaths.Root), separate from the settings-root-derivedenvStateRoot, matching the documented--repo-root/--root/--state-rootsemantics and covered byTestRunAgentHooksCommandExportsRepositoryRootBeforeSettingsRoot.go/cmd/coding-ethos-run/main_test.go (1)
231-248: LGTM!Also applies to: 336-345, 364-397
go/cmd/coding-ethos-run/parent_workflow.go (1)
119-225: LGTM!Also applies to: 402-412, 954-963
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
PR #264 squash notes Summary:
Notable files:
Validation:
Review:
Deferral gate:
Merge notes:
|
1 participant
Summary
runtime-policy sync/checkcommands without rewriting tracked consumer configurationWhy
Local agent supervisors need a stable policy boundary that does not depend on Claude, Codex, Gemini, or Kimi response schemas. They also need to keep provider credentials/settings private while Coding Ethos continues to own policy, MCP, runtime policy, and code-intelligence behavior.
Capability Surface
neutral-v1request validation, response encoding, correlation IDs, and provider-neutral effects.runtime-policy sync/checkartifacts under private state.Impact
Existing provider-native output and repo-local settings remain the default. Supervisors can opt into
neutral-v1, separate--rootfrom--repo-root, and pass a validated external hook command plus an explicit Coding Ethos MCP command. Static command validation rejects shell operators, substitutions, redirections, relative external executables, and invalid MCP commands.Checklist
Validation
go test -buildvcs=false -timeout=90s -short ./go/internal/codeintelcli -covermode=atomic -coverprofile=/tmp/codeintelcli.out -count=10make buildmake check4dc915711aa9e3461a5be45d62e1e0ae22f3ffd0: Build distribution, CodeQL, Coding Ethos SARIF Gate, Go coverage, Go fuzz smoke, OSV dependency scan, OpenSSF Scorecard, Python 3.11/3.13 tests, Unified lint, Validate GitHub workflows, Zizmor, CLANotes
The local and GitHub gates pass on the current branch head. The protected hook chains still emit the repository-wide non-blocking Go coverage goal warning; the blocking 80% coverage floor passes locally and in GitHub Actions.