Skip to content

feat: installer error-report generation (schema v1)#2

Merged
forkni merged 1 commit into
mainfrom
feat/installer-error-report
Jul 14, 2026
Merged

feat: installer error-report generation (schema v1)#2
forkni merged 1 commit into
mainfrom
feat/installer-error-report

Conversation

@forkni

@forkni forkni commented Jul 14, 2026

Copy link
Copy Markdown
Owner

Summary

  • Add sd_installer/report.py: builds a self-contained diagnostic report (schema v1) on installation phase failure, with an env-var allowlist/denylist so secrets never leak into the report
  • sd_installer/installer.py: track current_phase/_last_pip_stderr, add _write_install_error_report(), wrap the phase-calling loop in try/finally so a failure always writes a report before re-raising
  • sd_installer/cli.py: add a report subcommand

This is PR I-2, chained on deps/cuda-link-1.12-torch-modelopt-onnx (PR I-1, #1) because _write_install_error_report() reads current_phase values set by phase4b_cuda_link/phase4c_cuda_link_env, which only exist on that branch, not on main. Once PR I-1 merges, this PR's base should be retargeted to main.

Test plan

  • python -m sd_installer report writes an install_error_report_*.txt
  • A forced phase failure writes a report and re-raises via the new try/finally path
  • Claude Code review (requires CLAUDE_CODE_OAUTH_TOKEN secret on this fork)
  • GitHub Copilot review

@claude

claude Bot commented Jul 14, 2026

Copy link
Copy Markdown

Claude finished @forkni's task in 5m 15s —— View job


Code review — PR #2 (feat: installer error-report generation (schema v1))

Todo list

  • Gather context (PR diff, CLAUDE.md files)
  • Check precondition (not closed/draft/trivial/already reviewed)
  • Summarize PR changes
  • Run parallel review (CLAUDE.md compliance x2, bug scan x2)
  • Validate flagged issues
  • Post final review summary

Summary of changes

  • sd_installer/report.py (new, 184 lines): builds a schema-v1 diagnostic report with an env-var allowlist (CUDALINK_, HF_, SD_, SDTD_ prefixes) filtered further by a denylist (TOKEN, KEY, SECRET, PASSWORD, PASSWD, CRED), so HF_TOKEN/SD_API_KEY-style vars are dropped despite matching an allowed prefix.
  • sd_installer/installer.py: adds current_phase/_last_pip_stderr tracking, wraps the phase-calling loop in try/except so any phase failure writes a best-effort report via _write_install_error_report() before re-raising the original exception.
  • sd_installer/cli.py: adds a report subcommand for generating a report on demand (no failure required).

No CLAUDE.md files exist in this repository, so there was nothing to check for compliance.

Bug/security review

Two independent reviews focused on the diff itself (correctness, secret leakage into reports/logs, and whether error-reporting stays best-effort) came back clean. Specifically verified:

  • args.base_folder used in the new cmd_report (cli.py) resolves correctly — --base-folder is a shared top-level parser argument (cli.py:396), inherited by every subcommand, matching the existing pattern in all other cmd_* handlers.
  • The env allowlist/denylist in report.py correctly drops secret-shaped vars (e.g. HF_TOKEN, SD_AWS_SECRET_ACCESS_KEY) even though they match an allowed prefix — full os.environ is never dumped.
  • _write_install_error_report and write_error_report are both wrapped in broad try/except that print-and-swallow, so a bug in reporting can't mask the real install failure; the phase loop still re-raises the original exception in all cases.
  • The phase-loop refactor in installer.install() preserves the original phase order/semantics; success is only read on the non-exception path, so there's no unbound-variable risk on the raise path.
  • phase4c_cuda_link_env's new SDTD_BASE_FOLDER_PATH persistence is Windows-gated, independent of the other setx calls, and non-fatal on failure — consistent with existing CUDALINK_* handling.

One thing noted but not flagged as a defect: the TRACEBACK/LOG TAIL sections dump raw exception text and pip stderr verbatim (unlike the ENV section, these aren't allowlist-filtered). In this repo all pip index URLs are public (PyTorch/NVIDIA/NGC), so there's no credential-bearing URL in the current pip invocations — this isn't an active leak path today, just worth keeping in mind if a private/authenticated index URL is ever added later.

No issues found. Checked for bugs, security (secret leakage), and best-effort error-handling contract compliance.

forkni added a commit that referenced this pull request Jul 14, 2026
The multi-agent code-review plugin exhausted 60 turns on PR #2 (feat/installer-error-report,
a stacked diff with no CLAUDE.md to shortcut convention discovery) before posting a verdict.
Bumping headroom rather than trusting a partial/incomplete review.
forkni added a commit that referenced this pull request Jul 14, 2026
The multi-agent code-review plugin exhausted 60 turns on PR #2 (feat/installer-error-report,
a stacked diff with no CLAUDE.md to shortcut convention discovery) before posting a verdict.
Bumping headroom rather than trusting a partial/incomplete review.
@forkni

forkni commented Jul 14, 2026

Copy link
Copy Markdown
Owner Author

Re-triggering review with bumped max-turns (60 -> 120) — prior run exhausted its turn budget mid-review.

@forkni forkni closed this Jul 14, 2026
@forkni forkni reopened this Jul 14, 2026
@forkni forkni requested a review from CharlieHelps July 14, 2026 02:36
@forkni forkni changed the base branch from deps/cuda-link-1.12-torch-modelopt-onnx to main July 14, 2026 03:08
@forkni forkni merged commit a68aa8f into main Jul 14, 2026
1 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant