golem15/msd-core
3
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

chore(#2801): remove the hostBehaviors.reviewerCli deprecated alias (#3272)

Browse Source 
* test(#2801): failing-first suite for the hostBehaviors.reviewerCli alias removal

Inverts the Phase 5a rows that assert the derived legacy alias still
contributes a reviewer slug, and adds the removal-warning coverage the
alias's exit needs (ADR-2782 D9).

RED against unmodified production code, by design: the six shipped
manifests still declare the key and collectReviewerWarnings emits nothing
for hostBehaviors.

Refs #2801

* chore(#2801): remove the hostBehaviors.reviewerCli deprecated alias

ADR-2782 D9, Phase 7 — the final phase of epic #2782.

The derived legacy alias survived one release (Phase 5a shipped in 1.9.0;
1.9.1 and 1.10.0 have since gone out), so it goes. A declared reviewer
body is now the only route onto the reviewer roster.

- deriveReviewerSlugs no longer reads runtime.hostBehaviors.reviewerCli
- the key is stripped from the six manifests that carried it; each already
  declares a reviewer body whose slug equals its capability id, so the
  derived roster is unchanged at the same twelve slugs
- collectReviewerWarnings emits a presence-based, non-fatal removal notice
  for any manifest still declaring the key, reaching both the build-time
  registry generation and the third-party overlay load path. The check runs
  before the reviewer-body early-return, because the manifest it exists for
  is the alias-only one that has no body.
- hostBehaviors stays an open, unvalidated bag for its other 59 keys; this
  adds one keyed removal notice, not general validation

Refs #2801

* refactor(#2801): give the reviewer-warning channel a typed IR

Review finding: the new tests asserted with String#includes() on the
warning prose, which CONTRIBUTING.md's 'Prohibited: Raw Text Matching on
Test Outputs' bans in favor of a typed intermediate representation.

Adds the IR beside the renderer rather than replacing it, which is the
shape that section prescribes and bin/verify-reapply-patches.cjs already
models:

- REVIEWER_WARNING, a frozen code enum
- REMOVED_REVIEWER_CLI_FIELD, so the emitting site and its test share one
  symbol instead of duplicating a literal
- collectReviewerWarningRecords(cap), returning typed records

collectReviewerWarnings(cap) keeps its exact string[] contract as a thin
map over the records, so both production consumers are untouched. Every
section-K row now asserts on record.code/field/capId and none on the
rendered message. Locks the code surface, asserts the renderer stays
one-to-one with the records, and migrates the pre-existing Phase 2 test
on the same channel off prose matching.

Refs #2801

* test(#2801): invert the section F alias fall-through regression row

Caught by the remote runner: 2 unique failures on both Node lanes out of
31,692. tests/reviewer-lane-declarations.test.cjs section F — Phase 5a's
isolated-security-review regressions — asserted that a blank reviewer.slug
falls through to the hostBehaviors.reviewerCli alias rather than dropping
the lane. That is the direct inverse of this phase's contract.

The original rationale held only while the alias existed. With it gone
there is nothing to fall through to: a blank body is not a declaration,
and a declaration is the only route onto the roster.

Inverted rather than deleted — the row carries the adversarial-review
provenance for the slug trim, and removing a security regression guard to
make a change pass is backwards. The duplicate row added earlier in
section C is dropped instead; section F is its canonical home.

Also corrects two count strings Phase 5b left at eleven while asserting
twelve, which would misreport on failure.

Refs #2801

* docs(#2801): give the removed reviewerCli flag a migration path

The Reference edit alone satisfied CI — a file under docs/ moved, so
lint-docs-required.cjs was green — while the task-oriented quadrant said
nothing about the removal. A maintainer whose lane had just gone silent
would have found the field documented as removed and no page telling them
what to do about it.

Adds a migration section to the how-to: the symptom, the verbatim warning
they will see, the before/after manifest, and the note to keep the
reviewer slug equal to the capability id so existing
review.default_reviewers entries and --<slug> flags survive.

Refs #2801

* chore(#2801): backfill changeset pr number to 3272

* feat(#2801): close the runtime.hostBehaviors vocabulary

ADR-1016 closes twelve descriptor axes and rejects an open escape hatch
in the descriptor. It never mentioned runtime.hostBehaviors, and that
silence was read as permission: 59 keys across 18 manifests, 39 of them
set by a single capability, validated by nothing. The reference docs went
further and attributed the open seam to ADR-1016, which does not mention
the field at all.

KNOWN_HOST_BEHAVIORS enumerates the vocabulary. An undeclared key yields a
non-fatal UNKNOWN_HOST_BEHAVIOR record on the same D4.3 channel as the
alias removal notice, reaching both build-time generation and overlay
install.

Warning, never error, for the reason this phase exists: an error would
hard-break an out-of-tree descriptor carrying a bespoke key with no
deprecation window, which is what reviewerCli was given a release to
avoid. Escalation is a separate decision.

reviewerCli is excluded from the unknown-key sweep so it keeps its own
notice with the migration pointer rather than drawing two records.

A parity test binds the vocabulary to the shipped manifests in both
directions, and a second asserts no shipped capability draws a notice, so
the closure is provably inert in-tree.

Records the decision and the miscitation as an ADR-1016 amendment.

Refs #2801

* fix(#2801): bound and sanitize the unknown-key diagnostics

Two findings from an isolated adversarial review of the closure commit,
both proven by execution rather than asserted.

MAJOR, introduced by the closure: the new Object.keys(hostBehaviors) sweep
had no ceiling. An installed third-party manifest is bounded only by
MANIFEST_MAX_BYTES, and an 8.69MB manifest with 800,000 keys produced
800,000 records and ~139MB of message text, retained for the registry's
lifetime in OverlayMeta.diagnostics. Now capped at ten records plus a
summary carrying omittedCount, mirroring capability-loader's existing
slice(0,3) idiom. The same manifest now yields 11 records and 1748 chars.

MINOR, newly reachable: manifest-supplied key names were interpolated raw.
Unlike cap.id, which validateCapability gates on KEBAB_RE before these
diagnostics run, hostBehaviors keys have no grammar check anywhere, so
ANSI escapes and CRLF reached stderr and OverlayMeta.warnings intact. New
describeKey replaces C0/C1 controls and clips at 80 chars. The file
already had describeValue for this and applied it only to values.

Both fixes land on the pre-existing reviewer.* sweep too — it carried the
identical pair, and fixing only the new copy would leave the same defect
one screen from its own fix.

Refs #2801

---------

Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
2026-08-09 19:18:56 -04:00
committed by GitHub
parent 58d73dd220
commit 653f95e39f
18 changed files with 1007 additions and 117 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
tests/declarative-reference-antigravity.test.cjs
View File

@@ -174,7 +174,10 @@ test('capabilities/antigravity/capability.json validates — subagentToolkit "fu
test('antigravity descriptor declares runtime.hostBehaviors (the folded-in behaviors) + the subagentToolkit upgrade', () => {
const hb = ANTIGRAVITY_CAP.runtime.hostBehaviors;
assert.ok(hb && typeof hb === 'object');
assert.equal(hb.reviewerCli, true);
// `reviewerCli` was one of the folded-in behaviors originally; ADR-2782
// Phase 7 (#2801) removed the alias — antigravity's reviewer lane is declared
// by its `reviewer` body now, not by a boolean in the open hostBehaviors bag.
assert.equal(Object.prototype.hasOwnProperty.call(hb, 'reviewerCli'), false);
assert.equal(hb.projectInstructionFile, 'GEMINI.md');
assert.equal(hb.noPathRewrite, true);
assert.equal(hb.hookPathStyle, 'raw');

166
tests/reviewer-lane-declarations.test.cjs
View File

@@ -11,6 +11,13 @@ process.env.GSD_TEST_MODE = '1';
* A-E). See that phase's `40-design.md` for the behavior table the matrix
* derives from. Test names are copied verbatim from the matrix.
*
* AMENDED by ADR-2782 Phase 7 (chore #2801), which removes the
* `runtime.hostBehaviors.reviewerCli` derived legacy alias. Rows that asserted
* the alias still contributed a slug are inverted here rather than deleted, so
* the file keeps a guard against reintroduction. See
* `.gsd/phase/chore-2801-remove-reviewercli-alias/50-test-matrix.md` rows
* B1 and C1-C10, P1.
*
* THE SINGLE MOST IMPORTANT PROPERTY (matrix "Red-before-green"): the roster is
* eleven slugs before this phase and eleven after, with IDENTICAL membership —
* C1 is the keystone, asserted against a LITERAL list, never against a value
@@ -43,6 +50,7 @@ const { test, describe } = require('node:test');
const assert = require('node:assert/strict');
const fs = require('node:fs');
const path = require('node:path');
const fc = require('fast-check');
const {
loadAndValidate,
@@ -296,21 +304,27 @@ describe('B. The six existing runtime capabilities', () => {
}
});
test('reviewerCliAliasIsRetainedForTheDeprecationWindow', () => {
test('reviewerCliAliasIsRemovedFromEveryShippedManifest', () => {
// Phase 7 (#2801): the deprecation window opened in 1.9.0 (Phase 5a) and
// 1.9.1 + 1.10.0 have since shipped, so the alias goes. Each of these six
// already declares a `reviewer` body whose slug equals its capability id, so
// removing the key costs none of them a lane — C1 is the proof.
for (const id of RUNTIME_REVIEWER_IDS) {
const cap = SHIPPED.capMap.get(id);
const hb = cap.runtime.hostBehaviors || {};
assert.equal(
cap.runtime.hostBehaviors && cap.runtime.hostBehaviors.reviewerCli, true,
`"${id}" must retain hostBehaviors.reviewerCli:true for the deprecation window (removal is Phase 7 / #2801)`,
Object.prototype.hasOwnProperty.call(hb, 'reviewerCli'), false,
`"${id}" must no longer declare hostBehaviors.reviewerCli (removed in Phase 7 / #2801); declare a reviewer body instead`,
);
}
});
test('bodyAndAliasContributeOneSlugNotTwo', () => {
// Isolated synthetic fixture: one capability carrying BOTH a declared
// reviewer.slug AND the legacy alias, with slug !== capId, so a double
// contribution would be observable as two distinct roster entries rather
// than being hidden by an accidental string match.
test('vestigialAliasKeyDoesNotAddASecondSlug', () => {
// Post-#2801 this is no longer a PRECEDENCE rule (body beats alias) — the
// key is simply never read. Kept, with slug !== capId so a stray
// contribution would be observable as a distinct roster entry rather than
// hidden by an accidental string match, because an out-of-tree manifest can
// still carry the vestigial key for years.
const registry = {
capabilities: {
'dual-purpose-cap': {
@@ -323,14 +337,17 @@ describe('B. The six existing runtime capabilities', () => {
const roster = deriveReviewerSlugs(registry);
assert.equal(roster.length, 1, `expected exactly one contribution, not two, got: ${JSON.stringify(roster)}`);
assert.deepEqual(roster, ['dual-slug']);
assert.equal(roster.includes('dual-purpose-cap'), false, 'the legacy alias must not ALSO contribute the capability id');
assert.equal(roster.includes('dual-purpose-cap'), false, 'the removed alias key must not contribute the capability id');
});
});
// ─── C. Roster derivation — src/review-reviewer-selection.cts ─────────────
describe('C. Roster derivation — src/review-reviewer-selection.cts', () => {
test('rosterMembershipIsUnchangedByDerivationRefactor', () => {
test('rosterMembershipIsUnchangedByAliasRemoval', () => {
// KEYSTONE. This row is GREEN before and after #2801 — it is the invariant
// the phase must not break, not a red row. The literal list is never
// computed by the machinery under test.
assert.equal(KNOWN_REVIEWER_SLUGS.length, 12, 'roster must be exactly 12 — not 11, not 13');
assert.deepEqual(
[...KNOWN_REVIEWER_SLUGS].sort(), LITERAL_ROSTER,
@@ -343,14 +360,34 @@ describe('C. Roster derivation — src/review-reviewer-selection.cts', () => {
assert.deepEqual(deriveReviewerSlugs(registry), ['my-lane']);
});
test('aliasOnlyCapabilityStillContributesItsSlug', () => {
// No reviewer body yet — only the legacy hostBehaviors flag (B2's shape).
test('aliasOnlyCapabilityContributesNoSlug', () => {
// #2801 core inversion: no reviewer body, only the removed hostBehaviors
// flag. Before Phase 7 this contributed `legacy-cli`; now it contributes
// nothing, and `collectReviewerWarnings` says so (see section K of
// tests/reviewer-manifest-body.test.cjs).
const registry = {
capabilities: {
'legacy-cli': { role: 'runtime', runtime: { hostBehaviors: { reviewerCli: true } } },
},
};
assert.deepEqual(deriveReviewerSlugs(registry), ['legacy-cli']);
assert.deepEqual(deriveReviewerSlugs(registry), []);
});
test('aliasWithAnyValueContributesNoSlug', () => {
// Value sweep: the old branch was a strict `=== true`, so `false`/`"true"`/`1`
// never contributed even before. Locking all of them at once means a partial
// revert that restores only the truthy branch is still caught.
for (const value of [true, false, 'true', 0, 1, null, {}, []]) {
const registry = {
capabilities: {
'legacy-cli': { role: 'runtime', runtime: { hostBehaviors: { reviewerCli: value } } },
},
};
assert.deepEqual(
deriveReviewerSlugs(registry), [],
`reviewerCli = ${JSON.stringify(value)} must contribute no slug`,
);
}
});
test('nonReviewerCapabilityContributesNoSlug', () => {
@@ -366,10 +403,9 @@ describe('C. Roster derivation — src/review-reviewer-selection.cts', () => {
assert.equal(reviewerSelectionModule.NON_RUNTIME_REVIEWER_SLUGS, undefined);
});
test('reviewerBodyWinsOverTheLegacyAlias', () => {
// Body and alias disagree on membership: capId (what the alias would
// contribute) differs from reviewer.slug (what the body contributes), so
// the winner is unambiguous from the result alone.
test('declaredBodyIsUnaffectedByAVestigialAliasKey', () => {
// capId (what the removed alias used to contribute) differs from
// reviewer.slug, so the result alone shows which surface was read.
const registry = {
capabilities: {
'conflicting-cap-id': {
@@ -382,7 +418,7 @@ describe('C. Roster derivation — src/review-reviewer-selection.cts', () => {
const roster = deriveReviewerSlugs(registry);
assert.deepEqual(
roster, ['the-declared-slug'],
`expected only the declared body's slug to win over the alias, got: ${JSON.stringify(roster)}`,
`only the declared body's slug may contribute, got: ${JSON.stringify(roster)}`,
);
});
@@ -392,10 +428,13 @@ describe('C. Roster derivation — src/review-reviewer-selection.cts', () => {
});
test('rosterIsStableRegardlessOfRegistryOrder', () => {
// `gamma` was an alias-only capability before #2801. It is now body-declared
// so this row still proves a THREE-way sort; shrinking it to two entries
// would quietly weaken the ordering guarantee it exists to protect.
const capsForward = {
alpha: { role: 'reviewer', reviewer: { slug: 'zzz-lane' } },
beta: { role: 'reviewer', reviewer: { slug: 'aaa-lane' } },
gamma: { role: 'runtime', runtime: { hostBehaviors: { reviewerCli: true } } },
gamma: { role: 'runtime', runtime: { hostBehaviors: {} }, reviewer: { slug: 'gamma' } },
};
const reversedCaps = {};
for (const key of Object.keys(capsForward).reverse()) reversedCaps[key] = capsForward[key];
@@ -408,6 +447,70 @@ describe('C. Roster derivation — src/review-reviewer-selection.cts', () => {
'roster must be sorted, independent of declaration order',
);
});
test('derivedRosterNeverAdmitsAnAliasOnlyCapability', () => {
// P1 — property over arbitrary registries. Three invariants at once:
// (1) no capability that declares ONLY the removed alias ever reaches the
// roster, (2) the roster is sorted and duplicate-free, and (3) it does not
// depend on key insertion order. A partial revert of the alias branch is
// caught by (1) at any registry shape, not just the handful enumerated above.
fc.assert(
fc.property(
fc.array(
fc.record({
capId: fc.string({ minLength: 1, maxLength: 8 }).filter((s) => s.trim().length > 0),
declaresBody: fc.boolean(),
slug: fc.string({ minLength: 1, maxLength: 8 }).filter((s) => s.trim().length > 0),
aliasValue: fc.constantFrom(true, false, 'true', 1, 0, null, undefined),
}),
{ minLength: 0, maxLength: 12 },
),
(specs) => {
const capabilities = {};
const aliasOnlyIds = new Set();
const declaredSlugs = new Set();
for (const s of specs) {
if (s.capId === '__proto__' || s.capId === 'constructor' || s.capId === 'prototype') continue;
const cap = { role: 'runtime', runtime: { hostBehaviors: {} } };
if (s.aliasValue !== undefined) cap.runtime.hostBehaviors.reviewerCli = s.aliasValue;
if (s.declaresBody) {
cap.reviewer = { slug: s.slug };
declaredSlugs.add(s.slug.trim());
} else if (s.aliasValue !== undefined) {
aliasOnlyIds.add(s.capId);
}
capabilities[s.capId] = cap;
}
const roster = deriveReviewerSlugs({ capabilities });
// (1) An alias-only capability's id may appear ONLY if some other
// capability legitimately declared it as a body slug.
for (const id of aliasOnlyIds) {
if (declaredSlugs.has(id)) continue;
assert.equal(
roster.includes(id), false,
`alias-only capability "${id}" must not reach the roster; roster=${JSON.stringify(roster)}`,
);
}
// (2) sorted + unique
assert.deepEqual(roster, [...roster].sort(), `roster must be sorted, got: ${JSON.stringify(roster)}`);
assert.equal(new Set(roster).size, roster.length, `roster must be duplicate-free, got: ${JSON.stringify(roster)}`);
// (3) order-independent
const reversed = {};
for (const key of Object.keys(capabilities).reverse()) reversed[key] = capabilities[key];
assert.deepEqual(
deriveReviewerSlugs({ capabilities: reversed }), roster,
'roster must not depend on registry key insertion order',
);
return true;
},
),
{ numRuns: 200, seed: 2801 },
);
});
});
// ─── D. Cross-phase invariants that must not regress ────────────────────────
@@ -528,7 +631,7 @@ describe('E. Lane fidelity — no translation layer', () => {
}
assert.equal(REVIEWER_LANES.length, 12, 'expected exactly 12 declared descriptor lanes');
assert.equal(bySlug.size, 12, `expected exactly 11 capabilities declaring a reviewer body, got: ${bySlug.size}`);
assert.equal(bySlug.size, 12, `expected exactly 12 capabilities declaring a reviewer body, got: ${bySlug.size}`);
// Top-level scalar/array fields compared whole; the two fields that are
// themselves nested objects (probe, invoke) are compared sub-field-by-
@@ -611,16 +714,27 @@ describe('F. Isolated-security-review regressions', () => {
);
});
test('theAliasStillAppliesWhenABodyDeclaresOnlyWhitespace', () => {
// A body whose slug is blank is NOT a declaration, so the legacy alias must
// still contribute — otherwise a malformed body would silently REMOVE a lane
// that worked before, which is worse than the blank slug itself.
test('aBlankBodyContributesNothingAndNoAliasRescuesIt', () => {
// INVERTED by Phase 7 (#2801). While the alias existed, a blank slug fell
// through to it, so a malformed body could not silently remove a lane that
// worked before — that was the point of the original row. With the alias
// gone there is nothing to fall through TO: a blank body is not a
// declaration, and a declaration is now the only route onto the roster.
//
// The row is inverted rather than deleted because it is the combination the
// single-variable rows miss, and because it is the security-review
// provenance for the trim: `deriveReviewerSlugs` is exported and carries no
// other validation, so a whitespace slug must never occupy a roster entry
// it can never match.
const roster = deriveReviewerSlugs({
capabilities: {
claude: { reviewer: { slug: ' ' }, runtime: { hostBehaviors: { reviewerCli: true } } },
},
});
assert.deepEqual(roster, ['claude'], 'a blank body must fall through to the alias, not drop the lane');
assert.deepEqual(
roster, [],
'a blank body is not a declaration, and the removed alias cannot rescue it',
);
});
test('moduleLoadSurvivesAHostileRegistryShape', () => {
@@ -629,7 +743,7 @@ describe('F. Isolated-security-review regressions', () => {
// The module under test already imported successfully above; assert the
// derived roster is a usable array rather than a partially-initialised value.
assert.ok(Array.isArray([...KNOWN_REVIEWER_SLUGS]), 'roster must be iterable after module load');
assert.equal(KNOWN_REVIEWER_SLUGS.length, 12, 'the real registry still yields the eleven lanes');
assert.equal(KNOWN_REVIEWER_SLUGS.length, 12, 'the real registry still yields the twelve lanes');
// And the derivation itself is total over the shapes JSON can express.
for (const hostile of [null, undefined, [], 0, 'x', { capabilities: null }, { capabilities: [] }]) {
assert.doesNotThrow(

446
tests/reviewer-manifest-body.test.cjs
View File

@@ -3,7 +3,7 @@ process.env.GSD_TEST_MODE = '1';
/**
* reviewer-manifest-body.test.cjs — behavioral tests for the reviewer lane body
* (ADR-2782, chore #2795 Phase 2): `validateReviewerBody`, `collectReviewerWarnings`,
* (ADR-2782, chore #2795 Phase 2): `validateReviewerBody`, `collectReviewerWarnings` / `collectReviewerWarningRecords`,
* the `role:'reviewer'` dispatch branch of `validateCapability`, the reviewer-lane
* uniqueness rules inside `validateCrossCapability`, and the harvest widening in
* `buildRegistry` / `loadAndValidate`.
@@ -37,6 +37,9 @@ const {
LANE_SLUG_RE,
validateReviewerBody,
collectReviewerWarnings,
collectReviewerWarningRecords,
REVIEWER_WARNING,
REMOVED_REVIEWER_CLI_FIELD,
validateCapability,
validateCrossCapability,
VALID_LANE_EFFORT_CHANNELS,
@@ -45,6 +48,9 @@ const {
VALID_EVIDENCE_CLASSES,
VALID_LANE_HANDLERS,
KNOWN_REVIEWER_FIELDS,
KNOWN_HOST_BEHAVIORS,
MAX_REPORTED_UNKNOWN_KEYS,
MAX_REPORTED_KEY_CHARS,
} = require('../gsd-core/bin/lib/capability-validator.cjs');
const { loadAndValidate, buildRegistry } = require('../scripts/gen-capability-registry.cjs');
@@ -268,14 +274,19 @@ describe('A. Body presence / shape', () => {
const errs = validateReviewerBody(cap);
assert.deepEqual(errs, [], `unknown field must not be a validation error, got: ${JSON.stringify(errs)}`);
const warnings = collectReviewerWarnings(cap);
assert.ok(
warnings.some(
(w) => w.includes('cap-x') && w.includes('reviewer.futureField')
&& w.includes([...KNOWN_REVIEWER_FIELDS].join(', ')),
),
`expected a warning naming reviewer.futureField and the known-fields list, got: ${JSON.stringify(warnings)}`,
);
// Asserted on the typed IR, not the rendered prose (CONTRIBUTING.md,
// "Prohibited: Raw Text Matching on Test Outputs"). The `message` field
// exists for operator console output only.
const records = collectReviewerWarningRecords(cap);
assert.equal(records.length, 1, `expected exactly one record, got: ${JSON.stringify(records)}`);
assert.equal(records[0].code, REVIEWER_WARNING.UNKNOWN_REVIEWER_FIELD);
assert.equal(records[0].capId, 'cap-x');
assert.equal(records[0].field, 'reviewer.futureField');
assert.deepEqual(records[0].knownFields, [...KNOWN_REVIEWER_FIELDS]);
// The renderer still produces one string per record for the two production
// consumers (gen-capability-registry -> stderr, capability-loader -> OverlayMeta.warnings).
assert.equal(collectReviewerWarnings(cap).length, records.length);
});
test('unknownRoleIsRejectedWithEnumeratedMembers', () => {
@@ -1700,3 +1711,420 @@ describe('J. Property-based (fast-check)', () => {
);
});
});
// ─── K. Removed `hostBehaviors.reviewerCli` alias (ADR-2782 D9, chore #2801) ──
//
// Phase 7 deletes the derived legacy alias. `collectReviewerWarnings` is the
// channel the removal announces itself on, because it is already wired to BOTH
// surfaces a manifest can arrive through: the build-time generator
// (`gen-capability-registry.cjs` -> stderr) and the overlay loader
// (`capability-loader.cts` -> OverlayMeta.warnings, on the ACCEPT path for every
// accepted capability). An out-of-tree manifest still setting the alias reaches
// the second one.
//
// Rows K1-K9 implement W1-W9 of
// `.gsd/phase/chore-2801-remove-reviewercli-alias/50-test-matrix.md`.
//
// The load-bearing structural fact these rows pin down: the removal check must
// run BEFORE `collectReviewerWarningRecordFields`' `reviewer`-body early-return. An
// alias-only manifest — precisely the case the deprecation window existed for —
// has no `reviewer` body, so a check placed after that guard would fire only for
// capabilities that do not need it. K1 is the row that fails if it is misplaced.
/** A whole runtime manifest — the shape production passes to this function. */
function runtimeCapWithHostBehaviors(hostBehaviors, extra = {}) {
return {
id: 'legacy-cli',
role: 'runtime',
runtime: { hostBehaviors },
...extra,
};
}
describe('K. Removed hostBehaviors.reviewerCli alias (#2801)', () => {
/** Records for the removal notice only, keyed on the typed code. */
function removalRecords(cap) {
return collectReviewerWarningRecords(cap)
.filter((rec) => rec.code === REVIEWER_WARNING.REMOVED_HOST_BEHAVIOR);
}
test('reviewerWarningCodeSurfaceIsLocked', () => {
// The third of the three coordinated changes a new code requires. Without
// this, a code can be added or renamed with no test noticing.
assert.deepEqual(
Object.keys(REVIEWER_WARNING).sort(),
['REMOVED_HOST_BEHAVIOR', 'UNKNOWN_HOST_BEHAVIOR', 'UNKNOWN_REVIEWER_FIELD'],
);
assert.equal(Object.isFrozen(REVIEWER_WARNING), true, 'the code enum must be frozen');
assert.equal(REMOVED_REVIEWER_CLI_FIELD, 'runtime.hostBehaviors.reviewerCli');
});
test('removedReviewerCliAliasWarnsWhenPresentWithoutABody', () => {
// No `reviewer` body at all — the alias-only manifest. This is the row that
// proves the check runs before the body early-return.
const cap = runtimeCapWithHostBehaviors({ reviewerCli: true });
const records = removalRecords(cap);
assert.equal(
records.length, 1,
`expected exactly one removal record for an alias-only manifest, got: ${JSON.stringify(collectReviewerWarningRecords(cap))}`,
);
assert.equal(records[0].capId, 'legacy-cli');
assert.equal(records[0].field, REMOVED_REVIEWER_CLI_FIELD);
});
test('removedReviewerCliAliasWarnsAlongsideADeclaredBody', () => {
const cap = runtimeCapWithHostBehaviors({ reviewerCli: true }, { reviewer: validLane() });
assert.equal(removalRecords(cap).length, 1, 'a declared body must not suppress the removal notice');
assert.deepEqual(
validateReviewerBody(cap), [],
'the vestigial key must stay a WARNING — never a validation error (Postel: liberal in what we accept)',
);
});
test('removedReviewerCliAliasWarnsRegardlessOfItsValue', () => {
// Presence-based, deliberately: after removal the key is unknown at ANY
// value, exactly as an unknown `reviewer.*` field is. A value-sensitive
// warning would tell an author carrying `reviewerCli: false` that their
// stale key is fine, when it is simply dead.
// (40-design.md -> Rejected 3.)
for (const value of [true, false, 'true', 0, 1, null, {}, []]) {
const cap = runtimeCapWithHostBehaviors({ reviewerCli: value });
assert.equal(
removalRecords(cap).length, 1,
`expected a removal record for reviewerCli = ${JSON.stringify(value)}, got: ${JSON.stringify(collectReviewerWarningRecords(cap))}`,
);
}
});
test('similarlyNamedHostBehaviorKeysAreNotTheRemovedField', () => {
// Exact own-key match only: a near-miss name must never be reported as the
// removed `reviewerCli`. Since #2801 closed the vocabulary these names DO
// now draw an unknown-host-behavior notice, which is correct — they are not
// declared behaviors — but they must not draw the removal notice.
const cap = runtimeCapWithHostBehaviors({
reviewerCliPath: '/usr/bin/thing',
reviewer_cli: true,
reviewerCLI: true,
reapplyCommand: 'x',
});
const records = collectReviewerWarningRecords(cap);
assert.deepEqual(
records.filter((rec) => rec.code === REVIEWER_WARNING.REMOVED_HOST_BEHAVIOR), [],
'only the exact own key `reviewerCli` is the removed field',
);
assert.deepEqual(
records.map((rec) => rec.field).sort(),
[
'runtime.hostBehaviors.reviewerCLI',
'runtime.hostBehaviors.reviewer_cli',
'runtime.hostBehaviors.reviewerCliPath',
].sort(),
'the three undeclared names draw an unknown-host-behavior notice; the declared reapplyCommand does not',
);
});
test('malformedHostBehaviorsNeitherWarnsNorThrows', () => {
const shapes = [
['null', { id: 'c', role: 'runtime', runtime: { hostBehaviors: null } }],
['array', { id: 'c', role: 'runtime', runtime: { hostBehaviors: [] } }],
['string', { id: 'c', role: 'runtime', runtime: { hostBehaviors: 'reviewerCli' } }],
['number', { id: 'c', role: 'runtime', runtime: { hostBehaviors: 42 } }],
['empty object', { id: 'c', role: 'runtime', runtime: { hostBehaviors: {} } }],
['no hostBehaviors', { id: 'c', role: 'runtime', runtime: {} }],
['no runtime', { id: 'c', role: 'reviewer', reviewer: validLane() }],
['runtime null', { id: 'c', role: 'runtime', runtime: null }],
];
for (const [name, cap] of shapes) {
let records;
try {
records = collectReviewerWarningRecords(cap);
} catch (err) {
assert.fail(`collectReviewerWarningRecords threw for ${name}: ${err && err.message}`);
}
assert.deepEqual(
records.filter((rec) => rec.code === REVIEWER_WARNING.REMOVED_HOST_BEHAVIOR), [],
`${name} must not produce a removal record`,
);
}
});
test('removalWarningAndUnknownFieldWarningCoexist', () => {
// Two independent diagnostics on one manifest. Neither may swallow the other
// — an early `return` after the first would hide the second.
const lane = validLane();
lane.futureField = 'from-a-newer-gsd';
const cap = runtimeCapWithHostBehaviors({ reviewerCli: true }, { id: 'both-cap', reviewer: lane });
const records = collectReviewerWarningRecords(cap);
assert.deepEqual(
records.map((rec) => rec.code).sort(),
[REVIEWER_WARNING.REMOVED_HOST_BEHAVIOR, REVIEWER_WARNING.UNKNOWN_REVIEWER_FIELD].sort(),
`expected exactly one of each code, got: ${JSON.stringify(records)}`,
);
});
test('inheritedReviewerCliFromPrototypeDoesNotWarn', () => {
// Own-key read: a polluted prototype must not manufacture a removal record
// on every otherwise-innocent manifest.
const polluted = Object.create({ reviewerCli: true });
polluted.reapplyCommand = 'x';
const cap = runtimeCapWithHostBehaviors(polluted);
assert.deepEqual(
collectReviewerWarningRecords(cap), [],
'an inherited reviewerCli is not a declared field',
);
});
test('removalWarningNamesTheReviewerBodyReplacement', () => {
// A removal notice that does not say what to do instead is not a migration
// path. The field was undocumented for its whole life and only documented at
// 1.9.0 as ALREADY deprecated, so we cannot enumerate who depends on it
// (Hyrum) — the exit has to carry its own instructions. Asserted on the
// typed fields, never on the rendered sentence.
const [record] = removalRecords(runtimeCapWithHostBehaviors({ reviewerCli: true }));
assert.ok(record, 'expected a removal record');
assert.equal(record.replacement, 'reviewer');
assert.equal(record.docs, 'docs/how-to/ship-a-reviewer-lane.md');
});
test('renderedStringsStayOneToOneWithRecords', () => {
// The two production consumers still receive strings; the renderer must not
// drop or duplicate a diagnostic.
const lane = validLane();
lane.futureField = 'x';
for (const cap of [
runtimeCapWithHostBehaviors({ reviewerCli: true }),
runtimeCapWithHostBehaviors({ reviewerCli: true }, { reviewer: lane }),
runtimeCapWithHostBehaviors({ reapplyCommand: 'x' }),
]) {
const records = collectReviewerWarningRecords(cap);
const strings = collectReviewerWarnings(cap);
assert.equal(strings.length, records.length);
assert.deepEqual(strings, records.map((rec) => rec.message));
}
});
test('collectReviewerWarningsStaysTotalOverTheNewHostBehaviorsReadPath', () => {
// W9 — the totality contract (#1461 OVL-1) now covers a second read path.
// A throwing getter or Proxy trap fires on the READ, before any message is
// built, so only the structural wrapper can save these. Both the IR and the
// renderer must survive, since the renderer maps over the IR.
const throwing = () => { throw new Error('boom'); };
const hostBehaviorsGetterThrows = { id: 'x', role: 'runtime', runtime: {} };
Object.defineProperty(hostBehaviorsGetterThrows.runtime, 'hostBehaviors', { get: throwing });
const reviewerCliGetterThrows = { id: 'x', role: 'runtime', runtime: { hostBehaviors: {} } };
Object.defineProperty(reviewerCliGetterThrows.runtime.hostBehaviors, 'reviewerCli', { get: throwing });
const cases = [
['runtime getter throws', Object.defineProperty({ id: 'x' }, 'runtime', { get: throwing })],
['hostBehaviors getter throws', hostBehaviorsGetterThrows],
['reviewerCli getter throws', reviewerCliGetterThrows],
['hostBehaviors Proxy traps throw', {
id: 'x',
role: 'runtime',
runtime: { hostBehaviors: new Proxy({}, { has: throwing, get: throwing, getOwnPropertyDescriptor: throwing, ownKeys: throwing }) },
}],
];
for (const [name, cap] of cases) {
let records;
let strings;
try {
records = collectReviewerWarningRecords(cap);
strings = collectReviewerWarnings(cap);
} catch (err) {
assert.fail(`${name}: threw ${err && err.message}`);
}
assert.ok(Array.isArray(records), `${name}: records must always be an array`);
assert.ok(Array.isArray(strings), `${name}: strings must always be an array`);
}
});
});
// ─── L. Closed `hostBehaviors` vocabulary (ADR-1016, closed by #2801) ────────
describe('L. Closed hostBehaviors vocabulary (#2801)', () => {
const ROOT = path.resolve(__dirname, '..');
/** Every hostBehaviors key the shipped manifests actually declare. */
function shippedHostBehaviorKeys() {
const keys = new Set();
const capsDir = path.join(ROOT, 'capabilities');
for (const dir of fs.readdirSync(capsDir, { withFileTypes: true })) {
if (!dir.isDirectory()) continue;
const file = path.join(capsDir, dir.name, 'capability.json');
if (!fs.existsSync(file)) continue;
const cap = JSON.parse(fs.readFileSync(file, 'utf8'));
const hb = cap && cap.runtime && cap.runtime.hostBehaviors;
if (hb && typeof hb === 'object' && !Array.isArray(hb)) {
for (const key of Object.keys(hb)) keys.add(key);
}
}
return keys;
}
test('vocabularyExactlyMatchesWhatTheShippedManifestsDeclare', () => {
// DEFECT.GENERATIVE-FIX: two surfaces, one truth. A key added to a manifest
// without being declared here would warn on every build; a key left here
// after its last manifest drops it is dead vocabulary. Both directions fail.
const shipped = shippedHostBehaviorKeys();
assert.deepEqual(
[...shipped].sort(), [...KNOWN_HOST_BEHAVIORS].sort(),
'the closed vocabulary and the shipped manifests must name the same keys',
);
});
test('noShippedCapabilityDrawsAHostBehaviorWarning', () => {
// The closure must be inert for everything that ships today. If this fails,
// closing the vocabulary broke a real capability rather than a hypothetical one.
const capsDir = path.join(ROOT, 'capabilities');
const offenders = [];
for (const dir of fs.readdirSync(capsDir, { withFileTypes: true })) {
if (!dir.isDirectory()) continue;
const file = path.join(capsDir, dir.name, 'capability.json');
if (!fs.existsSync(file)) continue;
const cap = JSON.parse(fs.readFileSync(file, 'utf8'));
for (const rec of collectReviewerWarningRecords(cap)) {
if (rec.code === REVIEWER_WARNING.UNKNOWN_HOST_BEHAVIOR
|| rec.code === REVIEWER_WARNING.REMOVED_HOST_BEHAVIOR) {
offenders.push(`${dir.name}: ${rec.field}`);
}
}
}
assert.deepEqual(offenders, [], `no shipped capability may draw a hostBehaviors notice, got: ${JSON.stringify(offenders)}`);
});
test('anUndeclaredHostBehaviorWarnsAndIsNotAnError', () => {
const cap = runtimeCapWithHostBehaviors({ someFutureSwitch: true });
const records = collectReviewerWarningRecords(cap);
assert.equal(records.length, 1, `expected one record, got: ${JSON.stringify(records)}`);
assert.equal(records[0].code, REVIEWER_WARNING.UNKNOWN_HOST_BEHAVIOR);
assert.equal(records[0].field, 'runtime.hostBehaviors.someFutureSwitch');
// Forward-compat invariant: a warning, never a validation error.
assert.deepEqual(validateCapability({ ...cap, version: '1.0.0' }, cap.id).filter((e) => e.includes('someFutureSwitch')), []);
});
test('aDeclaredHostBehaviorIsSilentAtAnyValue', () => {
for (const value of [true, false, 'x', 0, null, {}, []]) {
const cap = runtimeCapWithHostBehaviors({ reapplyCommand: value });
assert.deepEqual(
collectReviewerWarningRecords(cap), [],
`a declared key must be silent regardless of value, got value ${JSON.stringify(value)}`,
);
}
});
test('theRemovedAliasDrawsItsOwnNoticeNotTheGenericOne', () => {
// reviewerCli is excluded from the unknown-key sweep on purpose: it has a
// migration pointer the generic notice does not carry, and two records for
// one key would be noise.
const records = collectReviewerWarningRecords(runtimeCapWithHostBehaviors({ reviewerCli: true }));
assert.equal(records.length, 1, `expected exactly one record, got: ${JSON.stringify(records)}`);
assert.equal(records[0].code, REVIEWER_WARNING.REMOVED_HOST_BEHAVIOR);
assert.equal(records[0].replacement, 'reviewer');
});
test('reservedKeysInTheBagAreIgnoredNotWarned', () => {
const hostile = JSON.parse('{"__proto__": {"polluted": true}, "constructor": 1, "prototype": 2, "reapplyCommand": "x"}');
const cap = runtimeCapWithHostBehaviors(hostile);
let records;
try {
records = collectReviewerWarningRecords(cap);
} catch (err) {
assert.fail(`collectReviewerWarningRecords threw: ${err && err.message}`);
}
assert.deepEqual(records, [], 'reserved names are skipped, not reported as unknown behaviors');
assert.equal({}.polluted, undefined, 'Object.prototype must not be polluted');
});
});
// ─── M. Diagnostics are bounded and control-safe (#2801 review findings) ─────
//
// Both loops iterate MANIFEST-SUPPLIED keys. An installed third-party manifest
// is attacker-controlled and bounded only by MANIFEST_MAX_BYTES (8MB), so the
// record count and each key's rendered length must both have a ceiling, and a
// key must not be able to carry terminal escapes or a forged newline into
// stderr / OverlayMeta.warnings.
describe('M. Diagnostics are bounded and control-safe (#2801)', () => {
function manyUnknownHostBehaviors(n) {
const hb = {};
for (let i = 0; i < n; i += 1) hb['undeclaredKey' + i] = true;
return runtimeCapWithHostBehaviors(hb);
}
test('unknownHostBehaviorRecordsAreCappedWithASummary', () => {
const n = MAX_REPORTED_UNKNOWN_KEYS + 25;
const records = collectReviewerWarningRecords(manyUnknownHostBehaviors(n));
assert.equal(
records.length, MAX_REPORTED_UNKNOWN_KEYS + 1,
`expected ${MAX_REPORTED_UNKNOWN_KEYS} records plus one summary, got ${records.length}`,
);
const summary = records[records.length - 1];
assert.equal(summary.truncated, true);
assert.equal(summary.omittedCount, 25);
assert.equal(summary.field, 'runtime.hostBehaviors');
});
test('exactlyAtTheCapThereIsNoSummaryRecord', () => {
// limit-1 / limit / limit+1 around the ceiling.
const below = collectReviewerWarningRecords(manyUnknownHostBehaviors(MAX_REPORTED_UNKNOWN_KEYS - 1));
assert.equal(below.length, MAX_REPORTED_UNKNOWN_KEYS - 1);
assert.equal(below.some((rec) => rec.truncated), false);
const at = collectReviewerWarningRecords(manyUnknownHostBehaviors(MAX_REPORTED_UNKNOWN_KEYS));
assert.equal(at.length, MAX_REPORTED_UNKNOWN_KEYS);
assert.equal(at.some((rec) => rec.truncated), false, 'no summary when nothing was omitted');
const above = collectReviewerWarningRecords(manyUnknownHostBehaviors(MAX_REPORTED_UNKNOWN_KEYS + 1));
assert.equal(above.length, MAX_REPORTED_UNKNOWN_KEYS + 1);
assert.equal(above[above.length - 1].omittedCount, 1);
});
test('unknownReviewerFieldRecordsAreCappedTheSameWay', () => {
const lane = validLane();
for (let i = 0; i < MAX_REPORTED_UNKNOWN_KEYS + 5; i += 1) lane['futureField' + i] = 1;
const records = collectReviewerWarningRecords({ id: 'cap-x', reviewer: lane });
assert.equal(records.length, MAX_REPORTED_UNKNOWN_KEYS + 1);
assert.equal(records[records.length - 1].truncated, true);
assert.equal(records[records.length - 1].omittedCount, 5);
});
test('controlCharactersInAKeyNeverReachTheDiagnostic', () => {
// ESC-based colour sequence, a CR overwrite, and an embedded newline that
// would forge a second log line.
const hostile = '\x1b[31mred\x1b[0m\r\nforged: everything is fine';
for (const cap of [
runtimeCapWithHostBehaviors({ [hostile]: true }),
{ id: 'cap-x', reviewer: { slug: 'x', [hostile]: true } },
]) {
for (const rec of collectReviewerWarningRecords(cap)) {
// eslint-disable-next-line no-control-regex
assert.equal(/[\x00-\x1f\x7f-\x9f]/.test(rec.field), false, `control char survived into field: ${JSON.stringify(rec.field)}`);
// eslint-disable-next-line no-control-regex
assert.equal(/[\x00-\x1f\x7f-\x9f]/.test(rec.message), false, `control char survived into message: ${JSON.stringify(rec.message)}`);
}
}
});
test('anEnormousKeyNameIsClipped', () => {
const huge = 'k'.repeat(5000);
const [record] = collectReviewerWarningRecords(runtimeCapWithHostBehaviors({ [huge]: true }));
assert.ok(record, 'expected a record');
assert.ok(
record.field.length < MAX_REPORTED_KEY_CHARS + 40,
`field must be bounded, got length ${record.field.length}`,
);
assert.ok(record.field.endsWith('…'), 'a clipped key is marked as clipped');
});
test('aDeclaredKeyIsNeverClippedOrAltered', () => {
// The sanitizer must not perturb the ordinary case: declared keys are silent,
// and an undeclared but well-formed key is reported verbatim.
const records = collectReviewerWarningRecords(runtimeCapWithHostBehaviors({ someFutureSwitch: true }));
assert.equal(records.length, 1);
assert.equal(records[0].field, 'runtime.hostBehaviors.someFutureSwitch');
});
});