Harden build manifest persistence#61
Open
samdark wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughBuildManifest strengthens error resilience and write safety. The manifest loader now handles JSON parse failures and missing files by resetting internal state, while the save method writes atomically via a temporary file and rename operation to prevent partial or corrupted writes. ChangesBuildManifest robustness and atomic writes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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)
⚔️ Resolve merge conflicts
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.
Comment on lines
35
to
+39
| $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
Comment on lines
+78
to
+87
| $temporaryPath = $this->manifestPath . '.tmp'; | ||
| if (file_put_contents($temporaryPath, $json) === false) { | ||
| throw new RuntimeException(sprintf('Unable to write build manifest temporary file "%s".', $temporaryPath)); | ||
| } | ||
|
|
||
| if (!rename($temporaryPath, $this->manifestPath)) { | ||
| if (is_file($temporaryPath)) { | ||
| unlink($temporaryPath); | ||
| } | ||
| throw new RuntimeException(sprintf('Unable to replace build manifest "%s".', $this->manifestPath)); |
There was a problem hiding this comment.
Avoid shared .tmp path collisions and detect short writes.
Line 78 uses a deterministic temp filename. Concurrent save() calls targeting the same manifest can race on the same temp file and cause failed/incorrect persistence. Also Line 79 treats short writes as success. Use a unique temp file per call and validate full-byte write.
Proposed fix
- $temporaryPath = $this->manifestPath . '.tmp';
- if (file_put_contents($temporaryPath, $json) === false) {
+ $temporaryPath = tempnam($dir, basename($this->manifestPath) . '.tmp.');
+ if ($temporaryPath === false) {
+ throw new RuntimeException(sprintf('Unable to create build manifest temporary file in "%s".', $dir));
+ }
+ $written = file_put_contents($temporaryPath, $json);
+ if ($written !== strlen($json)) {
+ if (is_file($temporaryPath)) {
+ unlink($temporaryPath);
+ }
throw new RuntimeException(sprintf('Unable to write build manifest temporary file "%s".', $temporaryPath));
}🤖 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 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.
Comment on lines
+135
to
+164
| public function testCorruptManifestLoadsAsEmptyCache(): void | ||
| { | ||
| $manifestPath = $this->tempDir . '/manifest.json'; | ||
| file_put_contents($manifestPath, '{"entries":'); | ||
|
|
||
| $manifest = new BuildManifest($manifestPath); | ||
| $manifest->load(); | ||
|
|
||
| $sourceFile = $this->tempDir . '/entry.md'; | ||
| file_put_contents($sourceFile, '# Hello'); | ||
|
|
||
| assertTrue($manifest->isChanged($sourceFile)); | ||
| assertSame([], $manifest->sourceFiles()); | ||
| assertSame([], $manifest->configFiles()); | ||
| assertFalse($manifest->hasTrackedDirectories()); | ||
| } | ||
|
|
||
| public function testSaveDoesNotLeaveTemporaryManifestFile(): void | ||
| { | ||
| $sourceFile = $this->tempDir . '/entry.md'; | ||
| file_put_contents($sourceFile, '# Hello'); | ||
| $manifestPath = $this->tempDir . '/manifest.json'; | ||
|
|
||
| $manifest = new BuildManifest($manifestPath); | ||
| $manifest->record($sourceFile, ['/out/entry/index.html']); | ||
| $manifest->save(); | ||
|
|
||
| assertTrue(is_file($manifestPath)); | ||
| assertFalse(is_file($manifestPath . '.tmp')); | ||
| } |
There was a problem hiding this comment.
Add tests for the remaining new error branches in load()/save().
The new tests are good, but they don’t cover all newly introduced branches (e.g., decoded non-array reset path and save() failure paths for write/rename). Please add targeted PHPUnit cases for those branches so persistence hardening is guarded against regressions.
As per coding guidelines, **/*Test.php: “For each piece of code add a test using phpunit.”
🤖 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 `@tests/Unit/Build/BuildManifestTest.php` around lines 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).
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Tests
Summary by CodeRabbit
Bug Fixes
Tests