Skip to content

Commit 258bd01

Browse files
akshay-vizCopilot
andcommitted
fix(model-apps): run role-privileges in the build's own --verify
Found by a LIVE end-to-end run, and invisible to CI. Against the same deployed app, the standalone verifier ran 10 checks while `build --apply --verify` ran 8 -- the two missing ones being `role-privileges` for each persona. `deps.verify` constructed its reader without `httpClient` or `envUrl`, so the `entityPrivileges` reader was never wired. verify-spec skips role-privileges unless BOTH readers are functions, so the check was silently absent and the build reported a clean `verify PASS` having never checked what any persona role actually grants. The graceful degradation is deliberate -- an unwired reader must not fire a false failure -- but it also means a caller that simply forgets to wire it gets silence rather than an error. `--apply --verify` is the primary path, so the feature was effectively off for most users while looking green. `makeSdk` now returns its `httpClient` and main threads it plus `envUrl` into the reader. Returning the SAME instance rather than constructing a second one keeps token acquisition and retry state shared. Verified live end to end: build (10 steps) -> `verify PASS (10/10)` where it had been 8/8, then a negative spec declaring a privilege the role does not hold fails correctly with `role-privileges: Live Probe Operator -- lpr_widget.delete: role does not hold prvDeletelpr_widget` and exit 1, then teardown left 0 leftovers. An unresolvable job surface is reported by spec-lint as designed. Tests assert against SOURCE because the wiring lives inside main(), which is not exported, and a behavioural test would need a live SDK. Red-green verified: removing the two arguments fails the wiring test. 1488 tests. NOTE: `telemetry-hook-pretool` is flaky under the full runner (it spawns a process against a local HTTPS probe); it passed on re-run and in isolation, and is untouched by this change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 42626da2-b66f-4162-acaa-b1127ef23d89
1 parent 38b20aa commit 258bd01

2 files changed

Lines changed: 36 additions & 3 deletions

File tree

plugins/model-apps/scripts/build-model-app.js

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,11 @@ function makeSdk(env, spec, workspaceDir) {
5555
const cleanup = () => {
5656
fs.rmSync(sdkTempDir, { recursive: true, force: true });
5757
};
58-
return { sdk, provisionSdk, cleanup };
58+
// `httpClient` is returned so the caller can wire verify's role-privilege reader, which needs the
59+
// raw client (and the org URL) to compose an absolute `EntityDefinitions(...)?$select=Privileges`
60+
// request — the SDK's entity metadata projects `Privileges` away. Returning the SAME instance
61+
// rather than constructing a second one keeps token acquisition and retry state shared.
62+
return { sdk, provisionSdk, httpClient, cleanup };
5963
}
6064

6165
// Turn engine progress events into a phase-grouped, status-marked build log:
@@ -431,7 +435,7 @@ async function main() {
431435
};
432436
// Construct for both dry-run and apply: proves the vendored bundle + adapter wire up
433437
// (offline), and apply needs it. A spec validation error short-circuits before any write.
434-
const { sdk, provisionSdk, cleanup } = makeSdk(env, spec, workspaceDir);
438+
const { sdk, provisionSdk, httpClient, cleanup } = makeSdk(env, spec, workspaceDir);
435439
// Durable build journal (apply runs only): a per-run record of steps + where a run halted,
436440
// written to <workspace>/build-log.jsonl. Resume = re-run the same command (idempotent).
437441
const journal = opts.apply
@@ -484,7 +488,13 @@ async function main() {
484488
const deps = {
485489
log: (m) => process.stderr.write(m + '\n'),
486490
sdk, provisionSdk, journal,
487-
verify: (s) => verifySpec(s, readerFor(provisionSdk, appUniqueName(s), { genpageCli: makeGenpageCli(env), workspaceDir })),
491+
// `httpClient` + `envUrl` are threaded through so the role-privileges check actually RUNS
492+
// here. verify-spec skips it unless BOTH `rolePrivileges` and `entityPrivileges` readers are
493+
// present, and `entityPrivileges` needs the raw client and the org URL to compose an absolute
494+
// EntityDefinitions request. Omitting them degraded silently: `--apply --verify` reported a
495+
// clean PASS having never checked what any persona's role actually grants. Caught live —
496+
// standalone verify ran 10 checks against the same app where the build's inline verify ran 8.
497+
verify: (s) => verifySpec(s, readerFor(provisionSdk, appUniqueName(s), { genpageCli: makeGenpageCli(env), workspaceDir, httpClient, envUrl: env })),
488498
};
489499
if (changedOnly && opts.apply) {
490500
// #changed-only: the flow decides fast (pages-only via the sdk-build seams) vs full, gated on the

plugins/model-apps/scripts/tests/build-model-app.test.js

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -488,3 +488,26 @@ test('unableToRun is propagated from verifySpec into r.verify (RECONCILIATION 1)
488488
assert.strictEqual(r.verify.ok, false);
489489
assert.strictEqual(r.verify.unableToRun, true, 'unableToRun propagated from verifySpec result');
490490
});
491+
492+
// -- verify wiring -----------------------------------------------------------------------------
493+
// Asserted against SOURCE because the wiring lives inside main(), which is not exported, and the
494+
// failure mode is SILENT: verify-spec skips role-privileges unless BOTH readers are present, so
495+
// omitting httpClient/envUrl made --apply --verify report a clean PASS having never checked what
496+
// any persona role actually grants. Found live -- the standalone verifier ran 10 checks against the
497+
// same app where the build inline verify ran 8. A behavioural test would need a live SDK; this pins
498+
// the exact regression at zero cost.
499+
test('build --verify wires the role-privilege readers (httpClient + envUrl)', () => {
500+
const src = fs.readFileSync(path.join(__dirname, '..', 'build-model-app.js'), 'utf8');
501+
const call = src.split(/\r?\n/).find((l) => l.includes('verify: (s) => verifySpec'));
502+
assert.ok(call, 'expected the deps.verify wiring line');
503+
assert.match(call, /httpClient/, 'entityPrivileges needs the raw client');
504+
assert.match(call, /envUrl: env/, 'entityPrivileges needs the org url to build an absolute request');
505+
});
506+
507+
test('makeSdk returns the httpClient so the caller can wire verify', () => {
508+
// Returning the SAME instance rather than constructing a second one keeps token acquisition and
509+
// retry state shared; a second client would re-acquire a token per verify run.
510+
const src = fs.readFileSync(path.join(__dirname, '..', 'build-model-app.js'), 'utf8');
511+
assert.match(src, /return \{ sdk, provisionSdk, httpClient, cleanup \}/, 'makeSdk must expose httpClient');
512+
assert.match(src, /const \{ sdk, provisionSdk, httpClient, cleanup \} = makeSdk\(/, 'main must destructure it');
513+
});

0 commit comments

Comments
 (0)