From 3ead85aefb8b63c8ea0019bb7c6d6527049e68a2 Mon Sep 17 00:00:00 2001 From: Hamhire Hu Date: Tue, 8 Sep 2026 15:34:59 +0800 Subject: [PATCH 1/2] chore(pragent): upgrade the embedded runtime to pr-agent 0.45.0 Assessed by diffing the 0.45.0 wheel against the vendored 0.39.0 rather than reading release notes: 82 of 123 files changed, including all eight we patch. Two patches are retired because upstream fixed the bugs. extract_hunk_headers now defaults an omitted hunk size to 1 instead of 0, so a single-line change no longer renders its old value as a still-present context line; get_diff_files now skips a file that fails to decode instead of crashing the review on a binary. Both were confirmed against the installed runtime, not assumed. The four remaining patches were probed under 0.45.0 and all apply -- none silently skipped by the version guard -- and get_line_link's call sites are unchanged, so the structured /review anchor still works. Three upstream features now default to on and append to the very output this app parses, which would have surfaced as bogus findings rather than as an obvious break: persistent_finding_state (upstream cross-run state the app already owns through drafts, finding closures and re-review verdicts) and two coverage footers. They are pinned off in buildPragentEnv, verified to be true by default and false under the override. Telemetry is new in 0.45.0 but ships disabled with no built-in endpoint, so it needs no handling. Also bumps the development version to 0.12.0-dev. Not exercised end to end: 0.45.0 revises six prompt files, and whether the rendered markdown still matches the parser needs a real /describe /review /ask against a live PR. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 1 + CHANGELOG.zh-CN.md | 1 + apps/desktop/package.json | 2 +- apps/desktop/scripts/pragent-runtime.json | 2 +- .../meebox_pragent_shim/__init__.py | 13 +-- .../patches/git_patch_processing.py | 88 ------------------- .../patches/local_git_provider.py | 67 ++------------ .../meebox_pragent_shim/runtime.py | 2 +- docs/arch/02-agent/05-pragent-runtime.md | 9 +- package-lock.json | 2 +- packages/pr-agent-bridge/src/env.ts | 15 ++++ 11 files changed, 38 insertions(+), 164 deletions(-) delete mode 100644 apps/desktop/scripts/pragent-shim/meebox_pragent_shim/patches/git_patch_processing.py diff --git a/CHANGELOG.md b/CHANGELOG.md index afa860cd..4f18dd6b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and the versioning follows [Semantic Versioning](https://semver.org/). ### ✨ Added +- The bundled review engine is now pr-agent 0.45.0 (from 0.39.0). Two long-standing local fixes are no longer needed — a single-line file change is rendered correctly upstream now, and a binary file no longer has to be worked around — and its YAML handling is more tolerant of imperfect model output. - Mentions now render as a pill instead of blending into the surrounding text, so it is obvious at a glance when someone is named — on the activity page, in the inline diff comments, in drafts, and in the PR description alike. - 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. diff --git a/CHANGELOG.zh-CN.md b/CHANGELOG.zh-CN.md index a0091c78..fa17c636 100644 --- a/CHANGELOG.zh-CN.md +++ b/CHANGELOG.zh-CN.md @@ -9,6 +9,7 @@ ### ✨ 新增 +- 内置评审引擎升级至 pr-agent 0.45.0（原 0.39.0）。两处长期存在的本地修补不再需要——单行文件变更在上游已渲染正确，二进制文件也无需再绕开——其 YAML 解析对不规范的模型输出也更宽容。 - @提及 改为胶囊标签展示，不再淹没在正文里，一眼即可看出点到了谁——活动页、内联 diff 评论、草稿与 PR 描述一致生效。 - 代理设置新增**直连地址**列表：列出的地址跳过代理直接连接，内网代码平台、它的 git 远端或自建模型服务因此保持可达，其余流量照常走代理。沿用通行的 `NO_PROXY` 写法（填域名同时覆盖子域），并对所有出站路径一并生效——REST、git 与 LLM 调用。 - 因模型不可用而失败的评审现在会明确说明，并给出处理方式——使用本地 CLI 供应商（claude / codex）时，模型来自该 CLI 自身的配置，需要在那里更换。 diff --git a/apps/desktop/package.json b/apps/desktop/package.json index 6f6f0220..b5ea2309 100644 --- a/apps/desktop/package.json +++ b/apps/desktop/package.json @@ -1,6 +1,6 @@ { "name": "@meebox/desktop", - "version": "0.11.3-dev", + "version": "0.12.0-dev", "private": true, "description": "meebox Electron desktop app", "author": { diff --git a/apps/desktop/scripts/pragent-runtime.json b/apps/desktop/scripts/pragent-runtime.json index 0216b6ef..99f495d7 100644 --- a/apps/desktop/scripts/pragent-runtime.json +++ b/apps/desktop/scripts/pragent-runtime.json @@ -8,6 +8,6 @@ }, "prAgent": { "_comment": "嵌入式运行时安装的 pr-agent 版本（pinned）；须与 sitecustomize.py 的 _EXPECTED_PRAGENT_VERSION 对齐，升级时两处同步", - "version": "0.39.0" + "version": "0.45.0" } } diff --git a/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/__init__.py b/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/__init__.py index 9a57b442..061d8203 100644 --- a/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/__init__.py +++ b/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/__init__.py @@ -10,7 +10,6 @@ applied it silently degrades, never letting a shim exception block the flow. """ from .patches.describe_assessment import patch as _patch_describe_assessment -from .patches.git_patch_processing import patch as _patch_git_patch_processing from .patches.litellm_handler import patch as _patch_litellm_handler from .patches.load_yaml import patch as _patch_load_yaml from .patches.local_git_provider import patch as _patch_local_git_provider @@ -18,9 +17,8 @@ def apply() -> None: - # local_git_provider two patches merged into one patch_fn (registering multiple finders for the - # same module shadows each other; only the meta_path[0] one takes effect): binary-safe - # get_diff_files + get_line_link anchor. + # local_git_provider: inject get_line_link (the structured /review anchor) + get_repo_file_content. Registered as a + # single patch_fn because multiple finders on one module shadow each other (only meta_path[0] takes effect). _register_post_import( "pr_agent.git_providers.local_git_provider", _patch_local_git_provider, @@ -40,11 +38,4 @@ def apply() -> None: "pr_agent.tools.pr_description", _patch_describe_assessment, ) - # git patch processing: fix the phantom "unchanged" line on single-line hunks (omitted hunk size defaulted to 0 - # instead of 1), which made single-line file changes look like they contained both the old and new value. Self- - # disabling once upstream fixes it — see patches/git_patch_processing.py. - _register_post_import( - "pr_agent.algo.git_patch_processing", - _patch_git_patch_processing, - ) _debug("meebox shim loaded") diff --git a/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/patches/git_patch_processing.py b/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/patches/git_patch_processing.py deleted file mode 100644 index cfe29630..00000000 --- a/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/patches/git_patch_processing.py +++ /dev/null @@ -1,88 +0,0 @@ -"""git_patch_processing patch (version-guarded, self-disabling): fix phantom "unchanged" line on single-line hunks. - -=============================================================================================================== -MAINTENANCE NOTE — remove this shim once pr-agent fixes the bug upstream. ---------------------------------------------------------------------------------------------------------------- -Bug: pr-agent renders a single-line file change (e.g. VERSION `1.5.2` -> `1.5.3`, a hash/checksum file, any -hunk whose OLD side is exactly one line) so the model sees the OLD value as a still-present, unchanged line — -it then wrongly reports "the file now contains both the old and new value". - -Root cause (in pr_agent/algo/git_patch_processing.py): - 1. git omits the hunk line-count when it is 1, so a single-line change emits `@@ -1 +1 @@` - (which per the unified-diff spec means `@@ -1,1 +1,1 @@`). - 2. `extract_hunk_headers` coerces every missing regex group to 0, so the omitted size becomes size1=0 - (it should be 1). - 3. `process_patch_lines`' trailing-context slice `file_original_lines[start1 + size1 - 1 : ...]` then reads - from `1 + 0 - 1 = 0` — i.e. re-reads the CHANGED line itself — and appends it as a phantom unchanged - context line, which `decouple_and_convert_to_hunks_with_lines_numbers` places into `__new hunk__`. - -Fix: default an OMITTED hunk size to 1 (spec-correct) instead of 0, keeping an EXPLICIT 0 (e.g. `@@ -0,0 +1 @@` -new-file case) as 0. This makes the trailing-context slice start after the hunk, so no phantom line is emitted. - -Upstream status (re-checked at pr-agent 0.39.0): still present — `extract_hunk_headers` is byte-identical to -0.36.0 and still coerces the omitted size to 0; no upstream issue/PR addresses it (PRs #2322 / #2330 guard a -DIFFERENT None — a fully malformed `@@` line where `match` itself is None — not the size default). - -Removal / upgrade checklist: - * This patch is BOTH version-guarded (only applies to the pinned _EXPECTED_PRAGENT_VERSION) AND self-disabling - (it probes the live `extract_hunk_headers`; if upstream already returns size1=1 for `@@ -1 +1 @@`, it does - nothing). So on a pr-agent bump, re-verify: if the probe reports the bug is gone, DELETE this file and its - registration in `../__init__` apply(); if it persists, bump _EXPECTED_PRAGENT_VERSION and keep it. - * Reproduce quickly against the vendored runtime with `MEEBOX_SHIM_DEBUG=1` (the probe logs which branch it took). -=============================================================================================================== -""" -from ..runtime import _EXPECTED_PRAGENT_VERSION, _debug, _pragent_version, _warn - - -def patch(module) -> None: - # Version guard: this reimplementation mirrors the pinned pr-agent's extract_hunk_headers structure; on a - # version mismatch skip rather than risk a wrong reimplementation (safe degradation, consistent with the - # other shim patches). The warning is the "pay attention on upgrade" signal. - installed = _pragent_version() - if installed != _EXPECTED_PRAGENT_VERSION: - _warn( - f"pr-agent {installed} does not match the {_EXPECTED_PRAGENT_VERSION} that the meebox patch is " - "adapted for; git_patch_processing patch skipped (single-line-hunk phantom-line fix disabled). If " - "this is an intentional upgrade, sync runtime.py's _EXPECTED_PRAGENT_VERSION + pragent-runtime.json, " - "re-verify whether upstream has fixed the omitted-hunk-size bug, and remove this shim if so." - ) - return - - orig_extract = module.extract_hunk_headers - re_hunk_header = module.RE_HUNK_HEADER - - # Self-disable if upstream already fixed it: git omits the size for `@@ -1 +1 @@`, so a correct implementation - # yields size1 == 1. If the live function already returns a non-zero size here, the bug is gone — do nothing. - probe = re_hunk_header.match("@@ -1 +1 @@") - try: - _, probe_size1, _, _, _ = orig_extract(probe) - except Exception as exc: # noqa: BLE001 - unexpected upstream signature change → skip, don't crash the run - _debug(f"git_patch_processing: extract_hunk_headers probe failed ({exc}); patch skipped") - return - if probe_size1 != 0: - _debug( - "git_patch_processing: upstream already defaults an omitted hunk size correctly " - "(probe size1=%r); phantom-line shim skipped — safe to delete this file" % probe_size1 - ) - return - - def extract_hunk_headers(match): - # Faithful mirror of upstream extract_hunk_headers, with ONE correction: an omitted hunk line-count - # (regex group is None) defaults to 1 per the unified-diff spec — git drops ",N" only when N == 1 — - # instead of upstream's blanket 0. An EXPLICIT 0 (present in the text, e.g. the `@@ -0,0 +1 @@` new-file - # case) is untouched because its group is "0", not None. Only the two size groups (indices 1 and 3) get - # the size default; any other missing group keeps the original 0 fallback. - res = list(match.groups()) - for i in range(len(res)): - if res[i] is None: - res[i] = 1 if i in (1, 3) else 0 - try: - start1, size1, start2, size2 = map(int, res[:4]) - except (ValueError, TypeError): # '@@ -0,0 +1 @@' case (mirrors upstream's bare except) - start1, size1, size2 = map(int, res[:3]) - start2 = 0 - section_header = res[4] - return section_header, size1, size2, start1, start2 - - module.extract_hunk_headers = extract_hunk_headers - _debug("git_patch_processing: applied single-line-hunk phantom-line fix (omitted hunk size -> 1)") diff --git a/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/patches/local_git_provider.py b/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/patches/local_git_provider.py index 62d153ac..a3cd8b5d 100644 --- a/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/patches/local_git_provider.py +++ b/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/patches/local_git_provider.py @@ -1,72 +1,23 @@ -"""LocalGitProvider patch (version-guarded): binary-safe get_diff_files + get_line_link anchor + repo-context file fetch.""" +"""LocalGitProvider patch (version-guarded): get_line_link anchor + dirty-tolerant repo prep + repo-context file fetch.""" from ..runtime import _EXPECTED_PRAGENT_VERSION, _pragent_version, _warn def patch(module) -> None: - """LocalGitProvider.get_diff_files blindly .decode('utf-8') on every diff file, and on a binary - file (images / build artifacts / UTF-16 etc., e.g. starting with 0xff) throws UnicodeDecodeError, crashing the whole review. - Replace with a binary-safe version: files that fail to decode are skipped (review doesn't handle binaries), the rest of the logic identical to upstream.""" - # version guard: only patch the pinned pr-agent version; on mismatch skip the whole group (including get_line_link). + """Inject the methods LocalGitProvider does not implement: the structured /review anchor (get_line_link), + a dirty-tolerant _prepare_repo, labels, and repo-context file reads. + + Binary-safe get_diff_files used to live here too; pr-agent 0.45.0 fixed it upstream (it now skips a file that + fails to decode, with a warning), so that override is gone — one less thing to keep in step with upstream. + """ + # version guard: only patch the pinned pr-agent version; on mismatch skip the whole group. installed = _pragent_version() if installed != _EXPECTED_PRAGENT_VERSION: _warn( f"pr-agent {installed} does not match the {_EXPECTED_PRAGENT_VERSION} that the meebox patch is adapted for; " - "patches skipped (/review line-number anchoring and binary-safe diff disabled). If this is an intentional upgrade, sync " + "patches skipped (/review line-number anchoring disabled). If this is an intentional upgrade, sync " "runtime.py's _EXPECTED_PRAGENT_VERSION + pragent-runtime.json and re-verify." ) return - from pr_agent.algo.types import EDIT_TYPE, FilePatchInfo - - def get_diff_files(self): - diffs = self.repo.head.commit.diff( - self.repo.merge_base(self.repo.head, self.repo.branches[self.target_branch_name]), - create_patch=True, - R=True, - ) - diff_files = [] - for diff_item in diffs: - try: - original_file_content_str = ( - diff_item.a_blob.data_stream.read().decode("utf-8") - if diff_item.a_blob is not None - else "" - ) - new_file_content_str = ( - diff_item.b_blob.data_stream.read().decode("utf-8") - if diff_item.b_blob is not None - else "" - ) - patch_str = diff_item.diff.decode("utf-8") - except (UnicodeDecodeError, ValueError): - # binary file can't be utf-8 decoded → skip this file - continue - edit_type = EDIT_TYPE.MODIFIED - if diff_item.new_file: - edit_type = EDIT_TYPE.ADDED - elif diff_item.deleted_file: - edit_type = EDIT_TYPE.DELETED - elif diff_item.renamed_file: - edit_type = EDIT_TYPE.RENAMED - diff_files.append( - FilePatchInfo( - original_file_content_str, - new_file_content_str, - patch_str, - # a deleted file's b_path is None → FilePatchInfo.filename=None, and downstream - # set_file_languages / extract_relevant_lines_str's filename.rsplit/strip - # would crash, and one crash interrupts the whole review's line-snippet extraction (even findings for non-deleted files lose - # code snippets). Fall back to a_path to guarantee filename is never None. - diff_item.b_path or diff_item.a_path, - edit_type=edit_type, - old_filename=None - if diff_item.a_path == diff_item.b_path - else diff_item.a_path, - ) - ) - self.diff_files = diff_files - return diff_files - - module.LocalGitProvider.get_diff_files = get_diff_files # _prepare_repo: upstream throws "repository is not in a clean state" when repo.is_dirty(). For CLI-mode # /ask worktrees we sanitize as needed — truncating the repo's own agent instruction files (CLAUDE.md / AGENTS.md / .cursor rules diff --git a/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/runtime.py b/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/runtime.py index 5e5ea899..998e4ccb 100644 --- a/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/runtime.py +++ b/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/runtime.py @@ -15,7 +15,7 @@ # skipped (safe degradation, preferring to apply fewer patches over applying wrong ones). # When upgrading pr-agent: sync this constant + prAgent.version in scripts/pragent-runtime.json # (the assemble script verifies the two match and extracts this constant from this file), and re-verify patch behavior. -_EXPECTED_PRAGENT_VERSION = "0.39.0" +_EXPECTED_PRAGENT_VERSION = "0.45.0" # System context "cache break" marker: assembleSystemContext (TS, packages/agent/src/assemble.ts) inserts this string # (along with the --- separators on both sides) between the **globally stable prefix** (SOUL/AGENTS/tool directory/memory/user diff --git a/docs/arch/02-agent/05-pragent-runtime.md b/docs/arch/02-agent/05-pragent-runtime.md index f253597a..27260a6a 100644 --- a/docs/arch/02-agent/05-pragent-runtime.md +++ b/docs/arch/02-agent/05-pragent-runtime.md @@ -56,16 +56,19 @@ Design principles: - **Version guard**: patches depend on a specific pr-agent version's internal implementation → if `_EXPECTED_PRAGENT_VERSION` ≠ the actually installed version at runtime, **skip all patches and emit a stderr WARNING** (safe degradation); at build time it hard-checks the shim constant == the manifest version, failing outright on mismatch. -Current patches: -- **Binary-safe diff**: the original `get_diff_files` blindly utf-8-decodes every file and crashes on binary → changed to skip on decode failure. +Current patches (pinned pr-agent **0.45.0**): - **anchor line numbers** (details in [Review workflow](../01-platform/03-review-workflow.md)): patch `get_line_link` to return `meebox:///#L-L`, letting `/review`'s key_issues render with a structured file:line. - **Anthropic drops temperature**: new Claude models deprecate temperature, so all `anthropic/*` are put into the "don't send temperature" set. - **load_yaml tolerance**: an anchor marker taking a whole line breaks YAML → on parse failure, strip the marker and retry, avoiding a whole review crash. -- **repo-context file fetch**: pr-agent 0.39.0 defaults `repo_context_files = ["AGENTS.md"]`, but `LocalGitProvider` inherits the base no-op `get_repo_file_content` → the feature is skipped with a per-run WARNING. Implement it by reading the blob from the base branch's tree (`git show :`, never the working tree), so `/review /describe /improve` inject the reviewed repo's `AGENTS.md`/etc. as ``; a missing file degrades to `""`. +- **repo-context file fetch**: pr-agent defaults `repo_context_files = ["AGENTS.md"]`, but `LocalGitProvider` inherits the base no-op `get_repo_file_content` → the feature is skipped with a per-run WARNING. Implement it by reading the blob from the base branch's tree (`git show :`, never the working tree), so `/review /describe /improve` inject the reviewed repo's `AGENTS.md`/etc. as ``; a missing file degrades to `""`. - **Local CLI provider**: when `MEEBOX_CLI_MODE` is set, replace `chat_completion` wholesale with the "call the local CLI" version (see below). - **token usage collection**: see below. +**Retired patches** (kept as a record, because "why is this no longer patched" is the question an upgrade raises): *binary-safe diff* and the *single-line-hunk phantom line* were both **fixed upstream in 0.45.0** — `get_diff_files` now skips a file that fails to decode, and `extract_hunk_headers` defaults an omitted hunk size to 1 rather than 0. Both were verified against the installed runtime before deletion, not assumed from release notes. + +**Upstream defaults that must stay pinned**: 0.45.0 turned on three features that append to the output this app *parses* — `pr_reviewer.persistent_finding_state` (a "resolved findings" section carrying upstream's own cross-run state, which the app already owns via drafts / finding closures / re-review verdicts), and two coverage footers. They are forced off in `buildPragentEnv` rather than left at their defaults. **Re-check this list on every upgrade**: a new default that adds a section is not a free improvement here — it lands as a bogus finding. + ### Real token usage Inline-wrap pr-agent's `_get_completion`, take `prompt/completion/total_tokens` from the returned `response.usage`, diff --git a/package-lock.json b/package-lock.json index 7cf37584..71027e61 100644 --- a/package-lock.json +++ b/package-lock.json @@ -32,7 +32,7 @@ }, "apps/desktop": { "name": "@meebox/desktop", - "version": "0.11.3-dev", + "version": "0.12.0-dev", "dependencies": { "@iconify-json/material-icon-theme": "^1.2.66", "@iconify/react": "^5.2.1", diff --git a/packages/pr-agent-bridge/src/env.ts b/packages/pr-agent-bridge/src/env.ts index ed637cbf..78caec30 100644 --- a/packages/pr-agent-bridge/src/env.ts +++ b/packages/pr-agent-bridge/src/env.ts @@ -78,6 +78,16 @@ function normalizeModel(provider: LlmProfile['provider'], model: string): string * - `CONFIG__FALLBACK_MODELS=[]`: pr-agent configures a fallback by default (usually pointing at the OpenAI family), * and after the main model fails it automatically tries OpenAI with a dummy key, polluting the log and easily misread as "OpenAI misconfigured". * We already specify the provider explicitly, so a fallback is unnecessary + * + * Plus a group that pins **upstream features whose defaults changed under us**. The app parses `/review` and + * `/describe` output as structured markdown (see poller/parse-output), so anything upstream appends to that output is + * not a free improvement — it lands as a bogus finding or a stray section. These are therefore turned off explicitly + * rather than left at whatever the pinned pr-agent version happens to default to (re-check on every upgrade): + * - `PR_REVIEWER__PERSISTENT_FINDING_STATE=false` (new in 0.45, defaults **true**): would append a "resolved findings" + * section carrying cross-run state. The app already owns that concept — drafts, finding closures, re-review verdicts + * — so upstream's version would both duplicate it and parse as extra findings. + * - `PR_REVIEWER__ENABLE_REVIEW_COVERAGE_FOOTER=false` / `PR_CODE_SUGGESTIONS__ENABLE_SUGGESTIONS_COVERAGE_FOOTER=false` + * (new in 0.45, both default **true**): a footer appended to the body, which the parser would read as content. */ export function buildPragentEnv(profile: LlmProfile, maxModelTokens?: number): Record { const env: Record = {}; @@ -88,6 +98,11 @@ export function buildPragentEnv(profile: LlmProfile, maxModelTokens?: number): R env['CONFIG__MAX_MODEL_TOKENS'] = String(contextTokens); env['CONFIG__CUSTOM_MODEL_MAX_TOKENS'] = String(contextTokens); env['CONFIG__FALLBACK_MODELS'] = '[]'; + // Pin upstream output-shaping features that default on (see the doc comment above): the app parses this output, so + // extra sections/footers are corruption rather than enrichment. + env['PR_REVIEWER__PERSISTENT_FINDING_STATE'] = 'false'; + env['PR_REVIEWER__ENABLE_REVIEW_COVERAGE_FOOTER'] = 'false'; + env['PR_CODE_SUGGESTIONS__ENABLE_SUGGESTIONS_COVERAGE_FOOTER'] = 'false'; // On import litellm fetches the remote model price table over the network (raw.githubusercontent.com); on an intranet/weak network // the SSL timeout slows startup and floods warnings. We only take the real token count (from API response.usage), // don't need the price table → force using only the in-package local backup, no network at all. See sitecustomize's usage callback. From 91702d0c24a01406e96a4853ca37bc72eca621db Mon Sep 17 00:00:00 2001 From: Hamhire Hu Date: Tue, 8 Sep 2026 15:54:16 +0800 Subject: [PATCH 2/2] fix(pragent): correct the describe file walkthrough and stop quoted headings splitting it Three defects surfaced by a real /describe run. None was introduced by the 0.45.0 upgrade -- the relevant upstream code is identical in 0.39.0 -- they had simply never been noticed. Line counts rendered as "+-1/--1". FilePatchInfo defaults num_plus_lines and num_minus_lines to -1 and only the real platform providers fill them in; LocalGitProvider does not. get_diff_files is wrapped rather than reimplemented to backfill both from the patch text, counted by the same rule the platform providers use, so upstream keeps owning how the diff is produced. Every walkthrough link pointed at "#L-1". Upstream passes relevant_line_start=-1 to mean "the whole file", and -1 is truthy, so a plain falsiness check let it through. The resulting fragment was then rejected by the anchor parser entirely (its line group accepts only digits), so the link yielded no anchor at all -- the fix therefore also recovers a path-level anchor for these rows. A merge tail in the PR description tore the result apart. `# Conflicts:` and the `#path` lines beneath it are markdown H1s, and /describe frames its own structure at H3, so the author's prose outranked the tool's structure and produced sections named after conflicted files. Section splitting now takes a minimum heading level, and describe splits only at H3 or deeper: quoted content stays inside the section that quotes it. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 3 ++ CHANGELOG.zh-CN.md | 3 ++ .../patches/local_git_provider.py | 41 ++++++++++++++-- docs/arch/02-agent/05-pragent-runtime.md | 3 +- packages/poller/src/parse-output.ts | 20 ++++++-- packages/poller/tests/parse-output.test.ts | 49 +++++++++++++++++-- 6 files changed, 107 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4f18dd6b..d623b4c0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,9 @@ and the versioning follows [Semantic Versioning](https://semver.org/). ### 🔧 Fixed +- The file list in a generated PR description now shows the real number of added and removed lines per file, instead of `+-1/--1`. +- Links in that file list now open the file instead of pointing at a non-existent line, so clicking through works. +- A PR description ending in a git merge tail (`# Conflicts:` and the file lines under it) no longer breaks up the generated description — those lines are part of the quoted description, not headings of their own. - A part of the interface that loads on demand — the diff editor, a comment's inline code context — no longer takes the whole app down with it when it fails to load; the failure now stays inside that pane. If it failed because the app was updated or rebuilt while the window was open, it says so and offers to reload, which is the only thing that actually helps in that case. - 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. diff --git a/CHANGELOG.zh-CN.md b/CHANGELOG.zh-CN.md index fa17c636..d3c62396 100644 --- a/CHANGELOG.zh-CN.md +++ b/CHANGELOG.zh-CN.md @@ -16,6 +16,9 @@ ### 🔧 修复 +- 生成的 PR 描述中，文件清单现在显示每个文件真实的增删行数，不再是 `+-1/--1`。 +- 该清单中的链接现在指向文件本身，而非一个不存在的行号，点击可正常跳转。 +- PR 描述末尾带有 git 合并残留（`# Conflicts:` 及其下的文件行）时，生成的描述不再被切碎——那些行属于被引用的描述正文，而不是标题。 - 按需加载的界面部分——diff 编辑器、评论中的内联代码上下文——加载失败时不再拖垮整个应用，失败被限制在该区域内。若失败原因是窗口开着时应用被更新或重新构建，会明确说明并提供重新加载，那也是这种情况下唯一有效的操作。 - 评审失败时现在会展示供应商返回的真实错误，而不再只有一句「所有备选模型均调用失败」——真正的原因（模型不可用、登录过期、额度耗尽）此前被吞掉，从未出现在运行卡片上。 - 本地 CLI 供应商正常退出却返回空回复时，现在会作为失败上报并指明该原因，而不再表现为一次无从解释的 LLM 调用失败。 diff --git a/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/patches/local_git_provider.py b/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/patches/local_git_provider.py index a3cd8b5d..9c88511f 100644 --- a/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/patches/local_git_provider.py +++ b/apps/desktop/scripts/pragent-shim/meebox_pragent_shim/patches/local_git_provider.py @@ -38,18 +38,51 @@ def _prepare_repo(self): # and parse-output derives the structured anchor from the link (same source as real providers, not dependent on the model self-reporting a marker). from urllib.parse import quote + def _line_no(value): + """A usable 1-based line number, or 0 for "no specific line".""" + try: + n = int(value) + except (TypeError, ValueError): + return 0 + return n if n > 0 else 0 + def get_line_link(self, relevant_file, relevant_line_start, relevant_line_end=None): f = quote((relevant_file or "").lstrip("/"), safe="/") if not f: return "" - if not relevant_line_start: + # `-1` is upstream's "whole file, no particular line" (pr_description passes it for every File Walkthrough + # row). It is truthy, so a plain falsiness check let it through and produced `#L-1` — a fragment the app then + # parsed into a bogus anchor at line -1. Anything that is not a positive line number yields a file-level link. + start = _line_no(relevant_line_start) + if not start: return f"meebox:///{f}" - if relevant_line_end and relevant_line_end != relevant_line_start: - return f"meebox:///{f}#L{relevant_line_start}-L{relevant_line_end}" - return f"meebox:///{f}#L{relevant_line_start}" + end = _line_no(relevant_line_end) + if end and end != start: + return f"meebox:///{f}#L{start}-L{end}" + return f"meebox:///{f}#L{start}" module.LocalGitProvider.get_line_link = get_line_link + # num_plus_lines / num_minus_lines: FilePatchInfo defaults both to -1, and only the real platform providers fill + # them in — LocalGitProvider does not, so /describe's File Walkthrough rendered every row as "+-1/--1". Wrap + # rather than reimplement get_diff_files: upstream owns how the diff is produced (0.45.0 also made it binary-safe), + # and this only backfills two derived counters, computed the same way the platform providers do. + _orig_get_diff_files = module.LocalGitProvider.get_diff_files + + def get_diff_files(self): + files = _orig_get_diff_files(self) + for f in files or []: + if getattr(f, "num_plus_lines", -1) >= 0: + continue # already counted (a future upstream that fills them in wins) + lines = (getattr(f, "patch", None) or "").splitlines() + # Count the same way the platform providers do: any line starting with +/-, which includes the + # `+++`/`---` file headers. Kept identical rather than "corrected" so the numbers agree across providers. + f.num_plus_lines = len([ln for ln in lines if ln.startswith("+")]) + f.num_minus_lines = len([ln for ln in lines if ln.startswith("-")]) + return files + + module.LocalGitProvider.get_diff_files = get_diff_files + # uniformly enable GFM: LocalGitProvider defaults to False for 'gfm_markdown', causing /describe's # enable_pr_diagram (on by default in configuration.toml) to be gated off by `enable and is_supported(gfm_markdown)`, # not producing the mermaid architecture diagram; /review etc. also take the non-GFM simplified branch. Here we make gfm_markdown diff --git a/docs/arch/02-agent/05-pragent-runtime.md b/docs/arch/02-agent/05-pragent-runtime.md index 27260a6a..e81a2ca7 100644 --- a/docs/arch/02-agent/05-pragent-runtime.md +++ b/docs/arch/02-agent/05-pragent-runtime.md @@ -58,7 +58,8 @@ Design principles: Current patches (pinned pr-agent **0.45.0**): - **anchor line numbers** (details in [Review workflow](../01-platform/03-review-workflow.md)): patch `get_line_link` to return `meebox:///#L-L`, - letting `/review`'s key_issues render with a structured file:line. + letting `/review`'s key_issues render with a structured file:line. **`-1` means "the whole file"** upstream — `/describe` passes it for every File Walkthrough row — and it is truthy, so it must be screened out explicitly or the link becomes `#L-1`, which the anchor parser then rejects outright (its line group only accepts digits), losing even the path. Anything that is not a positive line number yields a file-level link. +- **diff line counts**: `FilePatchInfo.num_plus_lines` / `num_minus_lines` default to `-1` and only the real platform providers fill them in, so `/describe`'s File Walkthrough rendered every row as `+-1/--1`. `get_diff_files` is **wrapped** (not reimplemented) to backfill the two counters from the patch text, counted by the same rule the platform providers use — upstream keeps owning how the diff is produced. - **Anthropic drops temperature**: new Claude models deprecate temperature, so all `anthropic/*` are put into the "don't send temperature" set. - **load_yaml tolerance**: an anchor marker taking a whole line breaks YAML → on parse failure, strip the marker and retry, avoiding a whole review crash. - **repo-context file fetch**: pr-agent defaults `repo_context_files = ["AGENTS.md"]`, but `LocalGitProvider` inherits the base no-op `get_repo_file_content` → the feature is skipped with a per-run WARNING. Implement it by reading the blob from the base branch's tree (`git show :`, never the working tree), so `/review /describe /improve` inject the reviewed repo's `AGENTS.md`/etc. as ``; a missing file degrades to `""`. diff --git a/packages/poller/src/parse-output.ts b/packages/poller/src/parse-output.ts index 265119ea..e54cc75b 100644 --- a/packages/poller/src/parse-output.ts +++ b/packages/poller/src/parse-output.ts @@ -107,19 +107,25 @@ interface Section { } /** - * Slice pr-agent 0.36.0's markdown output into sections by H1-H6. + * Slice pr-agent's markdown output into sections by heading. * Each section has level / title / body (body has leading/trailing whitespace stripped). * Leading content at the top with no header is also synthesized into a level=0 / title='' section, so /describe * can be pulled out as a whole segment. + * + * `minLevel` is the shallowest heading allowed to start a section; anything shallower stays as body text. The tool's + * own structure sits at a known depth, while the text it quotes is arbitrary user prose — so without this, a heading + * the user happened to write outranks the structure and tears it apart. The case that forced it: a PR description + * ending in a git merge tail, where `# Conflicts:` and the `#\tpath` lines under it are markdown H1s, split a + * `/describe` result into sections named after conflicted files. */ -export function splitMarkdownSections(md: string): Section[] { +export function splitMarkdownSections(md: string, minLevel = 1): Section[] { const lines = md.replace(/\r\n/g, '\n').split('\n'); const sections: Section[] = []; let cur: Section | null = { level: 0, title: '', body: '' }; const HEADER_RE = /^(#{1,6})\s+(.+?)\s*$/; for (const line of lines) { const m = HEADER_RE.exec(line); - if (m) { + if (m && m[1]!.length >= minLevel) { // First finalize the prev section (drop empty segments) if (cur && (cur.title || cur.body.trim())) { sections.push({ ...cur, body: cur.body.trim() }); @@ -889,7 +895,13 @@ export function parseReviewOutput(stdout: string, tool: ReviewRunTool): ParsedRe // The GFM path is only for /review (under gfm_markdown the whole thing is a ); describe/ask still go through markdown // slicing (their HTML/table/mermaid is rendered downstream by react-markdown, the section structure is unaffected). const gfm = tool === 'review' && isGfmReviewOutput(baseMd); - const allSections = gfm ? splitGfmTableSections(baseMd) : splitMarkdownSections(baseMd); + // /describe frames its whole structure at `###` (User description / PR Type / Description / Diagram Walkthrough / + // Assessment), and one of those sections quotes the PR description verbatim. Splitting at H1/H2 there would let the + // author's own prose define sections — a merge-conflict tail (`# Conflicts:`) being the case that surfaced it. Other + // tools keep the default, since their bodies are model output shaped by our prompts, not quoted user text. + const allSections = gfm + ? splitGfmTableSections(baseMd) + : splitMarkdownSections(baseMd, tool === 'describe' ? 3 : 1); const sections = allSections.filter((s) => !shouldSkipSection(s, tool)); if (sections.length === 0) { const fs = walkthroughFinding ? [walkthroughFinding] : []; diff --git a/packages/poller/tests/parse-output.test.ts b/packages/poller/tests/parse-output.test.ts index 0de269ab..fcf52e8c 100644 --- a/packages/poller/tests/parse-output.test.ts +++ b/packages/poller/tests/parse-output.test.ts @@ -666,9 +666,11 @@ describe('parseStructuredAsk', () => { }); it('suggestions section with no marker → the whole section as one ask-suggestions (same as old behavior)', () => { - const md = ['', 'General advice with no specific code location.', ''].join( - '\n', - ); + const md = [ + '', + 'General advice with no specific code location.', + '', + ].join('\n'); const { findings } = parseReviewOutput(md, 'ask'); expect(findings[0]!.sectionKey).toBe('ask-suggestions'); expect(findings[0]!.body).toBe('General advice with no specific code location.'); @@ -729,3 +731,44 @@ describe('classifyLlmFailure', () => { expect(classifyLlmFailure("CLI 'codex' returned an empty reply")).toBeUndefined(); }); }); + +describe('splitMarkdownSections minLevel', () => { + // Regression: a PR description ending in a git merge tail. `# Conflicts:` and the `#\tpath` lines under it are + // markdown H1s, so before minLevel they outranked /describe's own `###` structure and split the result into + // sections named after conflicted files — with the real description scattered across them. + const describeOut = [ + '### **User description**', + 'fix: isolate internal and external roles', + 'Merge remote-tracking branch', + '', + '# Conflicts:', + '#\tpackage-lock.json', + '#\tpackage.json', + '', + '___', + '', + '### **PR Type**', + 'Enhancement, Bug fix', + ].join('\n'); + + it('keeps a quoted merge tail inside the section that quotes it', () => { + const titles = splitMarkdownSections(describeOut, 3).map((s) => s.title); + expect(titles).toEqual(['**User description**', '**PR Type**']); + const userDesc = splitMarkdownSections(describeOut, 3)[0]!; + // The conflict tail stays put as body text rather than becoming structure. + expect(userDesc.body).toContain('# Conflicts:'); + expect(userDesc.body).toContain('package-lock.json'); + }); + + it('default level 1 still splits on every heading (unchanged for other tools)', () => { + const titles = splitMarkdownSections(describeOut).map((s) => s.title); + expect(titles).toContain('Conflicts:'); + }); + + it('describe output routes through the shallower minimum', () => { + const { findings } = parseReviewOutput(describeOut, 'describe'); + // No finding is named after a conflicted file. + expect(findings.some((f) => /conflicts/i.test(f.title ?? ''))).toBe(false); + expect(findings.some((f) => /package-lock/i.test(f.title ?? ''))).toBe(false); + }); +});