Repository navigation
fix(onboarding): recover failed builds with complete logs and reviewed AI fixes - #689
Salah-Eddine Lachkar (artpumpkin) wants to merge 2 commits into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
| Option | What it gives users | What it costs | Effort to change later |
|---|---|---|---|
| Current: copy only on error | Simple screen, no stale-log risk | Earlier failure is lost on retry without warning | Low |
| One-line note near Retry | Warns the user before the log is lost | A little more text | Low |
| Keep the previous failed log until the new run ends | Support can see both failures | More state, more rules | Medium |
A note near Retry is enough for most cases. The current approach is correct if users rarely share the first failure.
2. The Fix with AI button is hidden without explanation when the user has no AI set up or when the failure type cannot be fixed by AI.
Hiding the button for permission, cancellation and /etc conflict failures is correct. The AI cannot grant permissions or install apps. The suggestion text under the error partly explains these cases.
The prior concern still holds for the no-AI case. The failure panel shows no button and no hint. A new user does not know that AI recovery exists or how to turn it on. The screen already has a way to set up AI, but the failure panel does not use it. A one-line hint such as "Set up AI to get a suggested fix" with a link would remove the confusion.
3. Onboarding now offers the AI fix with a full review step inside the build screen. The user reviews, applies or discards, and saves. Retry stays disabled while a fix is pending, and Back to Prompt is hidden.
First-time users gain a recovery path that does not leave the wizard. Blocking Retry and Fix with AI while a proposal is open prevents two competing runs. Hiding Back to Prompt is right, because onboarding has no prompt screen. The explanatory text names what Build & Test, Discard and save do.
The concern still holds because the packaged-app smoke test is not done. The flow uses two surfaces for one state: the overlay during generation and the inline review after it. Mocked tests cannot show that the user understands the handoff. A user who discards the fix must also know that Retry is available again. Do not ship until a person has seen this flow on a real Mac.
4. The fixed list of generic suggestions is replaced by the real error message, a failure-type title and a suggestion. Activation failures are named separately from Nix build failures.
The old list (stale inputs, uncommitted changes) was often unrelated to the real failure. Showing the actual error and a type-specific suggestion matches what users need to act. Naming "Activation failed" separately tells the user that the problem happened after the build, which changes what they should do next.
5. The failure panel now states system safety from backend evidence. It says "no changes" only when the backend confirms it. After a partial activation it warns that some changes may be applied. When the state is unknown it says so.
The old text always said "Your Mac was not changed". This was false after a partial activation, and false trust is the worst result in a recovery screen. Three states (untouched, changed, unknown) match what the system can really know. The unknown state is honest and does not cause alarm.
6. Onboarding now shows progress, error, notice and the Review and Apply surfaces in the same window as the wizard. Permission notices are shared with the normal rebuild overlay.
A first-time user who needs a permission grant can now act on it inside the wizard. Reusing the same notice component keeps wording and behavior the same as in the main app. The risk is a crowded screen, which the smoke test should check.
Since the last review
- Still open: Copy log disappears when the user clicks Retry, with no warning, so the earlier failure log cannot be shared. (build-step.tsx still renders the Copy log button only when status === "error"; no note near Retry was added.)
- Still open: Fix with AI is hidden with no explanation, and a user with no AI provider sees no way to enable it. (The button is still gated on hasInference and an AI-fixable error type; the diff adds no hint or setup link in the failure panel.)
- Still open: The inline AI fix review in onboarding is not verified end to end in the packaged app. (The PR description says the packaged-app smoke test is still outstanding. The new tests mock native operations.)
- Addressed by explanation: What gate must the packaged-app smoke test pass before ship? (The PR description now lists the scenarios (evaluation failure, activation failure, long transcript, retry, AI fix, discard). It also states they are not yet run.)
- Still open: How often do users want to share a failure log before retrying, to decide if a notice near Retry is enough? (No usage data or discussion appears in the PR description or the diff.)
- Still open: Should a user with no AI provider see a setup path from the failure panel? (The diff adds no setup link in the failure panel. The onConfigureInference prop is not used there.)
Open questions
-
Will a person run the packaged-app smoke test (evaluation failure, activation or Homebrew failure, long transcript, AI fix, discard) before merge, and who owns the result?
-
How often do first-time users want to share a failure log before they retry? The answer decides whether a note near Retry is enough.
-
Should a user with no AI provider see a path to set one up from the failure panel?
Recommendation
Ship with changes
The safety text and run-specific logs are clear improvements. Three user-facing gaps remain: the log is lost on retry without warning, the missing Fix with AI button has no hint, and the real-Mac smoke test is not done. The first two are small text changes. The third must finish before merge.
![prelint[bot]](https://avatars.githubusercontent.com/in/2822344?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c1035088a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
| {evolve?.evolutionId != null && !isGenerating ? ( | ||
| evolve.step === "evolve" ? <ReviewStep /> : evolve.step === "commit" ? <CommitStep /> : null | ||
| ) : null} |
There was a problem hiding this comment.
When Fix with AI completes after an onboarding build failure, rebuildStatus still describes the failed run, so the existing top-level “Retry build” button remains active while this newly rendered ReviewStep is awaiting approval. That button calls runFirstBuild and then handleApply directly, allowing the AI-modified working tree to be activated without using the review surface's confirmation gate. Hide or disable the original retry/fix controls whenever an evolution review is pending.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 40cd50d. Retry is disabled (and its handler guarded) during generation and while evolution review/save is pending; the secondary Fix with AI action is hidden. Regression tests cover both review and commit states.
| <EtcClobberConflictList result={etcClobber} /> | ||
| ) : null} | ||
| {evolve?.evolutionId != null && !isGenerating ? ( | ||
| evolve.step === "evolve" ? <ReviewStep /> : evolve.step === "commit" ? <CommitStep /> : null |
There was a problem hiding this comment.
In this embedding, ReviewStep exposes a “Back to Prompt” button that only sets the main widget's activeStepOverride to begin. OnboardingFlow routes with its separate onboarding step state, while BuildStep continues rendering this review from evolve.step, so clicking the button during onboarding has no visible effect and the user cannot reach the prompt to refine the fix. Pass an onboarding-specific navigation callback or suppress the unsupported action here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 40cd50d. ReviewStep/DriftReview can disable unsupported prompt navigation; onboarding does so, while the normal widget keeps its existing Back to Prompt callback. Tests verify both action configurations. Added onboarding guidance for review, apply, discard and save; the PR describes the still-outstanding packaged-app smoke test.
 [Medium impact] The changes affect failed-build recovery in onboarding and shared rebuild status. The PR should not merge until the empty-review Retry trap and the shared error-reporting requirement are addressed. FindingsSummaryThe PR adds run-specific log copying and brings AI recovery into onboarding.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const isGenerating = useUiState((s) => s.isGenerating); | ||
| const etcClobber = useUiState((s) => s.etcClobber); | ||
| const evolve = useViewModel((s) => s.evolve); | ||
| const recoveryPending = isGenerating || (evolve?.evolutionId != null && (evolve.step === "evolve" || evolve.step === "commit")); |
There was a problem hiding this comment.
After an AI fix, discarding its last change through a file’s “Discard change” menu leaves onboarding stuck. That action refreshes the session without clearing its evolutionId, so recoveryPending keeps Retry disabled. Meanwhile, DriftReview returns nothing when no files remain, hiding the whole-session Discard action too.
Clear the empty session or keep a way to discard it and retry.
| if (!run?.logFile) throw new Error("The complete build log is unavailable."); | ||
| const contents = await client.darwin.readRebuildLog({ logFile: run.logFile }); | ||
| if (!contents) throw new Error("The build log is empty."); | ||
| if (currentRun.current?.isRunning || currentRun.current?.logFile !== run.logFile) { | ||
| throw new Error("The build changed. Copy the log from the completed build again."); |
There was a problem hiding this comment.
copyLog adds user-facing error strings directly in BuildStep. The repository requires errors to be defined and reported through nostics in errors.ts, not written as one-off strings in components. Add stable diagnostics for unavailable, empty, changed-run, and clipboard failures, then use them here. This repository requirement must be satisfied before merging.
Context Used: TypeScript error handling with nostics in the native app (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
40cd50d to
86719dd
Compare
|
|
Hi Salah-Eddine Lachkar (@artpumpkin)! The smoke test that was not performed, is a big part of the task, manually testing it. Please provide a video demonstration of the issue and the fix. You can download the packaged app from the "artifacts" in the Github Actions (it's an artifact of the |
cooper (czxtm)
left a comment
cooper (czxtm)
left a comment
There was a problem hiding this comment.
Requesting changes: the Fix with AI regression, the formatting failure and the packaged-app recording need to land before this can merge.
Four review threads are still open, including the Greptile P1 about onboarding getting stuck after the last AI-fix file is discarded. I confirmed that one at the current head and it also blocks merge, so please address it along with the items below.
Blocking
- Fix with AI now errors when the build log is missing or empty. On main,
run_fixusedunwrap_or_default()on the log tail, so a missing log only dropped the optional output context. Now it propagatesread_completed_logwith?(evolve.rs), which errors on an empty file, an unreadable file or a mismatched log path (rebuild_status.rs). A build that fails before writing any output, or a cleared~/Library/Logs/nixmac, now gives the user an error and no fix, even though the error text and type are already passed tobuild_fix_description. Fall back to an empty tail onErrinrun_fix, keep the strict errors for the Copy log command, and add tests for the empty and missing cases. - The Git Hooks check fails: the treefmt hook runs with
--fail-on-changeand rewrites two files. Inrebuild_status.rsit wraps the chain at L58 and the longifat L64. Indarwin.rsit drops the stray blank line at L1431. Runprek run --all-filesin the devenv shell and push the result. - The video requested on Oct 30, showing the issue and the fix in the packaged app, is still outstanding, and the PR body leaves the same smoke test unchecked.
Smaller things
read_completed_logreads the whole transcript into memory andrun_fixthen keeps only the last 200 lines. Read just the tail for the AI path.create_log_fileswitched to a nanosecond timestamp withcreate_new(true). A short test that two consecutive calls give distinct files would help.
Summary
Fixes #529. First-build failures now offer complete, run-specific log copying with clipboard feedback, evidence-based notices, and accurate safety guidance. Activation failures are identified separately from Nix build failures; the fixed generic suggestions are removed.
The completed rebuild status retains its transcript path and clears it on retry. Copying and AI recovery use that exact file, and log filenames no longer overwrite another run in the same second. Eligible failures offer the existing Fix with AI flow; onboarding now exposes its progress, error, Review and Apply surfaces.
Test Plan
Docs
Implementation and local validation were AI-assisted.
Packaged-app smoke test before merge
Still outstanding: on a disposable test Mac/VM, reproduce a Nix evaluation failure and an activation/Homebrew failure; copy a transcript exceeding 500 lines; retry and verify the new transcript; run an eligible AI fix and confirm Retry stays disabled during review, Discard works, and activation requires the review action. Confirm unknown safety state and permission guidance as well. This local contribution does not claim that smoke test passed.
Onboarding now explains proposed changes and the apply/discard/save actions. Unsupported Back to Prompt navigation is hidden within onboarding; it remains available in the normal widget. The existing completion latch in useOnboardingFlow remains unchanged.