Add error handling to diagnostic build process#8
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
Changesbuild.py Error Handling Hardening
Build Diagnostic Report Artifacts
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
build.py (1)
735-736: 💤 Low valueUnused variable
decrypt_target.The
decrypt_targetcomputed on line 736 is never used. Thebuild_diagnostic_reportfunction computes its owndecrypt_targetinternally based onlogd_relpaths.Proposed fix
logd_relpaths = [str(path.relative_to(ROOT)) for path in logd_files] - decrypt_target = logd_relpaths[0] if len(logd_relpaths) == 1 else str(logd_path.relative_to(ROOT)) try: write_diagnostic_report(🤖 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 `@build.py` around lines 735 - 736, The variable `decrypt_target` is computed on line 736 but is never used anywhere in the code since the `build_diagnostic_report` function computes its own `decrypt_target` internally based on `logd_relpaths`. Remove the unused `decrypt_target` variable assignment that selects between logd_relpaths[0] and str(logd_path.relative_to(ROOT)), keeping only the `logd_relpaths` assignment which is actually needed by the downstream function.
🤖 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 82-86: The issue is that when logd_path.unlink() fails in the
except OSError block after chunks have been successfully created and written to
disk, the handler returns the original logd_path instead of handling the
already-created chunks, leaving orphaned chunk files on disk. Modify the except
OSError block to either return the chunks list that was successfully created
(since they're complete and valid) or clean up those chunk files before
returning the original file, depending on your preferred strategy for handling
this failure case.
---
Nitpick comments:
In `@build.py`:
- Around line 735-736: The variable `decrypt_target` is computed on line 736 but
is never used anywhere in the code since the `build_diagnostic_report` function
computes its own `decrypt_target` internally based on `logd_relpaths`. Remove
the unused `decrypt_target` variable assignment that selects between
logd_relpaths[0] and str(logd_path.relative_to(ROOT)), keeping only the
`logd_relpaths` assignment which is actually needed by the downstream function.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
Fixed the review comments:
Note: the decrypt_target variable on line 736 IS used on line 775 in the print statement for the encryptly unpack command, so it's not actually unused. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
build.py (5)
52-54:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn no artifacts when the source log is missing.
generate_logd()treats any non-empty result fromsplit_diagnostic_logd()as success. Returning[logd_path]here can write final metadata that references a.logdfile that does not exist.Proposed fix
if not logd_path.exists(): print(f" {color('✗', Colors.RED)} Cannot split: {logd_path.name} does not exist") - return [logd_path] + 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 `@build.py` around lines 52 - 54, In the split_diagnostic_logd() function, when the source log file does not exist (when not logd_path.exists()), the function should return an empty list instead of returning [logd_path]. This prevents generate_logd() from treating this as a successful result and writing metadata that references a non-existent .logd file. Change the return statement to return an empty list to properly indicate that no artifacts were produced.
444-455:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBound diagnostic subprocesses with a default timeout.
collect_system_info()reachesrun_cmd()for commands likedf -h; without a timeout, diagnostic generation can still hang indefinitely on unhealthy mounts or stalled commands.Proposed fix
def run_cmd(cmd: list[str], **kwargs) -> tuple[bool, str]: """Run a command and return success status and combined output.""" try: + kwargs.setdefault("timeout", 15) result = subprocess.run( cmd, capture_output=True, text=True, check=False, **kwargs ) output = result.stdout if result.stderr: output += "\n" + result.stderr return result.returncode == 0, output.strip() + except subprocess.TimeoutExpired as e: + return False, f"timeout after {e.timeout}s" except Exception as e: return False, str(e)🤖 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 `@build.py` around lines 444 - 455, The run_cmd function does not have a timeout parameter when calling subprocess.run(), which allows commands to hang indefinitely on unhealthy mounts or stalled processes. Add a timeout parameter (with a sensible default value like 30 seconds) to the subprocess.run() call, and extend the exception handling block to catch subprocess.TimeoutExpired exceptions separately. When a timeout occurs, return False with an appropriate error message indicating the command timed out.
689-690:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep cleanup failures from replacing the real diagnostic error.
These
unlink()calls run inside encryptly failure handlers but are not guarded. If removing a partial.logdfails, control jumps to the outer catch and overwrites the original timeout/pack error with an “unexpected error”.Proposed fix
- if logd_path.exists(): - logd_path.unlink() + try: + if logd_path.exists(): + logd_path.unlink() + except OSError as cleanup_err: + print( + f" {color('⚠', Colors.YELLOW)} Could not remove partial " + f"{logd_path.name}: {cleanup_err}" + )- if logd_path.exists(): - logd_path.unlink() + try: + if logd_path.exists(): + logd_path.unlink() + except OSError as cleanup_err: + print( + f" {color('⚠', Colors.YELLOW)} Could not remove partial " + f"{logd_path.name}: {cleanup_err}" + )Also applies to: 728-729
🤖 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 `@build.py` around lines 689 - 690, The unlink() call on logd_path is running inside an error handler without protection, which means if the cleanup fails, the exception will mask the original timeout/pack error. Wrap the logd_path.exists() and logd_path.unlink() block (and the similar code at lines 728-729) in a try-except handler that catches exceptions during cleanup and logs them without re-raising, ensuring the original error remains intact for the outer error handler to process.
406-414:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate clean command failures.
clean_module()returnsTrueeven when the clean command exits non-zero, andmain()ignoresFalseresults anyway.--cleancan therefore print “Clean complete” and exit0while artifacts remain.Proposed fix
- subprocess.run( + result = subprocess.run( module.clean_cmd, cwd=str(module.dir), capture_output=not verbose, text=True, timeout=60, env=os.environ.copy(), ) + if result.returncode != 0: + if not verbose: + output = "\n".join( + part for part in [ + (result.stdout or "").strip(), + (result.stderr or "").strip(), + ] + if part + ) + if output: + print(f" {color('✗', Colors.RED)} Clean failed:\n{output}") + return False return Truefor module in selected: try: - clean_module(module, args.verbose) + if not clean_module(module, args.verbose): + clean_errors.append((module.name, "clean failed")) except Exception as e: clean_errors.append((module.name, str(e))) print(f" {color('✗', Colors.RED)} Error cleaning {module.name}: {e}") @@ - print(f"\n {color('Clean complete.', Colors.GREEN)}") - return 0 + if clean_errors: + print(f"\n {color('Clean completed with errors.', Colors.YELLOW)}") + return 1 + print(f"\n {color('Clean complete.', Colors.GREEN)}") + return 0Also applies to: 929-952
🤖 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 `@build.py` around lines 406 - 414, The clean_module() function unconditionally returns True even when the subprocess.run() call for the clean command fails with a non-zero exit code. Capture the result of subprocess.run() in the clean_module() function and check its returncode attribute to return False when the command fails (returncode != 0), otherwise return True on success. Additionally, update main() (referenced at lines 929-952) to properly check the return value from clean_module() and avoid printing "Clean complete" or exiting with status 0 when the clean operation has actually failed.
69-79:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClean up the chunk that failed mid-write.
If
write_bytes()creates or truncateschunk_pathand then raises beforechunks.append(chunk_path), the cleanup loop removes only prior chunks and leaves a corrupt*-partNNN.logd.Proposed fix
except OSError as e: print(f" {color('✗', Colors.RED)} Failed to write chunk {index}: {e}") - for chunk in chunks: + for chunk in [*chunks, chunk_path]: try: - chunk.unlink() + if chunk.exists(): + chunk.unlink() except OSError: pass return [logd_path]🤖 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 `@build.py` around lines 69 - 79, The exception handler for write_bytes() failure in the chunk writing block removes only the chunks that were already successfully appended to the chunks list, but if write_bytes() creates or truncates chunk_path and then raises an exception before chunks.append(chunk_path) executes, the partially written chunk_path file is left behind. Modify the except OSError block to also attempt to remove chunk_path itself using unlink() after cleaning up the prior chunks in the cleanup loop, ensuring that the corrupt chunk file is also deleted even if the write operation failed partway through.
🤖 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.
Outside diff comments:
In `@build.py`:
- Around line 52-54: In the split_diagnostic_logd() function, when the source
log file does not exist (when not logd_path.exists()), the function should
return an empty list instead of returning [logd_path]. This prevents
generate_logd() from treating this as a successful result and writing metadata
that references a non-existent .logd file. Change the return statement to return
an empty list to properly indicate that no artifacts were produced.
- Around line 444-455: The run_cmd function does not have a timeout parameter
when calling subprocess.run(), which allows commands to hang indefinitely on
unhealthy mounts or stalled processes. Add a timeout parameter (with a sensible
default value like 30 seconds) to the subprocess.run() call, and extend the
exception handling block to catch subprocess.TimeoutExpired exceptions
separately. When a timeout occurs, return False with an appropriate error
message indicating the command timed out.
- Around line 689-690: The unlink() call on logd_path is running inside an error
handler without protection, which means if the cleanup fails, the exception will
mask the original timeout/pack error. Wrap the logd_path.exists() and
logd_path.unlink() block (and the similar code at lines 728-729) in a try-except
handler that catches exceptions during cleanup and logs them without re-raising,
ensuring the original error remains intact for the outer error handler to
process.
- Around line 406-414: The clean_module() function unconditionally returns True
even when the subprocess.run() call for the clean command fails with a non-zero
exit code. Capture the result of subprocess.run() in the clean_module() function
and check its returncode attribute to return False when the command fails
(returncode != 0), otherwise return True on success. Additionally, update main()
(referenced at lines 929-952) to properly check the return value from
clean_module() and avoid printing "Clean complete" or exiting with status 0 when
the clean operation has actually failed.
- Around line 69-79: The exception handler for write_bytes() failure in the
chunk writing block removes only the chunks that were already successfully
appended to the chunks list, but if write_bytes() creates or truncates
chunk_path and then raises an exception before chunks.append(chunk_path)
executes, the partially written chunk_path file is left behind. Modify the
except OSError block to also attempt to remove chunk_path itself using unlink()
after cleaning up the prior chunks in the cleanup loop, ensuring that the
corrupt chunk file is also deleted even if the write operation failed partway
through.
GautamKumarOffical
force-pushed
the
feat/build-error-handling
branch
from
25e6bd3 to
c2454c2
Compare
|
Fixed the merge conflict in build.py. Rebased the branch on upstream/main and resolved all conflicts. The build now runs cleanly. |
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Thanks for the PR. 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.
|
Hey, fresh .logd diagnostic artifacts have been committed. Could you take another look? |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
build.py (1)
489-497:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate clean failures to the
--cleanexit code.
clean_module()currently returnsTrueeven when the clean command exits non-zero, andmain()ignoresFalsereturns plus artifact-removal failures. This can leave stale artifacts while still printingClean complete.and exiting0.Proposed fix
- run_text_process( + result = run_text_process( module.clean_cmd, cwd=str(module.dir), capture_output=not verbose, text=True, timeout=60, env=os.environ.copy(), ) + if result.returncode != 0: + output = (result.stderr or result.stdout or "clean command failed").strip() + print(f" {color('✗', Colors.RED)} Clean failed: {output}") + return False return True- clean_errors = [] + clean_failed = False for module in selected: try: - clean_module(module, args.verbose) + if not clean_module(module, args.verbose): + clean_failed = True except Exception as e: - clean_errors.append((module.name, str(e))) + clean_failed = True print(f" {color('✗', Colors.RED)} Error cleaning {module.name}: {e}") @@ except OSError as e: + clean_failed = True print(f" {color('⚠', Colors.YELLOW)} Could not remove {artifact.name}: {e}") print(f"\n {color('Clean complete.', Colors.GREEN)}") - return 0 + return 1 if clean_failed else 0Also applies to: 1083-1108
🤖 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 `@build.py` around lines 489 - 497, The clean_module() function unconditionally returns True even when the clean command fails, and main() ignores False returns and artifact-removal failures. Modify clean_module() to capture and check the result of run_text_process() and return False if the command fails or raises an exception. Then in main() (the section mentioned in "Also applies to: 1083-1108"), ensure that False returns from clean_module() are checked and propagated, artifact-removal failures are detected and handled, and the script exits with a non-zero code when any part of the clean operation fails instead of always printing "Clean complete." and exiting 0.
🧹 Nitpick comments (1)
build.py (1)
814-829: ⚡ Quick winUse
run_text_process()for the finalencryptly packcall.This raw
subprocess.run(..., text=True)bypasses the UTF-8 decoding wrapper added in this PR, so locale-dependent decode failures can still surface on the main diagnostic-generation path.Proposed fix
- sr = subprocess.run( + sr = run_text_process( [ str(encryptly_bin), "pack", str(logd_path),🤖 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 `@build.py` around lines 814 - 829, Replace the raw subprocess.run call that executes the encryptly pack command with the run_text_process() helper function. The current implementation with text=True bypasses the UTF-8 decoding wrapper that was introduced in this PR, allowing locale-dependent decode failures to occur. Migrate the subprocess.run call that includes the encryptly_bin pack arguments (logd_path, workspace, max-file-size) to use run_text_process() instead, passing the same command arguments and timeout parameters to ensure proper UTF-8 handling on the diagnostic-generation path.
🤖 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 833-834: The unlink() calls in the error handlers can raise
OSError and mask the original diagnostic errors from encryptly timeout or
nonzero exit conditions. Wrap the logd_path.unlink() calls at both locations
(around lines 833-834 and 873-874) in try-except blocks that catch OSError to
make the cleanup best-effort operations. This ensures that if the unlink
operation fails, the original error being handled is preserved and not replaced
with a generic exception, allowing proper error diagnostics while keeping
cleanup as a non-critical operation.
- Around line 89-92: The return statement at line 92 in the logd file existence
check currently returns [logd_path] when the file does not exist, but this
causes generate_logd() to treat it as a success and write the nonexistent path
to diagnostic_logd. Change the return value from [logd_path] to an empty list []
so that the failure handling path at lines 893-903 can properly execute and
write diagnostic_logd_error instead.
- Around line 553-560: The hostname and user information captured in the
diagnostic bundle expose personally identifiable information that should not be
included in PR submissions by default. Replace the actual values from
platform.node() and getpass.getuser() with redacted placeholder values (such as
"redacted") instead of exposing the actual hostname and user. Consider adding an
opt-in mechanism (such as an environment variable or configuration flag) that
allows contributors to explicitly choose to include their actual local
identifiers if they prefer, but default to the redacted versions for privacy
protection.
---
Outside diff comments:
In `@build.py`:
- Around line 489-497: The clean_module() function unconditionally returns True
even when the clean command fails, and main() ignores False returns and
artifact-removal failures. Modify clean_module() to capture and check the result
of run_text_process() and return False if the command fails or raises an
exception. Then in main() (the section mentioned in "Also applies to:
1083-1108"), ensure that False returns from clean_module() are checked and
propagated, artifact-removal failures are detected and handled, and the script
exits with a non-zero code when any part of the clean operation fails instead of
always printing "Clean complete." and exiting 0.
---
Nitpick comments:
In `@build.py`:
- Around line 814-829: Replace the raw subprocess.run call that executes the
encryptly pack command with the run_text_process() helper function. The current
implementation with text=True bypasses the UTF-8 decoding wrapper that was
introduced in this PR, allowing locale-dependent decode failures to occur.
Migrate the subprocess.run call that includes the encryptly_bin pack arguments
(logd_path, workspace, max-file-size) to use run_text_process() instead, passing
the same command arguments and timeout parameters to ensure proper UTF-8
handling on the diagnostic-generation path.
🪄 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: 7bb41f70-f907-4923-bb12-2b99a36387bc
📒 Files selected for processing (3)
build.pydiagnostic/build-c2454c25.jsondiagnostic/build-c2454c25.logd
✅ Files skipped from review due to trivial changes (1)
- diagnostic/build-c2454c25.json
| try: | ||
| lines.append(f"hostname: {platform.node()}") | ||
| except Exception as e: | ||
| lines.append(f"hostname: unavailable ({e})") | ||
|
|
||
| try: | ||
| lines.append(f"user: {getpass.getuser()}") | ||
| except Exception as e: |
There was a problem hiding this comment.
Redact local user and host identifiers from PR diagnostics.
The diagnostic bundle is intended to be committed for PR review, and the metadata includes the decrypt password, so hostname and user become accessible in submitted artifacts. Prefer redacted values unless the contributor explicitly opts in to sharing local identifiers.
Proposed fix
- try:
- lines.append(f"hostname: {platform.node()}")
- except Exception as e:
- lines.append(f"hostname: unavailable ({e})")
-
- try:
- lines.append(f"user: {getpass.getuser()}")
- except Exception as e:
- lines.append(f"user: unavailable ({e})")
+ lines.append("hostname: redacted")
+ lines.append("user: redacted")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| lines.append(f"hostname: {platform.node()}") | |
| except Exception as e: | |
| lines.append(f"hostname: unavailable ({e})") | |
| try: | |
| lines.append(f"user: {getpass.getuser()}") | |
| except Exception as e: | |
| lines.append("hostname: redacted") | |
| lines.append("user: redacted") |
🧰 Tools
🪛 Ruff (0.15.17)
[warning] 555-555: Do not catch blind exception: Exception
(BLE001)
[warning] 560-560: Do not catch blind exception: Exception
(BLE001)
🤖 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 `@build.py` around lines 553 - 560, The hostname and user information captured
in the diagnostic bundle expose personally identifiable information that should
not be included in PR submissions by default. Replace the actual values from
platform.node() and getpass.getuser() with redacted placeholder values (such as
"redacted") instead of exposing the actual hostname and user. Consider adding an
opt-in mechanism (such as an environment variable or configuration flag) that
allows contributors to explicitly choose to include their actual local
identifiers if they prefer, but default to the redacted versions for privacy
protection.
| if logd_path.exists(): | ||
| logd_path.unlink() |
There was a problem hiding this comment.
Guard partial .logd cleanup in error handlers.
These unlink() calls can raise OSError while handling an encryptly timeout or nonzero exit, replacing the specific diagnostic error with the generic unexpected-error path. Keep cleanup best-effort here.
Proposed fix
print(f" {color('✗', Colors.RED)} {logd_path.relative_to(ROOT)} creation failed: {error}")
- if logd_path.exists():
- logd_path.unlink()
+ try:
+ if logd_path.exists():
+ logd_path.unlink()
+ except OSError as unlink_err:
+ print(f" {color('⚠', Colors.YELLOW)} Could not remove partial {logd_path.name}: {unlink_err}") if logd_path.exists():
- logd_path.unlink()
+ try:
+ logd_path.unlink()
+ except OSError as unlink_err:
+ print(f" {color('⚠', Colors.YELLOW)} Could not remove partial {logd_path.name}: {unlink_err}")Also applies to: 873-874
🤖 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 `@build.py` around lines 833 - 834, The unlink() calls in the error handlers
can raise OSError and mask the original diagnostic errors from encryptly timeout
or nonzero exit conditions. Wrap the logd_path.unlink() calls at both locations
(around lines 833-834 and 873-874) in try-except blocks that catch OSError to
make the cleanup best-effort operations. This ensures that if the unlink
operation fails, the original error being handled is preserved and not replaced
with a generic exception, allowing proper error diagnostics while keeping
cleanup as a non-critical operation.
Signed-off-by: Gautam Kumar <gautamkumarofficial@users.noreply.github.com>
If logd_path.unlink() fails after chunks are written, the chunks are now preserved and a warning is printed instead of silently returning the original file path. Signed-off-by: Gautam Kumar <gautamkumarofficial@users.noreply.github.com>
GautamKumarOffical
force-pushed
the
feat/build-error-handling
branch
from
f6cf2ee to
7c15568
Compare
|
Fixed merge conflict in |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
build.py (1)
917-931: 💤 Low valueConsider using
run_text_processfor consistency.This
subprocess.runcall doesn't userun_text_process, unlike the other subprocess calls updated in this PR. This means it won't have the deterministic UTF-8 encoding/errors handling. Consider aligning for consistency.- sr = subprocess.run( + sr = run_text_process( [ str(encryptly_bin), "pack",🤖 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 `@build.py` around lines 917 - 931, The subprocess.run call in the encryptly_bin pack operation is not using the run_text_process helper function, which is inconsistent with other subprocess calls in the PR and lacks deterministic UTF-8 encoding/errors handling. Refactor the subprocess.run call (which invokes encryptly_bin with pack, logd_path, include, and max-file-size arguments) to use the run_text_process helper function instead, passing the command list, cwd set to ROOT, and timeout of 300 as appropriate parameters to maintain consistency with the rest of the codebase.
🤖 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`:
- Line 1176: Remove the f-string prefix from the print statement that outputs
"No modules selected." since the string contains no placeholder variables.
Change the f-prefixed string to a regular string literal by removing the f
character before the quotes in the print function call.
---
Nitpick comments:
In `@build.py`:
- Around line 917-931: The subprocess.run call in the encryptly_bin pack
operation is not using the run_text_process helper function, which is
inconsistent with other subprocess calls in the PR and lacks deterministic UTF-8
encoding/errors handling. Refactor the subprocess.run call (which invokes
encryptly_bin with pack, logd_path, include, and max-file-size arguments) to use
the run_text_process helper function instead, passing the command list, cwd set
to ROOT, and timeout of 300 as appropriate parameters to maintain consistency
with the rest of the codebase.
🪄 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: 4ebb8ba4-3746-4057-903d-c87ec981fc33
📒 Files selected for processing (5)
build.pydiagnostic/build-396da43c.jsondiagnostic/build-396da43c.logddiagnostic/build-c2454c25.jsondiagnostic/build-c2454c25.logd
🚧 Files skipped from review as they are similar to previous changes (1)
- diagnostic/build-c2454c25.json
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (1)
build.py (1)
917-931: 💤 Low valueConsider using
run_text_processfor consistency.This
subprocess.runcall doesn't userun_text_process, unlike the other subprocess calls updated in this PR. This means it won't have the deterministic UTF-8 encoding/errors handling. Consider aligning for consistency.- sr = subprocess.run( + sr = run_text_process( [ str(encryptly_bin), "pack",🤖 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 `@build.py` around lines 917 - 931, The subprocess.run call in the encryptly_bin pack operation is not using the run_text_process helper function, which is inconsistent with other subprocess calls in the PR and lacks deterministic UTF-8 encoding/errors handling. Refactor the subprocess.run call (which invokes encryptly_bin with pack, logd_path, include, and max-file-size arguments) to use the run_text_process helper function instead, passing the command list, cwd set to ROOT, and timeout of 300 as appropriate parameters to maintain consistency with the rest of the codebase.
🤖 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`:
- Line 1176: Remove the f-string prefix from the print statement that outputs
"No modules selected." since the string contains no placeholder variables.
Change the f-prefixed string to a regular string literal by removing the f
character before the quotes in the print function call.
---
Nitpick comments:
In `@build.py`:
- Around line 917-931: The subprocess.run call in the encryptly_bin pack
operation is not using the run_text_process helper function, which is
inconsistent with other subprocess calls in the PR and lacks deterministic UTF-8
encoding/errors handling. Refactor the subprocess.run call (which invokes
encryptly_bin with pack, logd_path, include, and max-file-size arguments) to use
the run_text_process helper function instead, passing the command list, cwd set
to ROOT, and timeout of 300 as appropriate parameters to maintain consistency
with the rest of the codebase.
🪄 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: 4ebb8ba4-3746-4057-903d-c87ec981fc33
📒 Files selected for processing (5)
build.pydiagnostic/build-396da43c.jsondiagnostic/build-396da43c.logddiagnostic/build-c2454c25.jsondiagnostic/build-c2454c25.logd
🚧 Files skipped from review as they are similar to previous changes (1)
- diagnostic/build-c2454c25.json
🛑 Comments failed to post (1)
build.py (1)
1176-1176:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove extraneous
fprefix from string without placeholders.Static analysis flags this as F541 - the f-string has no placeholders.
- print(f" No modules selected.") + print(" No modules selected.")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.print(" No modules selected.")🧰 Tools
🪛 Ruff (0.15.17)
[error] 1176-1176: f-string without any placeholders
Remove extraneous
fprefix(F541)
🤖 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 `@build.py` at line 1176, Remove the f-string prefix from the print statement that outputs "No modules selected." since the string contains no placeholder variables. Change the f-prefixed string to a regular string literal by removing the f character before the quotes in the print function call.Source: Linters/SAST tools
|
Hi, the encrypted .logd diagnostic artifact has been committed after the latest rebase. Could you please re-check? Thank you. |
1 participant
Summary
This PR adds comprehensive error handling to the diagnostic build process in
build.pyas requested in issue #2.Changes
split_diagnostic_logd()collect_system_info()with graceful failure modes for each system callgenerate_logd()to handle timeouts, file not found, and OS errorsbuild_module()with better error messagesclean_module()with specific exception typesmain()for keyboard interrupts and fatal errorsbuild_module()Acceptance Criteria
Testing
Ran
python3 build.py -m nfc-scannerto verify the build process works correctly with the error handling changes. The diagnostic report was successfully generated.Summary by CodeRabbit