Skip to content

Commit b843e39

Browse files
akshay-vizCopilot
andcommitted
fix(model-apps): never execute a mutating probe as a read
Caught by Copilot in review. `executeProbes` called `readOne` for every planned probe without consulting `probe.mutating`, and `readOne` is the only operation it performs. So with `--allow-mutations`, a `write`/`create`/`delete` probe was exercised as a GET, the 200 came back, and `interpretOutcome` reported PASS. That is a false pass on the privilege that matters most, on the exact path a maker opts into specifically to test it -- and it contradicted what the script header, AGENTS.md and the CHANGELOG all already claimed, which is that `--allow-mutations` PLANS mutating probes without executing them. The docs were right; the code was wrong. Mutating probes now short-circuit before any IO and are reported `inconclusive` with the reason, so they can never be counted as passes and never add round trips. `planProbes` still includes them, because showing what WOULD be exercised is the point of the flag; actually exercising them needs fixture creation and cleanup, which is a separate design. Three regression tests, red-green verified: disabling the guard fails all three, including one asserting an all-mutating run reports zero passes so it cannot look like success. 34 probe tests, 1514 total. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 42626da2-b66f-4162-acaa-b1127ef23d89
1 parent 3b7cb28 commit b843e39

2 files changed

Lines changed: 56 additions & 0 deletions

File tree

plugins/model-apps/scripts/lib/persona-probe.js

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -212,6 +212,21 @@ async function executeProbes(probes, io) {
212212
const principalCache = new Map();
213213

214214
for (const probe of probes) {
215+
// A mutating privilege cannot be proven by a read, and `readOne` is the only operation this
216+
// executor performs. Running one anyway would report `write`/`create`/`delete` as PASS on the
217+
// strength of a successful GET — a false pass on the privilege that matters most, and on the
218+
// exact path a maker opts into with --allow-mutations. `planProbes` includes them so the report
219+
// shows what WOULD be exercised; executing them needs fixture creation and cleanup, which is a
220+
// separate design. Until then they are reported as proved-nothing, never as passes.
221+
if (probe.mutating) {
222+
findings.push({
223+
probe,
224+
result: 'inconclusive',
225+
detail: `planned only — a ${probe.access} privilege cannot be proven by a read, and mutating probes are not executed`,
226+
});
227+
continue;
228+
}
229+
215230
if (!principalCache.has(probe.persona)) {
216231
principalCache.set(probe.persona, io.principalFor(probe.persona));
217232
}

plugins/model-apps/scripts/tests/persona-probe.test.js

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -322,3 +322,44 @@ test('principal resolver returns null when the persona declares no test user', a
322322
assert.strictEqual(resolve('Tech'), null);
323323
assert.strictEqual(resolve('Nobody'), null);
324324
});
325+
326+
test('a mutating probe is NEVER executed as a read', async () => {
327+
// Regression guard for a real review finding: executeProbes called readOne for every planned
328+
// probe, so with --allow-mutations a `write` probe was exercised as a GET and a 200 was reported
329+
// as PASS — a false pass on the privilege the maker specifically opted in to test.
330+
const calls = [];
331+
const probes = [
332+
{ persona: 'P', entity: 'co_workorder', access: 'write', expect: 'allow', mutating: true },
333+
{ persona: 'P', entity: 'co_workorder', access: 'read', expect: 'allow', mutating: false },
334+
];
335+
const f = await executeProbes(probes, io({ readOne: async (set) => { calls.push(set); return { status: 200, rowCount: 1 }; } }));
336+
337+
assert.strictEqual(calls.length, 1, 'only the read probe may reach the wire');
338+
assert.strictEqual(f[0].result, 'inconclusive', 'the write probe must not be a pass');
339+
assert.match(f[0].detail, /cannot be proven by a read/);
340+
assert.strictEqual(f[1].result, 'pass');
341+
});
342+
343+
test('a mutating probe does not even resolve a principal or entity set', async () => {
344+
// It short-circuits before any IO, so --allow-mutations cannot add round trips either.
345+
let touched = 0;
346+
const probes = [{ persona: 'P', entity: 'e', access: 'delete', expect: 'allow', mutating: true }];
347+
const f = await executeProbes(probes, io({
348+
principalFor: () => { touched++; return { header: 'MSCRMCallerID', value: 'u' }; },
349+
entitySetName: async () => { touched++; return 'es'; },
350+
readOne: async () => { touched++; return { status: 200, rowCount: 1 }; },
351+
}));
352+
assert.strictEqual(touched, 0, 'no IO for a probe that will not be executed');
353+
assert.strictEqual(f[0].result, 'inconclusive');
354+
});
355+
356+
test('summarize: an all-mutating run reports zero passes, not success', async () => {
357+
const probes = [
358+
{ persona: 'P', entity: 'a', access: 'create', expect: 'allow', mutating: true },
359+
{ persona: 'P', entity: 'b', access: 'delete', expect: 'allow', mutating: true },
360+
];
361+
const s = summarize(await executeProbes(probes, io()));
362+
assert.strictEqual(s.counts.pass, 0);
363+
assert.strictEqual(s.counts.inconclusive, 2);
364+
assert.strictEqual(s.ok, true, 'still not a failure — but the caller can see nothing was proven');
365+
});

0 commit comments

Comments
 (0)