From 1ca428f881adc4353704ca7be4ac5af4e5828c6b Mon Sep 17 00:00:00 2001 From: Hamhire Hu Date: Tue, 28 Jul 2026 10:09:57 +0800 Subject: [PATCH 01/17] chore(dev): bump to 0.11.3-dev Post-release development-phase marker after shipping 0.11.2. Co-Authored-By: Claude Opus 4.8 (1M context) --- apps/desktop/package.json | 2 +- package-lock.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/desktop/package.json b/apps/desktop/package.json index e5e8f328..6f6f0220 100644 --- a/apps/desktop/package.json +++ b/apps/desktop/package.json @@ -1,6 +1,6 @@ { "name": "@meebox/desktop", - "version": "0.11.2", + "version": "0.11.3-dev", "private": true, "description": "meebox Electron desktop app", "author": { diff --git a/package-lock.json b/package-lock.json index 647509c0..7cf37584 100644 --- a/package-lock.json +++ b/package-lock.json @@ -32,7 +32,7 @@ }, "apps/desktop": { "name": "@meebox/desktop", - "version": "0.11.2", + "version": "0.11.3-dev", "dependencies": { "@iconify-json/material-icon-theme": "^1.2.66", "@iconify/react": "^5.2.1", From 03a1fc168703aabe5dbafe63d4301af4972bab54 Mon Sep 17 00:00:00 2001 From: Hamhire Hu Date: Tue, 28 Jul 2026 10:35:33 +0800 Subject: [PATCH 02/17] ci(pages): rebuild the website when the changelog changes The site changelog is synced from CHANGELOG.md / CHANGELOG.zh-CN.md, but they weren't in pages.yml's path filter, so a release that only touched the changelog (+ code) didn't redeploy the website and its changelog went stale. Add both to the push / pull_request paths, and document the coupling (plus the manual-dispatch fallback) in the release guide. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/pages.yml | 6 ++++++ docs/development/packaging-release.md | 2 ++ 2 files changed, 8 insertions(+) diff --git a/.github/workflows/pages.yml b/.github/workflows/pages.yml index 2fc91f60..d818ab5a 100644 --- a/.github/workflows/pages.yml +++ b/.github/workflows/pages.yml @@ -2,17 +2,23 @@ name: Deploy Website (GitHub Pages) # Brand website (website/, VitePress). Decoupled from the v* release pipeline: # build on every PR touching the site/docs (as a check), deploy on push to master. +# CHANGELOG.md / CHANGELOG.zh-CN.md are included because the site changelog is synced from them (see +# website/scripts/sync-docs.mjs) — a release merges the changelog to master and must rebuild the site. on: push: branches: [master] paths: - 'website/**' - 'docs/**' + - 'CHANGELOG.md' + - 'CHANGELOG.zh-CN.md' - '.github/workflows/pages.yml' pull_request: paths: - 'website/**' - 'docs/**' + - 'CHANGELOG.md' + - 'CHANGELOG.zh-CN.md' - '.github/workflows/pages.yml' workflow_dispatch: diff --git a/docs/development/packaging-release.md b/docs/development/packaging-release.md index 644278ca..aaea7145 100644 --- a/docs/development/packaging-release.md +++ b/docs/development/packaging-release.md @@ -48,6 +48,8 @@ Complete these **in the same batch of changes**, flowing through `dev` → `mast 1. **Version** — set the `version` in [apps/desktop/package.json](../../apps/desktop/package.json) to the target version (drop the `v` prefix; prereleases carry a suffix like `0.5.0-alpha.1`). electron-builder's `artifactName: code-meeseeks-${version}-...` reads this value directly — not updating it means the installer filename won't match the tag. After the change, run `npm install` to sync the lockfile. 2. **CHANGELOG** — the changelog is **bilingual, two files**: [CHANGELOG.md](../../CHANGELOG.md) is the English canonical and [CHANGELOG.zh-CN.md](../../CHANGELOG.zh-CN.md) is the Chinese mirror (**keep both in sync**). In **each** file, rename `## [Unreleased]` to `## [] - ` and add a `[]: …/compare/…` link reference at the bottom. **Releasing consumes Unreleased — leave no empty section**; create a new one (in both files) on the next development-phase changelog change. release.yml extracts the `## []` section **from the English canonical** (CHANGELOG.md) literally to inject into the Release body — the GitHub Release page is **single-language English**, and RELEASE_NOTES.md **leads with per-language deep-links to this version on the bilingual [website changelog](https://huhamhire.github.io/code-meeseeks/changelog)** — `[English](…) · [简体中文](…)` markdown links (EN root, ZH under `/zh/`, both landing on this version) — for readers of either language; a missing section means the body falls back with no change notes. The deep-link is a stable, date-independent anchor `changelog#v` (e.g. `0.10.0` → `#v0-10-0`): [sync-docs.mjs](../../website/scripts/sync-docs.mjs) injects that `{#…}` anchor onto each version heading in both locales' site copies (GitHub's CHANGELOG stays clean), and release.yml builds the same slug from the tag to fill RELEASE_NOTES.md's `%%CHANGELOG_URL_EN%%` / `%%CHANGELOG_URL_ZH%%` placeholders — so the two always agree without hand-maintenance. **If the stable release's content comes from a prior alpha/prerelease**: the development phase usually has no separate Unreleased (the content is already in the prerelease section), so just rename that prerelease section to the stable-version section and remove the corresponding `[-alpha.N]:` link reference (the content merges into the stable section, no empty stub left); other prerelease sections with no corresponding stable version are kept. + + **Website changelog auto-syncs** — the site changelog is built from `CHANGELOG.md` / `CHANGELOG.zh-CN.md` (via [sync-docs.mjs](../../website/scripts/sync-docs.mjs)), and [pages.yml](../../.github/workflows/pages.yml) lists both in its `paths` filter, so the `dev → master` release merge (which always updates the changelog) auto-triggers the Pages deploy and the website changelog updates on its own. No manual step in the normal flow; if a deploy is ever skipped, run it by hand: `gh workflow run pages.yml --ref master`. 3. **Proofread** — confirm the `## []` section covers every key point (Added / Changed / Fixed) merged into `dev` since the last version, **in both language files** (the English canonical and the Chinese mirror must not drift). The tag name and the package.json version must match (`v`). A prerelease tag with a `-` in the name (e.g. `-alpha.N`) is automatically marked prerelease by release.yml and does not claim Latest. From 4dbe9937bdaa6bdb9a578887ee41f29e952e0c16 Mon Sep 17 00:00:00 2001 From: Hamhire Hu Date: Mon, 7 Sep 2026 17:32:45 +0800 Subject: [PATCH 03/17] fix(agent): surface the real LLM failure cause and hint an unavailable model pr-agent's retry_with_fallback_models logs the underlying exception into loguru's `artifact=` field, which the default format never prints, so a failed LLM call reached the run card as nothing but "all fallback models failed". For the local CLI providers this was compounded twice over: codex reports its failures on stdout (turn.failed / error events in the JSONL stream) and leaves stderr empty, while a turn that dies on a tool failure exits 0 with no assistant message at all, which then failed downstream as an unexplained empty prompt. - shim: extract the cause from the codex event stream on a non-zero exit, and raise explicitly when a zero exit carries an empty reply; - shim: emit `@@MEEBOX_LLM_ERROR@@` on stderr, alongside the usage sentinel, so the cause travels past pr-agent's lossy retry log; - main: prefer that sentinel over the generic marker for errorMessage, and classify it into errorHint (currently `model-unavailable`); - renderer: render one localized remedy line under the raw cause, since a local CLI provider's model lives in that CLI's own configuration. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 11 +++++ CHANGELOG.zh-CN.md | 11 +++++ .../meebox_pragent_shim/cli/install.py | 30 ++++++++++--- .../meebox_pragent_shim/cli/parsers.py | 42 ++++++++++++++++++ .../meebox_pragent_shim/cli/specs.py | 5 ++- .../pragent-shim/meebox_pragent_shim/usage.py | 26 ++++++++++- .../main/services/pr-agent/run-executor.ts | 19 +++++--- .../src/main/services/pr-agent/usage.ts | 44 +++++++++++++++++-- .../chat/components/RunResultView.tsx | 6 +++ .../src/renderer/src/i18n/locales/de-DE.json | 3 ++ .../src/renderer/src/i18n/locales/en-US.json | 3 ++ .../src/renderer/src/i18n/locales/ja-JP.json | 3 ++ .../src/renderer/src/i18n/locales/zh-CN.json | 3 ++ .../src/styles/features/chat/run.scss | 8 ++++ docs/arch/02-agent/05-pragent-runtime.md | 5 +++ packages/poller/src/parse-output.ts | 29 ++++++++++++ packages/poller/tests/parse-output.test.ts | 31 +++++++++++++ packages/shared/src/poller-contract.ts | 7 +++ 18 files changed, 270 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a487e0b3..57dd4253 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,17 @@ All notable changes to this project are recorded here. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and the versioning follows [Semantic Versioning](https://semver.org/). +## [Unreleased] + +### ✨ Added + +- A review that fails because the model is unavailable now says so and tells you what to do — with a local CLI provider (claude / codex) the model comes from that CLI's own configuration, so it has to be changed there. + +### 🔧 Fixed + +- A failed review now shows the provider's actual error instead of only "all fallback models failed" — the real cause (an unavailable model, an expired login, an exhausted quota) was previously swallowed and never reached the run card. +- A local CLI provider that exits successfully but returns an empty reply is now reported as a failure naming that cause, rather than as an unexplained LLM failure. + ## [0.11.2] - 2026-07-28 > Highlights of this release: diff --git a/CHANGELOG.zh-CN.md b/CHANGELOG.zh-CN.md index 7b3a085e..7af6d2b5 100644 --- a/CHANGELOG.zh-CN.md +++ b/CHANGELOG.zh-CN.md @@ -5,6 +5,17 @@ 本项目所有重要变更记录于此。格式参考 [Keep a Changelog](https://keepachangelog.com/zh-CN/1.1.0/), 版本号遵循 [语义化版本](https://semver.org/lang/zh-CN/)。 +## [Unreleased] + +### ✨ 新增 + +- 因模型不可用而失败的评审现在会明确说明,并给出处理方式——使用本地 CLI 供应商(claude / codex)时,模型来自该 CLI 自身的配置,需要在那里更换。 + +### 🔧 修复 + +- 评审失败时现在会展示供应商返回的真实错误,而不再只有一句「所有备选模型均调用失败」——真正的原因(模型不可用、登录过期、额度耗尽)此前被吞掉,从未出现在运行卡片上。 +- 本地 CLI 供应商正常退出却返回空回复时,现在会作为失败上报并指明该原因,而不再表现为一次无从解释的 LLM 调用失败。 + ## [0.11.2] - 2026-07-28 > 本次发布要点: diff --git a/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/cli/install.py b/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/cli/install.py index 8d4c3e09..caa0e91b 100644 --- a/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/cli/install.py +++ b/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/cli/install.py @@ -11,7 +11,7 @@ import sys from ..runtime import _debug, strip_cache_break -from ..usage import _emit_usage_tokens +from ..usage import _emit_llm_error, _emit_usage_tokens from .specs import _CLI_SPECS @@ -88,12 +88,18 @@ async def run_cli_chat(bin_name, system, user) -> str: except Exception as exc: # noqa: BLE001 raise RuntimeError(f"failed to start CLI '{bin_name}': {exc}") from exc out, err = await proc.communicate(prompt.encode("utf-8")) + out_text = (out or b"").decode("utf-8", "replace") if proc.returncode != 0: - raise RuntimeError( - f"CLI '{bin_name}' exit code {proc.returncode}: " - f"{(err or b'').decode('utf-8', 'replace')[:500]}" - ) - text, usage = spec["parser"]((out or b"").decode("utf-8", "replace")) + # Error source: some CLIs report the failure only on **stdout** (codex writes turn.failed / error events into its + # JSONL stream and leaves stderr empty), which would otherwise surface as an empty reason and lose the real cause + # (auth / model unavailable / quota). When the spec provides an extractor, take stdout's verdict first and fall + # back to stderr; either way keep the raise contract (pr-agent turns it into "all fallback models failed"). + extractor = spec.get("error_extractor") + detail = (extractor(out_text) if extractor else None) or (err or b"").decode("utf-8", "replace") + detail = (detail or "")[:500] + _emit_llm_error(bin_name, detail) + raise RuntimeError(f"CLI '{bin_name}' exit code {proc.returncode}: {detail}") + text, usage = spec["parser"](out_text) if usage: # prompt_tokens ≈ total input-side size, output_tokens ≈ completion (input/output_tokens share the same names across both). # The cache fields differ in convention between the two: @@ -115,6 +121,18 @@ async def run_cli_chat(bin_name, system, user) -> str: cache_read_tokens=cache_read if isinstance(cache_read, int) else None, turns=turns if isinstance(turns, int) else None, ) + # Empty reply on a zero exit code: the CLI ran to completion but produced no assistant message (codex does this when a turn + # dies on a tool/feature failure, e.g. Code Mode failing closed because its host binary is missing). Downstream this became + # an empty prompt into pr-agent's load_yaml → the opaque "all fallback models failed"; raise here instead, carrying any error + # event the stream did report, so the run card names the actual cause. Usage is emitted first, keeping the token count truthful. + if not (text or "").strip(): + extractor = spec.get("error_extractor") + detail = (extractor(out_text) if extractor else None) or "" + msg = f"CLI '{bin_name}' returned an empty reply" + ( + f": {detail[:500]}" if detail else " (no error reported in its output)" + ) + _emit_llm_error(bin_name, msg) + raise RuntimeError(msg) return text diff --git a/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/cli/parsers.py b/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/cli/parsers.py index 5dc7b27c..7e504720 100644 --- a/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/cli/parsers.py +++ b/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/cli/parsers.py @@ -71,3 +71,45 @@ def _parse_codex_output(stdout): if isinstance(usage, dict) and turns: usage["num_turns"] = turns return (text if isinstance(text, str) else ""), usage + + +def _extract_codex_error(stdout): + """Pull the failure cause out of the `codex exec --json` event stream, returning a message string or None. + + codex reports failures on **stdout** (the JSONL stream) and leaves stderr empty, so without this the caller would + raise with an empty detail and the real cause (auth, model 404, quota ...) would never reach the logs. Precedence: + - `turn.failed` → error.message is the terminal verdict, most accurate; + - otherwise the last `type=="error"` event (the intermediate retry notices carry the same upstream message); + - otherwise the last `item.completed` whose item.type=="error" (startup-stage errors, e.g. an unavailable feature host). + """ + import json + + turn_failed = None + last_error = None + last_item_error = None + for line in (stdout or "").splitlines(): + line = line.strip() + if not line: + continue + try: + ev = json.loads(line) + except Exception: # noqa: BLE001 - skip non-JSON lines (logs, etc.) + continue + if not isinstance(ev, dict): + continue + etype = ev.get("type") + if etype == "turn.failed": + err = ev.get("error") + if isinstance(err, dict) and isinstance(err.get("message"), str): + turn_failed = err["message"] + elif etype == "error": + msg = ev.get("message") + if isinstance(msg, str): + last_error = msg + elif etype == "item.completed": + item = ev.get("item") + if isinstance(item, dict) and item.get("type") == "error": + msg = item.get("message") + if isinstance(msg, str): + last_item_error = msg + return turn_failed or last_error or last_item_error diff --git a/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/cli/specs.py b/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/cli/specs.py index 8145fd61..9e0ef3af 100644 --- a/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/cli/specs.py +++ b/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/cli/specs.py @@ -1,6 +1,6 @@ """Spec table for adapted local CLI commands: argv flags (the prompt always goes via stdin) + output parser + billing env to strip. Registering one entry here is enough for a new command; the renderer-side whitelist validation must stay in sync (see LlmProfileForm.validateProfile).""" -from .parsers import _parse_claude_output, _parse_codex_output +from .parsers import _extract_codex_error, _parse_claude_output, _parse_codex_output # `low_effort_flags`: argv to append for the low-effort tier (only enabled by the Agent orchestration channel via MEEBOX_CLI_REASONING, # see install.py). Commands with a trailing `-` (stdin) insert these flags before the `-`, keeping `-` last. @@ -30,6 +30,9 @@ ], "low_effort_flags": ["-c", "model_reasoning_effort=low"], "parser": _parse_codex_output, + # codex writes its failures into the stdout event stream and leaves stderr empty, so a non-zero exit needs + # the cause extracted from stdout (see install.py); commands without this key just fall back to stderr. + "error_extractor": _extract_codex_error, "strip_env": ("OPENAI_API_KEY", "CODEX_API_KEY"), }, } diff --git a/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/usage.py b/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/usage.py index bccf7e6b..74b45105 100644 --- a/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/usage.py +++ b/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/usage.py @@ -1,5 +1,10 @@ """Real token usage collection: emitted to stderr as a sentinel line `@@MEEBOX_USAGE@@ {json}`, which the main process onLine -accumulates from (see apps/desktop/src/main/ipc.ts). Takes only tokens, not cost. Fault-tolerant throughout.""" +accumulates from (see apps/desktop/src/main/ipc.ts). Takes only tokens, not cost. Fault-tolerant throughout. + +The same stderr sentinel channel also carries `@@MEEBOX_LLM_ERROR@@ {json}` (see _emit_llm_error): pr-agent's +retry_with_fallback_models logs the underlying exception into loguru's `artifact=` field, which the default format never +prints, so a failing LLM call reaches stdout as nothing but "Failed to generate prediction with any model". The sentinel +carries the real cause out of band, past that lossy log line.""" import sys from .runtime import _debug @@ -67,3 +72,22 @@ def _emit_usage_tokens( print(f"@@MEEBOX_USAGE@@ {json.dumps(rec)}", file=sys.stderr, flush=True) except Exception as exc: # noqa: BLE001 _debug(f"emit cli usage failed (ignored): {exc}") + + +def _emit_llm_error(cli, message) -> None: + """Emit the real cause of a failed LLM call to stderr as `@@MEEBOX_LLM_ERROR@@ {json}`, right before raising. + + Emitting is purely additive — the exception is still raised and pr-agent still runs its fallback retry; this only + keeps the cause from being swallowed by that retry's log line, so the run card can show what actually went wrong + (and classify it, e.g. an unavailable model) instead of the generic "all fallback models failed". + """ + try: + import json + + print( + f"@@MEEBOX_LLM_ERROR@@ {json.dumps({'cli': cli, 'message': message})}", + file=sys.stderr, + flush=True, + ) + except Exception as exc: # noqa: BLE001 + _debug(f"emit llm error failed (ignored): {exc}") diff --git a/apps/desktop/src/main/services/pr-agent/run-executor.ts b/apps/desktop/src/main/services/pr-agent/run-executor.ts index 398e0234..7701eb8f 100644 --- a/apps/desktop/src/main/services/pr-agent/run-executor.ts +++ b/apps/desktop/src/main/services/pr-agent/run-executor.ts @@ -15,6 +15,7 @@ import { } from '@meebox/pr-agent-bridge'; import { addFindingClosure, + classifyLlmFailure, dropPendingFindingDrafts, finishReviewRun, parseReviewOutput, @@ -37,7 +38,8 @@ import { accumulateUsageSentinel, finalizeUsage, newUsageAcc, - stripUsageSentinels, + parseLlmErrorSentinel, + stripShimSentinels, } from './usage.js'; import { neutralizeWorktreeInstructions } from './worktree-sanitize.js'; @@ -171,12 +173,18 @@ export class RunExecutor { durationMs: Date.now() - t0, exitCode: result.exitCode, stdout, - stderr: stripUsageSentinels(result.stderr), + stderr: stripShimSentinels(result.stderr), tokenUsage, }; if (parsed.llmFailure) { + // pr-agent's fallback retry only logs "Failed to generate prediction with any model" to stdout and hides the real + // exception in loguru's artifact field, so prefer the shim's stderr sentinel when present — it carries the actual + // provider/CLI error, which is also the only text worth classifying into an actionable hint. + const sentinel = parseLlmErrorSentinel(result.stderr); + const message = sentinel?.message ?? parsed.llmFailure.message; + const errorHint = classifyLlmFailure(message); this.ctx.logger.warn( - { runId, reason: parsed.llmFailure.message }, + { runId, reason: message, cli: sentinel?.cli, hint: errorHint }, 'pragent exit 0 but LLM call failed; marking run as failed', ); // Failed runs get no structured collection — findings set empty, UI shows only raw output (no chatpane finding card). @@ -184,7 +192,8 @@ export class RunExecutor { ...base, status: 'failed', errorReason: 'llm-error', - errorMessage: parsed.llmFailure.message, + errorMessage: message, + ...(errorHint ? { errorHint } : {}), findings: [], }; } @@ -225,7 +234,7 @@ export class RunExecutor { errorReason: err.reason, errorMessage: err.message, stdout: err.result.stdout, - stderr: stripUsageSentinels(err.result.stderr), + stderr: stripShimSentinels(err.result.stderr), findings: [], tokenUsage, }; diff --git a/apps/desktop/src/main/services/pr-agent/usage.ts b/apps/desktop/src/main/services/pr-agent/usage.ts index 33ba4076..922fac06 100644 --- a/apps/desktop/src/main/services/pr-agent/usage.ts +++ b/apps/desktop/src/main/services/pr-agent/usage.ts @@ -73,13 +73,51 @@ export function finalizeUsage(acc: UsageAcc): TokenUsage | undefined { } /** - * Strip usage sentinel lines from stderr before persistence: onLine already intercepts them in real time without forwarding, but exec internally + * Strip shim sentinel lines from stderr before persistence: onLine already intercepts them in real time without forwarding, but exec internally * accumulates all stderr into result.stderr (including sentinels), so clear these noise lines before persisting. */ -export function stripUsageSentinels(stderr: string | undefined): string | undefined { +export function stripShimSentinels(stderr: string | undefined): string | undefined { if (!stderr) return stderr; return stderr .split('\n') - .filter((l) => !l.includes(USAGE_SENTINEL)) + .filter((l) => !l.includes(USAGE_SENTINEL) && !l.includes(LLM_ERROR_SENTINEL)) .join('\n'); } + +/** + * LLM-error sentinel-line prefix (kept consistent with the shim's usage._emit_llm_error). Shares the stderr sentinel + * channel with usage: pr-agent's fallback retry logs the underlying exception into loguru's `artifact=` field, which the + * default format drops, so the real cause never reaches stdout — the shim routes it out of band through this line. + */ +export const LLM_ERROR_SENTINEL = '@@MEEBOX_LLM_ERROR@@'; + +/** + * Pull the LLM-call cause out of stderr sentinel lines, returning the **last** one (a run may retry several times; the last + * is what actually sank the run). Bad JSON / a missing message is ignored — this is a diagnostic enrichment, never a + * reason to fail differently. + */ +export function parseLlmErrorSentinel( + stderr: string | undefined, +): { cli?: string; message: string } | undefined { + if (!stderr) return undefined; + let found: { cli?: string; message: string } | undefined; + for (const line of stderr.split('\n')) { + const i = line.indexOf(LLM_ERROR_SENTINEL); + if (i < 0) continue; + try { + const rec = JSON.parse(line.slice(i + LLM_ERROR_SENTINEL.length).trim()) as { + cli?: unknown; + message?: unknown; + }; + if (typeof rec.message === 'string' && rec.message.trim()) { + found = { + cli: typeof rec.cli === 'string' ? rec.cli : undefined, + message: rec.message.trim(), + }; + } + } catch { + // Malformed sentinel → skip, keep any earlier one. + } + } + return found; +} diff --git a/apps/desktop/src/renderer/src/components/features/chat/components/RunResultView.tsx b/apps/desktop/src/renderer/src/components/features/chat/components/RunResultView.tsx index 2387d75c..610122f7 100644 --- a/apps/desktop/src/renderer/src/components/features/chat/components/RunResultView.tsx +++ b/apps/desktop/src/renderer/src/components/features/chat/components/RunResultView.tsx @@ -229,6 +229,12 @@ export function RunResultView({ {run.errorMessage && !isCancelled && (
{run.errorMessage}
)} + {/* Actionable hint on top of the raw cause: the message above is the provider's own wording (English, technical), + which doesn't tell the user what to do. A recognized kind adds one localized line of remedy — currently + 「the model is unavailable, change it」, whose fix lives outside the app for a local CLI provider. */} + {run.errorHint === 'model-unavailable' && !isCancelled && ( +

{t('chatPane.llmErrorHint.modelUnavailable')}

+ )} {/* Failed / cancelled no longer show a separate output block: the pr-agent log is already in the collapsible「raw output」above (stdout contains the [pr-agent stdout log] segment, same source as stderr), avoiding duplicating the same log into two blocks. */} diff --git a/apps/desktop/src/renderer/src/i18n/locales/de-DE.json b/apps/desktop/src/renderer/src/i18n/locales/de-DE.json index f4083b55..47f5e205 100644 --- a/apps/desktop/src/renderer/src/i18n/locales/de-DE.json +++ b/apps/desktop/src/renderer/src/i18n/locales/de-DE.json @@ -98,6 +98,9 @@ "goToSettings": "Zu den Einstellungen", "inputAria": "Chat-Eingabe", "llmCallFailed": "LLM-Aufruf fehlgeschlagen", + "llmErrorHint": { + "modelUnavailable": "Das angeforderte Modell ist nicht verfügbar. Bei einem lokalen CLI-Anbieter stammt das Modell aus der Konfiguration dieser CLI — stellen Sie dort auf ein unterstütztes Modell um und führen Sie den Lauf erneut aus." + }, "mergeConfirmLabel": "Mergen", "mergeConfirmMessage": "„{{title}}“ in den Zielbranch mergen? Dies kann nicht rückgängig gemacht werden.", "mergeConfirmTitle": "PR mergen", diff --git a/apps/desktop/src/renderer/src/i18n/locales/en-US.json b/apps/desktop/src/renderer/src/i18n/locales/en-US.json index def9c738..45fbae10 100644 --- a/apps/desktop/src/renderer/src/i18n/locales/en-US.json +++ b/apps/desktop/src/renderer/src/i18n/locales/en-US.json @@ -98,6 +98,9 @@ "goToSettings": "Go to Settings", "inputAria": "Chat input", "llmCallFailed": "LLM call failed", + "llmErrorHint": { + "modelUnavailable": "The requested model is unavailable. With a local CLI provider the model comes from that CLI's own configuration — switch it to a supported model there, then run again." + }, "mergeConfirmLabel": "Merge", "mergeConfirmMessage": "Merge \"{{title}}\" into its target branch? This can't be undone.", "mergeConfirmTitle": "Merge PR", diff --git a/apps/desktop/src/renderer/src/i18n/locales/ja-JP.json b/apps/desktop/src/renderer/src/i18n/locales/ja-JP.json index 5f1542f7..2929031a 100644 --- a/apps/desktop/src/renderer/src/i18n/locales/ja-JP.json +++ b/apps/desktop/src/renderer/src/i18n/locales/ja-JP.json @@ -98,6 +98,9 @@ "goToSettings": "設定へ移動", "inputAria": "チャット入力", "llmCallFailed": "LLM の呼び出しに失敗しました", + "llmErrorHint": { + "modelUnavailable": "要求されたモデルは利用できません。ローカル CLI プロバイダーの場合、モデルはその CLI 自身の設定で決まります。CLI 側でサポートされているモデルに変更してから再実行してください。" + }, "mergeConfirmLabel": "マージ", "mergeConfirmMessage": "「{{title}}」をターゲットブランチにマージしますか?この操作は取り消せません。", "mergeConfirmTitle": "PR をマージ", diff --git a/apps/desktop/src/renderer/src/i18n/locales/zh-CN.json b/apps/desktop/src/renderer/src/i18n/locales/zh-CN.json index e220dc1b..ad5bec2d 100644 --- a/apps/desktop/src/renderer/src/i18n/locales/zh-CN.json +++ b/apps/desktop/src/renderer/src/i18n/locales/zh-CN.json @@ -98,6 +98,9 @@ "goToSettings": "去设置", "inputAria": "聊天输入框", "llmCallFailed": "LLM 调用失败", + "llmErrorHint": { + "modelUnavailable": "所请求的模型不可用。使用本地 CLI 供应商时,模型由该 CLI 自身的配置决定,请在 CLI 中改用受支持的模型后重新运行。" + }, "mergeConfirmLabel": "合并", "mergeConfirmMessage": "确定将「{{title}}」合并到目标分支?此操作不可撤销。", "mergeConfirmTitle": "合并 PR", diff --git a/apps/desktop/src/renderer/src/styles/features/chat/run.scss b/apps/desktop/src/renderer/src/styles/features/chat/run.scss index 020a84a3..4a861667 100644 --- a/apps/desktop/src/renderer/src/styles/features/chat/run.scss +++ b/apps/desktop/src/renderer/src/styles/features/chat/run.scss @@ -41,6 +41,14 @@ color: $text-muted; } +// Actionable remedy line under the raw cause: prose (not mono like .chat-error-detail), so it reads as guidance +// from the app rather than as more provider output. +.chat-error-hint { + margin: $space-2 0 0; + font-size: $fs-xs; + line-height: 1.5; +} + // stderr / stdout collapsible area embedded in a failed run: one extra level of indentation inside the chat-error red banner .chat-error-stderr, .chat-error-stdout { diff --git a/docs/arch/02-agent/05-pragent-runtime.md b/docs/arch/02-agent/05-pragent-runtime.md index b0b904e2..f253597a 100644 --- a/docs/arch/02-agent/05-pragent-runtime.md +++ b/docs/arch/02-agent/05-pragent-runtime.md @@ -104,6 +104,11 @@ litellm**. - **Reuse the CLI's own login state**: the subprocess inherits `HOME`/`USERPROFILE`, and the CLI reads its own login credentials (e.g. `~/.claude`) to run. To avoid a stray API key in the local environment leaking in and overriding the CLI's own login method, the shim explicitly strips `ANTHROPIC_API_KEY` / `ANTHROPIC_AUTH_TOKEN` from the subprocess env. The model used, the quota and compliance are all decided by that CLI's account and the user's authorization. +- **Error source on a non-zero exit**: not every CLI reports its failure on stderr — `codex exec --json` writes `turn.failed` / `error` + events into the **stdout** JSONL stream and leaves stderr empty, so taking stderr alone would raise with an empty reason and the real + cause (auth expired / model unavailable / quota) would never reach the log, leaving only pr-agent's generic "all fallback models failed". + A spec may therefore declare an `error_extractor` that pulls the verdict out of stdout (precedence `turn.failed` → the last `error` + event → an `item.completed` error item), with stderr as the fallback for commands without one. - **Proxy auto pass-through**: the subprocess env is copied from `os.environ` (only the two API keys above removed), and `HTTP(S)_PROXY` / `NO_PROXY` are kept as-is → `claude`'s egress automatically goes through the user-configured proxy (see [Networking & proxy](../99-core/03-networking-proxy.md)), no extra setup needed. - **token usage**: from the claude JSON's `usage`, construct the same `@@MEEBOX_USAGE@@` sentinel, accumulated by the same main-process path. The ↑ total input takes diff --git a/packages/poller/src/parse-output.ts b/packages/poller/src/parse-output.ts index 0a3bf21d..265119ea 100644 --- a/packages/poller/src/parse-output.ts +++ b/packages/poller/src/parse-output.ts @@ -22,6 +22,35 @@ export interface ParsedReviewOutput { llmFailure?: { message: string }; } +/** + * Actionable classification of an LLM failure, used by the UI to add a "what to do about it" hint on top of the raw + * technical message. Only cases with a concrete user action get a kind; everything else stays unclassified (undefined) + * and renders the message alone. + * + * - `model-unavailable`: the model the provider was asked for does not exist / is not accessible for this account. + * Typical with a local CLI provider, whose model is pinned in the CLI's own config (e.g. `~/.codex/config.toml`) + * and can be retired upstream at any time — the app never chose it, so the fix is to change it there. + */ +export type LlmFailureKind = 'model-unavailable'; + +/** + * Classify an LLM failure message into an actionable kind, or undefined when nothing actionable is recognized. + * Matches the wording of both the direct-API path (litellm `NotFoundError` / `model_not_found`) and the local CLI path + * (codex's `The model \`x\` does not exist or you do not have access to it` / `is not supported when using ...`). + */ +export function classifyLlmFailure(message: string): LlmFailureKind | undefined { + const m = stripAnsi(message); + if ( + /model_not_found/i.test(m) || + /\bmodel\b[^\n]{0,80}\bdoes not exist\b/i.test(m) || + /\bmodel\b[^\n]{0,80}\bis not supported\b/i.test(m) || + /\bdo(?:es)? not have access to (?:it|this model)\b/i.test(m) + ) { + return 'model-unavailable'; + } + return undefined; +} + /** * Scan stdout for a marker of all LLM calls failing. When pr-agent's fallback retry exhausts all alternate * models and still fails, it only logger.error's one line "Failed to PR: Failed to generate diff --git a/packages/poller/tests/parse-output.test.ts b/packages/poller/tests/parse-output.test.ts index 0e26e369..0de269ab 100644 --- a/packages/poller/tests/parse-output.test.ts +++ b/packages/poller/tests/parse-output.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it } from 'vitest'; import { + classifyLlmFailure, parseReviewOutput, parseStructuredAsk, sectionToFinding, @@ -698,3 +699,33 @@ describe('parseStructuredAsk', () => { expect(parseReviewOutput('x', 'ask').askVerdict).toBeUndefined(); }); }); + +describe('classifyLlmFailure', () => { + it('codex: model retired upstream (404 wording) → model-unavailable', () => { + expect( + classifyLlmFailure( + "CLI 'codex' exit code 1: unexpected status 404 Not Found: The model `gpt-5.5` does not exist or you do not have access to it., url: https://chatgpt.com/backend-api/codex/responses", + ), + ).toBe('model-unavailable'); + }); + + it('codex: model not entitled for the account → model-unavailable', () => { + expect( + classifyLlmFailure( + "The 'gpt-5.1-codex' model is not supported when using Codex with a ChatGPT account.", + ), + ).toBe('model-unavailable'); + }); + + it('direct API: litellm model_not_found → model-unavailable', () => { + expect( + classifyLlmFailure('litellm.NotFoundError: The model `x` was not found (model_not_found)'), + ).toBe('model-unavailable'); + }); + + it('unrelated failures stay unclassified (no misleading remedy)', () => { + expect(classifyLlmFailure('litellm.AuthenticationError: invalid api key')).toBeUndefined(); + expect(classifyLlmFailure('Read timed out after 600s')).toBeUndefined(); + expect(classifyLlmFailure("CLI 'codex' returned an empty reply")).toBeUndefined(); + }); +}); diff --git a/packages/shared/src/poller-contract.ts b/packages/shared/src/poller-contract.ts index 5b6ecd5b..be2e9380 100644 --- a/packages/shared/src/poller-contract.ts +++ b/packages/shared/src/poller-contract.ts @@ -345,6 +345,13 @@ export interface ReviewRun { exitCode?: number; errorReason?: ReviewRunFailureReason; errorMessage?: string; + /** + * Actionable classification of the failure, when one is recognized (see poller's classifyLlmFailure). errorMessage stays + * the raw technical cause; this drives an extra "how to fix it" line in the UI, localized by the frontend from the kind. + * Currently only `model-unavailable` (the provider rejected the requested model — for a local CLI provider the model + * lives in that CLI's own config, so the user has to change it there). Historical runs don't have it. + */ + errorHint?: 'model-unavailable'; /** Raw stdout text; still kept after M3-B2 parses it into findings, for "see original" debugging */ stdout?: string; /** Raw stderr text */ From f39375ae67809a9957aa45f31125ee921dd00b9a Mon Sep 17 00:00:00 2001 From: Hamhire Hu Date: Mon, 7 Sep 2026 20:28:54 +0800 Subject: [PATCH 04/17] feat(pr): reconcile remote state after merge and review verdicts A merge or review verdict returns as soon as the remote accepts it, while the state the UI reads is recomputed asynchronously and lands seconds later: a merged PR keeps reporting open and keeps coming back in the discovery list, and mergeStatus.canMerge still holds its pre-verdict value. Refreshing the instant the action returned therefore read pre-action state and looked like nothing had happened -- the merged PR sat in the list, and the merge button stayed hidden after the approval that had just unblocked it, both until the next periodic poll. Both actions now start a bounded backoff re-check in main: refresh that one PR on a growing delay until the expected change appears, broadcasting prs:changed for the renderer to reload. It is fire-and-forget (the IPC never waits on the remote settling) and past its window the periodic poll remains the backstop. A confirmed merge archives that one PR directly, via a new Poller method. The list filters on archivedAt rather than PR state, so a merged PR disappears only once archived -- and running a whole poll tick for it would put every connection's discovery fetch in front of the departure the user is waiting on. The departure a poll infers from absence is already established by the confirmed remote state, so it is asserted directly. For the same reason the renderer no longer fires a full poll on merge: that round is guaranteed to read pre-merge state, spending a round-trip to redraw the same list. Living in main covers every entry point with one implementation -- the buttons, the chat commands, and the CLI's review write actions share these controllers. The single-PR refresh is extracted for reuse, with comment-cache invalidation made opt-in: a background re-check must not make the open comment / diff panes re-fetch for a state the user never asked about. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 2 + CHANGELOG.zh-CN.md | 2 + apps/desktop/src/main/controllers/pr.ts | 74 +++---- .../src/main/services/pr-post-action.ts | 187 ++++++++++++++++++ .../features/pr/hooks/usePullRequests.ts | 12 +- .../src/renderer/src/hooks/useBootstrap.ts | 10 + docs/arch/01-platform/03-review-workflow.md | 1 + packages/ipc/src/events.ts | 7 + packages/poller/src/poller.ts | 26 +++ packages/poller/tests/poller.test.ts | 27 +++ 10 files changed, 293 insertions(+), 55 deletions(-) create mode 100644 apps/desktop/src/main/services/pr-post-action.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 57dd4253..e5798ef1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,8 @@ and the versioning follows [Semantic Versioning](https://semver.org/). - A failed review now shows the provider's actual error instead of only "all fallback models failed" — the real cause (an unavailable model, an expired login, an exhausted quota) was previously swallowed and never reached the run card. - A local CLI provider that exits successfully but returns an empty reply is now reported as a failure naming that cause, rather than as an unexplained LLM failure. +- A merged PR now leaves the list on its own shortly after you merge it, instead of lingering until the next periodic sync — the remote takes a few seconds to actually mark it merged, and the app now waits for that rather than refreshing too early and finding nothing changed. +- Approving a PR now updates whether it can be merged, so the merge button appears as soon as your approval satisfies the last requirement — previously it stayed hidden until the next periodic sync, because the remote recomputes mergeability only after the approval returns. ## [0.11.2] - 2026-07-28 diff --git a/CHANGELOG.zh-CN.md b/CHANGELOG.zh-CN.md index 7af6d2b5..d41aafa9 100644 --- a/CHANGELOG.zh-CN.md +++ b/CHANGELOG.zh-CN.md @@ -15,6 +15,8 @@ - 评审失败时现在会展示供应商返回的真实错误,而不再只有一句「所有备选模型均调用失败」——真正的原因(模型不可用、登录过期、额度耗尽)此前被吞掉,从未出现在运行卡片上。 - 本地 CLI 供应商正常退出却返回空回复时,现在会作为失败上报并指明该原因,而不再表现为一次无从解释的 LLM 调用失败。 +- 合并 PR 后,该 PR 会在稍后自动从列表中消失,而不再滞留到下一次周期同步——远端需要几秒才真正标记为已合并,应用现在会等待这一刻,而不是过早刷新、结果什么都没变。 +- 批准 PR 后会重新判断其是否可合并,当你的批准满足最后一项要求时合并按钮即刻出现——此前它会一直隐藏到下一次周期同步,因为远端要在批准返回之后才重新计算可合并性。 ## [0.11.2] - 2026-07-28 diff --git a/apps/desktop/src/main/controllers/pr.ts b/apps/desktop/src/main/controllers/pr.ts index 106c6467..176ff52f 100644 --- a/apps/desktop/src/main/controllers/pr.ts +++ b/apps/desktop/src/main/controllers/pr.ts @@ -31,6 +31,11 @@ import { } from '@meebox/shared'; import { annotateOwnership } from '../services/comments.js'; import { getContext } from '../services/context.js'; +import { + confirmMergeSettled, + confirmMergeabilityAfterReview, + refreshSinglePr, +} from '../services/pr-post-action.js'; import type { IpcController } from './types.js'; /* @@ -322,56 +327,8 @@ export const refreshPrs: IpcController<'prs:refresh'> = () => getContext().polle * the mirror has the new commits so the diff renders the new code. Returns the updated PR; the renderer then reloads the * list locally (no network poll of other PRs). */ -export const refreshOnePr: IpcController<'prs:refreshOne'> = async (_event, req) => { - const ctx = getContext(); - const existing = await ctx.pr.findPrOrThrow(req.localId); - const adapter = ctx.pr.adapterForOrThrow(existing); - // Remote fetch of just this PR. 403/404 normalize to error codes (matching openPrByUrl); other errors bubble up. - let fresh; - try { - fresh = await adapter.prs.getSinglePullRequest( - { projectKey: existing.repo.projectKey, repoSlug: existing.repo.repoSlug }, - existing.remoteId, - ); - } catch (err) { - const status = (err as { status?: number } | null)?.status; - if (status === 403) throw new AppError(ERROR_CODES.PR_FORBIDDEN, undefined, 'forbidden'); - if (status === 404) throw new AppError(ERROR_CODES.PR_NOT_FOUND, undefined, 'not found'); - throw err; - } - // localStatus mirrors the remote current user's reviewer status (remote authoritative, same mapping as the poll); - // when the current user is unknown (ping incomplete) keep the recorded status rather than downgrading to pending. - const me = adapter.connection.getCurrentUser(); - const mineStatus = me ? fresh.reviewers.find((r) => r.name === me.name)?.status : undefined; - const localStatus = !me - ? existing.localStatus - : mineStatus === 'approved' - ? 'approved' - : mineStatus === 'needsWork' - ? 'needs_work' - : 'pending'; - const stored: StoredPullRequest = { - ...fresh, - localId: existing.localId, - platform: existing.platform, - connectionId: existing.connectionId, - localStatus, - // Preserve local-only bookkeeping (a single-PR refresh isn't a discovery pass). - discoveryFilters: existing.discoveryFilters, - discoveredAt: existing.discoveredAt, - lastSeenAt: new Date().toISOString(), - }; - await writePrMeta(await ctx.pr.storeForPr(req.localId), req.localId, stored); - await ctx.pr.invalidateCommentsCache(req.localId); - if (fresh.sourceRef.sha !== existing.sourceRef.sha) { - try { - await ctx.pr.ensureMirrorReadyForPr(stored); - } catch { - /* non-fatal: the diff view self-heals / surfaces a readable error if the mirror still lacks the sha */ - } - } - return stored; -}; +export const refreshOnePr: IpcController<'prs:refreshOne'> = (_event, req) => + refreshSinglePr(getContext(), req.localId, { invalidateComments: true }); /** * The Poller's most recent completion time (used for startup initialization). @@ -382,6 +339,11 @@ export const getLastSync: IpcController<'prs:lastSync'> = () => ({ /** * Set review status: write remote first (on failure the frontend is unchanged), and persist locally after the remote is OK. + * + * A verdict can change the remote's mergeability verdict (an approval satisfying the last required rule makes the PR + * mergeable), but the remote recomputes that asynchronously — the value returned here is still the pre-verdict one. So + * kick off a background re-check that lands the new canMerge, rather than leaving the merge button a poll interval + * behind reality (see services/pr-post-action.ts). */ export const setPrStatus: IpcController<'prs:setLocalStatus'> = async (_event, req) => { const ctx = getContext(); @@ -398,7 +360,9 @@ export const setPrStatus: IpcController<'prs:setLocalStatus'> = async (_event, r pr.remoteId, remoteStatus, ); - return setLocalStatus(ctx.stateStore, req.localId, req.status); + const updated = await setLocalStatus(ctx.stateStore, req.localId, req.status); + confirmMergeabilityAfterReview(ctx, req.localId, pr.mergeStatus.canMerge); + return updated; }; /** @@ -408,7 +372,12 @@ export const markRead: IpcController<'prs:markRead'> = (_event, req) => markPrRead(getContext().stateStore, req.localId); /** - * Merge a PR; do not persist locally here, relying on renderer refresh → poll soft-delete to finish, to avoid local and remote disagreeing. + * Merge a PR; do not persist locally here, relying on refresh → poll soft-delete to finish, to avoid local and remote disagreeing. + * + * The remote does not settle synchronously: for a moment after the merge is accepted the PR still reports open and still + * comes back in the discovery list, so an immediate refresh would show it sitting in the list untouched. A background + * re-check confirms it actually left the open state and then archives it through a poll tick (see + * services/pr-post-action.ts); it is deliberately not awaited, so the caller isn't blocked on the remote settling. */ export const mergePr: IpcController<'prs:merge'> = async (_event, req) => { const ctx = getContext(); @@ -418,6 +387,7 @@ export const mergePr: IpcController<'prs:merge'> = async (_event, req) => { { projectKey: pr.repo.projectKey, repoSlug: pr.repo.repoSlug }, pr.remoteId, ); + confirmMergeSettled(ctx, req.localId); }; /** diff --git a/apps/desktop/src/main/services/pr-post-action.ts b/apps/desktop/src/main/services/pr-post-action.ts new file mode 100644 index 00000000..a1c6ebd5 --- /dev/null +++ b/apps/desktop/src/main/services/pr-post-action.ts @@ -0,0 +1,187 @@ +import { writePrMeta } from '@meebox/poller'; +import { AppError, ERROR_CODES, type StoredPullRequest } from '@meebox/shared'; +import type { ServiceContext } from './context.js'; + +/** + * Post-action remote reconciliation: after a write action (merge / review verdict) the platform does **not** settle + * synchronously — the call returns as soon as the request is accepted, while the state the UI reads is recomputed + * asynchronously on the remote and lands some seconds later: + * + * - **merge**: the PR keeps reporting `state: 'open'` and keeps appearing in the discovery list for a moment, so a + * refresh fired the instant the merge returns still shows the PR sitting in the list as if nothing happened; + * - **review verdict**: `mergeStatus.canMerge` is a server-side verdict over approvals / builds / branch protection. + * An approval that satisfies the last required rule flips it to true — but only once the remote has recomputed it, + * which is after the approve call has already returned. + * + * A single immediate refresh therefore reads pre-action state and looks like nothing happened; leaving it to the + * periodic poll means waiting a whole interval. So both actions kick off a **backoff re-check** here: refresh the one + * PR on a growing delay until the expected change shows up (or the attempts run out, where the periodic poll remains + * the backstop), and broadcast `prs:changed` whenever the state actually moved so the renderer reloads. + * + * Living in main rather than in the renderer keeps every entry point covered by one implementation: the merge / approve + * buttons, the chat `/merge` `/approve` commands, and the CLI's review write actions all route through the same + * controllers (see services/api-server/routes/pr.ts). + */ + +/** + * Delays before each re-check attempt, in ms. The first is short because the common case is a remote that has already + * settled by the time the action returns, and that attempt sets the latency the user actually perceives; the rest grow + * to cover a slow remote without hammering it. The tail (~15s total) is deliberately shorter than a poll interval — + * past that, the periodic poll is the backstop and a background loop adds nothing. + */ +const RECHECK_DELAYS_MS = [400, 1_200, 3_500, 10_000] as const; + +/** + * Confirmation loops already running, keyed by `:`, so repeated clicks don't stack duplicate loops. + * Keyed by kind as well as PR: approving and then merging the same PR are two independent settlements, and a merge + * confirmation must not be dropped just because the verdict's loop is still winding down. + */ +const inFlight = new Set(); + +const sleep = (ms: number): Promise => new Promise((resolve) => setTimeout(resolve, ms)); + +/** + * Re-fetch one PR from the remote and persist it, returning the updated record (null when the PR is gone locally). + * + * Shared by the `prs:refreshOne` handler and the confirmation loops below. `invalidateComments` is what separates the + * two callers: a user-driven refresh wants comments re-fetched as well, while a background confirmation only cares + * about PR metadata — invalidating on every attempt would make the open comment / diff panes re-fetch several times + * over for a state the user never asked about. + */ +export async function refreshSinglePr( + ctx: ServiceContext, + localId: string, + opts: { invalidateComments: boolean }, +): Promise { + const existing = await ctx.pr.findPrOrThrow(localId); + const adapter = ctx.pr.adapterForOrThrow(existing); + // Remote fetch of just this PR. 403/404 normalize to error codes (matching openPrByUrl); other errors bubble up. + let fresh; + try { + fresh = await adapter.prs.getSinglePullRequest( + { projectKey: existing.repo.projectKey, repoSlug: existing.repo.repoSlug }, + existing.remoteId, + ); + } catch (err) { + const status = (err as { status?: number } | null)?.status; + if (status === 403) throw new AppError(ERROR_CODES.PR_FORBIDDEN, undefined, 'forbidden'); + if (status === 404) throw new AppError(ERROR_CODES.PR_NOT_FOUND, undefined, 'not found'); + throw err; + } + // localStatus mirrors the remote current user's reviewer status (remote authoritative, same mapping as the poll); + // when the current user is unknown (ping incomplete) keep the recorded status rather than downgrading to pending. + const me = adapter.connection.getCurrentUser(); + const mineStatus = me ? fresh.reviewers.find((r) => r.name === me.name)?.status : undefined; + const localStatus = !me + ? existing.localStatus + : mineStatus === 'approved' + ? 'approved' + : mineStatus === 'needsWork' + ? 'needs_work' + : 'pending'; + const stored: StoredPullRequest = { + ...fresh, + localId: existing.localId, + platform: existing.platform, + connectionId: existing.connectionId, + localStatus, + // Preserve local-only bookkeeping (a single-PR refresh isn't a discovery pass). + discoveryFilters: existing.discoveryFilters, + discoveredAt: existing.discoveredAt, + lastSeenAt: new Date().toISOString(), + }; + await writePrMeta(await ctx.pr.storeForPr(localId), localId, stored); + if (opts.invalidateComments) await ctx.pr.invalidateCommentsCache(localId); + if (fresh.sourceRef.sha !== existing.sourceRef.sha) { + try { + await ctx.pr.ensureMirrorReadyForPr(stored); + } catch { + /* non-fatal: the diff view self-heals / surfaces a readable error if the mirror still lacks the sha */ + } + } + return stored; +} + +/** + * Run the backoff re-check loop for one PR: refresh, hand the result to `settled`, and stop as soon as it reports the + * expected change (or the attempts run out). Errors on an individual attempt are swallowed and retried — a transient + * remote hiccup shouldn't abort the confirmation, and a PR that vanished locally (archived by a concurrent poll) ends + * the loop, since there is nothing left to confirm. + */ +async function recheckUntilSettled( + ctx: ServiceContext, + localId: string, + what: 'merge' | 'review-verdict', + settled: (pr: StoredPullRequest) => boolean, +): Promise { + const key = `${what}:${localId}`; + if (inFlight.has(key)) return; + inFlight.add(key); + try { + for (const delay of RECHECK_DELAYS_MS) { + await sleep(delay); + let pr: StoredPullRequest; + try { + pr = await refreshSinglePr(ctx, localId, { invalidateComments: false }); + } catch (err) { + if (err instanceof AppError && err.code === ERROR_CODES.PR_NOT_FOUND) return; + ctx.logger.debug({ err, localId, what }, 'post-action recheck attempt failed; retrying'); + continue; + } + if (!settled(pr)) { + // Not the change we're waiting for, but the attempt has still persisted fresh remote state (reviewer verdicts, + // vetoes, head sha); leaving that on disk unannounced would show a list disagreeing with what was just stored. + // Reloading is a local read, so the extra broadcast costs nothing. + ctx.broadcast('prs:changed', { localId }); + continue; + } + // The remote has settled. The list filters on archivedAt, not on PR state, so a merged PR only disappears once it + // is archived — archive this one directly rather than running a full poll tick for it: a tick would fetch every + // connection's discovery lists before the PR the user just merged could leave, and all of that is latency the user + // watches. The departure a poll infers from absence is already established here by the confirmed remote state. + if (what === 'merge') { + try { + await ctx.poller.archivePullRequest(localId); + } catch (err) { + ctx.logger.warn({ err, localId }, 'archiving the merged PR failed; the periodic poll will catch up'); + } + } + ctx.logger.info({ localId, what }, 'post-action remote state settled'); + ctx.broadcast('prs:changed', { localId }); + return; + } + ctx.logger.debug( + { localId, what }, + 'post-action remote state did not settle within the recheck window; leaving it to the periodic poll', + ); + } finally { + inFlight.delete(key); + } +} + +/** + * After a merge is accepted: confirm the PR actually left the open state, then archive it through a poll tick so it + * disappears from the list. Fire-and-forget — the merge IPC returns immediately and the button must not stay busy for + * the length of the confirmation. + */ +export function confirmMergeSettled(ctx: ServiceContext, localId: string): void { + void recheckUntilSettled(ctx, localId, 'merge', (pr) => pr.state !== 'open'); +} + +/** + * After a review verdict is written: confirm whether it changed the remote's mergeability verdict, so the merge button + * appears (or disappears) without waiting for the periodic poll. `before` is the canMerge value observed at the moment + * the verdict was written; any move away from it is the settlement we're waiting for. + */ +export function confirmMergeabilityAfterReview( + ctx: ServiceContext, + localId: string, + before: boolean, +): void { + void recheckUntilSettled( + ctx, + localId, + 'review-verdict', + (pr) => pr.mergeStatus.canMerge !== before, + ); +} diff --git a/apps/desktop/src/renderer/src/components/features/pr/hooks/usePullRequests.ts b/apps/desktop/src/renderer/src/components/features/pr/hooks/usePullRequests.ts index 4a5ee78e..34a90ef2 100644 --- a/apps/desktop/src/renderer/src/components/features/pr/hooks/usePullRequests.ts +++ b/apps/desktop/src/renderer/src/components/features/pr/hooks/usePullRequests.ts @@ -114,10 +114,16 @@ export function usePullRequests({ notifyError }: { notifyError: (msg: string) => } finally { setMerging(false); } - // Merge succeeded: the PR has transitioned to MERGED and will leave the pending list. Deselect + refresh to make it disappear + // Merge accepted: deselect, since the PR is on its way out of the pending list. + // + // Deliberately NOT a remote refresh here: the remote takes a moment to actually mark the PR merged and drop it from + // the discovery list, so a poll fired now reads pre-merge state — it would cost a full round-trip across every + // connection only to redraw the same list, with the PR still in it. Main confirms the merge landed and archives the + // PR (see services/pr-post-action.ts), then broadcasts prs:changed; the local reload below just reflects whatever is + // already on disk in the meantime. if (selectedId === mergedId) setSelectedId(null); - await triggerRefresh(); - }, [selected, selectedId, triggerRefresh, notifyError, merging, t]); + await reloadPrs(); + }, [selected, selectedId, reloadPrs, triggerRefresh, notifyError, merging, t]); return { prs, diff --git a/apps/desktop/src/renderer/src/hooks/useBootstrap.ts b/apps/desktop/src/renderer/src/hooks/useBootstrap.ts index 9dabd248..31918e8d 100644 --- a/apps/desktop/src/renderer/src/hooks/useBootstrap.ts +++ b/apps/desktop/src/renderer/src/hooks/useBootstrap.ts @@ -121,6 +121,16 @@ export function useBootstrap({ setPrs, reloadPrs }: UseBootstrapParams): { }); }, [reloadPrs]); + // A PR settled after a write action (merge landed / a review verdict changed mergeability): main has already + // persisted the new state, so reload the list locally — no remote call. Separate from poll:tick because these land + // between ticks, on the user's own action, and are exactly the moments where waiting a whole interval is felt. + useEffect(() => { + if (!window.api) return; + return subscribe('prs:changed', () => { + void reloadPrs(); + }); + }, [reloadPrs]); + // Proactively refresh the remote when the window regains focus: fetch PR meta; on Bitbucket, after adding a comment / // changing status, PR.updatedAt jumps → PrPanel's prUpdatedAt dep fires → force listComments to fetch new comments. useEffect(() => { diff --git a/docs/arch/01-platform/03-review-workflow.md b/docs/arch/01-platform/03-review-workflow.md index 9b0e90b4..603c45ec 100644 --- a/docs/arch/01-platform/03-review-workflow.md +++ b/docs/arch/01-platform/03-review-workflow.md @@ -18,6 +18,7 @@ Responsible for: review-command orchestration, output parsing, the draft state m - **finding state machine**: `pending → accepted/edited/rejected/posted`; on successful publish, `posted_remote_id` is recorded as the idempotency key to prevent re-sending. - **Publishing goes through platform inline comments**: batch `publishInlineComment`, internally mapping the finding anchor to the platform anchor (see [Platform adaptation](01-adapter.md)). - **Secondary comment operations**: reply / edit / delete (with can-edit/can-delete pre-checks: only your own authored comments may be operated on, re-validated on the remote); PR merge's entry is controlled by `mergeStatus.canMerge`, merge is irreversible and re-validated on the remote. +- **Post-action reconciliation (write actions don't settle synchronously)**: a merge / review verdict returns as soon as the remote *accepts* it, while the state the UI reads is recomputed asynchronously and lands seconds later — a merged PR keeps reporting `open` and keeps appearing in the discovery list, and `mergeStatus.canMerge` (a server-side verdict over approvals / builds / branch protection) still holds its pre-verdict value. A refresh fired the instant the action returns therefore reads pre-action state and looks like nothing happened, while leaving it to the periodic poll costs a whole interval. Both actions consequently start a **backoff re-check** in main (`services/pr-post-action.ts`): refresh that one PR on a growing delay until the expected change appears, then broadcast `prs:changed` for the renderer to reload. A confirmed merge additionally **archives that one PR** (`Poller.archivePullRequest`) — the list filters on `archivedAt`, not on PR state, so a merged PR disappears only once archived, and running a whole poll tick for it would put every connection's discovery fetch in front of the departure the user is waiting on. The departure a poll infers from absence is already established here by the confirmed remote state, so it is asserted directly. The loop is fire-and-forget (the action's IPC never waits on the remote settling) and bounded (past its window the periodic poll is the backstop). It lives in main so every entry point is covered by one implementation — the buttons, the chat commands, and the CLI's review write actions all route through the same controllers. - **Token usage lands on the run**: the main process captures the subprocess stderr's `@@MEEBOX_USAGE@@` sentinel line by line and accumulates it (see [pr-agent runtime](../02-agent/05-pragent-runtime.md)), writing to `ReviewRun.tokenUsage`; the UI run meta shows ↑input / ↓output. - **LLM-failure detection**: pr-agent may exit 0 while stdout is actually a full LLM failure (auth error / no available model) → the parse layer marks llmFailure and lands failed rather than "completed". diff --git a/packages/ipc/src/events.ts b/packages/ipc/src/events.ts index b7ad9ada..c4c63947 100644 --- a/packages/ipc/src/events.ts +++ b/packages/ipc/src/events.ts @@ -39,6 +39,13 @@ export interface IpcEvents { 'findingClosures:changed': { localId: string }; /** Broadcast after a comment reply / status change; renderer components (CommentsPanel / DiffView inline) refetch */ 'comments:changed': { localId: string }; + /** + * A PR's stored state changed outside a poll tick, and the renderer should reload the list. Emitted when a + * post-action re-check observes the remote settling after a write action — a merge actually landing (the PR has by + * then been archived out of the list), or a review verdict flipping the remote's mergeable verdict (the merge button + * appears / disappears). Without it those changes would surface only on the next periodic poll. + */ + 'prs:changed': { localId: string }; /** * Queue change broadcast: triggered by active add/remove or waiting add/remove. Renderer syncs the * chat-pane running UI + StatusBar queue chip. `active` is the list of currently concurrent running runs diff --git a/packages/poller/src/poller.ts b/packages/poller/src/poller.ts index dc582bd7..48358509 100644 --- a/packages/poller/src/poller.ts +++ b/packages/poller/src/poller.ts @@ -146,6 +146,32 @@ export class Poller { } } + /** + * Archive one PR by localId (same soft-delete move as a poll round's departure path: relocate the tree into cold + * storage, then mark archivedAt), returning whether anything changed. A no-op when the PR is unknown or already archived. + * + * Exists so a **confirmed departure** can take effect immediately instead of riding on a full poll round. The + * departure a poll infers from absence — "not seen this round" — is the same conclusion a caller can reach directly by + * observing the remote state (a merge that has landed), and there the whole-round cost is pure latency: a tick fetches + * every connection's discovery lists before the one PR the user just acted on can leave the list. + * + * Like archiveConnectionsExcept, this is driven by an explicit fact rather than by a failure to see the PR, so it does + * not weaken the "one network blip must not wrongly delete the store" invariant. + */ + async archivePullRequest(localId: string): Promise { + const indexFile = await readPrIndex(this.opts.stateStore); + const entry = indexFile?.prs[localId]; + if (!indexFile || !entry || entry.archivedAt) return false; + const now = (this.opts.now?.() ?? new Date()).toISOString(); + // Move the whole tree into archive cold storage, then mark archivedAt (migration precedes index persistence; a crash can idempotently retry). + await relocateTree(this.opts.stateStore, this.opts.archiveStore, prDirKey(localId)); + await writePrIndex(this.opts.stateStore, { + schema_version: 1, + prs: { ...indexFile.prs, [localId]: { ...entry, archivedAt: now } }, + }); + return true; + } + /** * Hot-swap the poll interval (seconds). While running, rebuild the timer on the new period (does not tick immediately); * the new interval takes effect from the next trigger. Called after the settings page changes the poll interval, no restart needed. diff --git a/packages/poller/tests/poller.test.ts b/packages/poller/tests/poller.test.ts index 0856aa05..02eaeeb9 100644 --- a/packages/poller/tests/poller.test.ts +++ b/packages/poller/tests/poller.test.ts @@ -404,6 +404,33 @@ describe('Poller.tick', () => { expect(stored[0]!.remoteId).toBe('1'); }); + it('archivePullRequest: departs one PR immediately, without a poll round', async () => { + const adapter = new FakeAdapter([ + makePr('1', '2026-05-28T01:00:00.000Z'), + makePr('2', '2026-05-28T02:00:00.000Z'), + ]); + const poller = new Poller({ + connections: [{ connectionId: 'bb1', adapter }], + stateStore: store, + archiveStore, + intervalSeconds: 60, + logger: noopLogger, + }); + await poller.tick(); + const merged = (await listStoredPullRequests(store)).find((p) => p.remoteId === '2')!; + + // The remote still lists both (a just-merged PR lingers in the discovery list), so a tick would not drop it — + // archiving by localId is what makes it leave now. + expect(await poller.archivePullRequest(merged.localId)).toBe(true); + const stored = await listStoredPullRequests(store); + expect(stored).toHaveLength(1); + expect(stored[0]!.remoteId).toBe('1'); + + // Idempotent: already archived, and an unknown localId is a no-op rather than an error. + expect(await poller.archivePullRequest(merged.localId)).toBe(false); + expect(await poller.archivePullRequest('no-such-pr')).toBe(false); + }); + it('all connections fail in one tick: index file mtime untouched + state intact', async () => { // first a successful poll to lay down the baseline const ok1 = makePr('1', '2026-05-28T01:00:00.000Z'); From 45ca431680b1f5156282c87bb469a83879d343e7 Mon Sep 17 00:00:00 2001 From: Hamhire Hu Date: Mon, 7 Sep 2026 21:21:29 +0800 Subject: [PATCH 05/17] fix(gui): never leave the window blank on a renderer failure A renderer failure could leave the window on its bare background permanently, indistinguishable from a hung app and unrecoverable short of a restart, because nothing was watching at any level: main.tsx rendered with no boundary above it, so any error thrown while rendering unmounted the whole tree and emptied #root; main registered no webContents listeners, so a dead renderer process went unnoticed; and no crash of either kind reached meebox.log, which is why such a report leaves nothing behind to diagnose. Three layers, each covering what the others structurally cannot see: - render-phase errors -> a root ErrorBoundary with a full-window AppCrashScreen (retry / reload), and the boundary now relays the stack via log:write, since the renderer console is not written to file and a caught render error is not an uncaught window error either; - failures before React mounts -> boot-guard.ts, imported first so it is armed before any other module can throw. A module failing while it initializes takes down the entry before render() runs, leaving no React and thus no boundary; the guard paints a plain-DOM recovery screen, depending on no React, i18n or stylesheet, since each is a candidate cause; - renderer process death -> render-process-gone / did-fail-load in main, logged and reloaded with a bounded retry budget so a page that dies on load cannot spin in a reload loop. 'unresponsive' is logged but not recovered: it usually resolves on its own and reloading would discard in-flight state. Also fixes a race in the same family: mergeSelectedPr compared the selection against the selectedId its callback had closed over, so selecting another PR while the merge round-trip was in flight cleared the PR the user had just opened. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 2 + CHANGELOG.zh-CN.md | 2 + .../src/main/bootstrap/window-manager.ts | 61 ++++++++++++++ apps/desktop/src/renderer/src/App.scss | 1 + apps/desktop/src/renderer/src/boot-guard.ts | 84 +++++++++++++++++++ .../src/components/common/AppCrashScreen.tsx | 32 +++++++ .../src/components/common/ErrorBoundary.tsx | 18 ++-- .../renderer/src/components/common/index.ts | 1 + .../features/pr/hooks/usePullRequests.ts | 8 +- .../src/renderer/src/i18n/locales/de-DE.json | 12 ++- .../src/renderer/src/i18n/locales/en-US.json | 12 ++- .../src/renderer/src/i18n/locales/ja-JP.json | 12 ++- .../src/renderer/src/i18n/locales/zh-CN.json | 12 ++- apps/desktop/src/renderer/src/main.tsx | 15 +++- .../src/renderer/src/styles/common/crash.scss | 58 +++++++++++++ docs/arch/03-gui/01-ui-interaction.md | 4 + 16 files changed, 318 insertions(+), 16 deletions(-) create mode 100644 apps/desktop/src/renderer/src/boot-guard.ts create mode 100644 apps/desktop/src/renderer/src/components/common/AppCrashScreen.tsx create mode 100644 apps/desktop/src/renderer/src/styles/common/crash.scss diff --git a/CHANGELOG.md b/CHANGELOG.md index e5798ef1..1fcfc80a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,8 @@ and the versioning follows [Semantic Versioning](https://semver.org/). - A local CLI provider that exits successfully but returns an empty reply is now reported as a failure naming that cause, rather than as an unexplained LLM failure. - A merged PR now leaves the list on its own shortly after you merge it, instead of lingering until the next periodic sync — the remote takes a few seconds to actually mark it merged, and the app now waits for that rather than refreshing too early and finding nothing changed. - Approving a PR now updates whether it can be merged, so the merge button appears as soon as your approval satisfies the last requirement — previously it stayed hidden until the next periodic sync, because the remote recomputes mergeability only after the approval returns. +- The window no longer goes permanently black when the interface fails: a crash now shows what went wrong with a retry / reload, a failure during startup shows a recovery screen, and a renderer that dies outright is reloaded automatically. Crash details are also written to the application log, which they previously never were. +- Switching to another PR while a merge is still in flight no longer clears the PR you just opened. ## [0.11.2] - 2026-07-28 diff --git a/CHANGELOG.zh-CN.md b/CHANGELOG.zh-CN.md index d41aafa9..cba14dd4 100644 --- a/CHANGELOG.zh-CN.md +++ b/CHANGELOG.zh-CN.md @@ -17,6 +17,8 @@ - 本地 CLI 供应商正常退出却返回空回复时,现在会作为失败上报并指明该原因,而不再表现为一次无从解释的 LLM 调用失败。 - 合并 PR 后,该 PR 会在稍后自动从列表中消失,而不再滞留到下一次周期同步——远端需要几秒才真正标记为已合并,应用现在会等待这一刻,而不是过早刷新、结果什么都没变。 - 批准 PR 后会重新判断其是否可合并,当你的批准满足最后一项要求时合并按钮即刻出现——此前它会一直隐藏到下一次周期同步,因为远端要在批准返回之后才重新计算可合并性。 +- 界面出错时窗口不再永久黑屏:渲染崩溃会显示出错内容并提供重试/重新加载,启动阶段失败会显示恢复界面,渲染进程整个崩溃则自动重新加载。崩溃详情同时写入应用日志——此前从不记录。 +- 合并请求仍在进行时切换到其他 PR,不再清空你刚打开的那个 PR。 ## [0.11.2] - 2026-07-28 diff --git a/apps/desktop/src/main/bootstrap/window-manager.ts b/apps/desktop/src/main/bootstrap/window-manager.ts index 3cbb56da..f1d94c6c 100644 --- a/apps/desktop/src/main/bootstrap/window-manager.ts +++ b/apps/desktop/src/main/bootstrap/window-manager.ts @@ -18,6 +18,12 @@ const __dirname = path.dirname(fileURLToPath(import.meta.url)); const DEFAULT_SIZE = { width: 1280, height: 800 }; const MIN_SIZE = { width: 960, height: 600 }; +/** + * How many times a window may auto-reload after a renderer crash / load failure before giving up. Bounded so a page + * that dies on load can't spin in a reload loop; the counter resets on any successful load (see installCrashRecovery). + */ +const MAX_CRASH_RELOADS = 3; + // Colors of the system window-control buttons on the right of the self-drawn title bar (Windows // titleBarOverlay): same color as the .app-titlebar background (--bg-app), so the seam is invisible; // the symbol takes the primary text color. Dark/light sets, following the effective theme @@ -204,12 +210,67 @@ export class WindowManager { return { action: 'deny' }; }); + this.installCrashRecovery(win); + if (process.env.ELECTRON_RENDERER_URL) { void win.loadURL(process.env.ELECTRON_RENDERER_URL); } else { void win.loadFile(path.join(__dirname, '../renderer/index.html')); } } + + /** + * Recover the window from renderer-level failures that the React error boundary cannot see. + * + * A boundary only catches errors thrown while rendering; if the renderer **process** dies (OOM, GPU fault, a native + * crash inside Monaco) or the document fails to load, no JavaScript survives to report it — the window is simply left + * showing its background colour, permanently, since nothing was watching. These listeners are that watcher: log what + * happened (the only record there would otherwise be) and reload the window so the user gets a working UI back + * instead of a dead frame. + * + * Reloads are capped: a page that crashes on load would otherwise reload forever, burning CPU and hiding the fault. + * After the cap the window is left as-is with an error in the log, and the user can restart the app. + */ + private installCrashRecovery(win: BrowserWindow): void { + let reloads = 0; + const reload = (reason: string): void => { + if (win.isDestroyed()) return; + if (reloads >= MAX_CRASH_RELOADS) { + this.logger.error( + { reason, reloads }, + 'renderer failed repeatedly; not reloading again (restart the app)', + ); + return; + } + reloads += 1; + this.logger.warn({ reason, attempt: reloads }, 'reloading the renderer to recover'); + win.webContents.reload(); + }; + // A successful load means the previous failure (if any) is behind us; reset the budget so an unrelated crash + // hours later still gets its full set of retries. + win.webContents.on('did-finish-load', () => { + reloads = 0; + }); + win.webContents.on('render-process-gone', (_evt, details) => { + this.logger.error({ details }, 'renderer process gone'); + // 'clean-exit' is a normal teardown (window closing), not a crash to recover from. + if (details.reason !== 'clean-exit') reload(`render-process-gone:${details.reason}`); + }); + win.webContents.on('did-fail-load', (_evt, errorCode, errorDescription, validatedURL) => { + // -3 is ERR_ABORTED, which a superseded navigation raises normally; nothing failed for the user. + if (errorCode === -3) return; + this.logger.error({ errorCode, errorDescription, validatedURL }, 'renderer failed to load'); + reload(`did-fail-load:${errorCode}`); + }); + // Not recovered automatically: an unresponsive renderer is usually a long synchronous task that finishes on its + // own, and reloading would throw away the user's in-flight state. Logged so a hang leaves a trace. + win.webContents.on('unresponsive', () => { + this.logger.warn('renderer became unresponsive'); + }); + win.webContents.on('responsive', () => { + this.logger.info('renderer became responsive again'); + }); + } } /** Loads the window state (missing/corrupt → empty object, falling back to default size) and constructs the WindowManager. */ diff --git a/apps/desktop/src/renderer/src/App.scss b/apps/desktop/src/renderer/src/App.scss index 801115a7..b8dc3f11 100644 --- a/apps/desktop/src/renderer/src/App.scss +++ b/apps/desktop/src/renderer/src/App.scss @@ -29,6 +29,7 @@ @use './styles/features/pr-info'; @use './styles/features/chat'; @use './styles/features/drafts-panel'; +@use './styles/common/crash'; @use './styles/common/modal'; @use './styles/features/pr'; // pr 簇(含 publish-review,须在 common/modal 之后) @use './styles/features/settings/forms'; diff --git a/apps/desktop/src/renderer/src/boot-guard.ts b/apps/desktop/src/renderer/src/boot-guard.ts new file mode 100644 index 00000000..26c3d435 --- /dev/null +++ b/apps/desktop/src/renderer/src/boot-guard.ts @@ -0,0 +1,84 @@ +/** + * Last-resort guard against a blank window, sitting below React entirely. + * + * The root ErrorBoundary covers errors thrown *while rendering*, and main covers the renderer process dying. Between + * those two lies a gap neither can see: a module that throws while it initializes — i18n, theme, the app bundle itself + * — takes down the entry module before `createRoot().render()` ever runs. No React means no boundary; the window just + * stays on its background colour forever, which is the black screen users report and cannot recover from. + * + * So this module (imported first, so its side effect is armed before anything else can throw) watches for `#root` + * staying empty and paints a plain-DOM recovery screen if it does. No React, no i18n, no stylesheet — every one of + * those is a thing that could be the failure being reported, so the screen depends on none of them. + */ + +/** How long to wait for React's first commit before assuming boot failed. Generous: a cold start on a slow machine + * pulls a sizable bundle, and a false positive would replace a working (if slow) boot with an error screen. */ +const BOOT_TIMEOUT_MS = 15_000; + +/** Fixed English copy: i18n may itself be the module that failed, and English is the app's fallback language anyway. */ +const COPY = { + title: 'Code Meeseeks failed to start', + hint: 'The interface could not be loaded. Reloading usually resolves it; if it keeps happening, the details below and the application log (meebox.log) identify the cause.', + reload: 'Reload', +}; + +function rootIsEmpty(): boolean { + const root = document.getElementById('root'); + return !root || root.childElementCount === 0; +} + +/** Paint the recovery screen, once — later failures must not stack copies of it on top of each other. */ +function showRecovery(detail: string): void { + if (document.getElementById('boot-guard-screen')) return; + if (!rootIsEmpty()) return; // The app did render after all; leave the working UI alone. + const host = document.createElement('div'); + host.id = 'boot-guard-screen'; + // Inline styles on purpose: the stylesheet is part of the app bundle that may have failed to load. + host.setAttribute( + 'style', + 'position:fixed;inset:0;z-index:2147483647;display:flex;align-items:center;justify-content:center;' + + 'padding:16px;background:#1e1e1e;color:#ccc;font:13px/1.5 system-ui,sans-serif;', + ); + const card = document.createElement('div'); + card.setAttribute('style', 'max-width:560px;width:100%'); + const title = document.createElement('h1'); + title.setAttribute('style', 'margin:0 0 8px;font-size:15px;font-weight:600;color:#eee'); + title.textContent = COPY.title; + const hint = document.createElement('p'); + hint.setAttribute('style', 'margin:0 0 12px;color:#999'); + hint.textContent = COPY.hint; + const pre = document.createElement('pre'); + pre.setAttribute( + 'style', + 'margin:0 0 16px;padding:8px;max-height:220px;overflow:auto;background:#181818;' + + 'border:1px solid #333;border-radius:4px;white-space:pre-wrap;word-break:break-word;font-size:12px;color:#999', + ); + pre.textContent = detail; + const button = document.createElement('button'); + button.type = 'button'; + button.setAttribute( + 'style', + 'padding:4px 12px;background:#0e639c;color:#fff;border:none;border-radius:3px;cursor:pointer;font:inherit', + ); + button.textContent = COPY.reload; + button.addEventListener('click', () => window.location.reload()); + card.append(title, hint, pre, button); + host.append(card); + document.body.append(host); +} + +// An uncaught error while the app is still blank means boot failed — no need to wait out the timeout. Once the app has +// rendered, the same errors are the app's own business (the root boundary and the log relay handle them), so +// rootIsEmpty() inside showRecovery keeps this from hijacking a working window. +window.addEventListener('error', (e: ErrorEvent) => { + showRecovery(e.error instanceof Error ? (e.error.stack ?? e.error.message) : e.message); +}); +window.addEventListener('unhandledrejection', (e: PromiseRejectionEvent) => { + const reason: unknown = e.reason; + showRecovery(reason instanceof Error ? (reason.stack ?? reason.message) : String(reason)); +}); + +// Backstop for a silent failure — a module that never resolves, an import that hangs — where nothing throws at all. +setTimeout(() => { + showRecovery(`The interface did not render within ${BOOT_TIMEOUT_MS / 1000}s of startup.`); +}, BOOT_TIMEOUT_MS); diff --git a/apps/desktop/src/renderer/src/components/common/AppCrashScreen.tsx b/apps/desktop/src/renderer/src/components/common/AppCrashScreen.tsx new file mode 100644 index 00000000..087edb2d --- /dev/null +++ b/apps/desktop/src/renderer/src/components/common/AppCrashScreen.tsx @@ -0,0 +1,32 @@ +import { useTranslation } from 'react-i18next'; + +/** + * Full-window fallback for a crash in the app's root subtree. + * + * Without a boundary at the root, any error thrown while rendering unmounts the whole tree and leaves an empty `#root` + * — which reads as a permanently black window, with no way back short of restarting the app. This screen is what the + * user gets instead: what broke, and two ways out. + * + * `onRetry` re-renders the subtree, which is enough when the crash came from transient state (a stale record read + * during an in-flight update); reloading rebuilds the renderer from scratch and is the way out when it did not. + */ +export function AppCrashScreen({ err, onRetry }: { err: Error; onRetry: () => void }) { + const { t } = useTranslation(); + return ( +
+
+

{t('crash.title')}

+

{t('crash.hint')}

+
{err.message || String(err)}
+
+ + +
+
+
+ ); +} diff --git a/apps/desktop/src/renderer/src/components/common/ErrorBoundary.tsx b/apps/desktop/src/renderer/src/components/common/ErrorBoundary.tsx index 9488882e..956756ce 100644 --- a/apps/desktop/src/renderer/src/components/common/ErrorBoundary.tsx +++ b/apps/desktop/src/renderer/src/components/common/ErrorBoundary.tsx @@ -1,4 +1,5 @@ import { Component, type ErrorInfo, type ReactNode } from 'react'; +import { invoke } from '../../api'; interface ErrorBoundaryProps { children: ReactNode; @@ -27,11 +28,18 @@ export class ErrorBoundary extends Component { + /* the log relay must never be the thing that breaks the fallback UI */ + }); } reset = (): void => { diff --git a/apps/desktop/src/renderer/src/components/common/index.ts b/apps/desktop/src/renderer/src/components/common/index.ts index d49478f4..781175ee 100644 --- a/apps/desktop/src/renderer/src/components/common/index.ts +++ b/apps/desktop/src/renderer/src/components/common/index.ts @@ -2,6 +2,7 @@ // Cross-domain consumers (features/* · layout/* · App etc.) import via this barrel; common's internal modules reference each other // (markdownMermaid → MermaidDiagram, Modal → icons, ConfirmModal → Modal) via relative paths, // not through this barrel, to avoid circular dependencies. +export * from './AppCrashScreen'; export * from './Avatar'; export * from './BitbucketImage'; export * from './ConfirmModal'; diff --git a/apps/desktop/src/renderer/src/components/features/pr/hooks/usePullRequests.ts b/apps/desktop/src/renderer/src/components/features/pr/hooks/usePullRequests.ts index 34a90ef2..1cde9875 100644 --- a/apps/desktop/src/renderer/src/components/features/pr/hooks/usePullRequests.ts +++ b/apps/desktop/src/renderer/src/components/features/pr/hooks/usePullRequests.ts @@ -121,9 +121,13 @@ export function usePullRequests({ notifyError }: { notifyError: (msg: string) => // connection only to redraw the same list, with the PR still in it. Main confirms the merge landed and archives the // PR (see services/pr-post-action.ts), then broadcasts prs:changed; the local reload below just reflects whatever is // already on disk in the meantime. - if (selectedId === mergedId) setSelectedId(null); + // + // Compare against the **current** selection, not the `selectedId` this callback closed over: a merge is a remote + // round-trip, and clicking another PR while it is in flight leaves the closure holding the pre-merge id. Testing + // that stale value would clear a selection the user has since made, blanking the PR they just opened. + setSelectedId((cur) => (cur === mergedId ? null : cur)); await reloadPrs(); - }, [selected, selectedId, reloadPrs, triggerRefresh, notifyError, merging, t]); + }, [selected, reloadPrs, setSelectedId, triggerRefresh, notifyError, merging, t]); return { prs, diff --git a/apps/desktop/src/renderer/src/i18n/locales/de-DE.json b/apps/desktop/src/renderer/src/i18n/locales/de-DE.json index 47f5e205..b0cf0dcc 100644 --- a/apps/desktop/src/renderer/src/i18n/locales/de-DE.json +++ b/apps/desktop/src/renderer/src/i18n/locales/de-DE.json @@ -325,6 +325,12 @@ "testing": "Teste…", "tokenLabel": "Zugriffstoken (PAT)" }, + "crash": { + "hint": "Ein Neuladen behebt das in der Regel. Tritt es wiederholt auf, benennen die Details unten und das Anwendungsprotokoll (meebox.log) die Ursache.", + "reload": "Neu laden", + "retry": "Erneut versuchen", + "title": "Beim Rendern der Oberfläche ist ein Fehler aufgetreten" + }, "diffSearchPanel": { "caseSensitiveOff": "Groß-/Kleinschreibung beachten (aus)", "caseSensitiveOn": "Groß-/Kleinschreibung beachten (ein)", @@ -344,9 +350,11 @@ "binaryNotRendered": "⚠️ Binärdatei, Diff wird nicht dargestellt", "blameChangeRangeTitle": "Dieser Bereich wurde durch diesen PR geändert", "blameFailed": "blame fehlgeschlagen", - "blameFailedNamed": "blame für {{path}} fehlgeschlagen", "diffRenderFailed": "Diff-Darstellung fehlgeschlagen: {{message}}", + "blameFailedNamed": "blame für {{path}} fehlgeschlagen", + "diffRenderFailed": "Diff-Darstellung fehlgeschlagen: {{message}}", "diffRenderFailedHint": "Ein Dateiwechsel oder erneuter Versuch behebt das Problem meist. Der zugrunde liegende Fehler wurde in der console protokolliert.", - "dismissNotificationTitle": "Diese Benachrichtigung schließen", "fileCount_one": "{{count}} Datei", + "dismissNotificationTitle": "Diese Benachrichtigung schließen", + "fileCount_one": "{{count}} Datei", "fileCount_other": "{{count}} Dateien", "lfsManagedTitle": "Von Git LFS verwaltet", "loadChangedFilesFailed": "Geänderte Dateien konnten nicht geladen werden", diff --git a/apps/desktop/src/renderer/src/i18n/locales/en-US.json b/apps/desktop/src/renderer/src/i18n/locales/en-US.json index 45fbae10..de7892f1 100644 --- a/apps/desktop/src/renderer/src/i18n/locales/en-US.json +++ b/apps/desktop/src/renderer/src/i18n/locales/en-US.json @@ -325,6 +325,12 @@ "testing": "Testing…", "tokenLabel": "Access Token (PAT)" }, + "crash": { + "hint": "Reloading usually resolves it. If it keeps happening, the details below and the application log (meebox.log) identify the cause.", + "reload": "Reload", + "retry": "Retry", + "title": "Something went wrong rendering the interface" + }, "diffSearchPanel": { "caseSensitiveOff": "Case sensitive (off)", "caseSensitiveOn": "Case sensitive (on)", @@ -344,9 +350,11 @@ "binaryNotRendered": "⚠️ Binary file, diff not rendered", "blameChangeRangeTitle": "This range was changed by this PR", "blameFailed": "Blame failed", - "blameFailedNamed": "Blame failed for {{path}}", "diffRenderFailed": "Diff render failed: {{message}}", + "blameFailedNamed": "Blame failed for {{path}}", + "diffRenderFailed": "Diff render failed: {{message}}", "diffRenderFailedHint": "Switching files or retrying usually recovers. The underlying error has been logged to the console.", - "dismissNotificationTitle": "Dismiss this notification", "fileCount_one": "{{count}} file", + "dismissNotificationTitle": "Dismiss this notification", + "fileCount_one": "{{count}} file", "fileCount_other": "{{count}} files", "lfsManagedTitle": "Managed by Git LFS", "loadChangedFilesFailed": "Failed to load changed files", diff --git a/apps/desktop/src/renderer/src/i18n/locales/ja-JP.json b/apps/desktop/src/renderer/src/i18n/locales/ja-JP.json index 2929031a..a333b9a8 100644 --- a/apps/desktop/src/renderer/src/i18n/locales/ja-JP.json +++ b/apps/desktop/src/renderer/src/i18n/locales/ja-JP.json @@ -319,6 +319,12 @@ "testing": "テスト中…", "tokenLabel": "アクセストークン (PAT)" }, + "crash": { + "hint": "再読み込みで通常は復旧します。繰り返し発生する場合は、以下の詳細とアプリケーションログ(meebox.log)で原因を特定できます。", + "reload": "再読み込み", + "retry": "再試行", + "title": "画面の描画中にエラーが発生しました" + }, "diffSearchPanel": { "caseSensitiveOff": "大文字小文字を区別 (オフ)", "caseSensitiveOn": "大文字小文字を区別 (オン)", @@ -337,10 +343,12 @@ "binaryNotRendered": "⚠️ バイナリファイル、差分は表示されません", "blameChangeRangeTitle": "この範囲はこの PR で変更されました", "blameFailed": "blame に失敗しました", - "blameFailedNamed": "{{path}} の blame に失敗しました", "diffRenderFailed": "差分の表示に失敗しました: {{message}}", + "blameFailedNamed": "{{path}} の blame に失敗しました", + "diffRenderFailed": "差分の表示に失敗しました: {{message}}", "diffRenderFailedHint": "ファイルを切り替えるか再試行すると通常は復旧します。元のエラーは console に記録されています。", "dismissNotificationTitle": "この通知を閉じる", - "fileCount_other": "{{count}} ファイル", "lfsManagedTitle": "Git LFS で管理", + "fileCount_other": "{{count}} ファイル", + "lfsManagedTitle": "Git LFS で管理", "loadChangedFilesFailed": "変更ファイルの読み込みに失敗しました", "loadCommentsFailed": "コメントの読み込みに失敗しました", "loadingContentHint": "ローカルミラーから git blob を読み込んでいます。大きいファイルやバイナリファイルは時間がかかる場合があります", diff --git a/apps/desktop/src/renderer/src/i18n/locales/zh-CN.json b/apps/desktop/src/renderer/src/i18n/locales/zh-CN.json index ad5bec2d..1b2860b6 100644 --- a/apps/desktop/src/renderer/src/i18n/locales/zh-CN.json +++ b/apps/desktop/src/renderer/src/i18n/locales/zh-CN.json @@ -319,6 +319,12 @@ "testing": "测试中…", "tokenLabel": "访问令牌 (PAT)" }, + "crash": { + "hint": "重新加载通常即可恢复。若反复出现,下方详情与应用日志(meebox.log)可定位原因。", + "reload": "重新加载", + "retry": "重试", + "title": "界面渲染出错" + }, "diffSearchPanel": { "caseSensitiveOff": "区分大小写 (已关闭)", "caseSensitiveOn": "区分大小写 (已开启)", @@ -337,10 +343,12 @@ "binaryNotRendered": "⚠️ 二进制文件,不渲染 diff", "blameChangeRangeTitle": "此区段为本 PR 引入的改动", "blameFailed": "blame 失败", - "blameFailedNamed": "{{path}} blame 失败", "diffRenderFailed": "diff 渲染失败:{{message}}", + "blameFailedNamed": "{{path}} blame 失败", + "diffRenderFailed": "diff 渲染失败:{{message}}", "diffRenderFailedHint": "切换文件 / 重试通常能恢复。底层异常已记录到 console。", "dismissNotificationTitle": "收起此通知", - "fileCount_other": "{{count}} 个文件", "lfsManagedTitle": "由 Git LFS 管理", + "fileCount_other": "{{count}} 个文件", + "lfsManagedTitle": "由 Git LFS 管理", "loadChangedFilesFailed": "拉取变更文件列表失败", "loadCommentsFailed": "拉取评论失败", "loadingContentHint": "从本地镜像读 git blob,大文件 / 二进制判定时可能略慢", diff --git a/apps/desktop/src/renderer/src/main.tsx b/apps/desktop/src/renderer/src/main.tsx index 7b00c3ca..b7dc08d5 100644 --- a/apps/desktop/src/renderer/src/main.tsx +++ b/apps/desktop/src/renderer/src/main.tsx @@ -1,3 +1,7 @@ +// boot-guard must be the FIRST import: its side effect arms the last-resort watchdog before any other module runs, so a +// module that throws while initializing (i18n / theme / App below) still ends with a readable screen instead of a +// permanently blank window. Anything imported above it would be outside the guard. +import './boot-guard'; import { StrictMode } from 'react'; import { createRoot } from 'react-dom/client'; import { addCollection } from '@iconify/react'; @@ -10,6 +14,7 @@ import './i18n'; // avoiding a light-mode user flashing a frame of dark on startup. import './theme'; import App from './App'; +import { AppCrashScreen, ErrorBoundary } from './components/common'; import './App.scss'; // Preload the PKief Material Icon Theme so that @@ -21,6 +26,14 @@ if (!container) throw new Error('#root not found'); createRoot(container).render( - + {/* Root boundary: without one, any error thrown while rendering unmounts the entire tree and leaves an empty + #root — an all-black window with no way back short of restarting the app. Here the user gets what broke plus + a retry / reload, and the boundary relays the stack to main so the crash is on disk in meebox.log. */} + } + > + + , ); diff --git a/apps/desktop/src/renderer/src/styles/common/crash.scss b/apps/desktop/src/renderer/src/styles/common/crash.scss new file mode 100644 index 00000000..837dfde4 --- /dev/null +++ b/apps/desktop/src/renderer/src/styles/common/crash.scss @@ -0,0 +1,58 @@ +// Full-window crash fallback (AppCrashScreen): what replaces the app when the root subtree fails to render. +// Deliberately self-contained — it must stay legible even when the failure came from the very UI it is replacing. + +@use '../tokens' as *; + +.app-crash { + position: fixed; + inset: 0; + display: flex; + align-items: center; + justify-content: center; + padding: $space-8; + background: $bg-app; + color: $text-body; + // Above everything, including a modal backdrop left behind by the tree that just crashed. + z-index: $z-modal + 10; +} + +.app-crash-card { + max-width: 560px; + width: 100%; + background: $bg-elev; + border: 1px solid $border-default; + border-radius: 6px; + padding: $space-8; +} + +.app-crash-title { + margin: 0 0 $space-2; + font-size: $fs-lg; + font-weight: 600; +} + +.app-crash-hint { + margin: 0 0 $space-3; + color: $text-muted; + line-height: 1.5; +} + +.app-crash-detail { + margin: 0 0 $space-8; + padding: $space-3; + max-height: 220px; + overflow: auto; + background: $bg-app; + border: 1px solid $border-default; + border-radius: 4px; + white-space: pre-wrap; + word-break: break-word; + font-family: $font-mono; + font-size: $fs-xs; + color: $text-muted; +} + +.app-crash-actions { + display: flex; + gap: $space-2; +} diff --git a/docs/arch/03-gui/01-ui-interaction.md b/docs/arch/03-gui/01-ui-interaction.md index b5cb6926..ffb36453 100644 --- a/docs/arch/03-gui/01-ui-interaction.md +++ b/docs/arch/03-gui/01-ui-interaction.md @@ -90,6 +90,10 @@ Platform differences are decided via `AppInfo.platform` (delivered by the main p - **Second-level modal backdrop click only closes its own layer**: for nested modals (connection/LLM/proxy editing, confirm dialogs) the backdrop click calls `stopPropagation`, so it does not bubble up to close the outer settings modal (including createPortal confirm dialogs — React synthetic events still bubble along the component tree). - **Action-level toast vs. full-screen error**: a failed remote action (review decision/merge/publish) raises a toast, distinct from the full-screen error of a fatal bootstrap failure. +- **Never a blank window (three layers of failure containment)**: a renderer failure must always land on a readable screen, never on the window's bare background — which is indistinguishable from a hung app and has no way back short of a restart. Each layer covers what the one below it structurally cannot see: + 1. **Render-phase errors** → the root `ErrorBoundary` in `main.tsx` (fallback `AppCrashScreen`, offering retry / reload). Without one, any error thrown while rendering unmounts the whole tree and empties `#root`. The boundary also relays the stack through `log:write`, since the renderer console is never written to file and a caught render error is not an uncaught window error either — so a crash would otherwise leave nothing on disk to diagnose. + 2. **Failures before React mounts** → `boot-guard.ts`, imported **first** in `main.tsx` so it is armed before any other module can throw. A module that fails while initializing (i18n / theme / the app bundle) takes down the entry before `render()` runs, leaving no React and therefore no boundary. The guard paints a plain-DOM recovery screen — no React, no i18n, no stylesheet, since each of those is a candidate cause — on an uncaught error or when `#root` is still empty after a timeout. + 3. **Renderer process death** → `WindowManager.installCrashRecovery` in main (`render-process-gone` / `did-fail-load`), which logs and reloads the window with a bounded retry budget. When the process itself dies (OOM, GPU fault, a native crash), no in-page JavaScript survives to report it; only main is left watching. `unresponsive` is logged but deliberately not recovered — it usually resolves on its own, and reloading would discard the user's in-flight state. - **Auto-refresh on window focus**: when the window regains focus, proactively fetch PR meta once (to follow the "switch to the platform, make edits, then switch back" scenario). - **Layout preferences persisted**: sidebar/chat width and collapse state, diff view mode, etc. are stored in localStorage. From 1facdc88f9ce958e2e4b682864bad015e391ede8 Mon Sep 17 00:00:00 2001 From: Hamhire Hu Date: Tue, 8 Sep 2026 11:15:46 +0800 Subject: [PATCH 06/17] feat(proxy): let hosts bypass the proxy via a configurable no_proxy list The proxy is a single global egress, but part of what the app reaches commonly lives inside the network the proxy leads out of -- a self-hosted code platform, its git remote, an internal model server. Routing those through the proxy wastes a hop at best and makes them unreachable at worst, and turning the proxy off is not a fix, since the LLM egress still needs it. Only loopback was bypassed, and only as built-in behaviour, so there was no way to express any of this. Settings now take a bypass list, applying to every outbound path at once: the platform REST fetch, git over HTTPS, pr-agent and the local CLIs. The syntax mirrors the conventional NO_PROXY on purpose. The subprocess egresses are handed these rules as the environment variable and interpret them with their own libraries -- the app does not get to decide how git matches a host -- so any richer syntax would apply in-process and not in the subprocess, making the same config bypass on one egress while proxying on the other. Hence the portable subset (domain plus subdomains, IP literal, `*`, case-insensitive, port ignored) and the deliberate exclusion of CIDR ranges, which only some implementations honour. Matching lives in shared/no-proxy.ts, used by both paths so they cannot drift apart, and loopback is prepended to whatever the user configured: a local service must never be proxied, and that guarantee should not depend on the user having typed it. Normalization happens in the setProxy controller rather than the form, so the value stored is canonical however the config arrived, and what the user reads back is what is actually matched. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 1 + CHANGELOG.zh-CN.md | 1 + apps/desktop/src/main/controllers/config.ts | 12 ++- apps/desktop/src/main/utils/proxy.ts | 44 +++++--- .../settings/editors/ProxyEditorModal.tsx | 18 ++++ .../settings/sections/ProxySection.tsx | 10 +- .../src/renderer/src/i18n/locales/de-DE.json | 6 ++ .../src/renderer/src/i18n/locales/en-US.json | 6 ++ .../src/renderer/src/i18n/locales/ja-JP.json | 5 + .../src/renderer/src/i18n/locales/zh-CN.json | 5 + docs/arch/99-core/03-networking-proxy.md | 19 ++-- docs/guide/03-proxy.md | 20 +++- docs/guide/04-config-reference.md | 4 +- docs/guide/zh-CN/03-proxy.md | 20 +++- docs/guide/zh-CN/04-config-reference.md | 4 +- packages/shared/src/config.ts | 7 +- packages/shared/src/index.ts | 1 + packages/shared/src/no-proxy.ts | 67 ++++++++++++ packages/shared/tests/no-proxy.test.ts | 101 ++++++++++++++++++ 19 files changed, 321 insertions(+), 30 deletions(-) create mode 100644 packages/shared/src/no-proxy.ts create mode 100644 packages/shared/tests/no-proxy.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 1fcfc80a..b0c47941 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and the versioning follows [Semantic Versioning](https://semver.org/). ### ✨ Added +- Proxy settings now take a list of **direct connections**: hosts that bypass the proxy and connect straight out, so an internal code platform, its git remote, or a self-hosted model stays reachable while everything else still goes through the proxy. Uses the familiar `NO_PROXY` syntax (a domain covers its subdomains), and applies to every outbound path at once — REST, git and the LLM call. - A review that fails because the model is unavailable now says so and tells you what to do — with a local CLI provider (claude / codex) the model comes from that CLI's own configuration, so it has to be changed there. ### 🔧 Fixed diff --git a/CHANGELOG.zh-CN.md b/CHANGELOG.zh-CN.md index cba14dd4..22781768 100644 --- a/CHANGELOG.zh-CN.md +++ b/CHANGELOG.zh-CN.md @@ -9,6 +9,7 @@ ### ✨ 新增 +- 代理设置新增**直连地址**列表:列出的地址跳过代理直接连接,内网代码平台、它的 git 远端或自建模型服务因此保持可达,其余流量照常走代理。沿用通行的 `NO_PROXY` 写法(填域名同时覆盖子域),并对所有出站路径一并生效——REST、git 与 LLM 调用。 - 因模型不可用而失败的评审现在会明确说明,并给出处理方式——使用本地 CLI 供应商(claude / codex)时,模型来自该 CLI 自身的配置,需要在那里更换。 ### 🔧 修复 diff --git a/apps/desktop/src/main/controllers/config.ts b/apps/desktop/src/main/controllers/config.ts index 54f9da13..9e9fc30c 100644 --- a/apps/desktop/src/main/controllers/config.ts +++ b/apps/desktop/src/main/controllers/config.ts @@ -1,6 +1,6 @@ import { randomBytes } from 'node:crypto'; import { nativeTheme } from 'electron'; -import { editorThemeNativeSource } from '@meebox/shared'; +import { editorThemeNativeSource, normalizeNoProxy } from '@meebox/shared'; import { writeConfig } from '@meebox/config'; import { buildDraftAdapter } from '../adapters.js'; import { setMainLanguage } from '../i18n/index.js'; @@ -156,12 +156,16 @@ export const setConnections: IpcController<'config:setConnections'> = async (_ev */ export const setProxy: IpcController<'config:setProxy'> = async (_event, req) => { const { bootstrap, logger, reconfigureConnections } = getContext(); - const next = { ...bootstrap.config, proxy: req.proxy }; + // Normalize the bypass list on the way in, not in the form: whichever way the config arrives — this IPC, or a + // hand-edited config.yaml — what lands on disk is one canonical comma-separated line, so the value the user reads + // back is the value actually matched against. + const proxy = { ...req.proxy, no_proxy: normalizeNoProxy(req.proxy.no_proxy) }; + const next = { ...bootstrap.config, proxy }; await writeConfig(bootstrap.paths.configFile, next); - bootstrap.config.proxy = req.proxy; + bootstrap.config.proxy = proxy; await reconfigureConnections(); logger.info( - { enabled: req.proxy.enabled, host: req.proxy.host, port: req.proxy.port }, + { enabled: proxy.enabled, host: proxy.host, port: proxy.port, noProxy: proxy.no_proxy }, 'proxy config updated (hot-reloaded)', ); }; diff --git a/apps/desktop/src/main/utils/proxy.ts b/apps/desktop/src/main/utils/proxy.ts index d28e8c69..194298dd 100644 --- a/apps/desktop/src/main/utils/proxy.ts +++ b/apps/desktop/src/main/utils/proxy.ts @@ -4,15 +4,33 @@ // - shouldBypass: whether loopback/local goes direct (② decides at the call site whether to attach a dispatcher) // Phase one is HTTP proxy only; when enabled=false all forms yield "empty/direct connection", so call sites need not each check the switch. import { ProxyAgent, type Dispatcher } from 'undici'; -import { ERROR_CODES, errorCodeMessage, type ProxyConfig } from '@meebox/shared'; +import { + ERROR_CODES, + LOOPBACK_NO_PROXY, + errorCodeMessage, + matchesNoProxy, + normalizeNoProxy, + type ProxyConfig, +} from '@meebox/shared'; -// loopback / local: always direct connection, never through the proxy. The env path relies on NO_PROXY, the dispatcher path on shouldBypass. -const NO_PROXY = 'localhost,127.0.0.1,::1'; +/** + * Effective bypass rules = the built-in loopback set + whatever the user configured. Loopback is prepended rather than + * left to the user: a local model or local service must never be sent through a proxy, and that guarantee should not + * depend on the user having typed it. + */ +function effectiveNoProxy(proxy: ProxyConfig): string { + return normalizeNoProxy(`${LOOPBACK_NO_PROXY},${proxy.no_proxy ?? ''}`); +} -/** loopback / local host → true (should go direct connection, not through the proxy). */ -export function shouldBypass(host: string): boolean { - const h = host.toLowerCase().replace(/^\[|\]$/g, ''); // strip IPv6 literal brackets - return h === 'localhost' || h.endsWith('.localhost') || h === '127.0.0.1' || h === '::1'; +/** + * Host should egress directly (loopback, or covered by the user's `no_proxy`) rather than through the proxy. + * + * The env path hands the same rules to subprocesses via `NO_PROXY` and lets their libraries apply them; this is the + * in-process equivalent for the dispatcher path, which is why both derive from `effectiveNoProxy` — the two egress + * classes must not disagree about the same config. + */ +export function shouldBypass(proxy: ProxyConfig, host: string): boolean { + return matchesNoProxy(host, effectiveNoProxy(proxy)); } /** Build a standard proxy URL: `://[user:pass@]host:port`. undefined when disabled / no host. */ @@ -32,6 +50,7 @@ export function proxyUrl(proxy: ProxyConfig): string | undefined { export function buildProxyEnv(proxy: ProxyConfig): Record { const url = proxyUrl(proxy); if (!url) return {}; + const bypass = effectiveNoProxy(proxy); return { HTTP_PROXY: url, http_proxy: url, @@ -39,8 +58,8 @@ export function buildProxyEnv(proxy: ProxyConfig): Record { https_proxy: url, ALL_PROXY: url, all_proxy: url, - NO_PROXY, - no_proxy: NO_PROXY, + NO_PROXY: bypass, + no_proxy: bypass, }; } @@ -85,14 +104,15 @@ export async function testProxyConnectivity( /** * Build a "proxy-aware" fetch for a target host, to inject into BitbucketClient's opts.fetch. - * host hits loopback/local → returns undefined (the call site uses the default global fetch for a direct connection). - * Otherwise returns a fetch wrapper carrying the dispatcher. Also returns undefined when the proxy is disabled. + * host hits loopback/local or the configured bypass list → returns undefined (the call site uses the default global + * fetch for a direct connection). Otherwise returns a fetch wrapper carrying the dispatcher. Also returns undefined + * when the proxy is disabled. */ export function proxyFetchForHost( proxy: ProxyConfig, host: string, ): ((input: string, init?: RequestInit) => Promise) | undefined { - if (shouldBypass(host)) return undefined; + if (shouldBypass(proxy, host)) return undefined; const dispatcher = buildProxyDispatcher(proxy); if (!dispatcher) return undefined; return (input, init) => diff --git a/apps/desktop/src/renderer/src/components/features/settings/editors/ProxyEditorModal.tsx b/apps/desktop/src/renderer/src/components/features/settings/editors/ProxyEditorModal.tsx index d32a5b04..1cda4ea2 100644 --- a/apps/desktop/src/renderer/src/components/features/settings/editors/ProxyEditorModal.tsx +++ b/apps/desktop/src/renderer/src/components/features/settings/editors/ProxyEditorModal.tsx @@ -109,7 +109,25 @@ export function ProxyEditorModal({ +
{t('settings.proxyNoProxy')}
+
+ {/* Multi-line on purpose: a bypass list is usually several hosts, and one per line stays readable where a + single comma-separated line does not. Stored normalized to one line (see normalizeNoProxy), and the + parser accepts commas / whitespace / newlines alike, so a value pasted from NO_PROXY also works. */} +