Skip to content

engine(settings): normalizeCommandAuthorizationPolicy hands out the module-level DEFAULT command role arrays by reference #9998

Description

@JSONbored

⚠️ Definition of Done: this issue must be completed in full, in a single PR. Do not split this
work across multiple PRs, and do not defer any Deliverable below to a follow-up issue. A PR that
satisfies only some of the Deliverables, stubs a required test, or leaves a checkbox
partially-done does NOT resolve this issue and will be closed.

Context

normalizeCommandAuthorizationPolicy has two exit paths that disagree about who owns the returned object.

The non-record path deep-copies. packages/loopover-engine/src/settings/command-authorization.ts:67:

  if (!isRecord(input)) {
    if (input !== null && input !== undefined) warnings.push("commandAuthorization must be an object; using secure defaults.");
    return { policy: clonePolicy(DEFAULT_COMMAND_AUTHORIZATION_POLICY), warnings };
  }

clonePolicy (packages/loopover-engine/src/settings/command-authorization.ts:253) copies default and
every command's role array.

The record path does not. packages/loopover-engine/src/settings/command-authorization.ts:73:

  const commands: Record<string, CommandAuthorizationRole[]> = { ...DEFAULT_COMMAND_AUTHORIZATION_POLICY.commands };

That is a SHALLOW spread: every value is the same array instance held by the module-level
DEFAULT_COMMAND_AUTHORIZATION_POLICY (packages/loopover-engine/src/settings/command-authorization.ts:3),
which is a plain exported const and is not frozen. Only commands the caller explicitly overrode are
replaced with a fresh array (via normalizeRoleListdedupeRoles, which spreads a Set); every
un-overridden command keeps the shared reference.

So the following holds today and is directly assertable:

normalizeCommandAuthorizationPolicy({}).policy.commands.review === DEFAULT_COMMAND_AUTHORIZATION_POLICY.commands.review
// → true
normalizeCommandAuthorizationPolicy(null).policy.commands.review === DEFAULT_COMMAND_AUTHORIZATION_POLICY.commands.review
// → false

Two calls to the same function, one returning a policy the caller owns and one returning a policy whose
role arrays are process-global shared state. DEFAULT_COMMAND_AUTHORIZATION_POLICY is the security
vocabulary for the whole command surface — generate-tests is deliberately maintainer-only
(packages/loopover-engine/src/settings/command-authorization.ts:35-42), and
MAINTAINER_ONLY_DEFAULT_COMMANDS is derived from these very keys
(packages/loopover-engine/src/settings/command-authorization.ts:50) — so a single push through a
returned policy would widen that command for every repo handled by the same isolate, permanently, with no
config change and no audit trail.

The function is exported and reached with a record on the live settings-write path
(src/api/routes.ts:4802) and on every command-authorization read
(packages/loopover-engine/src/settings/command-authorization.ts:93), so the aliased arrays are handed to
real callers. No current caller mutates them, which is exactly why this is a latent correctness defect
rather than a live incident — and why the fix belongs in the normalizer, not in a convention that every
future caller has to remember.

Requirements

  • normalizeCommandAuthorizationPolicy must return a policy whose commands role arrays are all freshly
    allocated, on every input, including commands the caller did not override.
  • No array reachable from normalizeCommandAuthorizationPolicy(...).policy may be reference-identical to
    any array reachable from DEFAULT_COMMAND_AUTHORIZATION_POLICY, for ANY input.
  • DEFAULT_COMMAND_AUTHORIZATION_POLICY must additionally be made immutable at runtime: the object, its
    default array, its commands record, and every role array inside it must be Object.freezed, so a
    future aliasing regression fails loudly instead of silently corrupting the vocabulary. Its VALUES must not
    change.
  • What must NOT change: the resolved role lists for every input. normalizeCommandAuthorizationPolicy({})
    must still produce a commands record deep-equal to DEFAULT_COMMAND_AUTHORIZATION_POLICY.commands and
    a default deep-equal to DEFAULT_COMMAND_AUTHORIZATION_POLICY.default; every warning string, the
    /^[a-z][a-z-]{0,63}$/ command-key validation
    (packages/loopover-engine/src/settings/command-authorization.ts:78), the
    normalizeCommandRoleList maintainer-only clamp
    (packages/loopover-engine/src/settings/command-authorization.ts:195), and every
    evaluateCommandAuthorization decision must stay byte-identical.

⚠️ Required pattern: reuse the file's own clonePolicy
(packages/loopover-engine/src/settings/command-authorization.ts:253) as the seed for the record path
instead of the shallow spread — it already implements exactly the deep copy this path needs and is already
used by the sibling exit. What does NOT satisfy this issue: (a) deep-cloning at each CALL SITE in
src/api/routes.ts / src/queue/processors.ts rather than in the normalizer, which leaves the defect for
the next caller; (b) changing the return type to Readonly<...> / ReadonlyArray<...>, which is a public
API change pushed onto every consumer and does not stop a runtime push from a JS caller;
(c) freezing DEFAULT_COMMAND_AUTHORIZATION_POLICY alone and leaving the aliasing in place — a frozen
array handed out through a mutable-typed field turns a silent corruption into a strict-mode TypeError at
an unrelated call site; both halves are required; (d) a test-only PR.

Deliverables

  • packages/loopover-engine/src/settings/command-authorization.ts — the record path seeds commands
    from clonePolicy(DEFAULT_COMMAND_AUTHORIZATION_POLICY).commands (or an equivalent deep copy), not a
    shallow spread.
  • packages/loopover-engine/src/settings/command-authorization.ts
    DEFAULT_COMMAND_AUTHORIZATION_POLICY, its default array, its commands record, and every role
    array inside it are frozen.
  • A regression test at packages/loopover-engine/test/command-authorization.test.ts (this file does not
    exist yet; create it, importing from ../dist/settings/command-authorization.js per the convention in
    packages/loopover-engine/test/content-lane-flag.test.ts) named for this bug, asserting
    normalizeCommandAuthorizationPolicy({}).policy.commands["review"] is notStrictEqual to
    DEFAULT_COMMAND_AUTHORIZATION_POLICY.commands["review"] while remaining deepEqual to it.
  • The same test file asserts the two exit paths now agree: the same non-aliasing assertion holds for
    normalizeCommandAuthorizationPolicy(null) and for
    normalizeCommandAuthorizationPolicy({ commands: { plan: ["maintainer"] } }) (checking an
    un-overridden command such as pause, not just the overridden one).
  • The same test file asserts mutating a returned role array does not change
    DEFAULT_COMMAND_AUTHORIZATION_POLICY, and that
    Object.isFrozen(DEFAULT_COMMAND_AUTHORIZATION_POLICY.commands["generate-tests"]) is true.
  • The same test file asserts the preserved behaviour: commandAuthorizationAllowedRoles(null, "generate-tests")
    still resolves to exactly ["maintainer"], and evaluateCommandAuthorization for a COLLABORATOR
    commenter on generate-tests is still unauthorized.

All Deliverables above are required in a single PR. A PR that satisfies only some of them — for example one
that deep-copies in the normalizer but skips freezing DEFAULT_COMMAND_AUTHORIZATION_POLICY, so the next
regression is silent again — does not resolve this issue.

Test Coverage Requirements

This repo enforces 99%+ Codecov patch coverage, branch-counted. vitest.config.ts's
coverage.include covers src/**/*.ts and packages/loopover-engine/src/**/*.ts — the touched path
packages/loopover-engine/src/settings/command-authorization.ts IS measured.

Branches touched, each needing BOTH arms tested: the isRecord(input) guard (record → the fixed deep-seed
path; non-record → the existing clonePolicy path, and within it the input !== null && input !== undefined
warning arm both ways); the input.commands !== undefined guard; the isRecord(input.commands) guard; and
the /^[a-z][a-z-]{0,63}$/ key check (valid key → normalized and stored; malformed key → warning and
skipped, leaving the deep-copied default in place).

Engine lines are credited by two uploads whose hits are unioned — add the test to
packages/loopover-engine/test/** as well as any root test/** coverage, or the patch gate can still fail.
If a root test/** suite also exercises normalizeCommandAuthorizationPolicy, mirror the non-aliasing
assertions there too.

Expected Outcome

Every resolved RepositoryCommandAuthorizationPolicy is a fully owned object graph, so no caller can reach
through a per-repo policy into the process-wide command-authorization defaults, and the shipped defaults are
frozen so a future reintroduction of the aliasing fails immediately instead of silently widening a
maintainer-only command for every repo in the isolate.

Links & Resources

  • packages/loopover-engine/src/settings/command-authorization.ts:3 — the unfrozen module-level defaults
  • packages/loopover-engine/src/settings/command-authorization.ts:73 — the shallow spread that aliases them
  • packages/loopover-engine/src/settings/command-authorization.ts:69 — the sibling exit that deep-copies
  • packages/loopover-engine/src/settings/command-authorization.ts:253clonePolicy
  • packages/loopover-engine/src/settings/command-authorization.ts:35 — why generate-tests is maintainer-only
  • src/api/routes.ts:4802 — a live caller reaching the record path

Metadata

Metadata

Assignees

No one assigned

    Labels

    gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.help wantedExtra attention is needed

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions