mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-04 07:47:27 +01:00
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:
1 parent
5cb67a5031
commit
2c52585445
4 files changed
+22
-11
No files matched your search
@@ -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.
|
||||||
@@ -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
@@ -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] };
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -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]);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
Reference in new issue
Block a user