From c0450638d5858e713965085911dc196f28806a73 Mon Sep 17 00:00:00 2001 From: Hamhire Hu Date: Wed, 9 Sep 2026 16:24:48 +0800 Subject: [PATCH] fix(describe): drop the blank card and keep diagrams left-to-right Two regressions from the pr-agent 0.45.0 upgrade, both from new upstream defaults I did not pin, and neither reported as an error by anything. 0.45.0 stamps `` at the top of its output. That landed in the leading title-less section, whose body therefore counted as non-empty -- but an HTML comment renders to nothing, so every /describe result grew a card that looked blank. trimNoise already discarded blank lines, horizontal rules and the internal branch-name leak; it now also discards a line that is only an HTML comment, which makes the existing "no title and no content" rule cover this. 0.45.0 also added pr_diagram_direction='adaptive', which draws any chain past a few nodes top-down. Pinned to LR: upstream tunes that heuristic for a full-width page, while the diagram is read here in a narrow chat column, where a tall diagram pushes the rest of the result off screen and a wide one scrolls sideways and can be opened in the preview view. The pinning list in the design doc now separates the two ways an upstream default can be wrong here -- one adds output that parses as a bogus finding, the other changes presentation for a surface upstream cannot know about. The upgrade review only applied the first test, which is why the diagram default slipped through. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 2 ++ CHANGELOG.zh-CN.md | 2 ++ docs/arch/02-agent/05-pragent-runtime.md | 2 +- packages/poller/src/parse-output.ts | 4 ++++ packages/poller/tests/parse-output.test.ts | 27 ++++++++++++++++++++++ packages/pr-agent-bridge/src/env.ts | 5 ++++ 6 files changed, 41 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f3d04a17..83c0258a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,8 @@ and the versioning follows [Semantic Versioning](https://semver.org/). ### 🔧 Fixed +- A generated PR description no longer ends with a blank card — an invisible marker the review engine now stamps into its output was being treated as a section of its own. +- Diagrams in a generated PR description are laid out left-to-right again, instead of turning tall and top-down once they get past a few steps and pushing the rest of the result off screen. - The commit divider in the review timeline now stays where the commit boundary actually is, instead of sliding further down every time a new message appears below it. - 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. diff --git a/CHANGELOG.zh-CN.md b/CHANGELOG.zh-CN.md index efc42003..bdb06b80 100644 --- a/CHANGELOG.zh-CN.md +++ b/CHANGELOG.zh-CN.md @@ -18,6 +18,8 @@ ### 🔧 修复 +- 生成的 PR 描述末尾不再出现空白卡片——评审引擎新加入输出的一个不可见标记此前被当成了独立分块。 +- 生成的 PR 描述中的图表恢复为从左至右布局，不再在步骤稍多时变成纵向长图、把其余内容挤出屏幕。 - 评审时间线中的提交分割线现在固定在提交边界的真实位置，不再因为下方出现新消息而一路下移。 - 生成的 PR 描述中，文件清单现在显示每个文件真实的增删行数，不再是 `+-1/--1`。 - 该清单中的链接现在指向文件本身，而非一个不存在的行号，点击可正常跳转。 diff --git a/docs/arch/02-agent/05-pragent-runtime.md b/docs/arch/02-agent/05-pragent-runtime.md index e81a2ca7..21b067fa 100644 --- a/docs/arch/02-agent/05-pragent-runtime.md +++ b/docs/arch/02-agent/05-pragent-runtime.md @@ -68,7 +68,7 @@ Current patches (pinned pr-agent **0.45.0**): **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. +**Upstream defaults that must stay pinned** (in `buildPragentEnv`): 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 — plus `pr_description.pr_diagram_direction='adaptive'`, which turns a longer diagram top-down. **Re-check this list on every upgrade**, and note the two distinct ways a default can be wrong here: one *adds output* (landing as a bogus finding), the other *changes presentation* for a surface upstream does not know about — pr-agent assumes a full-width page, while this diagram is read in a narrow chat column. Neither shows up as an error. ### Real token usage diff --git a/packages/poller/src/parse-output.ts b/packages/poller/src/parse-output.ts index e54cc75b..7faae594 100644 --- a/packages/poller/src/parse-output.ts +++ b/packages/poller/src/parse-output.ts @@ -161,6 +161,10 @@ function trimNoise(body: string): string { const trimmed = l.trim(); if (trimmed === '') return true; if (/^(?:[-*_]\s*){3,}$/.test(trimmed)) return true; // markdown HR + // A line that is nothing but an HTML comment: invisible once rendered, so it is not content. pr-agent 0.45.0 + // stamps `` at the top of its output, and counting that as content produced a section + // with a body that renders to nothing — an empty card in the run result. + if (/^$/.test(trimmed)) return true; if (INTERNAL_BRANCH_RE.test(trimmed) && trimmed.length < 40) return true; // short line + contains branch name return false; }; diff --git a/packages/poller/tests/parse-output.test.ts b/packages/poller/tests/parse-output.test.ts index fcf52e8c..541580d5 100644 --- a/packages/poller/tests/parse-output.test.ts +++ b/packages/poller/tests/parse-output.test.ts @@ -772,3 +772,30 @@ describe('splitMarkdownSections minLevel', () => { expect(findings.some((f) => /package-lock/i.test(f.title ?? ''))).toBe(false); }); }); + +describe('invisible-only sections', () => { + // Regression: pr-agent 0.45.0 stamps `` at the top of its output. That landed in the + // leading (title-less) section, whose body was therefore "not empty" — but an HTML comment renders to nothing, so + // the run result grew a card that looked blank. + it('drops a section whose body is only an HTML comment', () => { + const md = [ + 'pr-7c82edaec417/head', + '', + '### **PR Type**', + 'Enhancement', + ].join('\n'); + const { findings } = parseReviewOutput(md, 'describe'); + expect(findings.map((f) => f.title)).toEqual(['PR Type']); + }); + + it('keeps a section that has real content alongside the comment', () => { + const md = [ + '', + 'actual prose', + '### **PR Type**', + 'Enhancement', + ].join('\n'); + const { findings } = parseReviewOutput(md, 'describe'); + expect(findings.some((f) => (f.body ?? '').includes('actual prose'))).toBe(true); + }); +}); diff --git a/packages/pr-agent-bridge/src/env.ts b/packages/pr-agent-bridge/src/env.ts index 78caec30..b93bbe1e 100644 --- a/packages/pr-agent-bridge/src/env.ts +++ b/packages/pr-agent-bridge/src/env.ts @@ -88,6 +88,10 @@ function normalizeModel(provider: LlmProfile['provider'], model: string): string * — 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. + * - `PR_DESCRIPTION__PR_DIAGRAM_DIRECTION=LR` (new in 0.45, defaults `adaptive`): `adaptive` turns any chain longer + * than a few nodes top-down, which is the wrong trade for where this diagram is read — a narrow chat column, where + * a tall diagram pushes everything else off screen while a wide one scrolls sideways and can be opened in the + * preview view. Pinned left-to-right rather than left to a heuristic tuned for a full-width page. */ export function buildPragentEnv(profile: LlmProfile, maxModelTokens?: number): Record { const env: Record = {}; @@ -103,6 +107,7 @@ export function buildPragentEnv(profile: LlmProfile, maxModelTokens?: number): R env['PR_REVIEWER__PERSISTENT_FINDING_STATE'] = 'false'; env['PR_REVIEWER__ENABLE_REVIEW_COVERAGE_FOOTER'] = 'false'; env['PR_CODE_SUGGESTIONS__ENABLE_SUGGESTIONS_COVERAGE_FOOTER'] = 'false'; + env['PR_DESCRIPTION__PR_DIAGRAM_DIRECTION'] = 'LR'; // 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.