mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-03 07:18:03 +01:00
fix(sealed): a landed-patch skip still runs sealed.label_join, which judges the whole file the base may never have held
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This commit is contained in:
1 parent
0202069f5f
commit
6403e2db85
4 files changed
+35
-7
No files matched your search
@@ -1,7 +1,7 @@
|
||||
**The sealed guard no longer rescans commits the destination already holds, so a stacked branch retargeted after its parent squash-landed stops being refused for the parent's history.** gstack-evals#22 listed its parent #6's seven commits after #6 landed as one squash. One of them had added and later deleted a held-out file, so the guard refused a push whose every byte GitHub already had as `refs/pull/6/head`, and the queue sat blocked for about 50 minutes until a waiver landed.
|
||||
|
||||
### Changed
|
||||
- **`lib/sealed-guard.ts`** (`evaluateSealed`, shared by the pre-push hook and `gstack-pregate sealed`): a listed commit is skipped when it is an ancestor of the destination's own `refs/pull/<n>/head` for a squash `(#n)` on the base since the merge-base (at most 50 refs, a 20-second budget, an egress receipt per remote call, fetched refs deleted afterwards), or when its own `git patch-id --verbatim` and diff equal a base commit's (off above 2,000 base or 400 listed commits). Nothing is matched against a combined diff, and any failed lookup scans everything with one line saying why. docs/pregate.md#sealed-skip.
|
||||
- **`lib/sealed-guard.ts`** (`evaluateSealed`, shared by the pre-push hook and `gstack-pregate sealed`): a listed commit is skipped when it is an ancestor of the destination's own `refs/pull/<n>/head` for a squash `(#n)` on the base since the merge-base (at most 50 refs, a 20-second budget, an egress receipt per remote call, fetched refs deleted afterwards), or when its own `git patch-id --verbatim` and diff equal a base commit's (off above 2,000 base or 400 listed commits; a landed commit still gets the `sealed.label_join` check, which reads the whole file). Nothing is matched against a combined diff, and any failed lookup scans everything with one line saying why. docs/pregate.md#sealed-skip.
|
||||
- **`gstack-pregate sealed`**: the `SEALED:` line gains `skipped=<n>` (and `commits=` counts the commits scanned), stderr names each skipped group, and `--no-skip` scans every listed commit.
|
||||
|
||||
### Tests
|
||||
|
||||
+3
-1
@@ -1130,7 +1130,9 @@ gstack-pregate sealed [--range <a..b>] [--remote origin] [--base <branch>] [--no
|
||||
`sealed-guard`) and deletes the fetched refs afterwards. Landed: the commit's
|
||||
own `git patch-id --verbatim` equals a base commit's since the merge-base and
|
||||
so does its diff, apart from blob ids and line numbers; a merge never matches,
|
||||
and above 2,000 base or 400 listed commits this check is off. Nothing
|
||||
and above 2,000 base or 400 listed commits this check is off. A landed commit
|
||||
skips only the path and text rules: `sealed.label_join` judges the whole file
|
||||
at that commit, which the base may never have held, so it still runs. Nothing
|
||||
matches a combined diff, so a parent whose commits were never published is
|
||||
scanned whole even when its squash landed. A lookup that fails, runs out of
|
||||
time or exceeds a limit prints one line and scans everything. Each PR group
|
||||
|
||||
+9
-5
@@ -35,7 +35,9 @@
|
||||
* for a squash `(#n)` on the base since the merge-base (at most 50 refs, a
|
||||
* 20 s budget, fetched from the destination remote), is not rescanned, nor is
|
||||
* one whose own `git patch-id --verbatim` and diff equal a base commit's
|
||||
* since the merge-base (off above 2,000 base or 400 listed commits). No
|
||||
* since the merge-base (off above 2,000 base or 400 listed commits). A
|
||||
* landed patch skips only the path and text rules: `sealed.label_join` judges
|
||||
* the whole file at the commit, which the base may never have held. No
|
||||
* combined-diff matching; any lookup failure scans everything.
|
||||
*/
|
||||
import { spawnSync } from 'node:child_process';
|
||||
@@ -200,10 +202,11 @@ function textFindings(repoRoot: string, commits: string[], p: SealedPolicy): Sea
|
||||
return out;
|
||||
}
|
||||
|
||||
export function scanCommits(repoRoot: string, commits: string[], policy: SealedPolicy): SealedFinding[] {
|
||||
if (!commits.length) return [];
|
||||
export function scanCommits(repoRoot: string, commits: string[], policy: SealedPolicy, joinCommits: string[] = commits): SealedFinding[] {
|
||||
if (!commits.length && !joinCommits.length) return [];
|
||||
const seen = new Set<string>();
|
||||
return [...pathFindings(repoRoot, commits, policy), ...textFindings(repoRoot, commits, policy), ...joinFindings(repoRoot, commits, policy)].filter(f => {
|
||||
const own = commits.length ? [...pathFindings(repoRoot, commits, policy), ...textFindings(repoRoot, commits, policy)] : [];
|
||||
return [...own, ...joinFindings(repoRoot, joinCommits, policy)].filter(f => {
|
||||
const key = `${f.rule}\0${f.file}\0${f.line ?? ''}\0${f.commit}`;
|
||||
if (seen.has(key)) return false;
|
||||
seen.add(key);
|
||||
@@ -378,8 +381,9 @@ export function evaluateSealed(o: EvaluateOptions): SealedReport {
|
||||
const { skipped, notes } = alreadyPublished(o, listed);
|
||||
const skip = new Set(skipped.map(k => k.commit));
|
||||
const commits = listed.filter(c => !skip.has(c));
|
||||
const held = new Set(skipped.filter(k => k.pr !== undefined).map(k => k.commit));
|
||||
let found: SealedFinding[];
|
||||
try { found = scanCommits(o.repoRoot, commits, policy); }
|
||||
try { found = scanCommits(o.repoRoot, commits, policy, listed.filter(c => !held.has(c))); }
|
||||
catch (e: any) { return { ...blank, policyLabels: labels, commits: commits.length, skipped, notes, status: 'error', error: `could not read the published commits: ${e.message}` }; }
|
||||
const exception = globMatcher(policy.allow_paths, false);
|
||||
const exempt: ExemptFinding[] = [];
|
||||
|
||||
@@ -156,6 +156,28 @@ describe('sealed skips what the destination already holds', () => {
|
||||
expect(preflight(s).out).toMatch(/^SEALED: refused findings=1 commits=1 skipped=0 /m);
|
||||
});
|
||||
|
||||
test('a landed commit still gets the label_join check, because the join can form only on the branch', () => {
|
||||
const s = scratch();
|
||||
const rec = (extra: Record<number, string>) => ['{', ' "id": "r1",', ' "k1": 1,', ' "k2": 2,', ' "k3": 3,', ' "k4": 4,', ' "k5": 5,', ' "k6": 6,', ' "k7": 7', '}'].flatMap((l, i) => extra[i] ? [l, extra[i]!] : [l]).join('\n') + '\n';
|
||||
const labeled = JSON.stringify({ sealed: { paths: ['heldout/**'], label_keys: ['stratum'] } }, null, 2) + '\n';
|
||||
commit(s, { '.gstack/pregate.json': labeled, 'rec.json': rec({}) }, 'labeled policy');
|
||||
sh(s, ['push', '-q', 'origin', 'main']);
|
||||
sh(s, ['checkout', '-q', '-b', 'results']);
|
||||
commit(s, { 'rec.json': rec({ 1: ' "stratum": "decoy",' }) }, 'stratum');
|
||||
const head = commit(s, { 'rec.json': rec({ 1: ' "stratum": "decoy",', 7: ' "head": "abc1234def5678",' }) }, 'head');
|
||||
sh(s, ['checkout', '-q', 'main']);
|
||||
sh(s, ['cherry-pick', head]);
|
||||
sh(s, ['push', '-q', 'origin', 'main']);
|
||||
expect(sh(s, ['show', 'HEAD:rec.json'])).not.toContain('stratum');
|
||||
sh(s, ['checkout', '-q', 'results']);
|
||||
const h = hook(s);
|
||||
expect(h.code).toBe(1);
|
||||
expect(h.err).toMatch(/sealed\.label_join\s+rec\.json/);
|
||||
const p = preflight(s);
|
||||
expect(p.code).toBe(1);
|
||||
expect(p.out).toMatch(/^SEALED: refused findings=1 commits=1 skipped=1 /m);
|
||||
});
|
||||
|
||||
test('a landed commit that differs only in bytes that are not UTF-8 is scanned and refuses', () => {
|
||||
const s = scratch();
|
||||
sh(s, ['checkout', '-q', '-b', 'results']);
|
||||
|
||||
Reference in new issue
Block a user