Harden build error handling and bugfixes#58
Merged
Conversation
📝 WalkthroughWalkthroughAdds a FileWriter utility and migrates writers/commands to it, implements atomic output-directory staging and permalink validation in BuildCommand, hardens front-matter/date parsing with file-aware exceptions, serializes live-reload builds with build-error SSE reporting, tightens parallel-worker exit checks, and updates docs/tests. ChangesBuild and Preview Reliability Hardening
Sequence Diagram(s)sequenceDiagram
participant ServeCommand
participant SiteBuildRunner
participant SSEStream
participant BrowserClient
ServeCommand->>SiteBuildRunner: buildLiveReloadSite()
SiteBuildRunner-->>ServeCommand: (bool success) + lastOutput()
alt success
ServeCommand->>SSEStream: formatServerSentEvent("reload", payload)
SSEStream->>BrowserClient: reload
else failure
ServeCommand->>SSEStream: formatServerSentEvent("build-error", lastOutput)
SSEStream->>BrowserClient: build-error
BrowserClient->>BrowserClient: console.error(event.data)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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 the build/import/preview pipeline by making filesystem writes safer, improving build-time error reporting with source context, and adding stricter validation around generated output paths (permalinks) and output directory replacement behavior.
Changes:
- Introduces
FileWriterand migrates manyfile_put_contentscall sites to checked/atomic writes. - Improves build-time diagnostics with friendly exceptions for invalid front matter/YAML and malformed dates, including file path context.
- Hardens build output behavior: validates permalinks for duplicates/traversal and adds marker-based atomic replacement for
--no-cacherebuilds; live-reload now reports build failures to the browser console.
Reviewed changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Unit/Web/LiveReload/SiteBuildRunnerTest.php | Asserts build failure output is captured and exposed. |
| tests/Unit/Content/Parser/FrontMatterParserTest.php | Adds coverage for friendly exceptions on malformed YAML front matter. |
| tests/Unit/Content/Parser/EntryParserTest.php | Adds coverage for friendly exceptions on malformed date front matter. |
| tests/Unit/Console/ImportCommandTest.php | Ensures non-path options aren’t mistakenly path-resolved. |
| tests/Unit/Console/BuildCommandTest.php | Adds coverage for friendly content config errors, permalink validation, and marker-based output replacement. |
| tests/Unit/Build/ParallelTaskRunnerTest.php | Ensures signaled workers are treated as failures. |
| tests/Unit/Build/BuildManifestTest.php | Ensures corrupted manifests load safely as empty. |
| src/Web/LiveReload/SiteBuildRunner.php | Serializes preview builds with a lock and captures last build output. |
| src/Web/DevServer/assets/live-reload.js | Logs build-error SSE events to the browser console. |
| src/Import/Telegram/TelegramContentImporter.php | Uses FileWriter and marks directory option as a path. |
| src/Import/ImporterOption.php | Adds path flag to distinguish path vs non-path options. |
| src/Content/Parser/FrontMatterParser.php | Throws InvalidContentConfigException with file context for invalid YAML front matter. |
| src/Content/Parser/EntryParser.php | Wraps malformed date parsing in a friendly content config exception. |
| src/Console/ServeCommand.php | Sends build-error SSE events and properly formats SSE data lines. |
| src/Console/NewCommand.php | Uses FileWriter for generated content file writes. |
| src/Console/InitCommand.php | Uses FileWriter for initial project scaffolding writes. |
| src/Console/ImportCommand.php | Resolves paths only for options explicitly marked as paths. |
| src/Console/BuildCommand.php | Adds permalink validation, safer output cleanup, marker-based atomic replacement for --no-cache, and friendly exception reporting. |
| src/Build/TaxonomyPageWriter.php | Uses FileWriter for taxonomy page writes. |
| src/Build/SearchIndexGenerator.php | Uses FileWriter for search index writes. |
| src/Build/RedirectPageWriter.php | Uses FileWriter for redirect page writes. |
| src/Build/ParallelTaskRunner.php | Uses FileWriter and treats signaled workers as failures. |
| src/Build/ParallelEntryWriter.php | Uses FileWriter and treats signaled workers as failures. |
| src/Build/NotFoundPageWriter.php | Uses FileWriter for 404 page writes. |
| src/Build/FileWriter.php | Adds checked and atomic write helpers. |
| src/Build/FeedGenerator.php | Writes feeds via FileWriter using in-memory generation. |
| src/Build/DateArchiveWriter.php | Uses FileWriter for archive page writes. |
| src/Build/CollectionListingWriter.php | Uses FileWriter for listing page writes. |
| src/Build/BuildManifest.php | Handles corrupted JSON gracefully and saves atomically. |
| src/Build/BuildCache.php | Uses FileWriter for cache writes. |
| src/Build/AuthorPageWriter.php | Uses FileWriter for author page writes. |
| docs/preview.md | Documents serialized rebuilds and build-error console reporting. |
| docs/importing-content.md | Documents ImporterOption::$path. |
| docs/content.md | Documents stricter front matter and permalink behavior. |
| docs/commands.md | Documents marker-based output replacement for --no-cache. |
Comments suppressed due to low confidence (1)
src/Build/BuildManifest.php:51
- In BuildManifest::load(), when the decoded JSON is not an array, only $entries is reset. If the same BuildManifest instance is reused after a successful load (or after in-memory mutations), $configFiles and $trackedDirectories can remain stale even though the manifest is effectively empty, which can lead to incorrect incremental decisions.
if (!is_array($data)) {
$this->entries = [];
return;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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)
48-51:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInconsistent state reset when manifest data is invalid.
When
json_decodethrows an exception (lines 40-47), all three properties (entries,configFiles,trackedDirectories) are cleared. However, when the decoded data is not an array (lines 48-51), onlyentriesis reset. Both cases indicate corrupted manifest data and should reset all properties consistently to avoid stale metadata.🛡️ Proposed fix to reset all properties consistently
if (!is_array($data)) { $this->entries = []; + $this->configFiles = []; + $this->trackedDirectories = []; 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 48 - 51, When decoded manifest data is not an array, reset the same three properties that are cleared on JSON decode failure to avoid stale metadata: set $this->entries, $this->configFiles, and $this->trackedDirectories to empty arrays and return; update the non-array handling in BuildManifest (the method that processes the decoded $data) so it clears all three properties consistently with the exception handler.
🤖 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/Web/LiveReload/SiteBuildRunner.php`:
- Around line 21-24: The current lock logic opens a temp lock file and calls
flock but ignores failures; update the SiteBuildRunner locking code to detect
and handle both fopen() and flock() failures: check the return of fopen(...)
into $lock and if it is false log a warning (or throw an exception) and abort
the build to avoid concurrent runs; if fopen succeeds, check the boolean return
of flock($lock, LOCK_EX) and if that call returns false log a warning (including
the lock filename/hash produced by hash('xxh128', $this->outputDir)) and abort
or fallback to a safe behavior rather than proceeding; ensure you close/unlink
the handle on error and use the same $lock handle for unlock after the build
completes.
In `@tests/Unit/Build/ParallelTaskRunnerTest.php`:
- Around line 72-89: The test uses posix_kill and the SIGKILL constant but they
are not referenced/imported, causing a fatal error; update the test file to
import or reference the POSIX functions/constants so posix_kill(getmypid(),
SIGKILL) resolves (e.g., add the POSIX extension function and signal constant
usage for posix_kill and SIGKILL at the top of ParallelTaskRunnerTest or
fully-qualify their usage) ensuring the call in
testRunTreatsSignaledWorkerAsFailure continues to signal the current process
correctly.
---
Outside diff comments:
In `@src/Build/BuildManifest.php`:
- Around line 48-51: When decoded manifest data is not an array, reset the same
three properties that are cleared on JSON decode failure to avoid stale
metadata: set $this->entries, $this->configFiles, and $this->trackedDirectories
to empty arrays and return; update the non-array handling in BuildManifest (the
method that processes the decoded $data) so it clears all three properties
consistently with the exception handler.
🪄 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: ee71d3c0-e09e-495b-b42d-86c2ca4ca4a1
📒 Files selected for processing (35)
docs/commands.mddocs/content.mddocs/importing-content.mddocs/preview.mdsrc/Build/AuthorPageWriter.phpsrc/Build/BuildCache.phpsrc/Build/BuildManifest.phpsrc/Build/CollectionListingWriter.phpsrc/Build/DateArchiveWriter.phpsrc/Build/FeedGenerator.phpsrc/Build/FileWriter.phpsrc/Build/NotFoundPageWriter.phpsrc/Build/ParallelEntryWriter.phpsrc/Build/ParallelTaskRunner.phpsrc/Build/RedirectPageWriter.phpsrc/Build/SearchIndexGenerator.phpsrc/Build/TaxonomyPageWriter.phpsrc/Console/BuildCommand.phpsrc/Console/ImportCommand.phpsrc/Console/InitCommand.phpsrc/Console/NewCommand.phpsrc/Console/ServeCommand.phpsrc/Content/Parser/EntryParser.phpsrc/Content/Parser/FrontMatterParser.phpsrc/Import/ImporterOption.phpsrc/Import/Telegram/TelegramContentImporter.phpsrc/Web/DevServer/assets/live-reload.jssrc/Web/LiveReload/SiteBuildRunner.phptests/Unit/Build/BuildManifestTest.phptests/Unit/Build/ParallelTaskRunnerTest.phptests/Unit/Console/BuildCommandTest.phptests/Unit/Console/ImportCommandTest.phptests/Unit/Content/Parser/EntryParserTest.phptests/Unit/Content/Parser/FrontMatterParserTest.phptests/Unit/Web/LiveReload/SiteBuildRunnerTest.php
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/Unit/Packaging/ConfigurationPackagingTest.php (1)
259-310: ⚡ Quick winConsider validating
release.ymlrunner and action pinning for consistency.The test
packageWorkflowPublishesNightlyBuilds()validates runner labels and SHA pinning forpackage-static.yml(line 261), but there's no equivalent validation forrelease.yml. WhilereleaseWorkflowBuildsNotesFromPreviousStableReleaseTag()(line 378) does loadrelease.yml, it only validates release note generation logic.Both workflows produce release artifacts and should use consistent runner versions and action SHAs. Without explicit validation, they could drift in future changes.
🧪 Suggested test enhancement
Consider adding assertions in an existing test or creating a new test to validate
release.yml:// In releaseWorkflowBuildsNotesFromPreviousStableReleaseTag() or a new test method self::assertStringContainsString('runs-on: windows-2022', $workflow); self::assertStringContainsString('runs-on: macos-14', $workflow); self::assertDoesNotMatchRegularExpression('/uses:\s+[^@\s]+@v\d+/', $workflow);This would ensure both workflows remain consistent and prevent drift.
🤖 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/Packaging/ConfigurationPackagingTest.php` around lines 259 - 310, Add equivalent runner and action-pinning assertions for release.yml to prevent drift: in releaseWorkflowBuildsNotesFromPreviousStableReleaseTag() (or a new test) load release.yml the same way packageWorkflowPublishesNightlyBuilds() loads package-static.yml, then assert presence of runner labels like 'runs-on: windows-2022' and 'runs-on: macos-14', assertDoesNotMatchRegularExpression('/uses:\s+[^@\s]+@v\d+/', $workflow) to ensure SHAs are pinned, and optionally mirror any other checks you rely on from packageWorkflowPublishesNightlyBuilds() (e.g., specific artifact names or persist-credentials count) for consistency.
🤖 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.
Nitpick comments:
In `@tests/Unit/Packaging/ConfigurationPackagingTest.php`:
- Around line 259-310: Add equivalent runner and action-pinning assertions for
release.yml to prevent drift: in
releaseWorkflowBuildsNotesFromPreviousStableReleaseTag() (or a new test) load
release.yml the same way packageWorkflowPublishesNightlyBuilds() loads
package-static.yml, then assert presence of runner labels like 'runs-on:
windows-2022' and 'runs-on: macos-14',
assertDoesNotMatchRegularExpression('/uses:\s+[^@\s]+@v\d+/', $workflow) to
ensure SHAs are pinned, and optionally mirror any other checks you rely on from
packageWorkflowPublishesNightlyBuilds() (e.g., specific artifact names or
persist-credentials count) for consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 47eb361a-4464-4518-be0d-cdf1b84915c8
📒 Files selected for processing (10)
.github/workflows/package-static.yml.github/workflows/release.ymldocs/content.mdsrc/Build/BuildManifest.phpsrc/Console/BuildCommand.phpsrc/Web/LiveReload/SiteBuildRunner.phptests/Unit/Build/BuildManifestTest.phptests/Unit/Build/ParallelTaskRunnerTest.phptests/Unit/Console/BuildCommandTest.phptests/Unit/Packaging/ConfigurationPackagingTest.php
✅ Files skipped from review due to trivial changes (1)
- docs/content.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Web/LiveReload/SiteBuildRunner.php
samdark
changed the title
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
New Features
Bug Fixes
Documentation