Harden build manifest persistence#61
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthrough
ChangesFileWriter Atomic Write Hardening and Save Cleanup Tests
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
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)
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 |
There was a problem hiding this comment.
Pull request overview
This PR hardens build manifest persistence so builds don’t crash when the manifest is unreadable/corrupt, and so manifest writes are performed via a temp file + rename to reduce the risk of partial writes.
Changes:
- Treat unreadable/corrupt manifest JSON as a cache miss (reset state instead of throwing).
- Write manifests to a same-directory temporary file and rename into place; throw on write/replace failures.
- Add unit tests covering corrupt manifest load behavior and ensuring no
.tmpfile remains after save.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/Build/BuildManifest.php |
Makes manifest loading tolerant to corrupt JSON and implements atomic-ish save via temp file + rename with explicit failure exceptions. |
tests/Unit/Build/BuildManifestTest.php |
Adds regression tests for corrupt-manifest handling and temp-file cleanup behavior on save. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $json = file_get_contents($this->manifestPath); | ||
| if ($json === false) { | ||
| $this->entries = []; | ||
| $this->reset(); | ||
| return; | ||
| } |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Build/BuildManifest.php (1)
30-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset full state when manifest file is absent.
On Line 30-33, only
$this->entriesis cleared. If the sameBuildManifestinstance is reused, staleconfigFiles/trackedDirectoriescan leak across loads. Usereset()here for a true cache miss state.Proposed fix
public function load(): void { if (!is_file($this->manifestPath)) { - $this->entries = []; + $this->reset(); return; }🤖 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 `@src/Build/BuildManifest.php` around lines 30 - 33, In the BuildManifest class, when the manifest file is absent (the if (!is_file($this->manifestPath)) branch) replace the partial reset that only sets $this->entries = [] with a full state reset by calling $this->reset() so that configFiles and trackedDirectories are also cleared and no stale state leaks across loads.
🤖 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 `@src/Build/BuildManifest.php`:
- Around line 78-87: The save() logic currently writes to a deterministic ".tmp"
file and treats any non-false return from file_put_contents as success; change
it to create a unique temp file per call (use
tempnam(dirname($this->manifestPath), basename($this->manifestPath) . '.tmp') or
similar) and write the JSON with exclusive lock, verifying the number of bytes
written equals strlen($json) (or perform a looped fwrite to guarantee full
write); on any write shortfall or error unlink the unique temp file and throw,
and only after a successful full-byte write perform the rename($tmp,
$this->manifestPath) and on rename failure unlink the temp and throw. Ensure you
reference the same $this->manifestPath for temp directory creation and cleanup,
and update the temporaryPath variable usage accordingly.
In `@tests/Unit/Build/BuildManifestTest.php`:
- Around line 135-164: Add PHPUnit tests that cover the remaining branches in
BuildManifest::load() and BuildManifest::save(): add a test (e.g.,
testLoadResetsWhenDecodedJsonIsNotArray) that writes a manifest file whose JSON
decodes to a non-array (for example "null" or a JSON string), calls
BuildManifest::load(), and asserts the manifest was reset (sourceFiles(),
configFiles() empty and isChanged() true for a new source); and add two tests
for save failure branches (e.g., testSaveHandlesWriteFailure and
testSaveHandlesRenameFailure) that simulate file write and rename failures (use
vfsStream or create a non-writable directory to force file_put_contents/rename
to fail), call BuildManifest::save() and assert the code follows the expected
failure behavior (the test should expect the specific exception or error
handling your implementation uses and verify no persistent .tmp file remains or
that the temporary file handling matches the branch behavior).
---
Outside diff comments:
In `@src/Build/BuildManifest.php`:
- Around line 30-33: In the BuildManifest class, when the manifest file is
absent (the if (!is_file($this->manifestPath)) branch) replace the partial reset
that only sets $this->entries = [] with a full state reset by calling
$this->reset() so that configFiles and trackedDirectories are also cleared and
no stale state leaks across loads.
🪄 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: b1cabc00-6f1b-432d-a793-07a9a4f76b90
📒 Files selected for processing (2)
src/Build/BuildManifest.phptests/Unit/Build/BuildManifestTest.php
samdark
force-pushed
the
fix-build-manifest-atomic-save
branch
from
3e1c60d to
5022ba1
Compare
Summary
Tests
Summary by CodeRabbit
Bug Fixes
Documentation
Tests