From 3d372db3ef7bf0a85dadecd63be3acbf0d55d3d2 Mon Sep 17 00:00:00 2001 From: garrytan Date: Wed, 28 Oct 2026 14:03:29 +0000 Subject: [PATCH] fix: pre-landing review fixes Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- CHANGELOG.md | 6 +- review/SKILL.md | 8 +-- review/SKILL.md.tmpl | 8 +-- scripts/harness-version.json | 2 +- .../preamble/generate-voice-directive.ts | 8 +-- test/fixtures/skill-md-size-allowlist.json | 16 ++--- test/helpers/llm-judge.ts | 34 ++++++---- test/llm-judge-abort.test.ts | 2 +- test/llm-judge-stream.test.ts | 63 +++++++++++++++++-- 9 files changed, 106 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d8355e18e..7d66c8517 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,15 +10,15 @@ - **`/benchmark` Phase 6 example**: the hero image recommendation no longer suggests lazy-loading (that delays the largest paint) and names one remedy, as the rule under it asks. ### Changed -- **Judge cancellations leave evidence.** When a judge request is cancelled (the workflow judge deadline), `callJudge` prints one `llm-judge-aborted` line: elapsed time, requests and 429 waits, first and last stream event times, the open block type, thinking and text character counts, message and request ids, and the reason. Counts only, never content. Deadlines, effort, samples and thresholds are unchanged. +- **Judge cancellations leave evidence.** When a judge request is cancelled (the workflow judge deadline), `callJudge` prints one `llm-judge-aborted` line: elapsed time, wrapper attempts and 429 waits, HTTP requests and statuses (so the SDK's own retries show), connection, first and last stream event times, the open block type, thinking and text character counts, message and request ids, and the reason (at most 200 characters). Counts only, never content; an observer that throws cannot change a judge result. Deadlines, effort, samples and thresholds are unchanged. ### Tests - `test/review-workflow-clarity.test.ts` and `test/eng-scope-entry-ap.test.ts` pin the repaired rules; both fail with the prose change stashed. -- `test/llm-judge-stream.test.ts` and `test/llm-judge-abort.test.ts` replay a stream cancelled mid-thinking, a stream with no events and a cancelled 429 wait, with no-abort controls; all three fail with the helper change stashed. `test/workflow-judge-cache.test.ts`'s SDK stream stand-ins gain the real stream's `on()`. +- `test/llm-judge-stream.test.ts` and `test/llm-judge-abort.test.ts` replay a stream cancelled mid-thinking, a stream with no events, one cancelled after headers, an SDK-retried 429, a cancelled 429 wait, an event the observer cannot read and an overlong reason, with no-abort controls; each fails with the helper change stashed. `test/workflow-judge-cache.test.ts`'s SDK stream stand-ins gain the real stream's `on()`. - Focused paid judge runs: review 4/4/4 at 1bdc4ce and again at 3463250, plan-eng-review 3/5/4 at 1bdc4ce ($0.18). ### Gate integrity -- No threshold, sample, trial, deadline or case-kind change. Generated SKILL.md size pins for review and plan-eng-review were lowered to their new sizes on every host. +- No threshold, sample, trial, deadline or case-kind change. `scripts/harness-version.json` records the `llm-judge.ts` change as non-behavioral (no series reset). Generated SKILL.md size pins for review and plan-eng-review were lowered to their new sizes on every host. ## [1.91.294.0] - 2026-10-28 diff --git a/review/SKILL.md b/review/SKILL.md index 6e01257f0..d8aba2c97 100644 --- a/review/SKILL.md +++ b/review/SKILL.md @@ -887,8 +887,7 @@ pass: report it with its reason. - Gate findings additionally carry `gate` (the RH tag, e.g. `"RH-15"`), `gate_id` (the tool's hunk identity), `reason` (for `kept`) and one of the actions `kept`, `restored` or `open`. Never store hunk text, test names or commit - messages in the record (it is brain-synced). `open` findings count in - `issues_found` and in `critical`/`informational` by severity. The logger stamps + messages in the record (it is brain-synced). The logger stamps `actor` from `gstack-session-kind` itself and discards any `actor` you supply; for a non-interactive session it also forces `action: "open"` on every gate finding. Dispositions are per invocation: `/ship`'s own review pass asks again. @@ -899,8 +898,9 @@ pass: report it with its reason. `TIMESTAMP` is ISO 8601; `COMMIT` is `git rev-parse --short HEAD`. `quality_score` is Step 4.6's specialist score (`10.0` when small-diff specialists -were skipped or this host omits Review Army). This default is not completion evidence -and never lowers defect counts. The logger builds trusted `review_binding` from the +were skipped or this host omits Review Army). This default is not completion evidence; +unresolved defects, `open` ones included, still count in `issues_found` and +by severity in `critical`/`informational`. The logger builds trusted `review_binding` from the validated captured branch digest, discarding caller bindings. Never invent a binding or replace REVIEW_START at log time; finish only this pass's own token. diff --git a/review/SKILL.md.tmpl b/review/SKILL.md.tmpl index 4bb0587e2..b22eaf442 100644 --- a/review/SKILL.md.tmpl +++ b/review/SKILL.md.tmpl @@ -438,8 +438,7 @@ pass: report it with its reason. - Gate findings additionally carry `gate` (the RH tag, e.g. `"RH-15"`), `gate_id` (the tool's hunk identity), `reason` (for `kept`) and one of the actions `kept`, `restored` or `open`. Never store hunk text, test names or commit - messages in the record (it is brain-synced). `open` findings count in - `issues_found` and in `critical`/`informational` by severity. The logger stamps + messages in the record (it is brain-synced). The logger stamps `actor` from `gstack-session-kind` itself and discards any `actor` you supply; for a non-interactive session it also forces `action: "open"` on every gate finding. Dispositions are per invocation: `/ship`'s own review pass asks again. @@ -450,8 +449,9 @@ pass: report it with its reason. `TIMESTAMP` is ISO 8601; `COMMIT` is `git rev-parse --short HEAD`. `quality_score` is Step 4.6's specialist score (`10.0` when small-diff specialists -were skipped or this host omits Review Army). This default is not completion evidence -and never lowers defect counts. The logger builds trusted `review_binding` from the +were skipped or this host omits Review Army). This default is not completion evidence; +unresolved defects, `open` ones included, still count in `issues_found` and +by severity in `critical`/`informational`. The logger builds trusted `review_binding` from the validated captured branch digest, discarding caller bindings. Never invent a binding or replace REVIEW_START at log time; finish only this pass's own token. diff --git a/scripts/harness-version.json b/scripts/harness-version.json index 0ba788e75..4ad1e964c 100644 --- a/scripts/harness-version.json +++ b/scripts/harness-version.json @@ -29,7 +29,7 @@ "test/helpers/eval-budgets.ts": {"blob":"667d0e09b20fe5d7366f57b77b5fb9b04baa37ef","since":6}, "test/helpers/eval-store.ts": {"blob":"a50ea2c28f508ad9f35ea9353dd2f502ee0dcfc2","since":6,"nonBehavioral":"credential rejections are classed infra instead of assertion: no case's pass or fail changes; only failed trials whose provider rejected the run's key move class, so no rerun spends on a rejected key"}, "test/helpers/hermetic-env.ts": {"blob":"cc072895461dc52ea317d1beff72121342420d30","since":6}, - "test/helpers/llm-judge.ts": {"blob":"6f742f6a0f751ddcc9616d7d590a885c7bb78d62","since":6,"nonBehavioral":"callJudge emits one llm-judge-aborted diagnostic line (timings, request and retry counts, block kind, character counts, message and request ids; no content) when its caller's signal aborts it; the request, retries, parsing, scores and the thrown abort reason are unchanged, so no case's pass or fail changes"}, + "test/helpers/llm-judge.ts": {"blob":"095f9c85dd0ae29da8ba10fed91510313a4442a4","since":6,"nonBehavioral":"callJudge emits one llm-judge-aborted diagnostic line (wrapper attempts, HTTP request and status counts seen through a pass-through fetch, 429 waits, connect and event timings, block kind, character counts, message and request ids, a capped reason; no content) when its caller's signal aborts it; observers swallow their own errors, and the request, retries, parsing, scores and the thrown abort reason are unchanged, so no case's pass or fail changes"}, "test/helpers/paid-test-set.ts": {"blob":"ff9bfe43c43ec2bdd11114f7b8ec7c5736116f75","since":6}, "test/helpers/pty-screen.ts": {"blob":"4f77569f9ce11f9cee9f347216e468cd7c956820","since":6}, "test/helpers/pty/auq.ts": {"blob":"16e5ea2bb5b9f28f44808a1a09024dbd3e6f28c4","since":6}, diff --git a/scripts/resolvers/preamble/generate-voice-directive.ts b/scripts/resolvers/preamble/generate-voice-directive.ts index 4d220d53d..07b3e0279 100644 --- a/scripts/resolvers/preamble/generate-voice-directive.ts +++ b/scripts/resolvers/preamble/generate-voice-directive.ts @@ -1,15 +1,13 @@ import type { TemplateContext } from '../types'; -export function generateVoiceDirective(ctx: TemplateContext, tier: number): string { - const emDashRule = 'No em dashes.'; - +export function generateVoiceDirective(_ctx: TemplateContext, tier: number): string { if (tier <= 1) { return `## Voice Direct, concrete, builder-to-builder. Name the file, function, command, and user-visible impact. No filler. -${emDashRule} No AI vocabulary: delve, crucial, robust, comprehensive, nuanced, multifaceted, load-bearing. Never corporate or academic. Short paragraphs. End with what to do. +No em dashes. No AI vocabulary: delve, crucial, robust, comprehensive, nuanced, multifaceted, load-bearing. Never corporate or academic. Short paragraphs. End with what to do. Reply in the language of the user's latest message unless asked otherwise. Code, commands, paths, identifiers and quoted output stay verbatim. @@ -26,7 +24,7 @@ GStack voice: Garry-shaped product and engineering judgment. - Be direct about quality. Bugs matter. Edge cases matter. Fix the whole thing, not the demo path. - Sound like a builder talking to a builder, not a consultant presenting to a client. - Never corporate, academic, PR, or hype. Avoid filler, throat-clearing, generic optimism, and founder cosplay. -- ${emDashRule} No AI vocabulary: delve, crucial, robust, comprehensive, nuanced, multifaceted, furthermore, moreover, additionally, pivotal, landscape, tapestry, underscore, foster, showcase, intricate, vibrant, fundamental, significant, load-bearing. +- No em dashes. No AI vocabulary: delve, crucial, robust, comprehensive, nuanced, multifaceted, furthermore, moreover, additionally, pivotal, landscape, tapestry, underscore, foster, showcase, intricate, vibrant, fundamental, significant, load-bearing. - Reply in the language of the user's latest message unless asked otherwise. Code, commands, paths, identifiers, quoted output and question markers (\`D\`, option letters, \`(recommended)\`) stay verbatim. - The user has context you do not: domain knowledge, timing, relationships, taste. Cross-model agreement is a recommendation, not a decision. The user decides. diff --git a/test/fixtures/skill-md-size-allowlist.json b/test/fixtures/skill-md-size-allowlist.json index 9adf91988..0caed9b6a 100644 --- a/test/fixtures/skill-md-size-allowlist.json +++ b/test/fixtures/skill-md-size-allowlist.json @@ -6,29 +6,29 @@ "agy/gstack-plan-design-review": 110044, "agy/gstack-plan-devex-review": 116496, "agy/gstack-plan-eng-review": 144907, - "agy/gstack-review": 103744, + "agy/gstack-review": 103743, "codex/gstack-autoplan": 108553, "codex/gstack-land-and-deploy": 100284, "codex/gstack-plan-design-review": 111116, "codex/gstack-plan-devex-review": 118004, "codex/gstack-plan-eng-review": 146072, - "codex/gstack-review": 103267, + "codex/gstack-review": 103266, "copilot/gstack-autoplan": 109253, "copilot/gstack-plan-design-review": 110355, "copilot/gstack-plan-devex-review": 115958, "copilot/gstack-plan-eng-review": 145072, - "copilot/gstack-review": 103325, + "copilot/gstack-review": 103324, "cursor/gstack-autoplan": 108485, "cursor/gstack-plan-design-review": 108900, "cursor/gstack-plan-devex-review": 115228, "cursor/gstack-plan-eng-review": 143662, - "cursor/gstack-review": 121615, + "cursor/gstack-review": 121614, "factory/gstack-autoplan": 110763, "factory/gstack-land-and-deploy": 100105, "factory/gstack-plan-design-review": 111675, "factory/gstack-plan-devex-review": 118591, "factory/gstack-plan-eng-review": 146770, - "factory/gstack-review": 125388, + "factory/gstack-review": 125387, "gbrain/gstack-autoplan": 108892, "gbrain/gstack-plan-eng-review": 126289, "hermes/gstack-autoplan": 108982, @@ -37,18 +37,18 @@ "kiro/gstack-plan-design-review": 108752, "kiro/gstack-plan-devex-review": 115080, "kiro/gstack-plan-eng-review": 143524, - "kiro/gstack-review": 121439, + "kiro/gstack-review": 121438, "openclaw/gstack-autoplan": 108715, "openclaw/gstack-plan-eng-review": 124804, "opencode/gstack-autoplan": 109234, "opencode/gstack-plan-design-review": 110187, "opencode/gstack-plan-devex-review": 115836, "opencode/gstack-plan-eng-review": 144931, - "opencode/gstack-review": 122237, + "opencode/gstack-review": 122236, "slate/gstack-autoplan": 108372, "slate/gstack-plan-design-review": 108826, "slate/gstack-plan-devex-review": 115154, "slate/gstack-plan-eng-review": 143593, - "slate/gstack-review": 121527 + "slate/gstack-review": 121526 } } diff --git a/test/helpers/llm-judge.ts b/test/helpers/llm-judge.ts index 6f742f6a0..095f9c85d 100644 --- a/test/helpers/llm-judge.ts +++ b/test/helpers/llm-judge.ts @@ -135,7 +135,21 @@ export async function callJudge( // could be exhausted before a frontier judge emitted any JSON. const resolvedModel = resolveEvalModel('judge', model); const maxTokens = opts?.max_tokens ?? DEFAULT_JUDGE_MAX_TOKENS; - const client = new Anthropic(); + // What a cancelled request had received, so a judge deadline names rate-limit + // waits, SDK retries, a stream with no events, or a long thinking phase. Counts + // only, never content; an observer that throws never changes the judge outcome. + const started = performance.now(); + const elapsedMs = () => Math.round(performance.now() - started); + const progress = { httpRequests: 0, httpStatuses: [] as number[], retryWaitsMs: [] as number[], connectedMs: null as number | null, + firstEventMs: null as number | null, lastEventMs: null as number | null, block: null as string | null, thinkingChars: 0, + textChars: 0, messageId: null as string | null, requestId: null as string | null }; + const observe = (record: () => void) => { try { record(); } catch {} }; + const client = new Anthropic({ fetch: async (...args: Parameters) => { + progress.httpRequests += 1; + const response = await globalThis.fetch(...args); + progress.httpStatuses.push(response.status); + return response; + } }); const request = { model: resolvedModel, @@ -146,16 +160,14 @@ export async function callJudge( ...(opts?.effort === undefined ? {} : { effort: opts.effort }) } }), messages: [{ role: 'user' as const, content: prompt }], }; - // What a cancelled request had received, so a judge deadline names rate-limit - // waits, a stream with no events, or a long thinking phase. Counts only, never content. - const started = performance.now(); - const elapsedMs = () => Math.round(performance.now() - started); - const progress = { retryWaitsMs: [] as number[], firstEventMs: null as number | null, lastEventMs: null as number | null, - block: null as string | null, thinkingChars: 0, textChars: 0, messageId: null as string | null, requestId: null as string | null }; const makeRequest = () => { if (!opts?.stream) return client.messages.create(request, signal ? { signal } : undefined); const stream = client.messages.stream(request, signal ? { signal } : undefined); - stream.on('streamEvent', event => { + stream.on('connect', () => observe(() => { + progress.connectedMs ??= elapsedMs(); + progress.requestId ??= stream.request_id ?? null; + })); + stream.on('streamEvent', event => observe(() => { progress.firstEventMs ??= elapsedMs(); progress.lastEventMs = elapsedMs(); progress.requestId ??= stream.request_id ?? null; @@ -163,7 +175,7 @@ export async function callJudge( if (event.type === 'content_block_start') progress.block = event.content_block.type; if (event.type === 'content_block_delta' && event.delta.type === 'thinking_delta') progress.thinkingChars += event.delta.thinking.length; if (event.type === 'content_block_delta' && event.delta.type === 'text_delta') progress.textChars += event.delta.text.length; - }); + })); return stream.finalMessage(); }; @@ -199,8 +211,8 @@ export async function callJudge( if (signal?.aborted) { console.error(JSON.stringify({ type: 'llm-judge-aborted', model: resolvedModel, stream: opts?.stream === true, elapsedMs: elapsedMs(), - requests: attempt + 1, ...progress, - reason: signal.reason instanceof Error ? signal.reason.message : String(signal.reason), + attempts: attempt + 1, ...progress, + reason: (signal.reason instanceof Error ? signal.reason.message : String(signal.reason)).slice(0, 200), })); } throw error; diff --git a/test/llm-judge-abort.test.ts b/test/llm-judge-abort.test.ts index 7723a963d..2cc1cabbc 100644 --- a/test/llm-judge-abort.test.ts +++ b/test/llm-judge-abort.test.ts @@ -96,7 +96,7 @@ describe('callJudge cancellation', () => { await expect(pending).rejects.toThrow('Expired during rate-limit backoff.'); expect(diagnostics).toHaveBeenCalledTimes(1); expect(JSON.parse(diagnostics.mock.calls[0]![0])).toMatchObject({ type: 'llm-judge-aborted', model: 'test-judge-model', - stream: false, requests: 1, retryWaitsMs: [30000], firstEventMs: null, reason: 'Expired during rate-limit backoff.' }); + stream: false, attempts: 1, httpRequests: 0, retryWaitsMs: [30000], firstEventMs: null, reason: 'Expired during rate-limit backoff.' }); create.mockResolvedValue(response as any); diagnostics.mockClear(); expect(await callJudge('Judge this.', undefined, { signal: new AbortController().signal })).toEqual({ passed: true }); diff --git a/test/llm-judge-stream.test.ts b/test/llm-judge-stream.test.ts index 4c0cef07c..8abc3eae9 100644 --- a/test/llm-judge-stream.test.ts +++ b/test/llm-judge-stream.test.ts @@ -139,8 +139,8 @@ test('a judge aborted mid-stream retains what it had received, as counts without const diagnostic = diagnostics.mock.calls[0]![0]; expect(diagnostic).not.toContain('PRIVATE_'); const event = JSON.parse(diagnostic); - expect(event).toMatchObject({ type: 'llm-judge-aborted', model: 'claude-fable-5-1', stream: true, requests: 1, - retryWaitsMs: [], block: 'thinking', thinkingChars: 'PRIVATE_THINKING'.length, textChars: 0, + expect(event).toMatchObject({ type: 'llm-judge-aborted', model: 'claude-fable-5-1', stream: true, attempts: 1, + httpRequests: 1, httpStatuses: [200], retryWaitsMs: [], block: 'thinking', thinkingChars: 'PRIVATE_THINKING'.length, textChars: 0, messageId: 'msg_stalled', requestId: 'req_stalled', reason: failure.message }); expect(event.firstEventMs).toBeGreaterThanOrEqual(0); expect(event.lastEventMs).toBeGreaterThanOrEqual(event.firstEventMs); // gstack-timing-ok: ordering of two recorded offsets, not an elapsed-time bound @@ -159,8 +159,8 @@ test('a stream that never sent an event is retained as such, and provider errors await requested; controller.abort(new Error('deadline')); await rejected; - expect(JSON.parse(diagnostics.mock.calls[0]![0])).toMatchObject({ type: 'llm-judge-aborted', firstEventMs: null, - lastEventMs: null, block: null, thinkingChars: 0, messageId: null, requestId: null, reason: 'deadline' }); + expect(JSON.parse(diagnostics.mock.calls[0]![0])).toMatchObject({ type: 'llm-judge-aborted', httpRequests: 1, + httpStatuses: [], connectedMs: null, firstEventMs: null, lastEventMs: null, block: null, thinkingChars: 0, messageId: null, requestId: null, reason: 'deadline' }); diagnostics.mockClear(); transport.mockResolvedValue(new Response(JSON.stringify({ type: 'error', error: { type: 'api_error', message: 'boom' } }), @@ -168,3 +168,58 @@ test('a stream that never sent an event is retained as such, and provider errors await expect(callJudge('score', 'claude-fable-5-1', { max_tokens: 65_536, stream: true, signal: new AbortController().signal })).rejects.toThrow(); expect(diagnostics).not.toHaveBeenCalled(); }); + +test('a deadline after headers but before any event retains the connection and its request id', async () => { + let reading!: () => void; + const connected = new Promise(resolve => { reading = resolve; }); + transport.mockImplementation(async (_url: string, init: RequestInit) => new Response(new ReadableStream({ + start(stream) { init.signal!.addEventListener('abort', () => stream.error(init.signal!.reason), { once: true }); }, + pull() { reading(); return new Promise(() => {}); }, + }, { highWaterMark: 0 }), { headers: { 'content-type': 'text/event-stream', 'request-id': 'req_quiet' } })); + const controller = new AbortController(); + const rejected = callJudge('score', 'claude-fable-5-1', { max_tokens: 65_536, stream: true, signal: controller.signal }).catch(error => error); + await connected; + controller.abort(new Error('deadline')); + await rejected; + const event = JSON.parse(diagnostics.mock.calls[0]![0]); + expect(event).toMatchObject({ httpRequests: 1, httpStatuses: [200], requestId: 'req_quiet', firstEventMs: null, block: null }); + expect(event.connectedMs).toBeGreaterThanOrEqual(0); +}); + +test('SDK-internal 429 retries are counted, so throttling never reads as a silent stream', async () => { + let resent!: () => void; + const retried = new Promise(resolve => { resent = resolve; }); + transport.mockResolvedValueOnce(new Response(JSON.stringify({ type: 'error', error: { type: 'rate_limit_error', message: 'slow down' } }), + { status: 429, headers: { 'content-type': 'application/json', 'retry-after-ms': '1' } })); + transport.mockImplementationOnce((_url: string, init: RequestInit) => new Promise((_resolve, reject) => { + init.signal!.addEventListener('abort', () => reject(init.signal!.reason), { once: true }); + resent(); + })); + const controller = new AbortController(); + const rejected = callJudge('score', 'claude-fable-5-1', { max_tokens: 65_536, stream: true, signal: controller.signal }).catch(error => error); + await retried; + controller.abort(new Error('deadline')); + await rejected; + expect(JSON.parse(diagnostics.mock.calls[0]![0])).toMatchObject({ attempts: 1, httpRequests: 2, httpStatuses: [429], + retryWaitsMs: [], connectedMs: null, firstEventMs: null }); +}); + +test('an event the diagnostic cannot read never changes the judge result', async () => { + const unreadable = response(); + const body = (await unreadable.text()).replace('"thinking":"PRIVATE_THINKING"', '"thinking":null'); + transport.mockResolvedValue(new Response(body, { headers: { 'content-type': 'text/event-stream' } })); + expect(await callJudge('score the whole bundle', 'claude-fable-5-1', { + max_tokens: 65_536, stream: true, jsonSchema: WORKFLOW_JUDGE_RESPONSE_SCHEMA, + })).toEqual(scores); +}); + +test('an abort reason is retained at most 200 characters long', async () => { + transport.mockImplementation((_url: string, init: RequestInit) => new Promise((_resolve, reject) => { + init.signal!.addEventListener('abort', () => reject(init.signal!.reason), { once: true }); + })); + const controller = new AbortController(); + const rejected = callJudge('score', 'claude-fable-5-1', { max_tokens: 65_536, stream: true, signal: controller.signal }).catch(error => error); + controller.abort(new Error('x'.repeat(500))); + await rejected; + expect(JSON.parse(diagnostics.mock.calls[0]![0]).reason).toBe('x'.repeat(200)); +});