From 69f465889350f2937ff0b7c861af3776d1789780 Mon Sep 17 00:00:00 2001 From: Sanjay Rai <10876964+sanrai@users.noreply.github.com> Date: Wed, 26 Aug 2026 18:37:28 +1000 Subject: [PATCH] fix(mwpw-184989): send x-session-id to the LLM proxy and stop Feature QA failing silently The proxy now rejects requests without x-session-id (HTTP 403 missing_required_header). Only qa-runner-v2.mjs sent it, so Agent QA Review kept working while every direct caller started 403ing. On PR #591 that killed feature-review.mjs at its first LLM call: three 403s, llm failed after retries, and the top-level catch exited 0. Green job, no comment - even though build-output-diff had already published NO_CHANGE and review-score-gate had auto-passed the PR. - add x-session-id to all eight direct proxy callers - feature-review.mjs: fall back to postNonInjectable when the triage model is unavailable, so the deterministic bundle-diff verdict is still reported (it is a REST status read, it never needed a model) - feature-review.mjs: write FEATURE_REVIEW_FAILED on an unhandled error - qa-feature-review.yml: log that marker to the shared monitor issue, mirroring qa-agent-review.yml, so a silent outage is visible Claude-Session: https://claude.ai/code/session_01QimdQaYPSvXAaSDAssrfK1 --- .github/qa/action-judge.mjs | 8 +++++- .github/qa/aggregate-report.mjs | 6 +++++ .github/qa/ai-judge-test.mjs | 7 ++++- .github/qa/feature-backtest-batch.mjs | 8 ++++-- .github/qa/feature-backtest-worker.mjs | 7 +++++ .github/qa/feature-review.mjs | 34 +++++++++++++++++++++++-- .github/qa/mobile-probe.mjs | 8 +++++- .github/qa/plan-probe.mjs | 8 +++++- .github/workflows/qa-feature-review.yml | 31 ++++++++++++++++++++++ 9 files changed, 109 insertions(+), 8 deletions(-) diff --git a/.github/qa/action-judge.mjs b/.github/qa/action-judge.mjs index 121044a74..5936883e9 100644 --- a/.github/qa/action-judge.mjs +++ b/.github/qa/action-judge.mjs @@ -1,5 +1,11 @@ // Feed the AFTER-CLICK difference (old good code vs new broken code) to the REAL LLM judge. +import { randomUUID } from 'node:crypto'; import { readFileSync } from 'fs'; + +// The LLM proxy rejects any request without `x-session-id` (HTTP 403 +// missing_required_header). One id per process, as in qa-runner-v2.mjs. +const SESSION_ID = randomUUID(); + const pre = JSON.parse(readFileSync('/tmp/after-good.json','utf8')); // old code after click const post = JSON.parse(readFileSync('/tmp/after-bugged.json','utf8')); // new (broken) code after click const summary = `[after action] The tool clicked the "Digital Trends" filter on both builds. ` @@ -23,7 +29,7 @@ async function judge(intent, domDiff) { + `- WORKS: NEW behaves like the feature intends (narrows on click).\n` + `- FLAG: NEW contradicts the intent (e.g. filtering no longer narrows; the click has no effect on NEW though it did on OLD).\n` + `- NO_CHANGE: no visible difference and none intended.`; - const res = await fetch(PROXY,{method:'POST',headers:{Authorization:`Bearer ${TOKEN}`,'Content-Type':'application/json','anthropic-version':'2023-06-01'},body:JSON.stringify({model:MODEL,max_tokens:400,stream:true,messages:[{role:'user',content:prompt}]})}); + const res = await fetch(PROXY,{method:'POST',headers:{Authorization:`Bearer ${TOKEN}`,'Content-Type':'application/json','anthropic-version':'2023-06-01','x-session-id':SESSION_ID},body:JSON.stringify({model:MODEL,max_tokens:400,stream:true,messages:[{role:'user',content:prompt}]})}); const raw=await res.text(); let text=''; for(const line of raw.split('\n')){const t=line.trim();if(!t.startsWith('data:'))continue;const d=t.slice(5).trim();if(!d||d==='[DONE]')continue;let e;try{e=JSON.parse(d)}catch{continue}if(e.type==='content_block_delta'&&e.delta?.type==='text_delta')text+=e.delta.text||''} const j=JSON.parse(text.slice(text.indexOf('{'),text.lastIndexOf('}')+1)); diff --git a/.github/qa/aggregate-report.mjs b/.github/qa/aggregate-report.mjs index b0aa0eb19..4dbca4b71 100644 --- a/.github/qa/aggregate-report.mjs +++ b/.github/qa/aggregate-report.mjs @@ -22,10 +22,15 @@ * VERDICT_SIDECAR - optional; default /tmp/qa-audit-verdict.txt */ +import { randomUUID } from 'node:crypto'; import { readFileSync, readdirSync, existsSync, writeFileSync } from 'fs'; import { join, basename } from 'path'; import { spawnSync } from 'child_process'; +// The LLM proxy rejects any request without `x-session-id` (HTTP 403 +// missing_required_header). One id per process, as in qa-runner-v2.mjs. +const SESSION_ID = randomUUID(); + const PROXY_URL = process.env.PROXY_URL || ''; const MODEL = process.env.MODEL || ''; const TOKEN = process.env.IMS_ACCESS_TOKEN || ''; @@ -173,6 +178,7 @@ function rewriteWithLLM() { '-H', `Authorization: Bearer ${TOKEN}`, '-H', 'Content-Type: application/json', '-H', 'anthropic-version: 2023-06-01', + '-H', `x-session-id: ${SESSION_ID}`, '--max-time', '90', '--data-binary', '@-', ], diff --git a/.github/qa/ai-judge-test.mjs b/.github/qa/ai-judge-test.mjs index 09b2e7cc0..54541f5e0 100644 --- a/.github/qa/ai-judge-test.mjs +++ b/.github/qa/ai-judge-test.mjs @@ -2,10 +2,15 @@ // Renders a scenario NEW vs OLD on localhost via window._qa, computes a real DOM diff, // and asks the ACTUAL LLM judge (same prompt as the batch) for WORKS/FLAG/NO_CHANGE. // Creds sourced from env (PROXY_URL/MODEL/IMS_ACCESS_TOKEN); values never printed. +import { randomUUID } from 'node:crypto'; import { chromium } from 'playwright'; import { readFileSync } from 'fs'; import { diffSignatures, summarizeDiff } from './dom-diff.mjs'; +// The LLM proxy rejects any request without `x-session-id` (HTTP 403 +// missing_required_header). One id per process, as in qa-runner-v2.mjs. +const SESSION_ID = randomUUID(); + const cat = JSON.parse(readFileSync('/private/tmp/schema-branch/.github/qa/mount-catalog.json', 'utf8')); const byId = Object.fromEntries(cat.entries.map((e) => [e.id, e])); const URL = 'http://localhost:8899/index.html'; @@ -70,7 +75,7 @@ async function judgeExpected(intent, domDiff, visualDiff) { + `- NO_CHANGE: the PR did not intend any visible change (pure refactor, comment/log/formatting tweak) and the page correctly shows no change.`; for (let attempt = 0; attempt < 4; attempt += 1) { try { - const res = await fetch(PROXY, { method: 'POST', headers: { Authorization: `Bearer ${TOKEN}`, 'Content-Type': 'application/json', 'anthropic-version': '2023-06-01' }, + const res = await fetch(PROXY, { method: 'POST', headers: { Authorization: `Bearer ${TOKEN}`, 'Content-Type': 'application/json', 'anthropic-version': '2023-06-01', 'x-session-id': SESSION_ID }, body: JSON.stringify({ model: MODEL, max_tokens: 600, stream: true, messages: [{ role: 'user', content: prompt }] }) }); const raw = await res.text(); let text = ''; for (const line of raw.split('\n')) { const t = line.trim(); if (!t.startsWith('data:')) continue; const d = t.slice(5).trim(); if (!d || d === '[DONE]') continue; let e; try { e = JSON.parse(d); } catch { continue; } if (e.type === 'content_block_delta' && e.delta?.type === 'text_delta') text += e.delta.text || ''; } diff --git a/.github/qa/feature-backtest-batch.mjs b/.github/qa/feature-backtest-batch.mjs index 2c1d9c260..30f3c5a9b 100644 --- a/.github/qa/feature-backtest-batch.mjs +++ b/.github/qa/feature-backtest-batch.mjs @@ -1,6 +1,6 @@ #!/usr/bin/env node import { execFileSync } from 'node:child_process'; -import { createHash } from 'node:crypto'; +import { createHash, randomUUID } from 'node:crypto'; import { cpSync, existsSync, mkdirSync, readFileSync, renameSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; @@ -9,6 +9,10 @@ import { diffSignatures, summarizeDiff } from './dom-diff.mjs'; import { PNG } from 'pngjs'; import pixelmatch from 'pixelmatch'; +// The LLM proxy rejects any request without `x-session-id` (HTTP 403 +// missing_required_header). One id per process, as in qa-runner-v2.mjs. +const SESSION_ID = randomUUID(); + // Visual diff of the pre-code vs post-code render (same forced scenario). Localised noise // (antialiasing) is ignored via a threshold; a taller/shorter page (content changed) counts // as a visual change directly. @@ -228,7 +232,7 @@ async function judgeExpected(intent, domDiff, visualDiff) { + `- NO_CHANGE: the PR did not intend any visible change (pure refactor, comment/log/formatting tweak) and the page correctly shows no change.`; for (let attempt = 0; attempt < 4; attempt += 1) { try { - const res = await fetch(PROXY, { method: 'POST', headers: { Authorization: `Bearer ${TOKEN}`, 'Content-Type': 'application/json', 'anthropic-version': '2023-06-01' }, + const res = await fetch(PROXY, { method: 'POST', headers: { Authorization: `Bearer ${TOKEN}`, 'Content-Type': 'application/json', 'anthropic-version': '2023-06-01', 'x-session-id': SESSION_ID }, body: JSON.stringify({ model: MODEL, max_tokens: 600, stream: true, messages: [{ role: 'user', content: prompt }] }) }); const raw = await res.text(); let text = ''; for (const line of raw.split('\n')) { const t = line.trim(); if (!t.startsWith('data:')) continue; const d = t.slice(5).trim(); if (!d || d === '[DONE]') continue; let e; try { e = JSON.parse(d); } catch { continue; } if (e.type === 'content_block_delta' && e.delta?.type === 'text_delta') text += e.delta.text || ''; } diff --git a/.github/qa/feature-backtest-worker.mjs b/.github/qa/feature-backtest-worker.mjs index 1d8bf9720..928a79e41 100644 --- a/.github/qa/feature-backtest-worker.mjs +++ b/.github/qa/feature-backtest-worker.mjs @@ -1,4 +1,5 @@ #!/usr/bin/env node +import { randomUUID } from 'node:crypto'; import { execFileSync } from 'node:child_process'; import { mkdirSync, readFileSync, writeFileSync } from 'node:fs'; import path from 'node:path'; @@ -10,6 +11,11 @@ import { buildValidationView } from './observation-view.mjs'; import { requestBoundedJson } from './llm-json.mjs'; import { shouldChallengeSkip } from './skip-challenge.mjs'; import { classifyChangedPaths } from './detect-gate.mjs'; + +// The LLM proxy rejects any request without `x-session-id` (HTTP 403 +// missing_required_header). One id per process, as in qa-runner-v2.mjs. +const SESSION_ID = randomUUID(); + import { applyScenarioRepair, findMissingRequiredInitial, @@ -90,6 +96,7 @@ async function llmResponse(prompt, maxTokens = 4000) { Authorization: `Bearer ${TOKEN}`, 'Content-Type': 'application/json', 'anthropic-version': '2023-06-01', + 'x-session-id': SESSION_ID, }, body, signal: controller.signal, diff --git a/.github/qa/feature-review.mjs b/.github/qa/feature-review.mjs index 101217783..6f2035228 100644 --- a/.github/qa/feature-review.mjs +++ b/.github/qa/feature-review.mjs @@ -12,6 +12,7 @@ * say should happen. */ import { execFileSync } from 'node:child_process'; +import { randomUUID } from 'node:crypto'; import { writeFileSync } from 'node:fs'; import path from 'node:path'; import { chromium } from 'playwright'; @@ -26,6 +27,10 @@ import { } from './feature-action.mjs'; import { buildScenarioConfig } from './scenario-config.mjs'; +// The LLM proxy rejects any request without `x-session-id` (HTTP 403 +// missing_required_header). One id per process, as in qa-runner-v2.mjs. +const SESSION_ID = randomUUID(); + const env = (k, d = '') => (process.env[k] ?? d); const PR = env('PR_NUMBER'); const REPO = env('GH_REPO', 'adobecom/caas'); @@ -127,6 +132,7 @@ async function llm(prompt, maxTokens = 4000) { Authorization: `Bearer ${TOKEN}`, 'Content-Type': 'application/json', 'anthropic-version': '2023-06-01', + 'x-session-id': SESSION_ID, }, body, signal: controller.signal, @@ -303,7 +309,15 @@ function postComment(verdict, bodyMd) { .slice(0, 14000); // ---- Step 1: decide whether the PR's feature can be exercised at all ---- - const detect = await llm( + // This triage is the only thing standing between the agent and its report, + // and it needs the LLM proxy. The build-output-diff verdict does NOT -- it is + // a commit status read over the REST API. So when the proxy is unavailable, + // fall through to that deterministic verdict rather than dying silently: a + // dependency bump whose shipped bundle is byte-identical is still provably + // safe, and saying so is the whole point of this cross-signal. + let detect; + try { + detect = await llm( `You are triaging an Adobe CaaS (Consonant card collection) pull request to decide if its feature can be EXERCISED by an automated harness. The harness renders the REAL PR build on a live page and can force the CaaS CONFIG and COLLECTION DATA. It can then perform ONE simple interaction: clicking one visible control or typing into one visible input, followed by a second DOM capture. @@ -326,6 +340,12 @@ Diff (truncated): ${diff} Respond with ONLY a JSON object: {"testable":true|false,"reason":"one sentence"}.`, 4000); + } catch (error) { + await postNonInjectable( + `Triage model unavailable (${error.message}) -- reporting the deterministic bundle diff only.`); + console.log('llm unavailable: posted the build-output-diff verdict without triage'); + process.exit(0); + } console.error('[detect raw first 400]:', String(detect).slice(0, 400)); let plan; @@ -587,4 +607,14 @@ ${after.cards.map((item) => `- ${item.n}. ${item.title || item.text.slice(0, 50) **Verdict:** ${res.reason}`); process.exit(0); -})().catch((e) => { console.error('feature-review error:', e.stack || e.message); process.exit(0); }); +})().catch((e) => { + // Advisory and non-blocking by design -- but a green run with no comment is + // exactly how this agent's outage stayed invisible. Leave a marker so the + // workflow's monitor step can log it, the way Agent QA Review already does. + console.error('feature-review error:', e.stack || e.message); + try { + writeFileSync(`${env('GITHUB_WORKSPACE', '.')}/FEATURE_REVIEW_FAILED`, + `PR #${PR}: ${e.stack || e.message}\n`, { flag: 'a' }); + } catch { /* best effort */ } + process.exit(0); +}); diff --git a/.github/qa/mobile-probe.mjs b/.github/qa/mobile-probe.mjs index 745e38b59..c6e7c93d2 100644 --- a/.github/qa/mobile-probe.mjs +++ b/.github/qa/mobile-probe.mjs @@ -1,9 +1,15 @@ +import { randomUUID } from 'node:crypto'; import { researchCode } from './code-search.mjs'; + +// The LLM proxy rejects any request without `x-session-id` (HTTP 403 +// missing_required_header). One id per process, as in qa-runner-v2.mjs. +const SESSION_ID = randomUUID(); + const PROXY=process.env.PROXY_URL, MODEL=process.env.MODEL, TOKEN=process.env.IMS_ACCESS_TOKEN; const REPO_ROOT='/private/tmp/schema-branch'; function extractJson(src){const t=String(src).replace(/```(?:json)?/gi,'').trim();const a=t.indexOf('{'),b=t.lastIndexOf('}');return JSON.parse(t.slice(a,b+1));} async function llm(prompt, maxTokens=3000){ - const res=await fetch(PROXY,{method:'POST',headers:{Authorization:`Bearer ${TOKEN}`,'Content-Type':'application/json','anthropic-version':'2023-06-01'},body:JSON.stringify({model:MODEL,max_tokens:maxTokens,stream:true,messages:[{role:'user',content:prompt}]})}); + const res=await fetch(PROXY,{method:'POST',headers:{Authorization:`Bearer ${TOKEN}`,'Content-Type':'application/json','anthropic-version':'2023-06-01','x-session-id':SESSION_ID},body:JSON.stringify({model:MODEL,max_tokens:maxTokens,stream:true,messages:[{role:'user',content:prompt}]})}); const raw=await res.text(); let text=''; for(const line of raw.split('\n')){const s=line.trim();if(!s.startsWith('data:'))continue;const d=s.slice(5).trim();if(!d||d==='[DONE]')continue;let e;try{e=JSON.parse(d)}catch{continue}if(e.type==='content_block_delta'&&e.delta?.type==='text_delta')text+=e.delta.text||''} return text.trim(); diff --git a/.github/qa/plan-probe.mjs b/.github/qa/plan-probe.mjs index 96420230a..ecbcfe7fe 100644 --- a/.github/qa/plan-probe.mjs +++ b/.github/qa/plan-probe.mjs @@ -1,4 +1,10 @@ +import { randomUUID } from 'node:crypto'; import { researchCode } from './code-search.mjs'; + +// The LLM proxy rejects any request without `x-session-id` (HTTP 403 +// missing_required_header). One id per process, as in qa-runner-v2.mjs. +const SESSION_ID = randomUUID(); + const PROXY=process.env.PROXY_URL, MODEL=process.env.MODEL, TOKEN=process.env.IMS_ACCESS_TOKEN; const REPO_ROOT='/private/tmp/schema-branch'; function extractJson(src){ @@ -7,7 +13,7 @@ function extractJson(src){ return JSON.parse(t.slice(a,b+1)); } async function llm(prompt, maxTokens=3000){ - const res=await fetch(PROXY,{method:'POST',headers:{Authorization:`Bearer ${TOKEN}`,'Content-Type':'application/json','anthropic-version':'2023-06-01'},body:JSON.stringify({model:MODEL,max_tokens:maxTokens,stream:true,messages:[{role:'user',content:prompt}]})}); + const res=await fetch(PROXY,{method:'POST',headers:{Authorization:`Bearer ${TOKEN}`,'Content-Type':'application/json','anthropic-version':'2023-06-01','x-session-id':SESSION_ID},body:JSON.stringify({model:MODEL,max_tokens:maxTokens,stream:true,messages:[{role:'user',content:prompt}]})}); const raw=await res.text(); let text=''; for(const line of raw.split('\n')){const s=line.trim();if(!s.startsWith('data:'))continue;const d=s.slice(5).trim();if(!d||d==='[DONE]')continue;let e;try{e=JSON.parse(d)}catch{continue}if(e.type==='content_block_delta'&&e.delta?.type==='text_delta')text+=e.delta.text||''} return text.trim(); diff --git a/.github/workflows/qa-feature-review.yml b/.github/workflows/qa-feature-review.yml index 59f3b38a9..0054a10f9 100644 --- a/.github/workflows/qa-feature-review.yml +++ b/.github/workflows/qa-feature-review.yml @@ -28,6 +28,7 @@ jobs: permissions: contents: read pull-requests: write + issues: write steps: - uses: actions/checkout@v4 with: @@ -95,6 +96,36 @@ jobs: rm -f /tmp/feature-render.png node feature-review.mjs + # feature-review.mjs is advisory and always exits 0, so a tool outage + # (dead proxy, CDP gone, GH API error) looks exactly like a healthy run: + # green job, no comment, nobody notified. Mirror the Agent QA Review + # monitor so those failures land on the one tracking issue instead. + - name: Log feature review failure to the monitor issue + if: always() + uses: actions/github-script@v7 + env: + RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + with: + script: | + const fs = require('fs'); + if (!fs.existsSync('FEATURE_REVIEW_FAILED')) { core.info('Feature QA Review OK; nothing to log.'); return; } + const reason = fs.readFileSync('FEATURE_REVIEW_FAILED', 'utf8').trim(); + const { owner, repo } = context.repo; + const title = 'CI: AI / Agent review tool failures'; + // Find the ONE monitor issue whether open OR closed, so we never create duplicates. + const sr = await github.rest.search.issuesAndPullRequests({ q: `repo:${owner}/${repo} is:issue in:title "${title}"`, per_page: 10 }); + let issue = (sr.data.items || []).find(i => i.title === title); + if (!issue) { + const res = await github.rest.issues.create({ owner, repo, title, body: 'Automated log of AI Code Review / Agent QA Review TOOL failures (upstream / CDP / stream errors, not code regressions). Each comment is one failed run and @-mentions the maintainer; the PR check stays green. Close this issue when handled; a later failure reopens it.' }); + issue = res.data; + } else if (issue.state === 'closed') { + await github.rest.issues.update({ owner, repo, issue_number: issue.number, state: 'open' }); + } + const cc = '\n\ncc @sanrai'; // mention on every failure -> a notification each time + const body = ['\u274c **Feature QA Review** could not run \u2014 ' + new Date().toISOString(), '', '- ' + reason.split('\n')[0], '- Run: ' + process.env.RUN_URL, cc].join('\n'); + await github.rest.issues.createComment({ owner, repo, issue_number: issue.number, body }); + core.info('Logged Feature QA Review failure on issue #' + issue.number); + - name: Upload render screenshot if: always() run: |