Harden diagnostic build error handling#9
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 (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthrough
ChangesBuild Error Handling and Fallback Diagnostics
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
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 `@build.py`:
- Around line 877-905: The fallback error handling path in safe_generate_logd
can still crash before returning False, defeating the safety guarantee. Wrap the
entire fallback logic starting from diagnostic_paths_for_commit through
write_diagnostic_report in an additional try-except block to catch any
exceptions that occur during diagnostic artifact creation, metadata reporting,
or artifact commitment. Ensure that regardless of whether
diagnostic_paths_for_commit, build_diagnostic_report, write_diagnostic_report,
or commit_diagnostic_artifacts succeed or fail, the function always returns
False to indicate failure gracefully.
- Around line 889-903: The fallback commit in the exception handler only stages
metadata_path with commit_diagnostic_artifacts, but it should also include any
logd_path artifacts that may have been successfully created before the error
occurred. Modify the commit_diagnostic_artifacts call to conditionally include
logd_path in the artifacts list (checking if logd_path exists) so that
diagnostic payloads are not lost when generate_logd fails partway through
execution.
In `@diagnostic/build-c72ed8a2.json`:
- Line 1: The file diagnostic/build-c72ed8a2.json contains a UTF-8 BOM (Byte
Order Mark, U+FEFF) at the very beginning before the opening brace, which causes
JSON parsing to fail when decoded with UTF-8 and passed to json.loads. Remove
the BOM character from the start of the file and save it with UTF-8 encoding
without BOM to ensure the JSON parser can successfully read the file content
starting directly with the opening brace.
- Around line 2-35: The diagnostic/build-c72ed8a2.json file has a schema
mismatch with the CI validator's expected contract. Replace the current custom
fields (logd_error, validation_status, message_blocker, pr_note, modules,
total_modules, passed, failed) with the expected fields that the CI validator
requires: diagnostic_logd, diagnostic_logd_error, and password. Ensure the JSON
structure conforms to the standard diagnostic metadata schema expected by the
build/CI pipeline validator for files named diagnostic/build-<commit>.json.
🪄 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: c1448413-56c2-4ae3-b146-81251f7f7f24
📒 Files selected for processing (2)
build.pydiagnostic/build-c72ed8a2.json
|
Addressed the CodeRabbit findings in the latest push:
Validation rerun:
|
|
Pushed a small follow-up for the remaining CodeRabbit pre-merge warnings:
Commit: Errordog2/TentOfTrials@71dc4f9 |
ghost
left a comment
There was a problem hiding this comment.
Thanks for the PR. I found diagnostic JSON metadata, but I don't see the encrypted build diagnostic log artifact (*.logd / builds.compact) committed on this branch yet. Please rerun the build diagnostic/encryption step with the fixed encryptly tooling and commit the generated encrypted log artifact to this PR (typically under diagnostic/).
Blocking review until the encrypted diagnostic log artifact is present.
Closes #2.
/attempt #2
Summary
build.pyfailures.safe_generate_logd(...)so diagnostic finalization errors produce fallback metadata instead of crashing and losing context.run_main_safely()as a top-level CLI guard so unexpected crashes still attempt to preserve diagnostic output.Validation
python -m py_compile build.pyformat_exception_diagnostic(RuntimeError("sample failure"))generate_logdto raiseRuntimeError;safe_generate_logdcaught it, returnedFalse, wrote fallback diagnostic metadata, and did not crash.Diagnostic artifacts
diagnostic/build-31d15a71.jsonwith schema-compatible validation metadata..logdpackaging was blocked by the host environment: WSL lackedlibstdc++/libgccfor the bundled linux-x64encryptly, and Windowsencryptly packexited 1 without producing a.logd. The JSON records the blocker through the standarddiagnostic_logd_errorfield so the failure mode is explicit instead of silent.Checklist
Closes #2).build.pycompiles withpython -m py_compile build.py.Summary by CodeRabbit