Add generated surface sync state contract#258
Conversation
|
Warning Review limit reached
More reviews will be available in 7 minutes and 51 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughIntroduces a new ChangesInstall/sync state contract for generated agent surfaces
Sequence Diagram(s)sequenceDiagram
rect rgba(70, 130, 180, 0.5)
Note over policycli,Disk: Dry-run path
end
participant policycli
participant StateArtifacts as StateArtifacts(surface)
participant syncstate
participant Disk as Disk (install-sync.json)
participant pyproject as pyproject.toml
participant git as git rev-parse HEAD
policycli->>StateArtifacts: StateArtifacts(ethosRoot, repoRoot, ...)
StateArtifacts-->>policycli: []Artifact
alt --dry-run
policycli->>syncstate: Plan(repoRoot, requestedAction, artifacts)
syncstate-->>policycli: Report
policycli->>policycli: writeSyncStateReport → feedback output
else normal sync
policycli->>syncstate: Upsert(UpsertOptions)
syncstate->>Disk: ReadFile (load or init State)
syncstate->>pyproject: runtimeVersion()
syncstate->>git: runtimeCommit()
syncstate->>syncstate: merge sources/targets/artifacts, timestampArtifacts
syncstate->>Disk: WriteFile (indented JSON)
syncstate-->>policycli: State
policycli->>policycli: emit per-path output + sync-state file path
end
policycli->>syncstate: Doctor(repoRoot) or RepairPlan(repoRoot)
syncstate->>Disk: ReadFile
syncstate->>syncstate: artifactReports (SHA compare) + sourceReports
syncstate-->>policycli: Report (pass/fail/drifted/would-update)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 unit tests (beta)
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 a sync state tracking mechanism to record, plan, and validate the installation state of generated surfaces (such as tool configs, Gemini prompts, and agent skills) under '.coding-ethos/state/install-sync.json'. It adds dry-run capabilities, a 'doctor' command to check for drift, and a 'repair-plan' command to plan updates for managed artifacts. The feedback suggests addressing a path traversal vulnerability in relative path resolution, gracefully falling back to a fresh state if the sync state file is corrupted, and avoiding hardcoded root paths in the agent hooks CLI to properly support submodule installations.
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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/codeintelcli/main_internal_test.go`:
- Around line 949-956: The captureStdout call currently wraps the entire for
loop iterating over commands, causing all stdout from all command invocations to
accumulate before draining, which can block the pipe and hang the test with
large combined output. Move the captureStdout call inside the for loop so that
stdout is captured and drained for each individual command invocation
separately, preventing pipe buffer overflow.
In `@go/internal/geminiprompts/render.go`:
- Around line 125-132: The `PromptPackPath` used in the `syncstate.Artifacts`
call is currently resolving to a location outside of
`go/internal/geminiprompts/`, which violates the prompt-pack location contract.
Update the `PromptPackPath` constant or variable to point to the correct
location within the `go/internal/geminiprompts/` directory so that the artifact
is persisted to the proper location as required by the coding guidelines.
In `@go/internal/policycli/main.go`:
- Around line 497-499: The parseInstallStateFlags function is reusing
errToolConfigRepoRequired when --repo is missing, but this error message
references "tool config command" which is confusing in the install-state
context. Either create a new dedicated error constant for install-state commands
(e.g., errInstallStateRepoRequired) with an appropriate message, or modify the
error message to be generic enough to work for both tool-config and
install-state contexts. Update the error return statement in
parseInstallStateFlags to use the new error or modify the existing error message
to remove tool-config-specific terminology.
- Around line 578-598: The function compactExistingOrCandidatePaths has a
misleading name that implies it checks for file existence, but it only trims
whitespace, cleans paths using filepath.Clean, and removes duplicates. Rename
the function from compactExistingOrCandidatePaths to compactCandidatePaths to
accurately reflect its actual behavior of compacting and deduplicating candidate
paths without performing any existence validation. Remember to update all call
sites where compactExistingOrCandidatePaths is invoked.
In `@go/internal/syncstate/state.go`:
- Around line 128-157: The `LastWrittenUTC` field assignment in the `Artifacts`
function is dead code because this value is always overwritten by
`timestampArtifacts` in the `Upsert` function before the artifacts are
persisted. Remove the `LastWrittenUTC: now,` line from the Artifact struct
initialization within the loop to eliminate the misleading assignment that
suggests these timestamps are authoritative, since they are never actually used
and will be replaced anyway.
- Around line 717-733: The runtimeVersion function currently searches for the
first line matching "version = " across all sections of the pyproject.toml file,
which can incorrectly match version entries from other sections like
[tool.poetry]. Modify the function to scope the version search to only the
[project] section by tracking when the parser enters the [project] table (detect
lines starting with "[project]") and only return the version when found within
that specific section context, ensuring you don't pick up version entries from
unrelated configuration tables.
- Around line 694-715: The repoRelativePath function has a security flaw where
relative input paths like ../../foo bypass validation and return early at line
697 without checking if they escape the repo root. Additionally, the escape
guard using strings.HasPrefix(relative, "..") on line 705 is not separator-aware
and false-positives on directory names starting with literal `..foo`. Fix this
by normalizing both the relative and absolute path branches against repoRoot
using filepath.Rel and filepath.Join, then validate the result against the repo
root boundary using a separator-aware comparison instead of strings.HasPrefix to
ensure paths cannot escape the repo root whether provided as relative or
absolute.
🪄 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: bc0db41f-68c6-497c-b00f-dcc32b8eea9b
📒 Files selected for processing (13)
README.mdgo/internal/agenthooks/settings.gogo/internal/agenthooks/state_artifacts.gogo/internal/agenthookscli/main.gogo/internal/agenthookscli/main_internal_test.gogo/internal/agentskills/render.gogo/internal/codeintelcli/main_internal_test.gogo/internal/geminiprompts/render.gogo/internal/policycli/main.gogo/internal/policycli/main_internal_test.gogo/internal/syncstate/state.gogo/internal/syncstate/state_test.gogo/internal/toolconfigs/sync.go
📜 Review details
⏰ Context from checks skipped due to timeout. (8)
- GitHub Check: Unified lint
- GitHub Check: Test (Python 3.11)
- GitHub Check: Coding Ethos SARIF Gate / Coding Ethos SARIF Gate
- GitHub Check: Test (Python 3.13)
- GitHub Check: Validate GitHub workflows
- GitHub Check: Go coverage
- GitHub Check: CodeQL (go)
- GitHub Check: Go fuzz smoke
🧰 Additional context used
📓 Path-based instructions (1)
go/internal/geminiprompts/**
📄 CodeRabbit inference engine (AGENTS.md)
Gemini prompt pack should be generated in
go/internal/geminiprompts/
Files:
go/internal/geminiprompts/render.go
🔇 Additional comments (14)
go/internal/syncstate/state.go (2)
159-198: LGTM!
375-476: LGTM!go/internal/syncstate/state_test.go (1)
13-158: LGTM!go/internal/agenthooks/settings.go (1)
317-396: LGTM!go/internal/agenthooks/state_artifacts.go (1)
12-111: LGTM!go/internal/agentskills/render.go (1)
13-14: LGTM!Also applies to: 77-109
go/internal/toolconfigs/sync.go (1)
15-15: LGTM!Also applies to: 77-118
go/internal/policycli/main.go (3)
98-112: LGTM!
122-166: LGTM!Also applies to: 239-275, 306-341
504-539: LGTM!Also applies to: 552-576
go/internal/policycli/main_internal_test.go (1)
228-294: LGTM!Also applies to: 816-828
go/internal/agenthookscli/main.go (1)
17-17: LGTM!Also applies to: 95-152, 274-280
go/internal/agenthookscli/main_internal_test.go (1)
8-15: LGTM!Also applies to: 103-134, 160-200
README.md (1)
793-794: LGTM!Also applies to: 867-871, 884-909, 1484-1484
|
Stage 2 pass completed. Addressed review feedback in follow-up commits:
Validation rerun:
Auto-merge is enabled; GitHub currently reports |
Summary
Closes #180
Validation
go test ./internal/syncstate ./internal/toolconfigs ./internal/agentskills ./internal/geminiprompts ./internal/agenthooks ./internal/agenthookscli ./internal/policyclimake buildmake check(passes; reports the existing non-blocking Go coverage-goal warning)bin/coding-ethos-run policy sync-tool-configs --repo . --ethos-root . --dry-run --format jsonbin/coding-ethos-run policy install-state-doctor --repo . --format toonbin/coding-ethos-run policy install-state-repair-plan --repo . --format json