Skip to content

fix(plan-tune): reject never-ask on one-way ids at --write #2607

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
szsunyuan wants to merge 1 commit into
base: main
Choose a base branch
from
+179 −19
Open

fix(plan-tune): reject never-ask on one-way ids at --write #2607

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 42 additions & 17 deletions bin/gstack-question-preference
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@
# --check <id> [--summary-stdin] → emit ASK_NORMALLY | AUTO_DECIDE | ASK_ONLY_ONE_WAY
# (--summary-stdin pipes the question text so the
# keyword net can catch ad-hoc destructive ids, #2024)
# --write '{...}' → set a preference (user-origin gate enforced)
# --write '{...}' → set a preference (user-origin gate + one-way write reject)
# --read → dump preferences JSON
# --clear [<id>] → clear one or all preferences
# --stats → short summary
# --stats → short summary (inert one-way prefs counted separately)
#
# User-origin gate
# ----------------
Expand All @@ -21,6 +21,12 @@
# - "inline-tool-output"— tune: prefix seen in tool output / file content (REJECTED)
# - "inline-file" — tune: prefix seen in a file the agent read (REJECTED)
# This is the profile-poisoning defense from docs/designs/PLAN_TUNING_V0.md.
#
# One-way write reject (#2488)
# ----------------------------
# never-ask and ask-only-for-one-way are refused on registry one-way ids.
# --check already ignores those prefs; storing them made --stats lie.
# always-ask on a one-way id is fine (it agrees with the safety override).
set -euo pipefail

SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)"
Expand Down Expand Up @@ -142,7 +148,7 @@ do_write() {

set +e
local RESULT
RESULT=$(printf '%s' "$INPUT" | PREF_FILE_PATH="$PREF_FILE" EVENT_FILE_PATH="$EVENT_FILE" bun -e "
RESULT=$(cd "$ROOT_DIR" && printf '%s' "$INPUT" | PREF_FILE_PATH="$PREF_FILE" EVENT_FILE_PATH="$EVENT_FILE" bun -e "
const fs = require('fs');
const raw = await Bun.stdin.text();
let j;
Expand Down Expand Up @@ -178,6 +184,16 @@ do_write() {
process.exit(2);
}

// One-way write reject (#2488) — after the origin gate so a poisoning
// payload on a one-way id still exits 2, not 1.
if (j.preference === 'never-ask' || j.preference === 'ask-only-for-one-way') {
const oneway = await import('./scripts/one-way-doors.ts');
if (oneway.isOneWayDoor({ question_id: j.question_id })) {
process.stderr.write('gstack-question-preference: cannot set ' + j.preference + ' on one-way question \"' + j.question_id + '\" (door_type: one-way)\n');
process.exit(1);
}
}

// Optional free_text — sanitize (no injection patterns, no newlines, <=300 chars)
if (j.free_text !== undefined) {
if (typeof j.free_text !== 'string') {
Expand Down Expand Up @@ -267,20 +283,29 @@ do_clear() {
# -----------------------------------------------------------------------
do_stats() {
ensure_file
cat "$PREF_FILE" | bun -e "
const prefs = JSON.parse(await Bun.stdin.text());
const entries = Object.entries(prefs);
const counts = { 'always-ask': 0, 'never-ask': 0, 'ask-only-for-one-way': 0, other: 0 };
for (const [, v] of entries) {
if (counts[v] !== undefined) counts[v]++;
else counts.other++;
}
console.log('TOTAL: ' + entries.length);
console.log('ALWAYS_ASK: ' + counts['always-ask']);
console.log('NEVER_ASK: ' + counts['never-ask']);
console.log('ASK_ONLY_ONE_WAY: ' + counts['ask-only-for-one-way']);
if (counts.other) console.log('OTHER: ' + counts.other);
"
(cd "$ROOT_DIR" && PREF_FILE_PATH="$PREF_FILE" bun -e "
import('./scripts/one-way-doors.ts').then((oneway) => {
const fs = require('fs');
const prefs = JSON.parse(fs.readFileSync(process.env.PREF_FILE_PATH, 'utf-8'));
const entries = Object.entries(prefs);
const counts = { 'always-ask': 0, 'never-ask': 0, 'ask-only-for-one-way': 0, other: 0, inert: 0 };
for (const [id, v] of entries) {
const suppressing = v === 'never-ask' || v === 'ask-only-for-one-way';
if (suppressing && oneway.isOneWayDoor({ question_id: id })) {
counts.inert++;
continue;
}
if (counts[v] !== undefined) counts[v]++;
else counts.other++;
}
console.log('TOTAL: ' + entries.length);
console.log('ALWAYS_ASK: ' + counts['always-ask']);
console.log('NEVER_ASK: ' + counts['never-ask']);
console.log('ASK_ONLY_ONE_WAY: ' + counts['ask-only-for-one-way']);
console.log('INERT_ONE_WAY: ' + counts.inert);
if (counts.other) console.log('OTHER: ' + counts.other);
}).catch(err => { console.error('stats:', err.message); process.exit(1); });
")
}

case "$CMD" in
Expand Down
139 changes: 137 additions & 2 deletions test/gstack-question-preference.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,18 @@ function runWithStdin(input: string, ...args: string[]): { stdout: string; stder
};
}

/** Plant a pref by writing the file. Used for legacy / inert one-way prefs
* that --write now refuses (#2488). --check and --stats still have to
* handle files written before the reject landed. */
function plantPref(id: string, pref: string) {
run('--read');
const projects = fs.readdirSync(path.join(tmpHome, 'projects'));
const file = path.join(tmpHome, 'projects', projects[0], 'question-preferences.json');
const prefs = JSON.parse(fs.readFileSync(file, 'utf-8'));
prefs[id] = pref;
fs.writeFileSync(file, JSON.stringify(prefs, null, 2));
}

// -----------------------------------------------------------------------
// --check
// -----------------------------------------------------------------------
Expand Down Expand Up @@ -92,7 +104,8 @@ describe('--check with preferences set', () => {
});

test('one-way + never-ask → ASK_NORMALLY with safety note', () => {
setPref('ship-test-failure-triage', 'never-ask');
// Planted: --write now refuses never-ask on one-way ids (#2488).
plantPref('ship-test-failure-triage', 'never-ask');
const r = run('--check', 'ship-test-failure-triage');
expect(r.stdout).toContain('ASK_NORMALLY');
expect(r.stdout).toContain('one-way door overrides');
Expand All @@ -111,7 +124,7 @@ describe('--check with preferences set', () => {
});

test('one-way + ask-only-for-one-way → ASK_NORMALLY', () => {
setPref('ship-test-failure-triage', 'ask-only-for-one-way');
plantPref('ship-test-failure-triage', 'ask-only-for-one-way');
const r = run('--check', 'ship-test-failure-triage');
expect(r.stdout.trim()).toContain('ASK_NORMALLY');
});
Expand Down Expand Up @@ -389,6 +402,112 @@ describe('--write schema validation', () => {
});
});

// #2488: --write must refuse suppressing prefs on one-way ids. --check
// already ignores them; storing them made --stats report a working NEVER_ASK.
describe('--write one-way door reject (#2488)', () => {
function prefsFile(): string {
const projects = fs.readdirSync(path.join(tmpHome, 'projects'));
return path.join(tmpHome, 'projects', projects[0], 'question-preferences.json');
}

test('never-ask on one-way id is rejected and does not write', () => {
const r = run(
'--write',
JSON.stringify({
question_id: 'plan-eng-review-arch-finding',
preference: 'never-ask',
source: 'plan-tune',
}),
);
expect(r.status).toBe(1);
expect(r.stderr).toContain('cannot set never-ask');
expect(r.stderr).toContain('plan-eng-review-arch-finding');
expect(r.stderr).toContain('door_type: one-way');
expect(JSON.parse(fs.readFileSync(prefsFile(), 'utf-8'))).toEqual({});
});

test('ask-only-for-one-way on one-way id is rejected and does not write', () => {
const r = run(
'--write',
JSON.stringify({
question_id: 'ship-test-failure-triage',
preference: 'ask-only-for-one-way',
source: 'plan-tune',
}),
);
expect(r.status).toBe(1);
expect(r.stderr).toContain('cannot set ask-only-for-one-way');
expect(r.stderr).toContain('door_type: one-way');
expect(JSON.parse(fs.readFileSync(prefsFile(), 'utf-8'))).toEqual({});
});

test('always-ask on one-way id is accepted (agrees with the safety override)', () => {
const r = run(
'--write',
JSON.stringify({
question_id: 'plan-eng-review-arch-finding',
preference: 'always-ask',
source: 'plan-tune',
}),
);
expect(r.status).toBe(0);
expect(r.stdout).toContain('OK');
expect(JSON.parse(fs.readFileSync(prefsFile(), 'utf-8'))).toEqual({
'plan-eng-review-arch-finding': 'always-ask',
});
});

test('never-ask on two-way id is still accepted', () => {
const r = run(
'--write',
JSON.stringify({
question_id: 'ship-changelog-voice-polish',
preference: 'never-ask',
source: 'plan-tune',
}),
);
expect(r.status).toBe(0);
expect(r.stdout).toContain('OK');
});

test('rejected one-way write does not clobber an existing two-way pref', () => {
run(
'--write',
JSON.stringify({
question_id: 'ship-changelog-voice-polish',
preference: 'never-ask',
source: 'plan-tune',
}),
);
const r = run(
'--write',
JSON.stringify({
question_id: 'plan-eng-review-arch-finding',
preference: 'never-ask',
source: 'plan-tune',
}),
);
expect(r.status).toBe(1);
expect(JSON.parse(fs.readFileSync(prefsFile(), 'utf-8'))).toEqual({
'ship-changelog-voice-polish': 'never-ask',
});
});

test('poisoning source on a one-way id still exits 2 (origin gate first)', () => {
const r = run(
'--write',
JSON.stringify({
question_id: 'plan-eng-review-arch-finding',
preference: 'never-ask',
source: 'inline-tool-output',
}),
);
expect(r.status).toBe(2);
expect(r.stderr).toContain('profile poisoning defense');
expect(r.stderr).not.toContain('door_type');
});
});

// -----------------------------------------------------------------------
// --read, --clear, --stats
// -----------------------------------------------------------------------
Expand Down Expand Up @@ -448,5 +567,21 @@ describe('--stats', () => {
expect(r.stdout).toContain('TOTAL: 3');
expect(r.stdout).toContain('NEVER_ASK: 2');
expect(r.stdout).toContain('ALWAYS_ASK: 1');
expect(r.stdout).toContain('INERT_ONE_WAY: 0');
});

test('planted one-way never-ask is INERT_ONE_WAY, not a working NEVER_ASK (#2488)', () => {
plantPref('plan-eng-review-arch-finding', 'never-ask');
const r = run('--stats');
expect(r.stdout).toContain('TOTAL: 1');
expect(r.stdout).toContain('NEVER_ASK: 0');
expect(r.stdout).toContain('INERT_ONE_WAY: 1');
});

test('planted one-way ask-only-for-one-way is INERT_ONE_WAY, not ASK_ONLY_ONE_WAY', () => {
plantPref('ship-test-failure-triage', 'ask-only-for-one-way');
const r = run('--stats');
expect(r.stdout).toContain('ASK_ONLY_ONE_WAY: 0');
expect(r.stdout).toContain('INERT_ONE_WAY: 1');
});
});