Skip to content

Harden build manifest persistence #61

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
samdark wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Harden build manifest persistence #61

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 34 additions & 11 deletions src/Build/BuildManifest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

namespace YiiPress\Build;

use JsonException;
use RuntimeException;

use function dirname;
Expand Down Expand Up @@ -33,13 +34,19 @@ public function load(): void

$json = file_get_contents($this->manifestPath);
if ($json === false) {
$this->entries = [];
$this->reset();
return;
}
Comment on lines 35 to +39

try {
$data = json_decode($json, true, 512, JSON_THROW_ON_ERROR);
} catch (JsonException) {
$this->reset();
return;
}

$data = json_decode($json, true, 512, JSON_THROW_ON_ERROR);
if (!is_array($data)) {
$this->entries = [];
$this->reset();
return;
}

Expand All @@ -62,14 +69,30 @@ public function save(): void
throw new RuntimeException(sprintf('Directory "%s" was not created', $dir));
}

file_put_contents(
$this->manifestPath,
json_encode([
'entries' => $this->entries,
'configFiles' => $this->configFiles,
'trackedDirectories' => $this->trackedDirectories,
], JSON_THROW_ON_ERROR | JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES),
);
$json = json_encode([
'entries' => $this->entries,
'configFiles' => $this->configFiles,
'trackedDirectories' => $this->trackedDirectories,
], JSON_THROW_ON_ERROR | JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES);

$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));
Comment on lines +78 to +87

@coderabbitai coderabbitai Bot Jun 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

}
}

private function reset(): void
{
$this->entries = [];
$this->configFiles = [];
$this->trackedDirectories = [];
}

public function isChanged(string $sourceFile): bool
Expand Down
31 changes: 31 additions & 0 deletions tests/Unit/Build/BuildManifestTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,37 @@ public function testEmptyManifestLoadsCleanly(): void
assertSame([], $manifest->removedOutputs([$sourceFile]));
}

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'));
}
Comment on lines +135 to +164

@coderabbitai coderabbitai Bot Jun 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

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


public function testReplaceReturnsOutputsThatAreNoLongerReferenced(): void
{
$sourceFile = $this->tempDir . '/asset.css';
Expand Down