Repository navigation
[3320 by Claude Opus 5.5] Desktop logger output failure can block the app with an uncaught main-process error - #3321
Merged
integry merged 3 commits intoOct 22, 2026
Conversation
![propr-dev[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…the log file can't be written, `log()` no longer throws back into its caller, so `DesktopCredentialService.reportCredentialDecision` can no longer trigger the "JavaScript error occurred in the main process" dialog. The new regression tests pass, and the full desktop suite passes (`npm test`: 1408 tests, 1352 passed, 0 failed, the rest skipped). Typecheck is clean. The repo's ESLint config doesn't cover `apps/desktop`, so no lint gate applies to these files. Fix, in `apps/desktop/src/logger.ts`: - I added a small helper that calls the console method inside a `try`/`catch` and discards any error the console write throws. Both places that write to the console now go through it: the main per-record output, and the `desktop.log.write_failed` message printed when appending to the file fails. - Because the console write can no longer throw, the file append is always queued, even when the console fails. - The append's `.catch` can no longer reject, so there's no unhandled rejection even when the append, `onWriteFailure` and the fallback console all fail. - Nothing else changed: redaction, field bounds, formatting, `0o700`/`0o600` permissions and the `onWriteFailure` containment are as before. Errors from outside the console calls still propagate, and I added no global exception handler. New tests, in `apps/desktop/src/logger-sinks.test.ts`. The existing formatting/redaction suite is untouched. The tests swap in fake console methods that throw ENOSPC or EPIPE, and they never fill a real disk: - Normal behaviour: each level goes to its expected console method, the console and file lines match, and the file and directory get `0600`/`0700` permissions. - Console fails (ENOSPC and EPIPE): `log()` doesn't throw for any level, every record still reaches the log file with secrets and paths redacted, and there are no unhandled rejections. - Append fails, then the failure hook and fallback console also throw: the append is made to fail by putting the log file under a path that is actually a regular file. `onWriteFailure` runs once per failure, the fixed `LOG_WRITE_FAILED` message is attempted, and there are no unhandled rejections. - Recovery: logging keeps working after the console recovers from a failure. I also ran the new tests against the old `logger.ts`: 4 of the 5 fail. The one that still passes is the normal-behaviour test, as it should. No preview was created because nothing in the change is visible. Nothing has been merged or published; the Astra review and root validation at the final head still need to happen.
Owner
|
/review gpt-6-astra Review exact head b30b8e2 for issue #3320. Independently assess console ENOSPC/EPIPE containment, file-sink independence, rejection containment when both append and fallback diagnostics fail, preserved redaction/permissions, and meaningful failure-path tests. The operator observed a real Linux native main-process error dialog, and independently reproduced synchronous failures on all three levels in the previous logger. Do not merge or publish. Root independent validation is separate from this review. |
Contributor
Author
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
Contributor
Author
|
CI failed: Validate unsigned linux-x64 package Please investigate and fix this CI failure.
Failure evidence
|
Contributor
Author
|
CI failed: Validate unsigned linux-arm64 package Please investigate and fix this CI failure.
Failure evidence
|
Contributor
Author
|
CI failed: Validate unsigned darwin-arm64 package Please investigate and fix this CI failure.
Failure evidence
|
Contributor
Author
🔍 AI Code Review — gpt-6-astraOverall EvaluationReady to merge within the reviewed scope, conditional on pending checks completing successfully. The change directly contains the reported synchronous console failures without disrupting file logging. ✅ Independent sinks — Both normal console writes and fallback diagnostics use the guarded helper. A synchronous ENOSPC/EPIPE exception no longer escapes or prevents scheduling the file append. ✅ Rejection containment — The file-write rejection handler contains exceptions from both the failure hook and fallback console output, allowing subsequent queued records to proceed. ✅ Preserved protections — Formatting, redaction, and directory/file creation permissions remain unchanged. New tests cover all logging levels, both console failure codes, simultaneous diagnostic failures, and console recovery. This assessment is a static review of the supplied code; no commands or tests were run. The supplied issue body is truncated, so unseen requirements could not be assessed. Merge blockersNo merge blockers. SuggestionsThese are optional follow-ups and are not sent to S1: 🟢 Exercise append failure directlyIn ScoreThe implementation is localized and correctly addresses the demonstrated failure paths, with meaningful regression coverage and no verified blockers. The supplied current-head status has no failed checks, though 13 checks remain pending. Score: 9/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
Contributor
Author
|
CI failed: Validate unsigned darwin-x64 package Please investigate and fix this CI failure.
Failure evidence
|
Contributor
Author
|
CI failed: Finalize unsigned validation checksums Please investigate and fix this CI failure.
Failure evidence
|
Contributor
Author
|
✅ Applied the requested follow-up changes in commit 8c5007c Processed 3 comments:
Summary of ChangesI fixed the three failing checks by moving Cause: none of the three package jobs got to packaging. They all stopped at the "Audit committed dependency resolution" step, where Fix: 10.7.4 is already inside Checks run:
The logger change and its tests are unchanged. Nothing visible changed, so there's no preview. 🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
…m 10.7.2 to 10.7.4 in the root `package-lock.json`. That's the only file changed, and it's a three-line edit to the lock entry. I fixed the three failing checks by moving `ip-address` from 10.7.2 to 10.7.4 in the root `package-lock.json`. That's the only file changed, and it's a three-line edit to the lock entry. **Cause:** none of the three package jobs got to packaging. They all stopped at the "Audit committed dependency resolution" step, where `npm run audit:runtime` reported `Blocking vulnerabilities: ip-address`. A new moderate advisory was published against `ip-address` versions up to 10.7.3 (GHSA-rxp7-6hxx-x8q5, a long string can stall the process). It's pulled in indirectly through `express-rate-limit`, which asks for `^10.2.0`. This PR never touched the lockfile, so the gate failed because the advisory data changed, not because of the logger change. The darwin-arm64 queue-time line in the log is just GitHub's notice about macOS runner capacity. **Fix:** 10.7.4 is already inside `express-rate-limit`'s `^10.2.0` range, so I didn't need to touch `package.json`. I also didn't add any exception to `scripts/audit-dependencies.mjs`. `docs/package-lock.json` doesn't include `ip-address`. **Checks run:** - `npm run desktop:audit`, which is the runtime and packaging audits together: both pass. - I made the edit with npm 11, then ran npm 10 (`npm install --package-lock-only`), which is what CI's `npm ci` uses on Node 22. It left the lockfile byte-for-byte the same, so CI won't rewrite it. The logger change and its tests are unchanged. Nothing visible changed, so there's no preview. PR: #3321 Comment by: @propr-dev[bot] (ID: 6352389766) Comment by: @propr-dev[bot] (ID: 6352390484) Comment by: @propr-dev[bot] (ID: 6352392472) Model: Claude Opus 5.5
Contributor
Author
|
ℹ️ Analyzed the follow-up request by @propr-dev[bot] Analysis SummaryNo code change was needed for these two failures. The previous follow-up commit on this branch (
The current head already pins The workflow runs on No code changes were necessary based on the current state of the branch. 🤖 Analysis Details
💡 ProPR Slash Commands
🤖 Generated by ProPR |
propr-dev Bot
pushed a commit
that referenced
this pull request
Oct 22, 2026
… in the root `package-lock.json`. That is the only change: three lines in the `node_modules/ip-address` entry (version, resolved URL, integrity). I didn't touch any `package.json`, audit script or exception list. What I found on current main - Before the change, `npm run audit:runtime` failed with "Blocking vulnerabilities: ip-address", matching the CI failure. `npm run desktop:audit:packaging` passed on its own. - The only copy of ip-address is through `express-rate-limit@8.6.2`, whose `^10.2.0` range already allows 10.7.4. The full chain from `npm explain` is: root project, `@propr/api`, `@modelcontextprotocol/sdk` and `repomix` (via `@propr/core`) → `express-rate-limit@8.6.2` → `ip-address@10.7.4`. - The only other committed lockfile, `docs/package-lock.json`, doesn't contain ip-address, so I left it alone. How I updated it - I ran `npm update ip-address --package-lock-only` with npm 10, the version CI's Node 22 uses. npm 11 also rewrote about 30 unrelated `peer` flags, so I threw that result away. - Running `npm install --package-lock-only` with npm 10 afterwards leaves the lock byte-for-byte unchanged. Checks run - A fresh `npm ci` succeeded, and the installed copy reports version 10.7.4. - `npm ls ip-address --all` shows only `express-rate-limit@8.6.2 → ip-address@10.7.4`, and `npm audit --omit=dev` reports nothing for ip-address. - `npm run audit:runtime` and `npm run desktop:audit:packaging` both pass with no exceptions. - The rate-limit and MCP tests all pass, 8 of 8 files: `requestRateLimits`, `extensionCsrfScope`, `oauthLoopbackOrigin`, `hostedConnectStorage`, `mcpBrowser`, `routeAuthorization`, `demoMode` and `githubAuthCallback`. Left as is - `THIRD_PARTY_LICENSES.md` still lists `ip-address@10.4.0`. It was already out of date before this change (main had 10.7.2), and `scripts/build-images.sh` rebuilds it when images are built, so I didn't edit it. - `scripts/validate-third-party-notices.mjs file THIRD_PARTY_LICENSES.md` fails because entries for other packages (better-sqlite3, braces, undici) are missing. It says nothing about ip-address or express-rate-limit. This is a lockfile-only change with nothing visible, so I made no previews. The logger PR #3321 is untouched, and merging is left to you after review and exact-head validation.
Owner
|
/review gpt-6-astra Review exact current head8c5007c4a1feda9ec9772c79a4db8910dc0f3f87. Compared with your previous reviewed b30b8e2, the automatic CI followup updates only the root ip-address lock entry10.7.2 ->10.7.4 to address GHSA-rxp7-6hxx-x8q5. Independently inspect the resulting complete PR. Root independently verified13 original/new logger cases at b30 and a separate actual append failure plus recovery probe; current-head source/lock continuity and audits are being validated separately. Do not merge or publish. CI is currently running for8c5007c; no rerun needed while those handles remain active. |
Contributor
Author
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
Contributor
Author
🔍 AI Code Review — gpt-6-astraOverall EvaluationThe supplied diff for head ✅ Independent sink attempts — Console exceptions are caught before file writes are queued, so synchronous ENOSPC/EPIPE failures no longer escape or suppress the file sink. ✅ Contained fallback failures — Both the failure callback and fallback console diagnostic are guarded. The promise chain therefore recovers after a file-write failure and can process subsequent records. ✅ Preserved logging protections — Formatting, redaction, and private creation modes remain unchanged. New tests exercise all log levels, console failures, combined sink failures, and console recovery. This was a static review of the supplied complete diff and context; no commands or tests were run. The original issue excerpt is truncated, so assessment covers its visible objective and the supplied review requirements. Authoritative checks show no failures and 11 pending checks. Merge blockersNo merge blockers. SuggestionsThese are optional follow-ups and are not sent to S2: 🟢 Exercise actual append failure recoveryIn ScoreThe focused implementation addresses the demonstrated failure without changing redaction or permissions, and its regression tests cover the principal failure combinations. No code blockers or current check failures are established; remaining CI results are pending. Score: 9/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
Contributor
Author
|
CI failed: Full Test Suite Shard 3/4 Please investigate and fix this CI failure.
Failure evidence
|
Contributor
Author
|
CI failed: Run Full Test Suite Please investigate and fix this CI failure.
Failure evidence
|
Full Test Suite ResultsValidation failed during: Test shard (shard 3).
View shard 3/4 outputDetails truncated; see the uploaded artifacts for complete failure rows and logs. |
Contributor
Author
|
✅ Applied the requested follow-up changes in commit 3d3b36b Summary of ChangesI fixed the Shard 3/4 failure. It wasn't caused by this PR's logger or lockfile changes; it was a test mock that fell behind a commit already on main. Cause: Commit Fix: In
Returning Checks:
This is a test-only change with nothing visual, so there are no previews. 🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
…s logger or lockfile changes; it was a test mock that fell behind a commit already on main.
I fixed the Shard 3/4 failure. It wasn't caused by this PR's logger or lockfile changes; it was a test mock that fell behind a commit already on main.
**Cause:** `test/prPublicationRecoveryJob.test.ts` crashed while loading, so none of its tests ran:
```
SyntaxError: The requested module './prCommentCollisionRecovery.js' does not provide an export named 'delayPRCommentJobInPlace'
```
Commit `f5e35100f` ("wait in place behind a blocked predecessor follow-up handoff") is in this branch's base. It made `src/jobs/prCommentHandoffIntegration.ts` import two new functions from `prCommentCollisionRecovery.js`: `delayPRCommentJobInPlace` and `predecessorWaitDelay`. This suite replaces that module with a mock that lists each export by name, and the list didn't include the two new ones. The suite loads the real handoff module, so the import failed.
**Fix:** In `test/prPublicationRecoveryJob.test.ts` I added the two missing exports to the mock:
- `delayPRCommentJobInPlace: async () => false`
- `predecessorWaitDelay: () => 60_000`
Returning `false` means "can't wait in place". The code then falls back to scheduling a replacement job, which this suite already mocks, so the suite tests the same behaviour as before. That one test file is the only change.
**Checks:**
- `test/prPublicationRecoveryJob.test.ts`: 39 passed, 0 failed.
- The unchanged copy from HEAD still fails with the same SyntaxError, so the fix is what makes the difference.
- The two other suites that mock this module still pass: `prPublicationJob` 29/29 and `prCommentCollisionRecovery` 12/12.
This is a test-only change with nothing visual, so there are no previews.
PR: #3321
Comment by: @propr-dev[bot] (ID: 6352800921)
Model: Claude Opus 5.5
Contributor
Author
|
ℹ️ Analyzed the follow-up request by @propr-dev[bot], @github-actions[bot] Analysis SummaryNo new change was needed: the current branch head Cause: Fix (already on the branch): Checked at the current head:
A new CI run on No code changes were necessary based on the current state of the branch. 🤖 Analysis Details
💡 ProPR Slash Commands
🤖 Generated by ProPR |
Owner
|
/review gpt-6-astra Review exact head3d3b36b5abc1e630d17cf505cfda4244585e3171. The only delta from your reviewed8c5007c is in test/prPublicationRecoveryJob.test.ts: its mock now exports delayPRCommentJobInPlace and predecessorWaitDelay, matching the real import graph that failed in full-suite shard3. Production logger and dependency fix are unchanged. Independently verify the whole final PR remains scoped and the test change does not weaken recovery assertions or hide real failures. Root logger13cases and runtime/packaging audits passed at identical product sources; exact-head CI and root fixture validation remain required. Do not merge or publish. |
Contributor
Author
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
Contributor
Author
🔍 AI Code Review — gpt-6-astraOverall EvaluationThe PR is scoped and appears merge-ready within the supplied code, conditional on exact-head CI and root fixture validation completing successfully. No verified code blocker was found. ✅ Independent output sinks — Console exceptions are contained before scheduling file writes. The fallback diagnostic is also guarded, allowing the promise chain to recover when both sinks fail. ✅ Preserved logging protections — Record sanitization, serialized file writes, private creation permissions, and fixed failure diagnostics remain intact. New tests cover synchronous ENOSPC/EPIPE failures across all levels. ✅ Narrow fixture repair — The recovery mock adds two missing exports without changing assertions or production recovery logic. Returning This was a static review of the supplied diff and context; no commands or tests were run. The complete recovery fixture was not supplied, so its runtime behavior remains unverified here. The issue specification is explicitly truncated; conclusions cover the visible requirements. Current checks show no failures, with 13 pending. Merge blockersNo merge blockers. SuggestionsThese are optional follow-ups and are not sent to S3: 🟢 Exercise actual append recoveryIn ScoreThe implementation is localized, preserves existing security behavior, and adds meaningful failure-path coverage. Merge readiness remains conditional on the pending exact-head checks and fixture validation. Score: 8/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
propr-dev Bot
pushed a commit
that referenced
this pull request
Oct 22, 2026
…ally (39/39). Two neighbouring test files that use the same module also pass. I didn't re-run the rest of the shard. I fixed the shard 2/4 failure: the test file now passes locally (39/39). Two neighbouring test files that use the same module also pass. I didn't re-run the rest of the shard. **Cause.** Only `test/prPublicationRecoveryJob.test.ts` failed in shard 2. It didn't fail on an assertion; the test crashed on load because of a missing import: ``` SyntaxError: The requested module './prCommentCollisionRecovery.js' does not provide an export named 'delayPRCommentJobInPlace' ``` The jobs fix f5e3510, which this branch includes, made `src/jobs/prCommentHandoffIntegration.ts` import two new functions, `delayPRCommentJobInPlace` and `predecessorWaitDelay`. This test replaces that module with a stand-in that lists every function by name, and those two weren't on the list. That commit isn't part of the planner fix, so this failure comes from the base the branch was cut from, not from the F1/S1 changes. **Fix.** I added the two missing functions to the stand-in at `test/prPublicationRecoveryJob.test.ts:309`, copying the change main already made in #3321 (960a437). The branch was cut before that commit, so it didn't have it. No product code changed and nothing visible changed, so there are no previews. PR: #3325 Comment by: @propr-dev[bot] (ID: 6353859686) Model: Claude Opus 5.5
integry
pushed a commit
that referenced
this pull request
Oct 23, 2026
…erver but studio remains on Define (#3325) * I reproduced the bug in tests and fixed it. A fresh plan now goes Define → Generating → Review without a reload or a second generation. I haven't tried the fix in the installed native app, and the independent Astra review and real native/mobile check you asked for still need to happen before merge. Cause. The bug shows up when Generate is clicked before the autosave delay finishes: 1. Clicking Generate creates the draft in place, so the page starts its own `GET` of that draft. 2. The server takes seconds to reply to `POST /generate`, because it prepares the repository before marking the draft `generating`. The page's `GET` comes back first, with `status: draft`. 3. When `/generate` is acknowledged, `useDraftCreation` navigates to `/studio/:id` and puts the generating draft in the router state. But in `App.tsx` the two studio routes render the same component tree, so `PlanStudioPage` is not remounted. Its `useDraft` still holds the `draft` copy and ignores the router state. 4. Nothing ever tells the page about the new run, so it doesn't subscribe for progress and stays on Define with the prompt. My first Playwright mock answered `/generate` instantly, and the test passed on the broken code. It only failed once the mock held the reply until after the draft read, which is how the real server behaves. Fix: - `useDraftCreation.ts`: after `/generate` is acknowledged and before navigating, it hands the run ID and the generating draft to the page, the same way the existing-draft Generate path already did. Navigation still replaces `/studio/new`, so back/forward is unchanged. - `useDraft.ts`: - It accepts that generating draft even if its first fetch hasn't returned yet. - A fetch started before the run was acknowledged can no longer overwrite it; fetches started afterwards still win. - Reopening a page from history or a reload with a generating snapshot triggers one background refresh, so a finished plan isn't shown as still generating. It's a single fetch, not polling. - `SetupWizard.tsx`, `setupWizardHooks.ts`, `PlanStudioPage.tsx`: pass the draft along with the run ID. - Run-ID checks on live updates are unchanged: an update from a different run still can't complete this one. Tests: - `PlanStudioPage.freshGenerate.test.tsx` (new): uses the real page, routing, `useDraft` and `SetupWizard`, with create, generate and the draft fetch each held and released by the test. It covers web and desktop/hash routing; the stale read arriving before and after the acknowledgement; ignoring an update from another run; then Review. It also checks one draft and one generation, the URL, back/forward, prompt and focus after autosave, and the prompt kept for retry when Generate fails. All four race cases fail on the original code and pass now. - `useDraft.test.tsx`: three new unit tests for the behaviour above. - `e2e/planner-fresh-generate.pw.ts` (new): Playwright at 320px, 390px, 1440px, and the native minimum window (880×620 with the Linux titlebar). It fails on the original code in all four layouts and passes now. These Playwright runs use the web router; the desktop hash routing is only covered by the vitest test. - One existing assertion in `setupWizardHooks.test.tsx` was updated for the extra argument. - Results: - Full UI unit suite: 3053 of 3054 passed. The one failure was that outdated assertion; after updating it, those files and the planner/page suites pass (927 tests). - 58 related planner Playwright tests pass. - `tsc` and `eslint --max-warnings 0` are clean. Preview screenshots of the Generating and Review states at 390px and in the native minimum window are attached. * feat(ai): I fixed F1 (a late acknowledgement from one draft could take over another draft) and made the browser test's ordering explicit (S1). The full UI unit suite passes (3058/3058), `tsc` and `eslint --max-warnings 0` are clean, and the fresh-generate Playwright test passes in all 4 layouts. I haven't tried it in the installed native app. I fixed F1 (a late acknowledgement from one draft could take over another draft) and made the browser test's ordering explicit (S1). The full UI unit suite passes (3058/3058), `tsc` and `eslint --max-warnings 0` are clean, and the fresh-generate Playwright test passes in all 4 layouts. I haven't tried it in the installed native app. **F1 fix** - **`useDraft.ts`:** `activateGenerationRun` now requires the ID of the draft the run was started for: `(runId, draftId, snapshot?)`. If that ID isn't the draft currently shown, or a snapshot belongs to another draft, it returns before touching any run ID, draft, fetch counter, error or loading state. This applies whether or not a snapshot is passed. - **`setupWizardHooks.ts`:** the same mistaken assumption was in the existing-draft Generate handler. The wizard stays mounted when you go from `/studio/A` to `/studio/B`, so A's late acknowledgement used to call `startPolling(runA)` while B was on screen. B's own updates would then be rejected for having the wrong run ID. The handler now checks which draft is shown before handing off the run or polling. - **Callers:** `useDraftCreation.ts`, `SetupWizard.tsx` and `PlanStudioPage.tsx` pass the new draft-ID argument. **Tests** - The old mismatched-snapshot test, which expected A's run ID to be applied to the other draft, is replaced. The new one requires status and run ID to stay unchanged, both with a foreign snapshot and with no snapshot. - New hook test for A → B while A's acknowledgement is pending. A's late acknowledgement arrives twice, once while B's fetch is still running and once after it finishes. B keeps its own status and run ID both times, and still accepts its own live updates. - New wizard-handler test: switching to B before A's acknowledgement means neither the activation callback nor polling runs. - With the fixes temporarily removed, all three new tests fail. **S1, `e2e/planner-fresh-generate.pw.ts`** - The five-second fallback timer is gone. The page's reads now return a name the create response doesn't have, and the generate route holds the acknowledgement until the page's title shows that name. That proves React has rendered the pre-generation read first. - The test asserts that this happened, so a missing read now fails with a clear message instead of passing by timing. - Against the pre-PR source, the test fails at "Generating visible" in all layouts. No preview images: nothing visible changed. **Possible separate issue (not changed, not tested):** in `useDraftCreation.ts`, the fresh-plan flow still does `navigate('/studio/<id>', { replace: true })` after the acknowledgement arrives. If the user leaves `/studio/new` while Generate is pending, it may send them back to the new draft. That code came before this PR and is outside F1. PR: #3325 Comment by: @integry (ID: 6353387716) Model: Claude Opus 5.5 * feat(ai): I fixed the shard 2/4 failure: the test file now passes locally (39/39). Two neighbouring test files that use the same module also pass. I didn't re-run the rest of the shard. I fixed the shard 2/4 failure: the test file now passes locally (39/39). Two neighbouring test files that use the same module also pass. I didn't re-run the rest of the shard. **Cause.** Only `test/prPublicationRecoveryJob.test.ts` failed in shard 2. It didn't fail on an assertion; the test crashed on load because of a missing import: ``` SyntaxError: The requested module './prCommentCollisionRecovery.js' does not provide an export named 'delayPRCommentJobInPlace' ``` The jobs fix f5e3510, which this branch includes, made `src/jobs/prCommentHandoffIntegration.ts` import two new functions, `delayPRCommentJobInPlace` and `predecessorWaitDelay`. This test replaces that module with a stand-in that lists every function by name, and those two weren't on the list. That commit isn't part of the planner fix, so this failure comes from the base the branch was cut from, not from the F1/S1 changes. **Fix.** I added the two missing functions to the stand-in at `test/prPublicationRecoveryJob.test.ts:309`, copying the change main already made in #3321 (960a437). The branch was cut before that commit, so it didn't have it. No product code changed and nothing visible changed, so there are no previews. PR: #3325 Comment by: @propr-dev[bot] (ID: 6353859686) Model: Claude Opus 5.5 * feat(ai): I fixed S3. If the user leaves a fresh plan while its generation acknowledgement is still pending, the late acknowledgement no longer navigates, replaces their history entry or touches another draft. Generation isn't cancelled: the run stays on the server, and reopening the draft shows Generating and then Review, with one create and one generation. The new tests pass in real Chromium for all four layouts. I haven't tried it in the installed native app. I fixed S3. If the user leaves a fresh plan while its generation acknowledgement is still pending, the late acknowledgement no longer navigates, replaces their history entry or touches another draft. Generation isn't cancelled: the run stays on the server, and reopening the draft shows Generating and then Review, with one create and one generation. The new tests pass in real Chromium for all four layouts. I haven't tried it in the installed native app. **What was wrong.** After the generate response arrived, the fresh-plan flow always called `navigate('/studio/<id>', { replace: true })`, even if the user had left. My first fix checked the router's location and whether the page was still mounted. It passed the unit tests but failed in the real browser. In the production build, Back loads the `/plans` page in the background before switching, so for a moment the studio is still mounted and the router still reports the old location. The browser's history has already moved by then. **The fix.** - **New `useHistoryEntryOwnership` hook** (`propr-ui/src/hooks/useHistoryEntryOwnership.ts`): it records which browser history entry the user is on when work starts. Later, the work only counts as "still here" if the page is mounted and the browser is on that same entry. It reads the browser's history, not React state. - **Autosave vs. leaving:** the autosave URL change from `/studio/new` to `/studio/<id>` keeps the same history entry, so it still counts as "still here". Back, Forward, opening another draft or opening another page all fail the check. - **`useDraftCreation.ts`:** the check runs right after `generatePlan` returns. If the user has left, it skips the run hand-off and the navigation. The request is never cancelled or repeated. - **`PlanStudioPage.tsx` (same defect, one step earlier):** if the user left while the draft was still being created (`POST /drafts` pending), the in-place URL update overwrote whatever entry they were on, e.g. `/plans` became `/studio/<id>`. It now uses the same check. This path wasn't in your reproduction; I fixed it because it's the same flow and the same defect. **Tests** - **Hook tests (`useDraftCreation.test.tsx`):** the acknowledgement is dropped both when the wizard unmounts and when it stays mounted but the browser has moved to another entry. The test harness now performs the real in-place URL update, so the existing tests prove the autosave path still navigates. - **Page tests (`PlanStudioPage.freshGenerate.test.tsx`)**, run under both BrowserRouter and native HashRouter: - Back, then the acknowledgement arrives: `/plans` stays, with no new history entry. - Opening another existing draft while the acknowledgement is pending: that draft is untouched and isn't shown as Generating. - Back while the draft is still being created: the `/plans` entry isn't rewritten. Each case then reopens the generated draft and checks Generating → Review with one create and one generation. - **Playwright (`planner-fresh-generate.pw.ts`):** your reproduction in all four layouts, then Forward to reopen and complete the run. These four failed against my first fix and pass now. The original four fresh-generate cases still pass. **Results** - Full UI unit suite: 3066/3066. - `tsc --noEmit` and `eslint --max-warnings 0` on `propr-ui`: clean. - Playwright fresh-generate file: 8/8. - With either check disabled, the new tests fail. HashRouter is covered by the jsdom page tests only, because the Playwright web build uses BrowserRouter. The `native-minimum` layout is still a BrowserRouter page with the native window frame added. I didn't create previews, because the result is the user staying on the page they chose, which a still image can't show. Permission, credential, provider and repository settings are unchanged. PR: #3325 Comment by: @integry (ID: 6354022697) Model: Claude Opus 5.5 * feat(ai): I fixed F2. A late read for draft A — whether it succeeds, fails, or finishes loading — can no longer change draft B, its active run, or a newer read of A after you come back to it. I fixed F2. A late read for draft A — whether it succeeds, fails, or finishes loading — can no longer change draft B, its active run, or a newer read of A after you come back to it. **Fix** (`propr-ui/src/hooks/useDraft.ts`): - Each read of a draft now gets a sequence number. Changing `draftId` or unmounting invalidates every read still in flight. - A read only updates the draft, the active run ID or the error if it is still the newest read and its draft is still selected. That second check is what covers A → B → A: the ID matches again, but the old read is no longer the newest. - Loading is cleared by the newest read for the current draft, so a late A read can't end B's loading state. If you go to `/studio/new` (nothing selected), an abandoned read still clears loading, as it did before. - `fetchDraft` won't start for a draft that is no longer selected. Otherwise a `refetch` callback held from A's render could start an A read after switching to B, and that read would outrank B's. - The existing guard that stops a pre-acknowledgement read from replacing a just-started run is unchanged. The draft/run checks on the acknowledgement path (F1) and the history-entry navigation checks (S3) aren't touched. Nothing cancels or repeats a generation request. One behaviour change for the same draft: if two reads overlap, only the newest result is used. Before, whichever finished last won. **New tests** (`propr-ui/src/hooks/useDraft.selection.test.tsx`, 7 cases). They live in their own file because adding them to `useDraft.test.tsx` broke the 400-line lint limit. Each starts by reopening A from a history entry whose saved state says "generating", with A's background read held open: - B loads, then A's read resolves: B and its run stay selected, B's live updates are still accepted and updates carrying A's run ID are ignored. - A's read resolves while B is still loading: B stays loading, then B is shown. - A's read fails while B is loading: no error appears on B and B stays loading. - A → B → A, then the obsolete A read resolves or fails (two cases): A's newer result stays. - A → B → A, where the obsolete read finishes before the new A read: loading stays on until the new read lands. - A `refetch` held from A's render is called after switching to B: no read starts and B stays. **Verification:** - All 7 new tests fail against the previous `useDraft.ts` and pass with the fix. - `useDraft.test.tsx` is byte-identical to HEAD. - Full UI unit suite passes (3073/3073). I ran it before moving the tests into their own file; after the move, `useDraft*`, `PlanStudioPage*` and `TaskPlanner` pass (17 and 299 tests). `tsc --noEmit` and `eslint --max-warnings 0` on `src/hooks/` are clean. - `planner-fresh-generate.pw.ts` passes in real Chromium in all 4 layouts (8/8), covering the original fresh-generation flow and leaving before the acknowledgement arrives. I haven't tried it in the installed native app; your independent diagnostic and signed-native validation are still separate. I made no preview images, since nothing visible changes beyond the wrong-draft state being gone. PR: #3325 Comment by: @integry (ID: 6354464426) Model: Claude Opus 5.5 --------- Co-authored-by: propr-dev[bot] <1316198+propr-dev[bot]@users.noreply.github.com>
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.
AI Implementation Summary
Closes #3320
Branch:
3320/claude-opus-5-5-desktop-logger-output-fai-20261022-2201-u8gCommits: ✅ Changes committed (b30b8e2)
AI Processing Completed
Execution Details:
Summary:
Logging from the desktop app is now best-effort. If stdout/stderr or the log file can't be written,
log()no longer throws back into its caller, soDesktopCredentialService.reportCredentialDecisioncan no longer trigger the "JavaScript error occurred in the main process" dialog. The new regression tests pass, and the full desktop suite passes (npm test: 1408 tests, 1352 passed, 0 failed, the rest skipped). Typecheck is clean. The repo's ESLint config doesn't coverapps/desktop, so no lint gate applies to these files.Fix, in
apps/desktop/src/logger.ts:try/catchand discards any error the console write throws. Both places that write to the console now go through it: the main per-record output, and thedesktop.log.write_failedmessage printed when appending to the file fails..catchcan no longer reject, so there's no unhandled rejection even when the append,onWriteFailureand the fallback console all fail.0o700/0o600permissions and theonWriteFailurecontainment are as before. Errors from outside the console calls still propagate, and I added no global exception handler.New tests, in
apps/desktop/src/logger-sinks.test.ts. The existing formatting/redaction suite is untouched. The tests swap in fake console methods that throw ENOSPC or EPIPE, and they never fill a real disk:0600/0700permissions.log()doesn't throw for any level, every record still reaches the log file with secrets and paths redacted, and there are no unhandled rejections.onWriteFailureruns once per failure, the fixedLOG_WRITE_FAILEDmessage is attempted, and there are no unhandled rejections.I also ran the new tests against the old
logger.ts: 4 of the 5 fail. The one that still passes is the normal-behaviour test, as it should.No preview was created because nothing in the change is visible. Nothing has been merged or published; the Astra review and root validation at the final head still need to happen.
Detailed Logs:
95f3e41a-0d1d-4e0b-a95b-afdfcd43812cLog files stored at:
/usr/src/app/logs/executions/issue-3320-2026-10-22T22-07-13-183Z-conversation.json/usr/src/app/logs/executions/issue-3320-2026-10-22T22-07-13-183Z-output.txtLatest Conversation Messages
This PR was created automatically by ProPR after processing issue #3320.
💡 Need changes?
Comment on this PR to request refinements — the AI agent monitors comments and will update the implementation based on your feedback. Keep iterating until you're satisfied!