fix(cloud): add fail-visible sync wrappers - #714
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded Bash and PowerShell wrappers for explicit cloud synchronization. The wrappers preserve output in append-only logs, continue across project failures, return aggregate status codes, and include cross-platform tests, documentation, and Windows CI coverage. ChangesCloud sync wrappers
Estimated code review effort: 3 (Moderate) | ~25 minutes Mergeability Score: 🟡 Moderate · up to The PR adds cross-platform sync wrappers, but its regression harness can pass without executing required wrapper cases and can mask fixture setup failures, allowing wrapper or logging defects to escape CI. Merge should wait for fail-fast test prerequisites and explicit handling of the remaining Windows compatibility and branch-coverage risks. Sequence Diagram(s)sequenceDiagram
participant Operator
participant CloudSyncWrapper
participant Engram
participant AppendOnlyLog
Operator->>CloudSyncWrapper: Supply projects and optional log path
CloudSyncWrapper->>AppendOnlyLog: Record wrapper start
loop For each project
CloudSyncWrapper->>Engram: Run sync --cloud --project
Engram-->>CloudSyncWrapper: Return output and status
CloudSyncWrapper->>AppendOnlyLog: Record output and project result
end
CloudSyncWrapper->>AppendOnlyLog: Record wrapper result
CloudSyncWrapper-->>Operator: Return aggregate exit code
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 47-50: Update every actions/checkout and actions/setup-go step in
the CI workflow to reference a full commit SHA instead of the v6 tag. Add
persist-credentials: false to each actions/checkout step, while preserving the
existing setup-go configuration.
In `@DOCS.md`:
- Around line 1501-1507: Update the “Inspecting the last failure” command in
DOCS.md to use ${ENGRAM_DATA_DIR:-$HOME/.engram} when locating
cloud-sync-projects.log, matching the wrapper’s default directory. Add an
equivalent PowerShell command using the same fallback behavior for Windows
operators.
In `@tools/cloud_sync_projects_test.go`:
- Around line 153-162: Add deterministic tests for log-write failure in the
wrapper test coverage: pass an existing directory as the log path to both
wrapper variants, assert each exits with status 1, and verify `engram` is not
executed. Reuse the existing test helpers and assertions around `run` and
`fakeEngram` without changing unrelated cases.
In `@tools/cloud-sync-projects.ps1`:
- Around line 81-82: Update the sync command pipeline in Invoke-Project to pass
Tee-Object output through Out-Host, ensuring the function returns only $exitCode
and does not add successful command output to the caller’s overall result.
🪄 Autofix
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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fafaeff4-bca2-4907-8bc6-b09b4e43960b
📒 Files selected for processing (5)
.github/workflows/ci.ymlDOCS.mdtools/cloud-sync-projects.ps1tools/cloud-sync-projects.shtools/cloud_sync_projects_test.go
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 (1)
tools/cloud-sync-projects.ps1 (1)
62-63: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse one encoding for the complete log.
Windows PowerShell 5.1 writes
Tee-Objectoutput as UTF-16LE by default, whileWrite-LogLineuses UTF-8. The test harness supports bothpwshandpowershell, so one run can append mixed encodings to$resolvedLog. ReplaceTee-Objectwith an explicit UTF-8 writer, and add a regression test with non-ASCII output.🤖 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 `@tools/cloud-sync-projects.ps1` around lines 62 - 63, Update the logging pipeline around Write-LogLine and the Tee-Object usage to write all output through an explicit UTF-8 mechanism compatible with both pwsh and Windows PowerShell 5.1, preventing mixed encodings in $resolvedLog. Remove the default Tee-Object path, preserve existing log/error behavior, and add a regression test covering non-ASCII output.
🤖 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 `@tools/cloud-sync-projects.ps1`:
- Around line 62-63: Update the logging pipeline around Write-LogLine and the
Tee-Object usage to write all output through an explicit UTF-8 mechanism
compatible with both pwsh and Windows PowerShell 5.1, preventing mixed encodings
in $resolvedLog. Remove the default Tee-Object path, preserve existing log/error
behavior, and add a regression test covering non-ASCII output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ef049f70-3876-4d10-8938-72e2496823fd
📒 Files selected for processing (1)
tools/cloud-sync-projects.ps1
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/cloud_sync_projects_test.go (1)
146-165: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd regression coverage for help handling.
The new validation coverage tests missing projects and invalid log paths, but it never invokes
-Helpor--help. The PowerShell wrapper added explicit help handling, including an exit-code contract, so this behavior can regress without detection.Add deterministic help tests for each available wrapper and assert the usage text and the expected successful help exit code.
As per path instructions, “Verify coverage of happy path, error paths, and edge cases. Tests must be deterministic. Behavior changes without tests should be blocked.”
🤖 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 `@tools/cloud_sync_projects_test.go` around lines 146 - 165, Add deterministic help-handling regression tests alongside “MissingArgsUsage2” and “InvalidLogExits1” for every available wrapper, invoking both supported help forms where applicable (-Help and --help). Assert each invocation returns the documented successful exit code and includes the expected usage text.Source: Path instructions
🤖 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 `@DOCS.md`:
- Around line 1481-1508: Update the documentation block around the headings
“Bash: tools/cloud-sync-projects.sh”, “PowerShell:
tools/cloud-sync-projects.ps1”, and “Inspecting the last failure” to add blank
lines before and after each heading and fenced code block, satisfying
markdownlint MD022 and MD031 without changing the documented content.
In `@tools/cloud-sync-projects.ps1`:
- Around line 32-39: Update the help handling around $helpRequested so both
-Help and --help exit successfully with code 0 instead of 2, while preserving
argument filtering and usage output. Add deterministic tests covering each help
form and asserting the successful exit status.
---
Outside diff comments:
In `@tools/cloud_sync_projects_test.go`:
- Around line 146-165: Add deterministic help-handling regression tests
alongside “MissingArgsUsage2” and “InvalidLogExits1” for every available
wrapper, invoking both supported help forms where applicable (-Help and --help).
Assert each invocation returns the documented successful exit code and includes
the expected usage text.
🪄 Autofix
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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e45779db-7ddd-462a-a8c1-a1ccb8502acd
📒 Files selected for processing (5)
.github/workflows/ci.ymlDOCS.mdtools/cloud-sync-projects.ps1tools/cloud-sync-projects.shtools/cloud_sync_projects_test.go
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 (2)
tools/cloud_sync_projects_test.go (2)
68-94: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCover log-path precedence and isolate the test environment.
Every current case passes an explicit
--logor-LogPathat Line 129. The suite does not verifyENGRAM_CLOUD_SYNC_LOGor the default log path.runalso returns a hard-coded log path at Line 89 and Line 94. Add deterministic cases for the default path, environment override, and explicit override. Allow the helper to set or clearENGRAM_CLOUD_SYNC_LOGand return the actual expected log path.As per path instructions, “Verify coverage of happy path, error paths, and edge cases. Tests must be deterministic. Behavior changes without tests should be blocked.”
Also applies to: 126-130
🤖 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 `@tools/cloud_sync_projects_test.go` around lines 68 - 94, Extend the test helper run to set or clear ENGRAM_CLOUD_SYNC_LOG and derive the returned log path from the effective configuration instead of always joining dataDir with a fixed filename. Add deterministic cases covering default-path logging, environment-variable override, and explicit --log/-LogPath precedence, including existing success and error scenarios, while keeping each test environment isolated.Source: Path instructions
97-110: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCover the unsupported PowerShell version path.
On Windows, this matrix runs only
pwsh, so the PowerShell 5.1 guard remains untested. Add a deterministic Windows-only test that invokespowershell.exewhen available and asserts exit code2and the diagnostic, or add a controlled seam for the version check.🤖 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 `@tools/cloud_sync_projects_test.go` around lines 97 - 110, Extend the wrapper test setup around the interpreter matrix to cover the PowerShell 5.1 rejection path on Windows. When powershell.exe is available, invoke the PowerShell script deterministically and assert exit code 2 plus its expected diagnostic; otherwise skip only that targeted case, while preserving the existing pwsh coverage.Source: Path instructions
🤖 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 `@tools/cloud_sync_projects_test.go`:
- Around line 68-94: Extend the test helper run to set or clear
ENGRAM_CLOUD_SYNC_LOG and derive the returned log path from the effective
configuration instead of always joining dataDir with a fixed filename. Add
deterministic cases covering default-path logging, environment-variable
override, and explicit --log/-LogPath precedence, including existing success and
error scenarios, while keeping each test environment isolated.
- Around line 97-110: Extend the wrapper test setup around the interpreter
matrix to cover the PowerShell 5.1 rejection path on Windows. When
powershell.exe is available, invoke the PowerShell script deterministically and
assert exit code 2 plus its expected diagnostic; otherwise skip only that
targeted case, while preserving the existing pwsh coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e1ecda75-5c1a-4d11-b46c-1fa44e50f580
📒 Files selected for processing (4)
DOCS.mdtools/cloud-sync-projects.ps1tools/cloud-sync-projects.shtools/cloud_sync_projects_test.go
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 `@tools/cloud_sync_projects_test.go`:
- Around line 110-111: Handle and assert the errors returned by both os.MkdirAll
calls in the test setup, for fakeDir and dataDir, so fixture creation failures
stop the test immediately instead of reaching later wrapper assertions.
- Around line 98-104: Update the interpreter setup in the test around the
interps collection to fail explicitly when the required bash or pwsh executable
cannot be found, instead of silently leaving interps empty. Ensure the Windows
path still selects the available PowerShell wrapper and preserves the existing
test execution behavior for supported interpreters.
🪄 Autofix
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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d2957886-62d6-4f7d-9771-4c6821a5a3b3
📒 Files selected for processing (1)
tools/cloud_sync_projects_test.go
| os.MkdirAll(fakeDir, 0o755) | ||
| os.MkdirAll(dataDir, 0o755) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Check fixture directory creation errors.
Lines 110-111 ignore os.MkdirAll errors. If setup fails, later assertions report wrapper behavior instead of the fixture failure.
Proposed fix
- os.MkdirAll(fakeDir, 0o755)
- os.MkdirAll(dataDir, 0o755)
+ if err := os.MkdirAll(fakeDir, 0o755); err != nil {
+ t.Fatal(err)
+ }
+ if err := os.MkdirAll(dataDir, 0o755); err != nil {
+ t.Fatal(err)
+ }📝 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.
| os.MkdirAll(fakeDir, 0o755) | |
| os.MkdirAll(dataDir, 0o755) | |
| if err := os.MkdirAll(fakeDir, 0o755); err != nil { | |
| t.Fatal(err) | |
| } | |
| if err := os.MkdirAll(dataDir, 0o755); err != nil { | |
| t.Fatal(err) | |
| } |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 110-110: Error return value of os.MkdirAll is not checked
(errcheck)
[error] 111-111: Error return value of os.MkdirAll is not checked
(errcheck)
🤖 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 `@tools/cloud_sync_projects_test.go` around lines 110 - 111, Handle and assert
the errors returned by both os.MkdirAll calls in the test setup, for fakeDir and
dataDir, so fixture creation failures stop the test immediately instead of
reaching later wrapper assertions.
Source: Linters/SAST tools
2 participants
🔗 Linked Issue
Closes #341
🏷️ PR Type
type:bug- Bug fixtype:feature- New featuretype:docs- Documentation onlytype:refactor- Code refactoring (no behavior change)type:chore- Maintenance, dependencies, toolingtype:breaking-change- Breaking change📝 Summary
📂 Changes
tools/cloud-sync-projects.shtools/cloud-sync-projects.ps1tools/cloud_sync_projects_test.go.github/workflows/ci.ymlwindows-latest.DOCS.md🧪 Test Plan
go test ./... -count=1go test -tags e2e ./internal/server/... -count=1go test ./tools/ -v -count=1shellcheck tools/cloud-sync-projects.shandbash -n tools/cloud-sync-projects.shgo build ./...andgo vet ./tools/engramexecutable.The new Windows CI job provides the PowerShell runtime proof that is unavailable on the local macOS host.
🤖 Automated Checks
Closes #N/Fixes #N/Resolves #Nstatus:approvedlabeltype:*labelgo test ./...passesgo test -tags e2e ./internal/server/...passeswindows-latest✅ Contributor Checklist
Closes #341)type:*label to this PRgo test ./... -count=1go test -tags e2e ./internal/server/... -count=1Co-Authored-Bytrailers in commits💬 Notes for Reviewers
Native
ENGRAM_CLOUD_AUTOSYNC=1remains the recommended mode whenengram serveorengram mcpcan stay running. These wrappers are intentionally scoped to no-daemon scheduler environments and require every project explicitly; they do not implement or implysync-all.Rollback is limited to these two wrappers, their focused harness and CI job, and the matching documentation section.
Summary by CodeRabbit
New Features
Documentation
Tests