Fix/shenron audit fixes - #6
Conversation
7-task plan addressing High/Medium findings from the deep audit: - H1: OpenCode prune of stale managed leaves - H2: lint failure + lock-test logic bug - H3: thread io.Writer through sync runtime - M1: persist Managed before native writes (crash safety) - M5: deduplicate InstallLocal/publishStaged - Doc drift + CI workflow
📝 WalkthroughWalkthroughThe PR adds CI validation and repository hygiene updates, documents audit fixes, separates sync stdout from stderr, persists push state earlier, adds OpenCode stale managed-entry pruning, and reuses staged publication logic for local package installation. ChangesShenron audit fixes
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RunPackagePush
participant runPushAt
participant Generate
participant OpenCodeAdapter
participant StateFile
RunPackagePush->>runPushAt: start package push
runPushAt->>StateFile: save managed state before native writes
runPushAt->>Generate: generate fragments with loaded state
Generate->>OpenCodeAdapter: prune stale managed leaves and merge fragments
OpenCodeAdapter-->>Generate: updated opencode.json
Generate-->>runPushAt: generated files
runPushAt-->>RunPackagePush: push result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/integration_test.go (1)
260-268: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
TestEndToEnd_TargetedPushNoClaudeOrphansno longer detects the regression it guards against.With the new stderr routing,
runPushAtemits orphan warnings viaprintOrphanWarnings(stderr, ...). The test captures stdout (out) and discards stderr (_), then checksoutfor"warning: orphaned". Since orphan warnings now go to stderr,outwill never contain them — the assertion always passes regardless of whether Claude orphan warnings are produced.🐛 Proposed fix: check stderr instead of stdout
- out, _, err := cli.CaptureOutput(func() error { + _, stderr, err := cli.CaptureOutput(func() error { return cli.RunPush(env.pushOpts("opencode")) }) if err != nil { t.Fatalf("targeted push: %v", err) } - if strings.Contains(out, "warning: orphaned") && strings.Contains(out, env.claudeDir) { - t.Fatalf("targeted push should not warn about Claude orphans, got:\n%s", out) + if strings.Contains(stderr, "warning: orphaned") && strings.Contains(stderr, env.claudeDir) { + t.Fatalf("targeted push should not warn about Claude orphans, got:\n%s", stderr) }🤖 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/integration_test.go` around lines 260 - 268, Update TestEndToEnd_TargetedPushNoClaudeOrphans to retain and inspect the stderr value returned by cli.CaptureOutput instead of discarding it, then perform the orphan-warning assertion against stderr while preserving the existing Claude-directory check.
🧹 Nitpick comments (4)
docs/plans/2026-07-11-shenron-audit-fixes.md (1)
66-66: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSpecify language for fenced code blocks.
Two fenced code blocks (lines 66 and 900) lack a language specifier, triggering markdownlint MD040. The block at line 66 is a bash snippet; the one at line 900 is a plain text list item.
♻️ Proposed fix
-``` +```bash # Backup files *.bakAnd:
-``` - **Markdown frontmatter** : `github.com/adrg/frontmatter` +```text +- **Markdown frontmatter** : `github.com/adrg/frontmatter`Also applies to: 900-900
🤖 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/plans/2026-07-11-shenron-audit-fixes.md` at line 66, Specify explicit languages for both unlabeled fenced code blocks in the documentation: mark the backup-file snippet near line 66 as bash and the plain-text list item near line 900 as text, preserving their existing contents.Source: Linters/SAST tools
.github/workflows/ci.yml (1)
16-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider using the official golangci-lint GitHub action.
Piping a remote install script to
shis a supply-chain risk. The officialgolangci/golangci-lint-actionpins versions, caches results, and handles Go compatibility. This is optional but improves supply-chain posture.♻️ Proposed refactor
- - name: Install golangci-lint - run: | - curl -sSf https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.62.2 + - uses: golangci/golangci-lint-action@v6 + with: + version: v1.62.2🤖 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 @.github/workflows/ci.yml around lines 16 - 18, Replace the remote install-script step named “Install golangci-lint” with the official golangci/golangci-lint-action in the workflow, configuring the required linter version and preserving the existing lint execution behavior.internal/cli/sync_runtime.go (1)
36-63: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
CaptureOutputcan deadlock on large output.Both pipes are written to by
fnbut only read afterfnreturns. Iffnwrites more than the pipe buffer (~64KB on Linux) to either stdout or stderr, the write will block and the test will hang. Consider reading from both pipes concurrently via goroutines whilefnruns.♻️ Suggested fix: read pipes concurrently
func CaptureOutput(fn func() error) (stdout, stderr string, err error) { oldOut, oldErr := os.Stdout, os.Stderr rOut, wOut, err := os.Pipe() if err != nil { return "", "", err } rErr, wErr, err := os.Pipe() if err != nil { _ = wOut.Close() return "", "", err } os.Stdout = wOut os.Stderr = wErr + var outData, errData []byte + var outErr, errErr error + doneOut := make(chan struct{}) + doneErr := make(chan struct{}) + go func() { outData, outErr = io.ReadAll(rOut); close(doneOut) }() + go func() { errData, errErr = io.ReadAll(rErr); close(doneErr) }() + runErr := fn() _ = wOut.Close() _ = wErr.Close() os.Stdout = oldOut os.Stderr = oldErr - outData, _ := io.ReadAll(rOut) - errData, _ := io.ReadAll(rErr) + <-doneOut + <-doneErr + _ = outErr + _ = errErr return string(outData), string(errData), runErr }🤖 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/sync_runtime.go` around lines 36 - 63, Update CaptureOutput so rOut and rErr are drained concurrently while fn executes, using separate readers or goroutines for stdout and stderr; wait for both reads to complete after fn returns, then restore the original streams and return the captured data and runErr.internal/adapter/opencode/adapter_test.go (1)
231-264: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a native entry and content verification to strengthen coverage.
The test verifies stale managed entries are pruned, but doesn't confirm that (1) native (non-managed) entries survive
PruneManagedand (2) the remainingagent.buildhas the updated description from fragments (confirmingMergeFileupsert runs after pruning). Adding a native agent and checkingbuild's description would cover both properties in one test.🧪 Suggested test enhancement
func TestMergeFilePrunesStaleManagedAgent(t *testing.T) { a := opencode.NewAdapter() existing := []byte(`{ "agent": { "build": {"description": "Build"}, + "native": {"description": "user-created, not managed"}, "stale": {"description": "was managed, now removed from pivot"} }, "command": {} }`) fragments := map[string]any{ "agent.build": map[string]any{"description": "Build and deploy agent"}, } managed := map[string][]string{ "agent": {"build", "stale"}, "command": {}, } pruned, err := a.PruneManaged("opencode.json", existing, managed, fragments) if err != nil { t.Fatal(err) } var root map[string]any if err := json.Unmarshal(pruned, &root); err != nil { t.Fatal(err) } agents := root["agent"].(map[string]any) if _, ok := agents["stale"]; ok { t.Error("stale managed agent should be pruned") } if _, ok := agents["build"]; !ok { t.Error("build agent should remain") } + if _, ok := agents["native"]; !ok { + t.Error("native (non-managed) agent should be preserved") + } + build := agents["build"].(map[string]any) + if desc, _ := build["description"].(string); desc != "Build and deploy agent" { + t.Errorf("build agent should have updated description from fragments, got %q", desc) + } }🤖 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/adapter/opencode/adapter_test.go` around lines 231 - 264, Enhance TestMergeFilePrunesStaleManagedAgent by adding a native, non-managed agent to the existing configuration and asserting it survives PruneManaged. Also verify the retained agent.build entry has the fragment’s updated description, confirming the post-pruning merge upsert.
🤖 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:
- Line 12: Update the actions/checkout@v4 step to disable persisted GitHub
credentials by setting persist-credentials to false, while leaving the checkout
behavior otherwise unchanged.
---
Outside diff comments:
In `@internal/integration_test.go`:
- Around line 260-268: Update TestEndToEnd_TargetedPushNoClaudeOrphans to retain
and inspect the stderr value returned by cli.CaptureOutput instead of discarding
it, then perform the orphan-warning assertion against stderr while preserving
the existing Claude-directory check.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 16-18: Replace the remote install-script step named “Install
golangci-lint” with the official golangci/golangci-lint-action in the workflow,
configuring the required linter version and preserving the existing lint
execution behavior.
In `@docs/plans/2026-07-11-shenron-audit-fixes.md`:
- Line 66: Specify explicit languages for both unlabeled fenced code blocks in
the documentation: mark the backup-file snippet near line 66 as bash and the
plain-text list item near line 900 as text, preserving their existing contents.
In `@internal/adapter/opencode/adapter_test.go`:
- Around line 231-264: Enhance TestMergeFilePrunesStaleManagedAgent by adding a
native, non-managed agent to the existing configuration and asserting it
survives PruneManaged. Also verify the retained agent.build entry has the
fragment’s updated description, confirming the post-pruning merge upsert.
In `@internal/cli/sync_runtime.go`:
- Around line 36-63: Update CaptureOutput so rOut and rErr are drained
concurrently while fn executes, using separate readers or goroutines for stdout
and stderr; wait for both reads to complete after fn returns, then restore the
original streams and return the captured data and runErr.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: cc0a5313-6dd6-4506-aa63-39576c19373e
📒 Files selected for processing (20)
.github/workflows/ci.yml.gitignoreMakefiledocs/ARCHITECTURE.mddocs/plans/2026-07-11-shenron-audit-fixes.mddocs/prd/shenron.mdinternal/adapter/adapter.gointernal/adapter/opencode/adapter.gointernal/adapter/opencode/adapter_test.gointernal/adapter/opencode/ordered.gointernal/cli/package_apply.gointernal/cli/package_test.gointernal/cli/sync.gointernal/cli/sync_runtime.gointernal/cli/sync_test.gointernal/diff/differ.gointernal/diff/state.gointernal/integration_test.gointernal/package/package.gointernal/package/package_test.go
💤 Files with no reviewable changes (1)
- docs/prd/shenron.md
| verify: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Add persist-credentials: false to the checkout step.
By default, actions/checkout@v4 persists the GITHUB_TOKEN in .git/config for subsequent steps. Since this job only needs the source tree (no git pushes), disabling credential persistence reduces the risk of token leakage if a later step is compromised.
🔒️ Proposed fix
steps:
- - uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ persist-credentials: false
- uses: actions/setup-go@v5📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - uses: actions/checkout@v4 | |
| - uses: actions/checkout@v4 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 12-12: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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 @.github/workflows/ci.yml at line 12, Update the actions/checkout@v4 step to
disable persisted GitHub credentials by setting persist-credentials to false,
while leaving the checkout behavior otherwise unchanged.
Source: Linters/SAST tools
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores