mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-08 09:48:20 +01:00
385 lines
20 KiB
TypeScript
385 lines
20 KiB
TypeScript
/**
|
|
* /plan-ceo-review per-finding AskUserQuestion count (periodic, paid, real-PTY).
|
|
*
|
|
* Asserts the load-bearing rule "One issue = one AskUserQuestion call" by
|
|
* driving /plan-ceo-review against a 5-finding seeded plan and counting
|
|
* distinct review-phase AUQs. Passes when count is in [N-1, N+2].
|
|
*
|
|
* Two tests in this file:
|
|
* - 5-finding distinct fixture: count band assertion + D19 review-report-at-bottom.
|
|
* - 2-finding paired control (D12 positive control): related findings still
|
|
* produce 2 distinct AUQs, not 1 batched, when the rule is honored.
|
|
*
|
|
* Tier: periodic. Each run drives Step 0 + 11 review sections end-to-end
|
|
* (~25 min, ~$5/run). Sequential by default per plan §D15. See
|
|
* test/helpers/claude-pty-runner.ts for runPlanSkillCounting internals.
|
|
*/
|
|
|
|
import { test } from 'bun:test';
|
|
import { describeE2ETier } from './helpers/e2e-gate';
|
|
import { isCeoCompletionHandoff } from './helpers/ceo-completion-handoff';
|
|
import { pickCeoCountQuestion } from './helpers/ceo-approach-pick';
|
|
import * as fs from 'node:fs';
|
|
import * as os from 'node:os';
|
|
import * as path from 'node:path';
|
|
import {
|
|
runPlanSkillCounting,
|
|
ceoStep0Boundary,
|
|
ceoFirstReviewAUQ,
|
|
assertReviewReportAtBottom,
|
|
type AskUserQuestionFingerprint,
|
|
} from './helpers/claude-pty-runner';
|
|
|
|
/**
|
|
* /plan-ceo-review's first AUQ asks "what scope?" with options like
|
|
* 1. Branch diff vs main
|
|
* 2. A specific plan file or design doc
|
|
* 3. An idea you'll describe inline
|
|
* ...
|
|
* 7. Skip interview and plan immediately
|
|
*
|
|
* The default pick (1) routes to "branch diff vs main" — the wrong target
|
|
* for our seeded fixture (the agent would review the gstack PR itself,
|
|
* recursively). Picking "Skip interview and plan immediately" bypasses
|
|
* Step 0 and routes the agent to review the fixture request, which is
|
|
* already present in its initial project context.
|
|
*/
|
|
function pickSkipInterview(fp: AskUserQuestionFingerprint): number {
|
|
const skipOpt = fp.options.find((o) =>
|
|
/skip\s+interview|plan\s+immediately/i.test(o.label),
|
|
);
|
|
if (skipOpt) return skipOpt.index;
|
|
// Fallback: "describe inline" also routes to using our pasted plan.
|
|
const inlineOpt = fp.options.find((o) =>
|
|
/describe.*inline|inline.*idea/i.test(o.label),
|
|
);
|
|
if (inlineOpt) return inlineOpt.index;
|
|
return 1;
|
|
}
|
|
|
|
const describeE2E = describeE2ETier('periodic');
|
|
|
|
const N_DISTINCT = 5;
|
|
const FLOOR_DISTINCT = N_DISTINCT - 1; // 4 (D11)
|
|
const CEILING_DISTINCT = N_DISTINCT + 2; // 7 (D11)
|
|
|
|
const N_PAIRED = 2;
|
|
const FLOOR_PAIRED = 2;
|
|
const CEILING_PAIRED = 4;
|
|
|
|
// Keep the five seeded defects distinct from already-satisfied surrounding
|
|
// contracts. Live controls correctly found extra ingress, missing-user,
|
|
// observability, and rollout gaps when those baseline facts were unspecified.
|
|
const planCeo5Findings = (planPath: string) => [
|
|
`Please review this plan thoroughly in HOLD SCOPE mode. As you go, write your plan-mode plan to ${planPath} (use Edit/Write to that exact path).`,
|
|
'Proceed directly to the requested CEO review; skip the optional /office-hours prerequisite.',
|
|
'Finish after this CEO review; I will handle subsequent reviews manually.',
|
|
'',
|
|
'# Plan: Payment Processing Integration',
|
|
'',
|
|
'## Existing contracts retained',
|
|
'The approved motivation is to move payment orchestration out of the prior',
|
|
'library-adapter handler into application-owned code while retaining the',
|
|
'existing payment and receipt product behavior. The shared dispatcher remains',
|
|
'available; the proposed bypass below is still an architectural choice to review.',
|
|
'The existing ingress middleware verifies the Stripe signature against the',
|
|
'raw request body and rejects invalid signatures before invoking handlers.',
|
|
'The existing ingress forwards only `payment_intent.succeeded` events to',
|
|
'this handler; other Stripe event types are acknowledged without invoking it.',
|
|
'The existing payload adapter exposes `event.data.object.metadata.user_id`',
|
|
'as `request.params.userId`. This params object is the parsed body-data map,',
|
|
'not URL query/path parameters; all users share one webhook URL.',
|
|
'The adapter acknowledges missing, nil, or empty user_id metadata with',
|
|
'HTTP 200 and an event-correlated warning before invoking this handler.',
|
|
'For every nonempty external string it performs no SQL-format validation.',
|
|
'The adapter forwards that external string unchanged. It does not cast,',
|
|
'escape, or SQL-sanitize it; a valid signature does not make it safe for SQL.',
|
|
'User IDs are opaque TEXT values, including punctuation and Unicode. The',
|
|
'lookup has no integer/UUID cast or ID-format restriction; every nonempty',
|
|
'string is a valid identifier representation.',
|
|
'An existing ingress ownership guard checks the PaymentIntent ID against',
|
|
'its stored opaque user-ID binding before invoking the handler. A mismatch',
|
|
'is acknowledged with HTTP 200 and an event-correlated warning. This is an',
|
|
'identity comparison, not SQL-format validation; the adapter still forwards',
|
|
'the original string unchanged.',
|
|
'The existing webhook event guard deduplicates deliveries by Stripe event ID,',
|
|
'and an existing per-user lock serializes payment updates.',
|
|
'The event guard acquires the existing per-user lock before checking the',
|
|
'committed completion marker, and rechecks after any lock wait. It holds',
|
|
'that lock through the handler and completion bookkeeping; an overlapping',
|
|
'completed duplicate does not invoke the handler.',
|
|
'The new handler runs inside those unchanged guards; this plan does not',
|
|
'replace signature verification, event deduplication, or update locking.',
|
|
'The existing user update assigns payment_status=paid and the payment intent',
|
|
'ID; it does not increment a balance or counter. Repeating the same payment',
|
|
'intent assigns the same values, independently of the event-ID guard.',
|
|
'The existing lookup-result guard acknowledges unknown/deleted users with',
|
|
'HTTP 200, logs the event, and stops before user updates or email fan-out.',
|
|
'The retained recipient-policy helper treats a nil or empty email address as',
|
|
'skipped_missing_address: payment processing continues normally, and no mail',
|
|
'client call is attempted. It persists an event/user/PaymentIntent-correlated',
|
|
'skip record, emits a structured warning, and increments the existing counter.',
|
|
'The existing notification runbook already covers that skip result: correct',
|
|
'the account address, then retry only its recorded notification using the',
|
|
'same PaymentIntent idempotency key. It never replays the payment for this case.',
|
|
'That recipient policy does not catch failures from sends to nonempty addresses;',
|
|
'the shared mail client still rethrows those exceptions to this handler.',
|
|
'Account deletion uses the same per-user lock. The handler holds it from',
|
|
'lookup through update and inline email, so deletion either precedes lookup',
|
|
'(the existing unknown/deleted-user path) or follows the handler; it cannot',
|
|
'remove the user between lookup and update.',
|
|
'The ingress wrapper already logs event IDs, outcomes, and durations, with',
|
|
'alerts for failed webhook processing. Those controls remain in place.',
|
|
'The existing DB and mail clients attach the adapter user ID and event ID',
|
|
'to outcome traces, including update success and email delivery success or',
|
|
'failure. These shared clients rethrow exceptions unchanged; tracing does',
|
|
'not rescue email errors or change the inline email call below.',
|
|
'The shared mail client also publishes its delivery failure rate to the',
|
|
'existing dashboard and tested on-call alert, including caught exceptions.',
|
|
'The existing incident runbook uses the correlated DB and mail outcomes to',
|
|
'distinguish committed payments from failed notifications. It directs on-call',
|
|
'to check provider status and retry only the failed notification through the',
|
|
'existing notification retry procedure, never replay the payment blindly.',
|
|
'DB lookup/update exceptions propagate to that ingress wrapper, which logs',
|
|
'the failure and returns HTTP 500 so Stripe retries the event. The existing',
|
|
'event-ID dedup guard records completion only after the database transaction',
|
|
'commits; failed or rolled-back database attempts remain retryable.',
|
|
'The deployment already has a handler feature flag and a documented, tested',
|
|
'rollback to the prior handler; this change uses that existing rollout path.',
|
|
'That documented manual rollout checklist already requires a staging',
|
|
'payment-event replay for this handler and verification of the user update,',
|
|
'email delivery, and correlated outcome trace before enabling it broadly.',
|
|
'This is manual deployment verification, not automated handler regression',
|
|
'coverage; no new automated tests are planned in the Tests section below.',
|
|
'The existing notification contract sends one payment receipt per PaymentIntent,',
|
|
'including a summary of the user orders. With zero orders it still sends one',
|
|
'receipt with an empty order summary; the order loop is data loading, never',
|
|
'one email or payment update per order. These product semantics are retained.',
|
|
'The shared mail client already derives a provider idempotency key from that',
|
|
'PaymentIntent ID. The provider durably suppresses duplicate successful sends',
|
|
'for the same key across process crashes, webhook retries, and manual retries.',
|
|
'Before rethrowing a failed or timed-out send, that client durably records the',
|
|
'notification attempt for the existing retry procedure. The dashboard and',
|
|
'on-call alert already monitor failed-notification age and backlog after an',
|
|
'outage clears, as well as failure rate; the runbook retries those records.',
|
|
'The existing mail-client deadline is one second, enforced by cancellation',
|
|
'of the provider request with no inline retries. It raises MailTimeout on',
|
|
'expiry. The retained DB/ingress deadlines bound their combined work to two',
|
|
'seconds, leaving headroom inside the existing ten-second webhook deadline.',
|
|
'Neither deadlines nor retry records catch the mail exception for this handler;',
|
|
'the shared client still rethrows it to the inline caller described below.',
|
|
'Every existing event-correlated outcome trace includes the active handler',
|
|
'identity (prior or new), so rollout attribution is already available.',
|
|
'If a separate handler class is retained, its already-approved name is',
|
|
'`Webhooks::StripePaymentWebhookHandler` in the application-owned namespace,',
|
|
'never the Stripe library namespace. This naming choice is settled; whether',
|
|
'to add a separate implementation or reuse WebhookDispatcher remains open.',
|
|
'',
|
|
'## Architecture',
|
|
"We're adding a new `StripePaymentWebhookHandler` class that will handle Stripe webhooks.",
|
|
'This bypasses the existing `WebhookDispatcher` module — we want a clean',
|
|
'namespace separation.',
|
|
'',
|
|
'## Database access',
|
|
'The new endpoint reads `request.params.userId` directly into a raw SQL',
|
|
'fragment for the lookup query.',
|
|
'',
|
|
'## Webhook fan-out',
|
|
'On payment success we update the user record AND fire a notification email.',
|
|
'Both happen inline; no error handling on the email leg.',
|
|
'',
|
|
'## Tests',
|
|
"None planned. We'll rely on the existing integration suite catching regressions.",
|
|
'',
|
|
'## Performance',
|
|
'Each webhook lookup hits the database for the user, then fetches each',
|
|
'order in a loop.',
|
|
].join('\n');
|
|
|
|
const planCeo2PairedFindings = (planPath: string) => [
|
|
`Please review this plan thoroughly in HOLD SCOPE mode. As you go, write your plan-mode plan to ${planPath} (use Edit/Write to that exact path).`,
|
|
'Proceed directly to the requested CEO review; skip the optional /office-hours prerequisite.',
|
|
'Finish after this CEO review; I will handle subsequent reviews manually.',
|
|
'',
|
|
'# Plan: Payment Processing — Test Coverage',
|
|
'',
|
|
'## Existing coverage and test infrastructure retained',
|
|
'This changes unit tests only; processPayment() production behavior stays as-is.',
|
|
'The Stripe adapter suite already covers network timeouts, card declines (402),',
|
|
'rate limits (429), and recovery when an initial 502 is followed by a successful',
|
|
'charge. Receipt-builder failure behavior has its own passing regression tests.',
|
|
'The payment test factory explicitly configures max_retries=1 and exposes the',
|
|
'Stripe mock call history. Its injected virtual sleeper records backoff without',
|
|
'real delays, so an exhausted 502 operation makes exactly two charge attempts.',
|
|
'These existing helpers and regression suites remain in use for this change.',
|
|
'',
|
|
'## Existing behavior retained',
|
|
'A successful charge returns a receipt with chargeId copied from Stripe,',
|
|
'amountCents equal to the requested integer amount, and currency equal to',
|
|
'the requested currency. For a 1000-cent USD charge returning id ch_paid,',
|
|
'the receipt is { chargeId: "ch_paid", amountCents: 1000, currency: "USD" }.',
|
|
'On repeated 502 responses, max_retries=1 means two total charge attempts',
|
|
'separated by one recorded 100 ms backoff, followed by PaymentUnavailable.',
|
|
'These contracts are already implemented; this plan adds their unit coverage.',
|
|
'',
|
|
'## Proposed tests',
|
|
'Add two tests in the existing processPayment suite using its current factory,',
|
|
'Stripe mock and virtual sleeper. Other tests and production code stay as-is.',
|
|
'',
|
|
'1. Successful charge: arrange the Stripe mock to return id ch_paid, call',
|
|
' processPayment with amountCents=1000 and currency=USD, and assert only',
|
|
' that the returned receipt is truthy. This is the complete planned assertion.',
|
|
'2. Repeated 502: arrange two consecutive Stripe 502 responses, call',
|
|
' processPayment, and assert only that it rejects with PaymentUnavailable.',
|
|
' No assertion about the mock call history or virtual sleeper record',
|
|
' is planned for this test.',
|
|
].join('\n');
|
|
|
|
describeE2E('/plan-ceo-review per-finding AskUserQuestion count (periodic)', () => {
|
|
test(
|
|
`5-finding plan emits ${FLOOR_DISTINCT}-${CEILING_DISTINCT} review-phase AskUserQuestions`,
|
|
async () => {
|
|
// Per-run artifact dir: a hardcoded shared /tmp path collides under
|
|
// --retry, EVALS_JOBS>1, or concurrent worktrees (a sibling's finally-
|
|
// rmSync deletes this run's artifact → spurious D19 failure).
|
|
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-e2e-plan-ceo-'));
|
|
const planPath = path.join(tmpDir, 'gstack-test-plan-ceo.md');
|
|
|
|
try {
|
|
const obs = await runPlanSkillCounting({
|
|
skillName: 'plan-ceo-review',
|
|
slashCommand: '/plan-ceo-review',
|
|
followUpPrompt: planCeo5Findings(planPath),
|
|
expectedPlanPath: planPath,
|
|
isLastStep0AUQ: ceoStep0Boundary,
|
|
isFirstReviewAUQ: ceoFirstReviewAUQ,
|
|
isCompletionHandoffAUQ: isCeoCompletionHandoff,
|
|
pickAUQ: pickCeoCountQuestion,
|
|
reviewCountCeiling: CEILING_DISTINCT + 1, // hard cap above assertion ceiling
|
|
firstAUQPick: pickSkipInterview, // bypass scope-selection, route to review
|
|
timeoutMs: 1_500_000, // 25 min
|
|
env: { QUESTION_TUNING: 'false', EXPLAIN_LEVEL: 'default' },
|
|
});
|
|
|
|
if (!['plan_ready', 'completion_summary', 'ceiling_reached'].includes(obs.outcome)) {
|
|
throw new Error(
|
|
`plan-ceo-review finding-count FAILED: outcome=${obs.outcome}\n` +
|
|
`step0=${obs.step0Count} review=${obs.reviewCount} elapsed=${obs.elapsedMs}ms\n` +
|
|
`fingerprints (last 8):\n` +
|
|
obs.fingerprints
|
|
.slice(-8)
|
|
.map(
|
|
(f, i) =>
|
|
` ${i}. preReview=${f.preReview} sig=${f.signature.slice(0, 12)} prompt="${f.promptSnippet.slice(0, 60)}"`,
|
|
)
|
|
.join('\n') +
|
|
`\n--- evidence (last 3KB) ---\n${obs.evidence}`,
|
|
);
|
|
}
|
|
if (obs.reviewCount < FLOOR_DISTINCT) {
|
|
throw new Error(
|
|
`BAND FAIL (below floor): reviewCount=${obs.reviewCount} < FLOOR=${FLOOR_DISTINCT}.\n` +
|
|
`Likely batching regression — agent collapsed multiple findings into fewer questions.\n` +
|
|
`Fingerprints (review-phase only):\n` +
|
|
obs.fingerprints
|
|
.filter((f) => !f.preReview && !f.administrative)
|
|
.map((f) => ` - "${f.promptSnippet.slice(0, 80)}"`)
|
|
.join('\n'),
|
|
);
|
|
}
|
|
if (obs.reviewCount > CEILING_DISTINCT) {
|
|
throw new Error(
|
|
`BAND FAIL (above ceiling): reviewCount=${obs.reviewCount} > CEILING=${CEILING_DISTINCT}.\n` +
|
|
`Captured observation:\n${JSON.stringify(obs, null, 2)}`,
|
|
);
|
|
}
|
|
|
|
// D19: review report at bottom of plan file.
|
|
if (!fs.existsSync(planPath)) {
|
|
throw new Error(
|
|
`D19 FAIL: agent did not produce expected plan file at ${planPath}.\n` +
|
|
`Either the agent ignored the path instruction in the follow-up prompt, or\n` +
|
|
`the helper exited before the agent wrote the file. ` +
|
|
`outcome=${obs.outcome} review=${obs.reviewCount}`,
|
|
);
|
|
}
|
|
const planContent = fs.readFileSync(planPath, 'utf-8');
|
|
const verdict = assertReviewReportAtBottom(planContent);
|
|
if (!verdict.ok) {
|
|
throw new Error(
|
|
`D19 FAIL: plan file at ${planPath} ${verdict.reason}\n` +
|
|
(verdict.trailingHeadings
|
|
? `Trailing headings: ${verdict.trailingHeadings.join(' | ')}\n`
|
|
: '') +
|
|
`--- plan content (last 1KB) ---\n${planContent.slice(-1024)}`,
|
|
);
|
|
}
|
|
} finally {
|
|
try {
|
|
fs.rmSync(tmpDir, { recursive: true, force: true });
|
|
} catch {
|
|
/* best-effort */
|
|
}
|
|
}
|
|
},
|
|
1_500_000 /* physical ceiling: the 25-min CI job + 1800s shard wall cap what can actually execute */,
|
|
);
|
|
|
|
test(
|
|
`paired-finding positive control: ${N_PAIRED} related findings produce ${FLOOR_PAIRED}-${CEILING_PAIRED} AskUserQuestions`,
|
|
async () => {
|
|
// Per-run artifact dir — see the distinct-findings test above.
|
|
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-e2e-plan-ceo-paired-'));
|
|
const planPath = path.join(tmpDir, 'gstack-test-plan-ceo-paired.md');
|
|
|
|
try {
|
|
const obs = await runPlanSkillCounting({
|
|
skillName: 'plan-ceo-review',
|
|
slashCommand: '/plan-ceo-review',
|
|
followUpPrompt: planCeo2PairedFindings(planPath),
|
|
expectedPlanPath: planPath,
|
|
isLastStep0AUQ: ceoStep0Boundary,
|
|
isFirstReviewAUQ: ceoFirstReviewAUQ,
|
|
isCompletionHandoffAUQ: isCeoCompletionHandoff,
|
|
pickAUQ: pickCeoCountQuestion,
|
|
reviewCountCeiling: CEILING_PAIRED + 1,
|
|
timeoutMs: 1_500_000,
|
|
env: { QUESTION_TUNING: 'false', EXPLAIN_LEVEL: 'default' },
|
|
});
|
|
|
|
if (!['plan_ready', 'completion_summary', 'ceiling_reached'].includes(obs.outcome)) {
|
|
throw new Error(
|
|
`paired-finding control FAILED: outcome=${obs.outcome}\n` +
|
|
`step0=${obs.step0Count} review=${obs.reviewCount}\n` +
|
|
`--- evidence (last 3KB) ---\n${obs.evidence}`,
|
|
);
|
|
}
|
|
if (obs.reviewCount < FLOOR_PAIRED) {
|
|
throw new Error(
|
|
`PAIRED CONTROL FAIL: reviewCount=${obs.reviewCount} < FLOOR=${FLOOR_PAIRED}.\n` +
|
|
`Two deliberately related findings were batched into <2 questions — the rule failed under D12.\n` +
|
|
`Review-phase fingerprints:\n` +
|
|
obs.fingerprints
|
|
.filter((f) => !f.preReview && !f.administrative)
|
|
.map((f) => ` - "${f.promptSnippet.slice(0, 80)}"`)
|
|
.join('\n'),
|
|
);
|
|
}
|
|
if (obs.reviewCount > CEILING_PAIRED) {
|
|
throw new Error(
|
|
`PAIRED CONTROL FAIL: reviewCount=${obs.reviewCount} > CEILING=${CEILING_PAIRED} (over-asking on a 2-finding fixture).\n` +
|
|
`Captured observation:\n${JSON.stringify(obs, null, 2)}`,
|
|
);
|
|
}
|
|
} finally {
|
|
try {
|
|
fs.rmSync(tmpDir, { recursive: true, force: true });
|
|
} catch {
|
|
/* best-effort */
|
|
}
|
|
}
|
|
},
|
|
1_500_000 /* physical ceiling: the 25-min CI job + 1800s shard wall cap what can actually execute */,
|
|
);
|
|
});
|