fix(ship-patch): a base change past 2,000 lines counts in full; drop the text-matched context predicate (GST-1226 review round 1)

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This commit is contained in:
garrytanandcapy-ai[bot] committed 2026-11-03 15:27:12 +00:00
1 parent 5cb67a5031
commit 2c52585445
4 files changed
+22 -11

No files matched your search

+2 -2
View File
@@ -1,7 +1,7 @@
**A base merge next to a reviewed hunk no longer forces a second full review because of the base's own lines.** On founderstar#515, a base merge put 62 lines of another PR, already merged at base, next to the PR's 76 reviewed and unchanged lines in `tests/ui_shots.gd`. The scoped-review section went past its 200-line limit, so `gstack-ship-release finish` took the behavior route, which a review-loop exception can't accept, and asked for Steps 5 to 11.5 again. #3923 hit the same limit when only one trailing context line changed (GST-1226). **A base merge next to a reviewed hunk no longer forces a second full review because of the base's own lines.** On founderstar#515, a base merge put 62 lines of another PR, already merged at base, next to the PR's 76 reviewed and unchanged lines in `tests/ui_shots.gd`. The scoped-review section went past its 200-line limit, so `gstack-ship-release finish` took the behavior route, which a review-loop exception can't accept, and asked for Steps 5 to 11.5 again. #3923 hit the same limit when only one trailing context line changed (GST-1226).
### Fixed ### Fixed
- **The scoped-review packet counts only the branch's own lines toward its 200-line section limit.** The base's change and the base-added lines that appear as context in the current hunks are still shown to the reader but no longer count, in finish and in the receipt carry. A section the branch's own lines push past the limit still takes the full loop. Context-only drift is not carried silently: it still gets the scoped read. - **The scoped-review packet counts only the branch's own lines toward its 200-line section limit.** The base's +/- lines, already merged at base, are still shown to the reader but no longer count, in finish and in the receipt carry. A base change over 2,000 lines in one file counts in full again, and a section the branch's own lines push past the limit still takes the full loop. Context-only drift is not carried silently: it still gets the scoped read.
### Tests ### Tests
- `test/ship-rereview-triggers.test.ts`: 210 lines another PR merged at base next to the branch's 76 reviewed lines, and a changed trailing context line beside a 221-line base rewrite, both reach the scoped read (both took the behavior route on main). Negative control: a section the branch's own 230 lines push past the limit still takes the behavior route. - `test/ship-rereview-triggers.test.ts`: 210 lines another PR merged at base next to the branch's 76 reviewed lines, and a changed trailing context line beside a 221-line base rewrite, both reach the scoped read (both took the behavior route on main). Negative controls: a section the branch's own 230 lines push past the limit, and a 2,100-line base insertion beside the hunk, still take the behavior route.
+4 -3
View File
@@ -6033,9 +6033,10 @@ The refusal names the paths and a packet file with the hunks that differ and
the base's own change. The same lines moved somewhere else are the base's own change. The same lines moved somewhere else are
`RECEIPT_CARRY_PRODUCT_CHANGED`, not this code. A packet section over 200 `RECEIPT_CARRY_PRODUCT_CHANGED`, not this code. A packet section over 200
lines takes the full loop instead, but only the branch's own lines count toward lines takes the full loop instead, but only the branch's own lines count toward
that limit: the base's change and the base-added lines that sit as context in that limit: the base's +/- lines, already merged at base, are shown and not
the current hunks are shown and not counted (GST-1226), so a large base edit counted (GST-1226), so a large base edit next to the branch's hunk still gets
next to the branch's hunk still gets the scoped read. the scoped read. A base change over 2,000 lines in one file counts in full
again and takes the full loop.
**Fix.** Dispatch one fix-check reviewer **Fix.** Dispatch one fix-check reviewer
([review-policy.md](review-policy.md)) on the packet. When it comes back clean, ([review-policy.md](review-policy.md)) on the packet. When it comes back clean,
+6 -6
View File
@@ -126,13 +126,14 @@ function hunkTexts(diff: string): string[] {
/** The most lines one packet section may hold; a path with a longer section takes the full loop instead. */ /** The most lines one packet section may hold; a path with a longer section takes the full loop instead. */
export const PACKET_SECTION_CAP = 200; export const PACKET_SECTION_CAP = 200;
/** The most lines the base's change section may hold before its uncounted base lines count again (GST-1226). */
export const BASE_SECTION_CEILING = 2000;
/** /**
* The scoped-review packet: per context path, the hunks that differ between the two branch patches and the base's own * The scoped-review packet: per context path, the hunks that differ between the two branch patches and the base's own
* change; per test path, its test hunks. `over` names every path with a section past PACKET_SECTION_CAP: one reviewer * change; per test path, its test hunks. `over` names every path with a section past PACKET_SECTION_CAP: one reviewer
* cannot be shown all of it, so the caller sends it to the full loop. Only the branch's own lines count toward the cap * cannot be shown all of it, so the caller sends it to the full loop. The base's +/- lines, already merged at base, are
* (GST-1226): the base's +/- lines, already merged at base, and the base-added lines that sit as context in the current * shown but do not count toward the cap (GST-1226) until that section passes BASE_SECTION_CEILING lines.
* hunks are shown but not counted.
*/ */
export function contextPacket(repoRoot: string, s: PatchSides, paths: readonly string[], tests: readonly string[] = []): { text: string; over: string[] } { export function contextPacket(repoRoot: string, s: PatchSides, paths: readonly string[], tests: readonly string[] = []): { text: string; over: string[] } {
const over = new Set<string>(); const over = new Set<string>();
@@ -148,9 +149,8 @@ export function contextPacket(repoRoot: string, s: PatchSides, paths: readonly s
const r = hunkTexts(pathDiff(repoRoot, s.reviewedBase, s.reviewed, rel, 3) ?? ''); const r = hunkTexts(pathDiff(repoRoot, s.reviewedBase, s.reviewed, rel, 3) ?? '');
const c = hunkTexts(pathDiff(repoRoot, s.currentBase, s.current, rel, 3) ?? ''); const c = hunkTexts(pathDiff(repoRoot, s.currentBase, s.current, rel, 3) ?? '');
const b = hunkTexts(pathDiff(repoRoot, s.reviewedBase, s.currentBase, rel, 3) ?? '').join(''); const b = hunkTexts(pathDiff(repoRoot, s.reviewedBase, s.currentBase, rel, 3) ?? '').join('');
const merged = new Set(b.split('\n').filter(l => l.startsWith('+')).map(l => ` ${l.slice(1)}`)); parts.push(`## ${rel}\n`, fence('Reviewed hunks', r.filter(h => !c.includes(h)).join('')), fence('Current hunks', c.filter(h => !r.includes(h)).join('')),
parts.push(`## ${rel}\n`, fence('Reviewed hunks', r.filter(h => !c.includes(h)).join('')), fence('Current hunks', c.filter(h => !r.includes(h)).join(''), l => !merged.has(l)), fence("The base's change", b, b.split('\n').length > BASE_SECTION_CEILING ? undefined : l => !/^[+-]/.test(l)));
fence("The base's change", b, l => !/^[+-]/.test(l)));
} }
return { text: parts.join('\n'), over: [...over] }; return { text: parts.join('\n'), over: [...over] };
} }
+10
View File
@@ -915,4 +915,14 @@ describe('base lines do not count toward the scoped-review cap (GST-1226, founde
expect(scoped(r)).toEqual([3, 'behavior=1 docs=0 metadata=0 tests=0 base=0 context=0', false, true]); expect(scoped(r)).toEqual([3, 'behavior=1 docs=0 metadata=0 tests=0 base=0 context=0', false, true]);
expect(r.stdout.includes('STOP: behavior, tests or build inputs changed since review (src/shots.ts)')).toBe(true); expect(r.stdout.includes('STOP: behavior, tests or build inputs changed since review (src/shots.ts)')).toBe(true);
}); });
test('a base change past BASE_SECTION_CEILING counts in full again and takes the behavior route', () => {
const pr = block('pr', 76), other = block('other', 2100);
const f = fx({ 'src/shots.ts': lines(20) }, { 'src/shots.ts': join(at(20, 10, pr)) });
const reviewed = wtree(f), reviewedBase = mergeBase(f), post = snapshot(f);
f.advanceBase({ 'src/shots.ts': join(at(20, 11, other)) });
const merged = at(20, 11, other); merged.splice(10, 0, ...pr);
mergeMain(f, { 'src/shots.ts': join(merged) });
expect(scoped(finish(f, reviewed, post, ['--reviewed-base', reviewedBase]))).toEqual([3, 'behavior=1 docs=0 metadata=0 tests=0 base=0 context=0', false, true]);
});
}); });