Screenshot guardrails: name every screenshot not COMPLETE on 409 ASSET_IN_POST_PROCESSING (submit, validate; stuck after 5 min in UPLOAD_COMPLETE), apple screenshots --repair-stuck replaces only those in place, part uploads without Expect: 100-continue (failed part deletes the reservation, one retry, never committed), storebound api --confirm <operationId> for agents' plain writes (never owner-only)
This commit is contained in:
parent
164182012f
commit
00756e6908
19 changed files with 475 additions and 45 deletions
|
|
@ -5,7 +5,7 @@ import { appleRows, generate, playRows, scanSource, templateRegex, wrappedOps }
|
|||
import { APPLE_OPS } from '../cli/api/apple.gen';
|
||||
import { PLAY_APIS, PLAY_OPS } from '../cli/api/play.gen';
|
||||
import { allOps, appleApi, backoffMs, buildRequest, getOp, mergePages, nextRequest, type Op, playApi, resolveOp, send, sendAll } from '../cli/api/client';
|
||||
import { decide, gateOf, type GateInput, OWNER_ONLY, ownerReason } from '../cli/api/gate';
|
||||
import { decide, gateOf, type GateInput, OWNER_NO_CONFIRM, OWNER_ONLY, ownerReason } from '../cli/api/gate';
|
||||
import { coverage, describeRequest, parseParams, rows, search } from '../cli/api';
|
||||
import { parseArgs } from '../cli/lib/args';
|
||||
import { TOOLS, toArgv, validateInput } from '../cli/mcp-tools';
|
||||
|
|
@ -180,6 +180,20 @@ describe('write gating', () => {
|
|||
expect(decide({ ...base, getOnly: true, method: 'POST' }).action).toBe('refuse');
|
||||
expect(decide({ ...base, getOnly: true })).toEqual({ action: 'send' });
|
||||
});
|
||||
test('--confirm <operationId>: an agent without a terminal sends a plain write, never an owner-only one', () => {
|
||||
const w = { ...base, kind: 'write' as const, method: 'PATCH', yes: true, tty: false, opId: 'appStoreVersions_updateInstance' };
|
||||
expect(decide({ ...w, confirm: 'appStoreVersions_updateInstance' })).toEqual({ action: 'send' });
|
||||
expect(decide({ ...w, confirm: 'appInfos_updateInstance' })).toMatchObject({ action: 'refuse', why: expect.stringContaining('does not equal') });
|
||||
expect(decide({ ...w, confirm: 'x', opId: undefined }).action).toBe('refuse');
|
||||
// still dry without --yes, still nothing under MCP / desktop
|
||||
expect(decide({ ...w, yes: false, confirm: w.opId }).action).toBe('dry');
|
||||
expect(decide({ ...w, agent: true, confirm: w.opId }).action).toBe('refuse');
|
||||
// owner-only: --confirm never replaces the owner's typed id, with or without a terminal
|
||||
const o = { ...w, kind: 'owner' as const, method: 'DELETE', opId: 'appScreenshots_deleteInstance' };
|
||||
expect(decide({ ...o, confirm: o.opId })).toMatchObject({ action: 'refuse', why: OWNER_NO_CONFIRM });
|
||||
expect(decide({ ...o, tty: true, confirm: o.opId })).toMatchObject({ action: 'refuse', why: OWNER_NO_CONFIRM });
|
||||
expect(decide({ ...o, tty: true })).toEqual({ action: 'confirm', typed: true });
|
||||
});
|
||||
test('the dry run shows the exact request without the credential', () => {
|
||||
const r = buildRequest(op('apple', 'appStoreVersions_updateInstance'), { id: '1' }, { data: { type: 'appStoreVersions', id: '1' } });
|
||||
const text = describeRequest(r);
|
||||
|
|
|
|||
|
|
@ -529,9 +529,12 @@ describe('crash midway and unclear answers', () => {
|
|||
const md5 = (b: Uint8Array) => new Bun.CryptoHasher('md5').update(b).digest('hex');
|
||||
s.asc.put({ type: 'appScreenshots', id: 'shot-a', attributes: { sourceFileChecksum: md5(png(1)), assetDeliveryState: { state: 'COMPLETE' }, fileName: '01.png' }, rels: { appScreenshotSet: set.id } });
|
||||
s.asc.put({ type: 'appScreenshots', id: 'shot-x', attributes: { sourceFileChecksum: 'stale', assetDeliveryState: { state: 'COMPLETE' }, fileName: 'old.png' }, rels: { appScreenshotSet: set.id } });
|
||||
s.failPart = n => n === 2; // the 2nd uploaded part (03.png) fails
|
||||
s.failPart = n => n === 2 || n === 3; // 03.png fails, and its one retry too
|
||||
const r1 = await capture(() => appleScreenshots(p.ctx(['--only', 'en-US']))).catch(e => ({ out: String(e), err: '', exit: 1 }));
|
||||
expect(r1.out + r1.err).toContain('03.png part 0 -> 400');
|
||||
expect(r1.out + r1.err).toContain('nothing committed');
|
||||
// the failed reservations were deleted, not left AWAITING_UPLOAD
|
||||
expect(s.asc.children(set.id, 'appScreenshots').filter(x => (x.attributes.assetDeliveryState as { state: string } | undefined)?.state !== 'COMPLETE')).toEqual([]);
|
||||
expect(s.asc.children(set.id, 'appScreenshots').map(x => x.id)).toContain('shot-a');
|
||||
s.failPart = undefined;
|
||||
const r2 = await capture(() => appleScreenshots(p.ctx(['--only', 'en-US'])));
|
||||
|
|
|
|||
127
tests/screenshot-health.test.ts
Normal file
127
tests/screenshot-health.test.ts
Normal file
|
|
@ -0,0 +1,127 @@
|
|||
// Screenshots that hold up a submit (409 ASSET_IN_POST_PROCESSING.APP_SCREENSHOT): classification (stuck vs still
|
||||
// processing), the 409 explanation, validate findings, --repair-stuck in place, and the part upload rules (no
|
||||
// `Expect: 100-continue`, a failed part deletes the reservation and is retried once, never committed). No network.
|
||||
import { afterAll, beforeAll, describe, expect, test } from 'bun:test';
|
||||
import { mkdtempSync, rmSync, writeFileSync } from 'node:fs';
|
||||
import { tmpdir } from 'node:os';
|
||||
import { join } from 'node:path';
|
||||
import { uploadAssetVia } from '../cli/apple/asc';
|
||||
import { describePending, explain409, isScreenshotProcessing409, partHeaders, pendingScreenshots, readLedger, repairPlan, repairSet, setRows, STUCK_MS, updateLedger, type ShotRow } from '../cli/apple/screenshot-health';
|
||||
import { appleRules } from '../cli/lib/validate-rules';
|
||||
|
||||
let dir = '';
|
||||
beforeAll(() => { dir = mkdtempSync(join(tmpdir(), 'sb-shots-')); for (const n of ['01.png', '02.png', '03.png']) writeFileSync(join(dir, n), `png ${n}`); });
|
||||
afterAll(() => rmSync(dir, { recursive: true, force: true }));
|
||||
|
||||
const shot = (id: string, fileName: string, state: string) => ({ id, type: 'appScreenshots', attributes: { fileName, assetDeliveryState: { state }, sourceFileChecksum: 'x' } });
|
||||
const NOW = 10_000_000;
|
||||
|
||||
describe('classification', () => {
|
||||
const rows: ShotRow[] = setRows([shot('a', '01.png', 'COMPLETE'), shot('b', '02.png', 'UPLOAD_COMPLETE'), shot('c', '03.png', 'UPLOAD_COMPLETE'), shot('d', '04.png', 'FAILED'), shot('e', '05.png', 'AWAITING_UPLOAD')], 'en-US', 'APP_IPHONE_67');
|
||||
test('only non-COMPLETE rows; UPLOAD_COMPLETE over 5 min is stuck, younger or unknown is processing', () => {
|
||||
const p = pendingScreenshots(rows, { b: NOW - STUCK_MS - 1, c: NOW - 60_000 }, NOW);
|
||||
expect(p.map(x => [x.id, x.kind, x.position])).toEqual([['b', 'stuck', 2], ['c', 'processing', 3], ['d', 'failed', 4], ['e', 'awaiting-upload', 5]]);
|
||||
expect(pendingScreenshots(rows, {}, NOW).find(x => x.id === 'b')!.kind).toBe('processing');
|
||||
const lines = describePending(p);
|
||||
expect(lines[0]).toBe('en-US APP_IPHONE_67 #2 02.png: UPLOAD_COMPLETE for 5 min -> STUCK (not processing)');
|
||||
expect(lines[1]).toContain('still processing');
|
||||
});
|
||||
test('the 409 is recognised and explained with the repair command', () => {
|
||||
const e = new Error('PATCH /v1/reviewSubmissions/1 -> 409 STATE_ERROR.ASSET_IN_POST_PROCESSING.APP_SCREENSHOT: screenshots are processing');
|
||||
expect(isScreenshotProcessing409(e)).toBe(true);
|
||||
expect(isScreenshotProcessing409(new Error('-> 409 not ready to be submitted'))).toBe(false);
|
||||
const p = pendingScreenshots(rows, { b: NOW - STUCK_MS - 1 }, NOW);
|
||||
expect(explain409(p)).toContain('--repair-stuck');
|
||||
expect(explain409(p)).toContain('#2 02.png');
|
||||
expect(explain409(pendingScreenshots(rows.slice(0, 3), {}, NOW))).toContain('still processing');
|
||||
});
|
||||
test('validate: stuck is a blocker, processing a warning', () => {
|
||||
const p = pendingScreenshots(rows.slice(0, 3), { b: NOW - STUCK_MS - 1 }, NOW);
|
||||
const f = appleRules({ version: null, build: null, expectedLocales: [], locales: [], requiredDisplayTypes: [], reviewDetail: null, ageRating: null, priceSchedule: true, availability: true,
|
||||
iaps: [], expectedProducts: [], privacyUrl: { url: 'https://example.com/privacy', status: 200 }, screenshotsPending: p }).filter(x => x.code.startsWith('screenshot-'));
|
||||
expect(f.map(x => [x.code, x.level])).toEqual([['screenshot-stuck', 'blocker'], ['screenshot-processing', 'warn']]);
|
||||
expect(f[0]!.fix).toContain('--repair-stuck');
|
||||
});
|
||||
test('ledger keeps the first-seen time and drops screenshots that left UPLOAD_COMPLETE', () => {
|
||||
updateLedger(dir, [{ id: 'b', state: 'UPLOAD_COMPLETE' }, { id: 'z', state: 'UPLOAD_COMPLETE' }], 100);
|
||||
updateLedger(dir, [{ id: 'b', state: 'UPLOAD_COMPLETE' }, { id: 'z', state: 'COMPLETE' }], 900);
|
||||
expect(readLedger(dir)).toEqual({ b: 100 });
|
||||
});
|
||||
});
|
||||
|
||||
/** A fake screenshot set: list, delete, upload (append), reorder. */
|
||||
function fakeSet(start: ReturnType<typeof shot>[]) {
|
||||
let items = [...start];
|
||||
let n = 0;
|
||||
const calls: string[] = [];
|
||||
return {
|
||||
calls, get items() { return items; },
|
||||
all: async (_p: string) => items.map(x => ({ ...x })),
|
||||
asc: async (method: string, path: string, b?: unknown) => {
|
||||
const body = b as { data: { id: string }[] } | undefined;
|
||||
calls.push(`${method} ${path}`);
|
||||
if (method === 'DELETE') items = items.filter(x => !path.endsWith('/' + x.id));
|
||||
if (method === 'PATCH') items = body!.data.map(d => items.find(x => x.id === d.id)!);
|
||||
return {};
|
||||
},
|
||||
uploadAsset: async (_t: string, _r: unknown, file: string) => { const id = `new${++n}`; calls.push(`UPLOAD ${file.split('/').pop()}`); items.push(shot(id, file.split('/').pop()!, 'COMPLETE')); return id; },
|
||||
};
|
||||
}
|
||||
|
||||
describe('--repair-stuck', () => {
|
||||
test('replaces only the stuck one, same file, same position, count unchanged; a re-run does nothing', async () => {
|
||||
const set = fakeSet([shot('a', '01.png', 'COMPLETE'), shot('b', '02.png', 'UPLOAD_COMPLETE'), shot('c', '03.png', 'UPLOAD_COMPLETE')]);
|
||||
const rows = setRows(set.items, 'en-US', 'APP_IPHONE_67');
|
||||
const pending = pendingScreenshots(rows, { b: NOW - STUCK_MS - 1, c: NOW - 1000 }, NOW);
|
||||
const files = ['01.png', '02.png', '03.png'].map(f => join(dir, f));
|
||||
const steps = repairPlan(rows, pending, files);
|
||||
expect(steps.map(s => [s.id, s.position, s.file])).toEqual([['b', 2, join(dir, '02.png')]]);
|
||||
expect(await repairSet(set, 'S', steps)).toEqual({ replaced: 1 });
|
||||
expect(set.items.map(x => [x.id, x.attributes.fileName])).toEqual([['a', '01.png'], ['new1', '02.png'], ['c', '03.png']]);
|
||||
expect(set.calls.filter(c => !c.startsWith('PATCH'))).toEqual(['DELETE /v1/appScreenshots/b', 'UPLOAD 02.png']);
|
||||
// re-run with the same (now stale) plan: the stuck id is gone, nothing is touched
|
||||
const before = set.calls.length;
|
||||
expect(await repairSet(set, 'S', steps)).toEqual({ replaced: 0 });
|
||||
expect(set.calls.length).toBe(before);
|
||||
});
|
||||
test('no local file: nothing deleted', async () => {
|
||||
const set = fakeSet([shot('b', 'gone.png', 'FAILED')]);
|
||||
const rows = setRows(set.items, 'en-US', 'APP_IPHONE_67');
|
||||
const steps = repairPlan(rows, pendingScreenshots(rows, {}, NOW), []);
|
||||
await expect(repairSet(set, 'S', steps)).rejects.toThrow('no local file');
|
||||
expect(set.calls).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
describe('part uploads', () => {
|
||||
test('Expect is never sent', () => {
|
||||
expect(partHeaders({ requestHeaders: [{ name: 'Content-Type', value: 'image/png' }, { name: 'Expect', value: '100-continue' }, { name: 'expect', value: '100-continue' }] })).toEqual({ 'Content-Type': 'image/png' });
|
||||
});
|
||||
function store(partStatuses: number[]) {
|
||||
const calls: string[] = [];
|
||||
let r = 0;
|
||||
return {
|
||||
calls,
|
||||
asc: async (method: string, path: string) => {
|
||||
calls.push(`${method} ${path}`);
|
||||
return method === 'POST' ? { data: { id: `r${++r}`, attributes: { uploadOperations: [{ method: 'PUT', url: 'https://upload.example.com/p', offset: 0, length: 4, requestHeaders: [{ name: 'Expect', value: '100-continue' }] }] } } } : {};
|
||||
},
|
||||
fetch: async (_u: string, i: RequestInit) => {
|
||||
expect(Object.keys(i.headers as Record<string, string>).map(k => k.toLowerCase())).not.toContain('expect');
|
||||
const s = partStatuses.shift()!; calls.push(`PART ${s}`); return new Response(null, { status: s });
|
||||
},
|
||||
};
|
||||
}
|
||||
const rel = { appScreenshotSet: { data: { type: 'appScreenshotSets', id: 'S' } } };
|
||||
test('a non-2xx part (also 100 / 3xx): the reservation is deleted, retried once, committed only after a good part', async () => {
|
||||
const s = store([302, 200]);
|
||||
expect(await uploadAssetVia(s, 'appScreenshots', rel, join(dir, '01.png'))).toBe('r2');
|
||||
expect(s.calls).toEqual(['POST /v1/appScreenshots', 'PART 302', 'DELETE /v1/appScreenshots/r1', 'POST /v1/appScreenshots', 'PART 200', 'PATCH /v1/appScreenshots/r2']);
|
||||
});
|
||||
test('two failed parts: throws, never commits', async () => {
|
||||
const s = store([500, 403]);
|
||||
await expect(uploadAssetVia(s, 'appScreenshots', rel, join(dir, '01.png'))).rejects.toThrow('nothing committed');
|
||||
expect(s.calls.some(c => c.startsWith('PATCH'))).toBe(false);
|
||||
expect(s.calls.filter(c => c.startsWith('DELETE'))).toEqual(['DELETE /v1/appScreenshots/r1', 'DELETE /v1/appScreenshots/r2']);
|
||||
});
|
||||
});
|
||||
Loading…
Add table
Add a link
Reference in a new issue