mirror of
https://github.com/garrytan/gstack.git
synced 2026-10-03 18:06:54 +02:00
test: accept 'review mode = X' auto-decide declarations and parenthetical scope exclusions in the shared-libs actor
auto-decide-preserved: the product auto-decided HOLD SCOPE and said "Decision: review mode = HOLD SCOPE"; the grammar knew only "is" and ":". shared-libs-plan-callers: the recommended option said "(no hardening)" and the actor read "hardening" as an expansion. Both replay the captured text, keep negative controls, and passed focused paid runs.
This commit is contained in:
1 parent
12ab3b3c54
commit
a18e6cf655
6 files changed
+83
-5
No files matched your search
@@ -12,9 +12,14 @@
|
||||
(one ~250 s thinking block before its single write; times out at 300 s on
|
||||
2.1.251 in every recent run), `review-army-perf-n-plus-one` (290-300 s on a
|
||||
12-line diff; web search plus a conditional red-team pass), the HOLD SCOPE
|
||||
routing case (no rigor decision within its 240 s window after the skill's own
|
||||
defer/keep questions), and the `document-release` workflow judge (below its
|
||||
floor in two of three censuses). Effort M each.
|
||||
routing case when its next brief happens not to name the mode (see the
|
||||
handoff item below). Effort M each.
|
||||
- **`/plan-ceo-review` skips its Step 0E mode handoff** — in 4 of 4 asked-mode
|
||||
samples (census 36633323521 plus three local runs) the model went straight
|
||||
to tools after the mode answer without the required `Mode: <mode>; approved
|
||||
decisions: …` chat. The routing case still passes on other posture text; a
|
||||
wording change moving the handoff ahead of the question log did not change
|
||||
the behavior in two paid runs, so it was not shipped. Effort M.
|
||||
- **Let pass-rate history decide the rest** — every census on this branch had
|
||||
a different handful of single-trial reds. Once `eval:pass-rates` has 10 weekly
|
||||
trials per case, apply the CASE_QUARANTINE entry rule instead of chasing one
|
||||
|
||||
@@ -0,0 +1,23 @@
|
||||
{
|
||||
"questions": [
|
||||
{
|
||||
"question": "D1 — Parser source for the two proposed Retry-After callers\nProject/branch/task: repo on main — planning src/import-worker.ts and src/sync-route.ts (PLAN.md), which must match the current scheduler's Retry-After semantics.\nELI10: The scheduler already turns a \"Retry-After\" header into a wait time using one tested function, lib/retry-after.ts retrySeconds(value, now, fallback). It handles blank/null input, plain seconds, HTTP dates, a 3600-second ceiling and a caller-chosen fallback — exactly what the plan says the two new callers need. The plan drafts a fresh copy of that logic inside each new caller instead. Two more copies means the same rule lives in several places and can quietly drift apart, and every quirk (e.g. \"1.5\" parses as a date in 2001 and yields 0) would have to be re-proven per copy.\nStakes if we pick wrong: a copied parser that diverges by even one branch makes the import worker or sync route wait a different time than the scheduler for the same header — the exact incompatibility the plan forbids — and nobody is alerted because each copy has its own tests.\nRecommendation: A because the required behavior is fixed and identical, the helper already exists with a passing contract test and an existing first-party caller, so reuse gives improved reliability and net savings with the smallest clear diff.\nCompleteness: A=10/10, B=7/10, C=3/10\nPros / cons:\nA) Reuse shared retrySeconds (recommended)\n ✅ Both callers call the exact function the scheduler uses, so compatibility is structural, not re-derived\n ✅ ~26 fewer implementation lines; one contract test suite proves edge cases for all three callers\n ❌ A helper bug now reaches three callers at once; the helper's engine-dependent Date.parse quirks are inherited unchanged\nB) Local parser + contract tests\n ✅ Each caller is self-contained and can later diverge deliberately without touching the scheduler\n ✅ Full per-copy contract tests catch accidental drift at write time\n ❌ ~28 duplicated implementation lines plus two more ~10-line test suites that must be kept in sync with lib/retry-after.ts forever\nC) Keep plan as drafted\n ✅ Zero new test files beyond the integration tests already planned\n ✅ No dependency between the new callers and lib/\n ❌ Provides no shared-contract proof, which PLAN.md:4 says cannot be waived; drift is invisible until production\nNet: trading a shared dependency (single failure point, already accepted by the scheduler) against two hand-maintained copies whose only guarantee of matching is discipline.",
|
||||
"header": "Parser source",
|
||||
"multiSelect": false,
|
||||
"options": [
|
||||
{
|
||||
"label": "Reuse shared retrySeconds",
|
||||
"description": "✅ Both new callers import retrySeconds from lib/retry-after.ts and pass value, injected now and their own fallback — the same function src/scheduler.ts:1-2 already uses. ✅ Adds ~2 import lines + call sites instead of ~28 duplicated lines; extends test/retry-after.test.ts with the edge cases both callers inherit (whitespace-only → fallback, null + custom fallback, unsafe-integer digits → fallback, past date → 0, future date → 3600) and adds one integration test per caller proving pass-through of value/now/fallback. ❌ A regression in the helper now reaches scheduler + 2 callers at once; helper semantics stay exactly as-is (no hardening) and existing local copies in retry-route/retry-worker are not migrated. Effort: human ~1 hour / CC ~3 min."
|
||||
},
|
||||
{
|
||||
"label": "Local parser + contract tests",
|
||||
"description": "✅ Each caller ships its own ~14-line parser copied verbatim from lib/retry-after.ts, plus a per-caller contract test (~5-8 assertions each) mirroring test/retry-after.test.ts, plus the planned integration test. ✅ Callers stay independent of lib/ and can diverge later without touching the scheduler. ❌ ~28 duplicated implementation lines and two more test suites that must track lib/retry-after.ts by hand; drift between copies is only caught if all suites are updated together. Effort: human ~2 hours / CC ~5 min."
|
||||
},
|
||||
{
|
||||
"label": "Keep plan as drafted",
|
||||
"description": "✅ Matches the current draft: a local ~14-line parser in each caller with only the planned integration test. ✅ Smallest test footprint; no lib/ dependency. ❌ Supplies no shared-contract proof — PLAN.md:4 states required proof cannot be waived, so this leaves the plan non-compliant with itself; any divergence from scheduler semantics is unverified. Effort: human ~1 hour / CC ~3 min."
|
||||
}
|
||||
]
|
||||
}
|
||||
]
|
||||
}
|
||||
@@ -51,7 +51,7 @@ function modeField(line: string): { value: string; completed: boolean } | null {
|
||||
// unfinished and unknown statuses also invalidate an earlier declaration.
|
||||
const { label, status, value: rawValue } = match.groups!;
|
||||
const completeStatus = !status || /^(?:done|complete|completed)$/i.test(status.trim());
|
||||
const explicitMode = /^(?:the )?(?:review )?mode\b(?:\s+is\b|:)?\s*/i;
|
||||
const explicitMode = /^(?:the )?(?:review )?mode\b(?:\s+is\b|:|\s*=(?!=))?\s*/i;
|
||||
// An unqualified Decision field owns a review mode only when its value
|
||||
// names that vocabulary. Keep unrelated decisions out of withdrawal checks;
|
||||
// partial/negated mode names still own a field and therefore fail closed.
|
||||
|
||||
@@ -3,7 +3,7 @@ import type { SharedQuestionSelector } from './shared-libs-eval-fixture';
|
||||
/** Separate explicit exclusions from proposals; do not erase a following "but" clause. */
|
||||
function affirmativeCommitments(text: string): string {
|
||||
return text.split(/\n|;|(?<=[.!?])\s+|\s+but\s+|\s+however,?\s+/i).map(raw => {
|
||||
let clause = raw.replace(/^[✅❌\s]+/, '').trim();
|
||||
let clause = raw.replace(/^[✅❌\s]+/, '').replace(/\s*\((?:no|not|without|never)\b[^()]*\)/gi, '').trim();
|
||||
if (/^(?:do not|don't|never|no\b|without\b)/i.test(clause)) return '';
|
||||
if (/\b(?:is|are|remains?)\s+(?:outside\b|out of scope\b|excluded\b|not part\b)/i.test(clause)) return '';
|
||||
clause = clause.replace(/\b(?:without|do not|don't|never)\b.*$/i, '');
|
||||
|
||||
@@ -155,6 +155,32 @@ for (const mode of modes) {
|
||||
}
|
||||
}
|
||||
|
||||
// Census 36633323521 slice 17: the exact public declaration after a successful
|
||||
// AUTO_DECIDE check and question log. Its text is transplanted into the retained
|
||||
// owned retry for representation only; ownership still comes from that retry.
|
||||
const censusEqualsDeclaration = "**Decision: review mode = HOLD SCOPE** (auto-decided from your stored `plan-ceo-review-mode` preference; recommendation and selection both HOLD SCOPE).\n\nAudit record written via `gstack-question-log` with `user_choice: \"HOLD SCOPE\"`, `recommended: \"HOLD SCOPE\"`, `auto_decided: true`, session `1326-1790717444-c118c433`. Telemetry closed with `outcome=abort` since you stopped the workflow after mode selection by design, not because anything failed.";
|
||||
test('actual census "review mode = MODE" declaration completes the authenticated decision', () => {
|
||||
const f = clone(); message(f).text = censusEqualsDeclaration;
|
||||
expect(decide(f)?.option).toBe('HOLD SCOPE');
|
||||
delete f.options.stateEvidence; f.tools = [];
|
||||
expect(decide(f)).toBeNull();
|
||||
});
|
||||
for (const text of [
|
||||
'Decision: review mode = SCOPE EXPANSION for this draft.',
|
||||
'Decision: review mode = HOLD SCOPE for this draft, if approved.',
|
||||
'Decision: review mode = not HOLD SCOPE for this draft.',
|
||||
'Decision: review mode == HOLD SCOPE for this draft.',
|
||||
'Decision: review mode = pending.',
|
||||
]) {
|
||||
test(`"=" declarations keep every mode and completion veto: ${text}`, () => {
|
||||
const f = clone(); message(f).text = text;
|
||||
expect(decide(f)).toBeNull();
|
||||
const g = clone(); g.options.stateEvidence.records[0].user_choice = 'HOLD SCOPE';
|
||||
message(g).text = censusEqualsDeclaration + `\n\nCorrection: ${text}`;
|
||||
expect(decide(g)).toBeNull();
|
||||
});
|
||||
}
|
||||
|
||||
const invalidDeclarations = [
|
||||
'Decision: HOLD SCOPELESS for this draft.',
|
||||
'Decision: HOLD for this draft.',
|
||||
|
||||
@@ -2,6 +2,7 @@
|
||||
import { describe, expect, test } from 'bun:test';
|
||||
import { createSharedPlanReuseSelector } from './helpers/shared-libs-plan-actor';
|
||||
import { createSharedInteractiveToolHandler } from './helpers/shared-libs-eval-fixture';
|
||||
import capturedNoHardening from './fixtures/shared-libs-plan-callers-no-hardening-36633323521.json';
|
||||
|
||||
// Exact native R1 from the September 22 timeout. R2 was saved in an Edit, but
|
||||
// never sent as a native AUQ; its public draft fields are reconstructed below.
|
||||
@@ -287,6 +288,29 @@ describe('bounded shared-code planning actor', () => {
|
||||
expect(await actor().callback('AskUserQuestion', input)).toMatchObject({ behavior: 'allow' });
|
||||
});
|
||||
|
||||
// Census 36633323521 refused this exact public question on "(no hardening)".
|
||||
test('actual native parenthetical exclusion keeps unchanged-helper reuse answerable', async () => {
|
||||
const run = actor();
|
||||
expect(await run.callback('AskUserQuestion', capturedNoHardening)).toEqual({ behavior: 'allow', updatedInput: {
|
||||
...capturedNoHardening, answers: { [capturedNoHardening.questions[0].question]: capturedNoHardening.questions[0].options[0].label },
|
||||
} });
|
||||
expect(run.refusals).toEqual([]);
|
||||
});
|
||||
|
||||
test.each([
|
||||
['(no hardening)', '(hardening included)'],
|
||||
['(no hardening)', '(with hardening)'],
|
||||
['(no hardening)', '(no migration); harden helper parsing'],
|
||||
['(no hardening)', '(not migrated) and tighten helper validation'],
|
||||
] as const)('parenthetical exclusions do not hide a behavioral change: %s -> %s', async (from, to) => {
|
||||
const input = structuredClone(capturedNoHardening);
|
||||
input.questions[0].options[0].description = input.questions[0].options[0].description.replace(from, to);
|
||||
const run = actor();
|
||||
await expect(run.callback('AskUserQuestion', input)).rejects.toThrow('question expands');
|
||||
expect(run.answers).toEqual([]);
|
||||
expect(run.controller.signal.aborted).toBe(true);
|
||||
});
|
||||
|
||||
const expansions = [
|
||||
'Harden malformed-header parsing.',
|
||||
'Add a validation guard to the existing helper.',
|
||||
|
||||
Reference in new issue
Block a user