From b30b8e2c83c64b248c79a658129510d256f2b195 Mon Sep 17 00:00:00 2001 From: "propr-dev[bot]" <1316198+propr-dev[bot]@users.noreply.github.com> Date: Thu, 22 Oct 2026 22:07:07 +0000 Subject: [PATCH 1/3] 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, 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. --- apps/desktop/src/logger-sinks.test.ts | 145 ++++++++++++++++++++++++++ apps/desktop/src/logger.ts | 17 ++- 2 files changed, 160 insertions(+), 2 deletions(-) create mode 100644 apps/desktop/src/logger-sinks.test.ts diff --git a/apps/desktop/src/logger-sinks.test.ts b/apps/desktop/src/logger-sinks.test.ts new file mode 100644 index 000000000..a9b7af2b2 --- /dev/null +++ b/apps/desktop/src/logger-sinks.test.ts @@ -0,0 +1,145 @@ +import assert from 'node:assert/strict'; +import { mkdtemp, readFile, rm, stat, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, beforeEach, describe, it } from 'node:test'; +import { createDesktopLogger, type LogLevel } from './logger'; + +type ConsoleMethodName = 'log' | 'warn' | 'error'; + +const CONSOLE_METHOD_FOR_LEVEL: Record = { + debug: 'log', + info: 'log', + warn: 'warn', + error: 'error', +}; + +const syncWriteError = (code: 'ENOSPC' | 'EPIPE'): NodeJS.ErrnoException => Object.assign( + new Error(`${code}: simulated synchronous console write failure, write`), + { code, errno: code === 'ENOSPC' ? -28 : -32, syscall: 'write' }, +); + +const waitFor = async (condition: () => Promise | boolean, label: string): Promise => { + const deadline = Date.now() + 5_000; + while (!(await condition())) { + if (Date.now() > deadline) throw new Error(`Timed out waiting for ${label}`); + await new Promise(resolve => setTimeout(resolve, 5)); + } +}; + +const readLogLines = async (logPath: string): Promise>> => { + const content = await readFile(logPath, 'utf8').catch(() => ''); + return content.split('\n').filter(Boolean).map(line => JSON.parse(line) as Record); +}; + +describe('desktop logger sinks', () => { + const originalConsole = { log: console.log, warn: console.warn, error: console.error }; + const consoleCalls: Array<{ method: ConsoleMethodName; line: string }> = []; + const unhandledRejections: unknown[] = []; + const onUnhandledRejection = (reason: unknown) => { unhandledRejections.push(reason); }; + let directory = ''; + + const recordConsole = (method: ConsoleMethodName, failure?: NodeJS.ErrnoException) => { + console[method] = (line: string) => { + consoleCalls.push({ method, line }); + if (failure) throw failure; + }; + }; + + beforeEach(async () => { + directory = await mkdtemp(join(tmpdir(), 'propr-desktop-logger-')); + consoleCalls.length = 0; + unhandledRejections.length = 0; + process.on('unhandledRejection', onUnhandledRejection); + for (const method of ['log', 'warn', 'error'] as const) recordConsole(method); + }); + + afterEach(async () => { + Object.assign(console, originalConsole); + process.off('unhandledRejection', onUnhandledRejection); + await rm(directory, { recursive: true, force: true }); + }); + + it('writes each level to its console method and to a private log file', async () => { + const logPath = join(directory, 'logs', 'desktop.log'); + const logger = createDesktopLogger(logPath); + const levels: LogLevel[] = ['debug', 'info', 'warn', 'error']; + for (const level of levels) logger.log(level, `desktop.test.${level}`, { detail: 'value' }); + + assert.deepEqual(consoleCalls.map(call => call.method), levels.map(level => CONSOLE_METHOD_FOR_LEVEL[level])); + await waitFor(async () => (await readLogLines(logPath)).length === levels.length, 'file sink records'); + const records = await readLogLines(logPath); + assert.deepEqual(records.map(record => [record.level, record.event, record.detail]), + levels.map(level => [level, `desktop.test.${level}`, 'value'])); + assert.deepEqual(consoleCalls.map(call => call.line), (await readFile(logPath, 'utf8')).trimEnd().split('\n')); + if (process.platform !== 'win32') { + assert.equal((await stat(logPath)).mode & 0o777, 0o600); + assert.equal((await stat(join(directory, 'logs'))).mode & 0o777, 0o700); + } + }); + + for (const code of ['ENOSPC', 'EPIPE'] as const) { + it(`contains synchronous ${code} console failures and still writes the file sink`, async () => { + for (const method of ['log', 'warn', 'error'] as const) recordConsole(method, syncWriteError(code)); + const logPath = join(directory, 'desktop.log'); + const logger = createDesktopLogger(logPath); + const levels: LogLevel[] = ['info', 'warn', 'error', 'debug']; + + for (const level of levels) { + assert.doesNotThrow(() => logger.log(level, 'desktop.credentials.decision', { + token: { secret: 'never-logged' }, + reason: new Error('/home/user/secret-path'), + })); + } + + assert.equal(consoleCalls.length, levels.length); + await waitFor(async () => (await readLogLines(logPath)).length === levels.length, 'file sink records'); + const records = await readLogLines(logPath); + assert.deepEqual(records.map(record => record.level), levels); + for (const record of records) { + assert.deepEqual(record.token, { code: 'DETAIL_REDACTED' }); + assert.deepEqual(record.reason, { code: 'OPERATION_FAILED' }); + } + assert.doesNotMatch(await readFile(logPath, 'utf8'), /never-logged|secret-path/); + assert.deepEqual(unhandledRejections, []); + }); + } + + it('contains a failed append whose fallback console and failure hook also throw', async () => { + const blockingFile = join(directory, 'not-a-directory'); + await writeFile(blockingFile, ''); + recordConsole('log', syncWriteError('ENOSPC')); + recordConsole('error', syncWriteError('EPIPE')); + let writeFailures = 0; + const logger = createDesktopLogger(join(blockingFile, 'desktop.log'), () => { + writeFailures += 1; + throw new Error('smoke sink unavailable'); + }); + + assert.doesNotThrow(() => logger.log('info', 'desktop.test.first')); + assert.doesNotThrow(() => logger.log('error', 'desktop.test.second')); + await waitFor(() => consoleCalls.filter(call => call.line.includes('desktop.log.write_failed')).length === 2, + 'fallback console attempts'); + await new Promise(resolve => setImmediate(resolve)); + + assert.equal(writeFailures, 2); + assert.deepEqual(consoleCalls.map(call => call.method), ['log', 'error', 'error', 'error']); + assert.deepEqual(JSON.parse(consoleCalls[2]!.line), + { level: 'error', event: 'desktop.log.write_failed', code: 'LOG_WRITE_FAILED' }); + assert.deepEqual(unhandledRejections, []); + }); + + it('keeps writing later records after a console failure recovers', async () => { + const logPath = join(directory, 'desktop.log'); + const logger = createDesktopLogger(logPath); + recordConsole('warn', syncWriteError('ENOSPC')); + logger.log('warn', 'desktop.test.during_failure'); + recordConsole('warn'); + logger.log('warn', 'desktop.test.after_recovery'); + + await waitFor(async () => (await readLogLines(logPath)).length === 2, 'file sink records'); + assert.deepEqual((await readLogLines(logPath)).map(record => record.event), + ['desktop.test.during_failure', 'desktop.test.after_recovery']); + assert.equal(consoleCalls.length, 2); + }); +}); diff --git a/apps/desktop/src/logger.ts b/apps/desktop/src/logger.ts index bead94f04..94c18bba6 100644 --- a/apps/desktop/src/logger.ts +++ b/apps/desktop/src/logger.ts @@ -205,6 +205,16 @@ export const formatDesktopLogRecord = ( ...sanitizeDesktopLogFields(event, fields), }); +// Diagnostic console output is best-effort: a synchronous stdout/stderr failure (for example +// ENOSPC or EPIPE from a SyncWriteStream) must not escape into the caller or skip the file sink. +const writeConsoleBestEffort = (consoleMethod: (line: string) => void, line: string): void => { + try { + consoleMethod.call(console, line); + } catch { + // The console sink is unavailable; the independent file sink still receives the record. + } +}; + export const createDesktopLogger = ( logPath: string, onWriteFailure?: () => void, @@ -213,7 +223,7 @@ export const createDesktopLogger = ( const log = (level: LogLevel, event: string, fields: Record = {}) => { const record = formatDesktopLogRecord(level, event, fields); const consoleMethod = level === 'error' ? console.error : level === 'warn' ? console.warn : console.log; - consoleMethod(record); + writeConsoleBestEffort(consoleMethod, record); pending = pending .then(async () => { await mkdir(dirname(logPath), { recursive: true, mode: 0o700 }); @@ -225,7 +235,10 @@ export const createDesktopLogger = ( } catch { // Keep the fixed logger diagnostic available even if the smoke-only sink also fails. } - console.error(JSON.stringify({ level: 'error', event: 'desktop.log.write_failed', code: 'LOG_WRITE_FAILED' })); + writeConsoleBestEffort( + console.error, + JSON.stringify({ level: 'error', event: 'desktop.log.write_failed', code: 'LOG_WRITE_FAILED' }), + ); }); }; return { log }; From 8c5007c4a1feda9ec9772c79a4db8910dc0f3f87 Mon Sep 17 00:00:00 2001 From: "propr-dev[bot]" <1316198+propr-dev[bot]@users.noreply.github.com> Date: Thu, 22 Oct 2026 22:12:21 +0000 Subject: [PATCH 2/3] feat(ai): 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. 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 --- package-lock.json | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/package-lock.json b/package-lock.json index 173e2d76f..4429a1f1a 100644 --- a/package-lock.json +++ b/package-lock.json @@ -8671,9 +8671,9 @@ } }, "node_modules/ip-address": { - "version": "10.7.2", - "resolved": "https://registry.npmjs.org/ip-address/-/ip-address-10.7.2.tgz", - "integrity": "sha512-7H/2gFSIitxc0hG3nOI1glS8QLo/EHBFFLk8vEUjXY/xu0AdL8jZ9U1IzO2PUm0d2D/ofQcAifb0g6OBkt8U7w==", + "version": "10.7.4", + "resolved": "https://registry.npmjs.org/ip-address/-/ip-address-10.7.4.tgz", + "integrity": "sha512-DWBMKyeM25D70nrIiTkgQth59/G+naKOTep6huF9j+GmeK3DMD60Zvxg5d5qQnz+38AHgou6ean2LECX+lrKDg==", "license": "MIT", "engines": { "node": ">= 12" From 3d3b36b5abc1e630d17cf505cfda4244585e3171 Mon Sep 17 00:00:00 2001 From: "propr-dev[bot]" <1316198+propr-dev[bot]@users.noreply.github.com> Date: Thu, 22 Oct 2026 22:38:35 +0000 Subject: [PATCH 3/3] feat(ai): 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. 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 --- test/prPublicationRecoveryJob.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/prPublicationRecoveryJob.test.ts b/test/prPublicationRecoveryJob.test.ts index 62c06ce96..b65caea42 100644 --- a/test/prPublicationRecoveryJob.test.ts +++ b/test/prPublicationRecoveryJob.test.ts @@ -306,7 +306,7 @@ const modules: Record> = { ensurePRProcessingLockToken: async () => 'token', releasePRProcessingLock: async (_redis: unknown, key: string) => { heldLocks.delete(key); }, startPRProcessingLockHeartbeat: () => noOp, }, - prCommentCollisionRecovery: { createPRCommentTaskStateIfMissing: async ({ taskId }: { taskId: string }) => { if (!taskStates.has(taskId)) taskStates.set(taskId, 'processing'); }, evaluatePRCommentPreExecutionRecovery: async () => ({}), handlePRCommentLockContention: async () => ({ status: 'deferred' }), schedulePRCommentRecovery: async () => undefined }, + prCommentCollisionRecovery: { createPRCommentTaskStateIfMissing: async ({ taskId }: { taskId: string }) => { if (!taskStates.has(taskId)) taskStates.set(taskId, 'processing'); }, evaluatePRCommentPreExecutionRecovery: async () => ({}), handlePRCommentLockContention: async () => ({ status: 'deferred' }), schedulePRCommentRecovery: async () => undefined, delayPRCommentJobInPlace: async () => false, predecessorWaitDelay: () => 60_000 }, prCompletionComment: { buildCompletionComment: async (commit: unknown, comments: unknown, options: unknown, result: unknown) => JSON.stringify({ commit, comments, options, result }), noChangeAnalysisSummary: () => '' }, reviewCommentGatherer: { markReviewFindingsProcessed: noOp }, // Follow-up assignment is covered by test/prAutoAssignmentFollowup.test.ts.