mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-07 17:28:23 +01:00
fix: pre-landing review fixes
Review-Finding: p1/red-team#1 reader=red-team Review-Finding: p1/specialist-batched#1 reader=security Review-Finding: p1/specialist-batched#2 reader=testing Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This commit is contained in:
1 parent
7e7b248d37
commit
b0d38491b4
5 files changed
+23
-11
No files matched your search
+2
-2
@@ -1858,8 +1858,8 @@ reader, when all of these hold:
|
||||
canonical (no leading `a` digit unless it is `a`), at most 13 digits and at
|
||||
most 2^63−1 (`d4n4ub6itg400`);
|
||||
- neither it nor its sibling matches the base policy's
|
||||
[`review_exclude_paths`](#review-exclude-paths), since no reader saw an
|
||||
excluded script.
|
||||
[`review_exclude_paths`](#review-exclude-paths) or `readers.forbid_paths`,
|
||||
since no reader saw an excluded script.
|
||||
|
||||
Each proven sidecar prints `UID_SIDECAR: <path> (beside <sibling>)`. Any other
|
||||
`.uid` keeps its old route, usually behavior, and `finish` prints
|
||||
|
||||
@@ -3209,7 +3209,7 @@ range added or modified or the review-loop exception accepted`, `has no
|
||||
sibling <path> at the current tree`, `existed at the reviewed tree or the
|
||||
current base`, `is not a regular non-executable file (mode <mode>)`, `is not
|
||||
exactly one Godot uid://<id> line`, or `or its sibling matches the base
|
||||
policy's review_exclude_paths`.
|
||||
policy's review_exclude_paths or readers.forbid_paths`.
|
||||
|
||||
**Fix.** Commit the sidecar exactly as Godot wrote it, beside a script this PR
|
||||
added or changed: one `uid://<id>` line, a regular file, no symlink. Keep a
|
||||
|
||||
+6
-4
@@ -22,6 +22,7 @@ import { mechanicalPaths, writeMechanicalRecord } from './ship-mechanical';
|
||||
import { uidSidecars } from './ship-sidecar';
|
||||
import type { MechanicalConstant } from './ship-policy-mechanical';
|
||||
import { reviewFreshness } from './review-evidence';
|
||||
import { forbidSpecs } from './review-fetch';
|
||||
import { readRuledNotes, ruledNoteText } from './ruled-notes';
|
||||
import { parseReviewLoopException, type ReviewLoopException } from './ship-receipt-depth';
|
||||
import { parseReceiptDocs, type ReceiptDocs } from './ship-receipt-docs';
|
||||
@@ -375,7 +376,7 @@ function route(cwd: string, base: string, reviewed: string, reviewedBase: Review
|
||||
* version, each against the audited bytes; GST-906), and --docs-requested fixes the audit's own result asked for do not
|
||||
* stale the audit (GST-391).
|
||||
*/
|
||||
function docsFresh(cwd: string, base: string, postFile: string, moved: readonly string[], requested: string[], resultFile: string | undefined, loopAccepted: readonly string[] | undefined, parent: string | undefined, r: Report): boolean {
|
||||
function docsFresh(cwd: string, base: string, postFile: string, moved: readonly string[], requested: string[], resultFile: string | undefined, loopAccepted: readonly string[] | undefined, parent: string | undefined, testCode: readonly string[], r: Report): boolean {
|
||||
const cmp = bin(cwd, 'gstack-docs-candidate', ['compare', postFile], 120_000);
|
||||
let stale: string[] | null = null;
|
||||
try { stale = JSON.parse(cmp.stdout).content_changed; } catch { stale = null; }
|
||||
@@ -428,7 +429,7 @@ function docsFresh(cwd: string, base: string, postFile: string, moved: readonly
|
||||
const merged = mergeChangelogs(audited.stdout, atNowBase.stdout, atAuditBase.stdout);
|
||||
return kept !== null && merged !== null && normalizeChangelog(kept) === normalizeChangelog(audited.stdout) && normalizeChangelog(merged) === normalizeChangelog(now);
|
||||
});
|
||||
const carried = { tests: stale.filter(p => isTestCode(p)), base: stale.filter(p => !isTestCode(p) && (moved.includes(p) || restamped.includes(p) || (!!auditBase && !!nowBase && !branchAtAudit.has(p) && !branchNow.has(p))
|
||||
const carried = { tests: stale.filter(p => isTestCode(p, testCode)), base: stale.filter(p => !isTestCode(p, testCode) && (moved.includes(p) || restamped.includes(p) || (!!auditBase && !!nowBase && !branchAtAudit.has(p) && !branchNow.has(p))
|
||||
|| (!!auditBase && !!nowBase && /^[0-9a-f]{40,64}$/.test(nowTree) && ![...record.staged, ...record.unstaged, ...record.untracked].includes(p) && comparePath(cwd, { reviewedBase: auditBase, reviewed: record.head, currentBase: nowBase, current: nowTree }, p) === 'same'))), requested: [] as string[], accepted: [] as string[] };
|
||||
carried.accepted = stale.filter(p => loopAccepted?.includes(p) && !carried.tests.includes(p) && !carried.base.includes(p));
|
||||
let audit: any = null;
|
||||
@@ -495,15 +496,16 @@ async function finishStages(argv: readonly string[], cwd: string): Promise<Repor
|
||||
try { const h = JSON.parse(fs.readFileSync(flag('--docs-post')!, 'utf8')).hashes; audited = h && typeof h === 'object' && !Array.isArray(h) ? h : null; } catch { audited = null; }
|
||||
const globs = (key: string): string[] => Array.isArray(policy?.[key]) ? policy![key].filter((g: unknown): g is string => typeof g === 'string') : [];
|
||||
const bookkeeping = globs('bookkeeping');
|
||||
const forbidden = forbidSpecs({ forbid_paths: Array.isArray(policy?.readers?.forbid_paths) ? policy!.readers.forbid_paths.filter((g: unknown): g is string => typeof g === 'string') : [] }).map(s => s.slice(':(top,exclude,glob)'.length));
|
||||
const mechanical: MechanicalConstant[] = Array.isArray(policy?.mechanical_retake) ? policy!.mechanical_retake : [];
|
||||
const reviewedBaseArg = flag('--reviewed-base');
|
||||
const fragments = typeof policy?.changelog_fragments?.dir === 'string' ? [`:(top,exclude,glob)${policy.changelog_fragments.dir.replace(/\/+$/, '')}/*.md`] : [];
|
||||
const reviewedBase = reviewedBaseArg === undefined ? undefined : reviewedBaseCommit(cwd, `origin/${base.replace(/^origin\//, '')}`, reviewedBaseArg, [...releasePaths(policy, cwd, flag('--reviewed-tree')!, base, true).map(p => `:(top,exclude,literal)${p}`), ...fragments], r);
|
||||
if (reviewedBase === null) return r;
|
||||
const baseRelease = reviewedBase?.parent ? releasePaths(policy, cwd, git(cwd, ['merge-base', `origin/${base.replace(/^origin\//, '')}`, 'HEAD']).stdout.trim(), base, true) : undefined;
|
||||
const routed = route(cwd, base, flag('--reviewed-tree')!, reviewedBase, release, { docsRechecked: has('--docs-rechecked'), contextReviewed: flag('--context-reviewed'), loop, audited, bookkeeping, mechanical, testCode: globs('test_code'), excluded: globs('review_exclude_paths'), baseRelease }, r);
|
||||
const routed = route(cwd, base, flag('--reviewed-tree')!, reviewedBase, release, { docsRechecked: has('--docs-rechecked'), contextReviewed: flag('--context-reviewed'), loop, audited, bookkeeping, mechanical, testCode: globs('test_code'), excluded: [...globs('review_exclude_paths'), ...forbidden], baseRelease }, r);
|
||||
if (!routed) return r;
|
||||
if (!docsFresh(cwd, base, flag('--docs-post')!, [...routed.base, ...routed.context, ...routed.mechanical, ...stampedFiles(policy, cwd, flag('--reviewed-tree')!, base)], all('--docs-requested'), flag('--docs-result'), loop ? routed.accepted : undefined, reviewedBase?.parent ? reviewedBase.commit : undefined, r)) return r;
|
||||
if (!docsFresh(cwd, base, flag('--docs-post')!, [...routed.base, ...routed.context, ...routed.mechanical, ...stampedFiles(policy, cwd, flag('--reviewed-tree')!, base)], all('--docs-requested'), flag('--docs-result'), loop ? routed.accepted : undefined, reviewedBase?.parent ? reviewedBase.commit : undefined, globs('test_code'), r)) return r;
|
||||
if (!list.length && !has('--no-lanes-approved')) return r.stop('no test lanes: require Step 5\'s explicit untested-scope approval (rerun with --no-lanes-approved) or run Steps 5-15');
|
||||
const allow = evidenceExempt(release).join(',');
|
||||
const check = (l: { label: string; command: string }) => bin(cwd, 'gstack-evidence', ['check', '--label', l.label, '--expect-cmd', l.command, '--max-age', '24', '--allow-paths', allow], 60_000).stdout;
|
||||
|
||||
+5
-4
@@ -10,7 +10,8 @@
|
||||
* - its sibling (the path without `.uid`) is a regular file now, and one the reviewed range added or modified, or one
|
||||
* the review-loop exception accepted;
|
||||
* - it is a regular, non-executable file whose whole content is exactly what Godot writes for one id;
|
||||
* - neither it nor its sibling matches the base policy's review_exclude_paths (a sibling no reader saw is not reviewed).
|
||||
* - neither it nor its sibling matches the base policy's review_exclude_paths or readers.forbid_paths (a sibling no reader
|
||||
* saw is not reviewed).
|
||||
* Any other `.uid` is refused with the reason and stays behavior; a `.import` file is never a candidate.
|
||||
*/
|
||||
import { spawnSync } from 'node:child_process';
|
||||
@@ -22,7 +23,7 @@ export interface SidecarInput {
|
||||
candidates: readonly string[];
|
||||
/** Paths the review-loop exception accepted. */
|
||||
accepted: readonly string[];
|
||||
/** The base policy's review_exclude_paths (git glob pathspecs). */
|
||||
/** The git glob pathspecs no reader saw: the base policy's review_exclude_paths and readers.forbid_paths. */
|
||||
excluded: readonly string[];
|
||||
}
|
||||
export interface SidecarRefusal { path: string; reason: string }
|
||||
@@ -43,7 +44,7 @@ export function isGodotUidText(text: string): boolean {
|
||||
export const sidecarLine = (p: string, sibling: string) => `Godot UID sidecar: ${p} (beside ${sibling})`;
|
||||
|
||||
function git(cwd: string, args: string[]): string | null {
|
||||
const r = spawnSync('git', ['-c', 'core.quotepath=off', ...args], { cwd, encoding: 'latin1', timeout: 120_000, maxBuffer: 64 * 1024 * 1024 });
|
||||
const r = spawnSync('git', ['-c', 'core.quotepath=off', ...args], { cwd, encoding: 'utf8', timeout: 120_000, maxBuffer: 64 * 1024 * 1024 });
|
||||
return r.status === 0 ? r.stdout : null;
|
||||
}
|
||||
|
||||
@@ -78,7 +79,7 @@ export function uidSidecars(cwd: string, i: SidecarInput): { proven: Record<stri
|
||||
if (e?.mode !== '100644') { refuse(`is not a regular non-executable file (mode ${e?.mode ?? 'unreadable'})`); continue; }
|
||||
const text = git(cwd, ['cat-file', 'blob', e.blob]);
|
||||
if (text === null || !isGodotUidText(text)) { refuse('is not exactly one Godot uid://<id> line'); continue; }
|
||||
if (excluded.has(p) || excluded.has(sibling)) { refuse('or its sibling matches the base policy\'s review_exclude_paths, so no reader saw the script'); continue; }
|
||||
if (excluded.has(p) || excluded.has(sibling)) { refuse('or its sibling matches the base policy\'s review_exclude_paths or readers.forbid_paths, so no reader saw the script'); continue; }
|
||||
proven[p] = sibling;
|
||||
}
|
||||
return { proven, refused };
|
||||
|
||||
@@ -13,6 +13,7 @@ import * as path from 'node:path';
|
||||
import { readMechanicalLines } from '../lib/ship-mechanical';
|
||||
import { isTestCode } from '../lib/ship-patch';
|
||||
import { REVIEW_EXCLUDE_KEY_VERSION, TEST_CODE_KEY_VERSION, validatePolicy } from '../lib/ship-policy';
|
||||
import { READERS_KEY_VERSION } from '../lib/ship-policy-keys';
|
||||
import { isGodotUidText } from '../lib/ship-sidecar';
|
||||
import { lineOf, RELEASE, shipFixture, type ShipFx } from './helpers/ship-bin-fixture';
|
||||
|
||||
@@ -120,6 +121,14 @@ describe('a Godot UID sidecar added after the fix-check (GST-1604: founderstar#6
|
||||
expect([imported.status, lineOf(imported.stdout, 'STOP')]).toEqual([3, behaviorStop('tests/icon.png.import')]);
|
||||
expect(imported.stdout).not.toMatch(/UID_SIDECAR|ROUTE_SIDECAR_REFUSED/);
|
||||
});
|
||||
|
||||
test('a sidecar beside a readers.forbid_paths script, or beside an excluded script with a non-ASCII name, refuses too', () => {
|
||||
const forbidden = founderstar611({ base: { '.gstack/ship-policy.json': JSON.stringify({ readers: { forbid_paths: ['tests'] }, min_gstack_version: READERS_KEY_VERSION }) + '\n' } }).run();
|
||||
expect([forbidden.status, lineOf(forbidden.stdout, 'ROUTE_SIDECAR_REFUSED')]).toEqual([3, expect.stringContaining(`ROUTE_SIDECAR_REFUSED: ${SIDECAR} or its sibling matches`)]);
|
||||
const accented = founderstar611({ base: { '.gstack/ship-policy.json': JSON.stringify({ review_exclude_paths: ['tests/**'], min_gstack_version: REVIEW_EXCLUDE_KEY_VERSION }) + '\n' }, feat: { 'tests/menü_check.gd': CHECK }, after: f => f.write({ 'tests/menü_check.gd.uid': UID }) }).run();
|
||||
expect([accented.status, lineOf(accented.stdout, 'ROUTE_SIDECAR_REFUSED')]).toEqual([3, expect.stringContaining('ROUTE_SIDECAR_REFUSED: tests/menü_check.gd.uid or its sibling matches')]);
|
||||
expect(accented.stdout).not.toContain('UID_SIDECAR:');
|
||||
});
|
||||
});
|
||||
|
||||
describe('base-declared test_code (GST-1604: GDScript tests)', () => {
|
||||
|
||||
Reference in new issue
Block a user