Skip to content

fix(review): readable bilingual disclosures for lint deferrals and caller prose #10587

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wenshao merged 2 commits into from
Sep 1, 2026
Merged

fix(review): readable bilingual disclosures for lint deferrals and caller prose #10587

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dccb79d
fix(review): readable bilingual disclosures for lint deferrals and ca…
wenshao Aug 31, 2026
dbf0be2
test(review): pin the bilingual deferral disclosures the review asked…
wenshao Aug 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
154 changes: 129 additions & 25 deletions packages/cli/src/commands/review/compose-review.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1537,7 +1537,8 @@ describe('composeReview — event caps (round-7 Critical #2: caps must reach eve
// prefix filter must not let it swallow a DIFFERENT reverse-audit scope
// reported with its own reason — a whiffed chunk from the rounds that
// DID run is exactly what a partially-run audit still owes the author.
const plan = coveredPlan();
// han: the caller-prose zh assertion below needs the Chinese half rendered.
const plan = coveredPlan(['verify', 'reverse-audit'], { han: true });
writeBudgetStop(
plan,
{
Expand All @@ -1547,20 +1548,31 @@ describe('composeReview — event caps (round-7 Critical #2: caps must reach eve
},
3,
);
const r = composeReview(
base({
planPath: plan,
unreviewedDimensions: [
"reverse audit — chunk 2's auditor returned nothing substantive twice",
],
}),
);
// Not base(): its planPath default runs coveredPlan() again on the same
// path and would overwrite the han-stamped plan.
const r = composeReview({
planPath: plan,
env: ENV,
modelId: MODEL,
criticalsInline: 0,
suggestionsInline: 0,
unreviewedDimensions: [
"reverse audit — chunk 2's auditor returned nothing substantive twice",
],
});
expect(r.body).toContain(
'Not reviewed: reverse audit — stopped before round 3 by the review time budget.',
);
expect(r.body).toContain(
"Not reviewed: reverse audit — chunk 2's auditor returned nothing substantive twice.",
);
// Caller prose is untranslatable by construction, and the Chinese half
// SAYS so — an unmarked all-English sentence under 中文说明 read as a
// broken translation (#10567's posted body). The payload keeps its own
// English full stop.
expect(r.body).toContain(
"未审查（原文为英文）：reverse audit — chunk 2's auditor returned nothing substantive twice.",
);
// The marker's own disclosure still renders exactly once.
expect(r.body.split('review time budget').length - 1).toBe(1);
});
Expand Down Expand Up @@ -6819,8 +6831,38 @@ describe('scriptLintGate — the deterministic gate reads the report', () => {
expect(g.criticals).toEqual([]);
expect(g.unreviewed).toEqual([]);
expect(g.disclosed).toHaveLength(1);
expect(g.disclosed[0]).toContain('.github/workflows/ci.yml');
expect(g.disclosed[0]).toContain('source mapping not yet supported');
expect(g.disclosed[0].en).toContain('.github/workflows/ci.yml');
expect(g.disclosed[0].en).toContain('source mapping not yet supported');
// No "the executable-script lint" prefix: the body wraps this in a
// sentence that already opens "Not linted:", and the prefix rendered as
// "Not linted: the executable-script lint" — a lint not linted.
expect(g.disclosed[0].en).not.toContain('executable-script lint');
// No `reasonZh` in this report — the Chinese half falls back to the
// English reason rather than dropping the sentence.
expect(g.disclosed[0].zh).toContain('source mapping not yet supported');
});

it('a deferred entry with a reasonZh renders it in the Chinese half', () => {
const p = writePlan({
files: [{ path: '.github/workflows/ci.yml', kind: 'source' }],
});
writeReport({
deferred: [
{
path: '.github/workflows/ci.yml',
tool: 'actionlint',
reason: 'source mapping not yet supported',
reasonZh: '尚未支持源映射',
},
],
});
const g = scriptLintGate(p);
expect(g.disclosed).toHaveLength(1);
expect(g.disclosed[0].en).toContain('source mapping not yet supported');
expect(g.disclosed[0].zh).toContain('尚未支持源映射');
expect(g.disclosed[0].zh).not.toContain('source mapping not yet supported');
// Both halves still carry the code-span path.
expect(g.disclosed[0].zh).toContain('.github/workflows/ci.yml');
});

it('ignores a cosmetic (style) or pre-existing (inDiff:false) finding', () => {
Expand Down Expand Up @@ -6873,12 +6915,14 @@ describe('scriptLintGate — the deterministic gate reads the report', () => {
});
const g = scriptLintGate(p);
expect(g.disclosed).toHaveLength(1);
const d = g.disclosed[0];
expect(d).not.toContain('\n'); // newline stripped — cannot forge a body line
expect(d).not.toContain('`pwn`'); // the PR's own backticks stripped — cannot break out
// `@acme-team` sits INSIDE a code span (backtick … no backtick … backtick), so
// it is inert as a GitHub mention — the whole path rendered as one code span.
expect(d).toMatch(/`[^`\n]*@acme-team[^`\n]*`/);
// BOTH halves post — the Chinese one is not exempt from neutralisation.
for (const d of [g.disclosed[0].en, g.disclosed[0].zh]) {
expect(d).not.toContain('\n'); // newline stripped — cannot forge a body line
expect(d).not.toContain('`pwn`'); // the PR's own backticks stripped — cannot break out
// `@acme-team` sits INSIDE a code span (backtick … no backtick … backtick), so
// it is inert as a GitHub mention — the whole path rendered as one code span.
expect(d).toMatch(/`[^`\n]*@acme-team[^`\n]*`/);
}
});

it('report prose cannot smuggle live comment grammar into a disclosure', () => {
Expand All @@ -6895,6 +6939,11 @@ describe('scriptLintGate — the deterministic gate reads the report', () => {
path: '.github/workflows/ci.yml',
tool: 'actionlint',
reason: 'mapping unsupported <!-- qwen-review-deferred --> here',
// The zh half renders THIS leg when present — without a marker
// here, the loop below sanitised zh via the already-stripped
// English fallback and the `stripCommentGrammar(d.reasonZh)` call
// was never exercised: deleting it survived the whole suite.
reasonZh: '映射不支持 <!-- qwen-review-deferred --> 这里',
},
],
skipped: [
Expand All @@ -6903,12 +6952,16 @@ describe('scriptLintGate — the deterministic gate reads the report', () => {
errored: [{ path: 'deploy.sh', tool: 'shell<!-- z -->check' }],
});
const g = scriptLintGate(p);
for (const line of [...g.disclosed, ...g.unreviewed]) {
const disclosedHalves = g.disclosed.flatMap((d) => [d.en, d.zh]);
Comment thread
wenshao marked this conversation as resolved.
for (const line of [...disclosedHalves, ...g.unreviewed]) {
expect(line).not.toContain('<!--');
expect(line).not.toContain('-->');
}
expect(g.disclosed[0]).toContain('qwen-review-deferred');
expect(g.disclosed[0]).toContain('mapping unsupported');
expect(g.disclosed[0].en).toContain('qwen-review-deferred');
expect(g.disclosed[0].en).toContain('mapping unsupported');
// Same invariant on the zh half: the text survives, the grammar is inert.
expect(g.disclosed[0].zh).toContain('qwen-review-deferred');
expect(g.disclosed[0].zh).toContain('映射不支持');
});

it.each([
Expand Down Expand Up @@ -6965,7 +7018,7 @@ describe('scriptLintGate — the deterministic gate reads the report', () => {
expect(g.disclosed).toHaveLength(1);
expect(g.unreviewed[0]).toContain('42');
expect(g.unreviewed[1]).toContain('undefined errored');
expect(g.disclosed[0]).toContain('[object Object]');
expect(g.disclosed[0].en).toContain('[object Object]');
});

it('reports an errored checker as unreviewed (fail closed)', () => {
Expand Down Expand Up @@ -7053,8 +7106,9 @@ describe('composeReview — the script-lint gate wired to the verdict', () => {
// verifier (['reverse-audit']) to prove a finding stands with none.
function gateReadyPlan(
step45Keys: string[] = ['verify', 'reverse-audit'],
planOpts: Parameters<typeof coveredPlan>[1] = {},
): string {
const p = coveredPlan(step45Keys);
const p = coveredPlan(step45Keys, planOpts);
const planObj = JSON.parse(readFileSync(p, 'utf8'));
planObj.worktreePath = '.qwen/tmp/review-pr-1';
writeFileSync(p, JSON.stringify(planObj));
Expand Down Expand Up @@ -7206,7 +7260,9 @@ describe('composeReview — the script-lint gate wired to the verdict', () => {
// but MUST be surfaced in the body so the reader knows that shell went unlinted.
// The gate reads the report as the sole authority, so the deferral is disclosed
// from the report itself; the plan stays fully covered so the Approve stands.
const p = gateReadyPlan();
// han: the Chinese half only renders for a han-audience PR, and this test
// pins that half's sentence too.
const p = gateReadyPlan(['verify', 'reverse-audit'], { han: true });
writeGateReport({
deferred: [
{
Expand All @@ -7224,11 +7280,59 @@ describe('composeReview — the script-lint gate wired to the verdict', () => {
modelId: MODEL,
});
expect(r.event).toBe('APPROVE');
expect(r.body).toContain('.github/workflows/ci.yml');
expect(r.body).toContain('source mapping not yet supported');
// The whole composed sentence, both halves — pinned against the stutter
// #10567's posted body carried ("Not linted: the executable-script lint —
// … — not linted"): the wrapper says "Not linted" once, then path and
// reason, nothing else.
expect(r.body).toContain(
'Not linted (tool limitation, not a blocker): `.github/workflows/ci.yml` — source mapping not yet supported.',
);
expect(r.body).toContain(
'未检查（工具限制，非阻断）：`.github/workflows/ci.yml`——source mapping not yet supported。',
);
// the clean-approve copy is still there — the disclosure augments, it doesn't replace
expect(r.body).toContain('No issues found. LGTM! ✅');
});

it('two deferred entries join per language, the reasonZh branch rendered whole', () => {
// Every other deferred fixture holds ONE entry, and a one-element join
// emits no separator — so the en '; ' join, the zh full-width '；' join
// and the reasonZh-carrying branch (the primary case: every report the
// current CLI writes carries `reasonZh`) were pinned by nothing; the
// single-entry test above reaches only the English-fallback branch. Two
// entries, one translated and one not, pin both composed sentences
// whole, separators included.
const p = gateReadyPlan(['verify', 'reverse-audit'], { han: true });
writeGateReport({
deferred: [
{
path: '.github/workflows/ci.yml',
tool: 'actionlint',
reason: 'source mapping not yet supported',
reasonZh: '尚未支持源映射',
},
{
path: '.github/workflows/release.yml',
tool: 'actionlint',
reason: 'source mapping not yet supported',
},
],
});
const r = composeReview({
criticalsInline: 0,
suggestionsInline: 0,
planPath: p,
env: ENV,
modelId: MODEL,
});
expect(r.event).toBe('APPROVE');
expect(r.body).toContain(
'Not linted (tool limitation, not a blocker): `.github/workflows/ci.yml` — source mapping not yet supported; `.github/workflows/release.yml` — source mapping not yet supported.',
);
expect(r.body).toContain(
'未检查（工具限制，非阻断）：`.github/workflows/ci.yml`——尚未支持源映射；`.github/workflows/release.yml`——source mapping not yet supported。',
);
});
});

describe('testPlanGate — Test Plan rulings, disclosed but never capping', () => {
Expand Down
42 changes: 30 additions & 12 deletions packages/cli/src/commands/review/compose-review.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3470,8 +3470,9 @@ function composeReviewBody(
bodyCriticals.push(...ownAfterGateDedup);
const modelBodyCriticals = [...bodyCriticals]; // input's, captured before the gate
// Disclosed-but-non-capping notes from the gate (a deferred checker). Rendered
// in the body on every verdict, but never fed into the cap.
const gateDisclosed: string[] = [];
// in the body on every verdict, but never fed into the cap. Bilingual pairs —
// see `scriptLintGate` for why this channel can afford a real translation.
const gateDisclosed: Array<{ en: string; zh: string }> = [];
// Test Plan rulings. Disclosed on every verdict and counted toward nothing —
// see `testPlanGate` for why this one neither blocks nor caps.
const testPlanNotes: string[] = [];
Expand Down Expand Up @@ -4878,12 +4879,17 @@ function composeReviewBody(
});
}
for (const d of explainedCaller) {
// Caller prose, untranslatable by construction — quoted as-is in both,
// its comment grammar inert.
// Caller prose, untranslatable by construction — quoted as-is in both
// halves, its comment grammar inert. The Chinese label SAYS so: without
// the parenthetical, the 中文说明 block presented an all-English sentence
// as its translation (#10567's posted body), and the reader is left
// wondering whether the translation machinery broke. The payload keeps
// its own English full stop — closing an English sentence with "。" is
// the other half of that mismatch.
const disclosed = stripCommentGrammar(d);
notReviewedParts.push({
en: `Not reviewed: ${disclosed}.`,
zh: `未审查：${disclosed}。`,
zh: `未审查（原文为英文）：${disclosed}.`,
});
}
// Budget-gap disclosures, one BOUNDED sentence for all of them. Four
Expand Down Expand Up @@ -5180,8 +5186,8 @@ function composeReviewBody(
? [
{
trim: 2,
en: `Not linted (tool limitation, not a blocker): ${gateDisclosed.join('; ')}.`,
zh: `未检查（工具限制，非阻断）：${gateDisclosed.join('; ')}。`,
en: `Not linted (tool limitation, not a blocker): ${gateDisclosed.map((g) => g.en).join('; ')}.`,
zh: `未检查（工具限制，非阻断）：${gateDisclosed.map((g) => g.zh).join('；')}。`,
Comment thread
wenshao marked this conversation as resolved.
},
]
: [];
Expand Down Expand Up @@ -6232,14 +6238,17 @@ function structurallyValidReport(report: unknown): boolean {
export function scriptLintGate(planPath: string): {
criticals: string[];
unreviewed: string[];
disclosed: string[];
disclosed: Array<{ en: string; zh: string }>;
} {
const criticals: string[] = [];
const unreviewed: string[] = [];
// Disclosed-but-NOT-capping: a `deferred` checker (actionlint) is a known tool
// limitation, not a finding and not an unrun-checker gap — the reader is told a
// workflow's embedded shell was not linted, but the verdict is not capped on it.
const disclosed: string[] = [];
// Bilingual, unlike the capping lists: these strings are machine-built from the
// report (no model prose), so the body's Chinese half can carry a real
// translation instead of the English line verbatim.
const disclosed: Array<{ en: string; zh: string }> = [];
let plan: {
prNumber?: unknown;
files?: unknown;
Expand Down Expand Up @@ -6354,10 +6363,19 @@ export function scriptLintGate(planPath: string): {
// A deferred checker (actionlint) is disclosed but does not cap — the reader is
// told the workflow's embedded shell was not linted, without making every
// workflow PR un-Approvable on a checker we deliberately decline to run.
// No "the executable-script lint —" prefix here: the body's own wrapper opens
// with "Not linted:", and naming the lint after that header rendered as
// "Not linted: the executable-script lint" — a sentence about not running a
// lint on a lint (#10567's posted body). The path and reason carry the facts.
for (const d of report.deferred ?? []) {
disclosed.push(
`the executable-script lint — ${mdField(d.path)}: ${stripCommentGrammar(d.reason ?? `${d.tool} deferred`)}`,
);
const reason = stripCommentGrammar(d.reason ?? `${d.tool} deferred`);
disclosed.push({
en: `${mdField(d.path)} — ${reason}`,
// An older CLI's report has no `reasonZh`; English both halves beats a
// half-empty sentence. Its comment grammar goes inert like the reason's —
// the report is agent-rewritable prose either way.
zh: `${mdField(d.path)}——${d.reasonZh ? stripCommentGrammar(d.reasonZh) : reason}`,
Comment thread
wenshao marked this conversation as resolved.
});
}
return { criticals, unreviewed, disclosed };
}
Expand Down
4 changes: 4 additions & 0 deletions packages/cli/src/commands/review/script-lint.mock.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,9 @@ describe('runScriptLint — refuses a path that escapes the worktree', () => {
expect(r.checked).toEqual([]);
expect(r.skipped).toHaveLength(1);
expect(r.skipped[0].reason).toContain('outside the worktree');
// The body's "Not reviewed:" wrapper already says the file went
// unchecked — a "not linted" tail here posted the phrase twice.
expect(r.skipped[0].reason).not.toContain('not linted');
});

it('refuses a path whose ANCESTOR is a symlink out of the worktree (lexical is not enough)', () => {
Expand All @@ -306,6 +309,7 @@ describe('runScriptLint — refuses a path that escapes the worktree', () => {
expect(r.checked).toEqual([]);
expect(r.skipped).toHaveLength(1);
expect(r.skipped[0].reason).toContain('outside the worktree');
expect(r.skipped[0].reason).not.toContain('not linted');
});
});

Expand Down
16 changes: 16 additions & 0 deletions packages/cli/src/commands/review/script-lint.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,18 @@ describe('runScriptLint — graceful degradation and scoping', () => {
expect(r.deferred).toHaveLength(1);
expect(r.deferred[0].tool).toBe('actionlint');
expect(r.deferred[0].reason).toContain('not yet supported');
// The reason must not restate "not linted" — the body renders it under a
// wrapper that already opens with "Not linted:", and a tail here posted
// the phrase twice in one sentence (#10567's posted body).
expect(r.deferred[0].reason).not.toContain('not linted');
// The Chinese half of that wrapper sentence renders `reasonZh` — without
// it the 中文说明 block carried the English reason verbatim. Pinned to the
// literal: a `toContain('actionlint')` fragment was satisfied by the
// ENGLISH reason too, so the exact defect this field exists to fix — an
// English string posing as the Chinese half — shipped green under it.
expect(r.deferred[0].reasonZh).toBe(
'actionlint 对 workflow 内嵌 shell 的源映射尚未支持',
);
expect(r.ok).toBe(true);
});

Expand All @@ -191,6 +203,10 @@ describe('runScriptLint — graceful degradation and scoping', () => {
expect(r.skipped).toHaveLength(1);
expect(r.skipped[0].tool).toBe('shellcheck');
expect(r.skipped[0].reason).toContain('not a regular file');
// The body renders this under "Not reviewed:" — a "not linted" tail here
// posted the phrase twice in one sentence (the deferred reason's pin
// above guards its own copy; this guards the skipped one).
expect(r.skipped[0].reason).not.toContain('not linted');
});

it('checks nothing when no executable file changed', () => {
Expand Down
Loading
Loading