Skip to content

engine(miner): stop parseAmsPolicySpec handing out the frozen DEFAULT_AMS_POLICY_SPEC sub-objects by reference #9995

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

DEFAULT_AMS_POLICY_SPEC is a deep-frozen shared singleton and its own doc comment says so
(packages/loopover-engine/src/ams-policy-spec.ts:121):

/**
 * The safe defaults applied when a field is absent from `.loopover-ams.yml` (or the file itself is
 * missing). Deep-frozen: a shared singleton, clone before layering overrides on top.
 */
export const DEFAULT_AMS_POLICY_SPEC: Readonly<AmsPolicySpec> = Object.freeze({
  ...
  capLimits: Object.freeze({ budget: 5, turns: 20, elapsedMs: 1_800_000 }),
  convergenceThresholds: Object.freeze({ ...DEFAULT_PORTFOLIO_CONVERGENCE_THRESHOLDS }),

parseAmsPolicySpec does not clone before returning. Two of its field normalizers return the fallback
BY REFERENCE when the field is absent. packages/loopover-engine/src/ams-policy-spec.ts:210:

function normalizeCapLimits(value: unknown, fallback: AmsCapLimits, warnings: string[]): AmsCapLimits {
  if (value === undefined || value === null) return fallback;

and packages/loopover-engine/src/ams-policy-spec.ts:224:

function normalizeConvergenceThresholds(
  value: unknown,
  fallback: PortfolioConvergenceThresholds,
  warnings: string[],
): PortfolioConvergenceThresholds {
  if (value === undefined || value === null) return fallback;

Both are called with the frozen singleton's own sub-objects
(packages/loopover-engine/src/ams-policy-spec.ts:354):

    capLimits: normalizeCapLimits(record.capLimits, DEFAULT_AMS_POLICY_SPEC.capLimits, warnings),
    convergenceThresholds: normalizeConvergenceThresholds(
      record.convergenceThresholds,
      DEFAULT_AMS_POLICY_SPEC.convergenceThresholds,
      warnings,
    ),

So for any real config that omits those blocks — e.g. parseAmsPolicySpec({ submissionMode: "enforce" }),
which returns present: true — the resulting spec.capLimits IS
DEFAULT_AMS_POLICY_SPEC.capLimits, a frozen object, while ParsedAmsPolicySpec.spec is typed as the
fully mutable AmsPolicySpec (packages/loopover-engine/src/ams-policy-spec.ts:115). The engine package
emits ESM, which is strict-mode, so a caller layering an override — spec.capLimits.turns = 10 — throws
TypeError: Cannot assign to read only property 'turns' rather than being a plain assignment the type
system said was legal.

This is inconsistent with the same function's own siblings. emptyAmsPolicySpec
(packages/loopover-engine/src/ams-policy-spec.ts:156) returns cloneDefaultAmsPolicySpec(), which deep-copies
both blocks (packages/loopover-engine/src/ams-policy-spec.ts:143-144), and the no-configured-fields path at
packages/loopover-engine/src/ams-policy-spec.ts:380 does the same. So parseAmsPolicySpec(null) hands back
mutable capLimits and parseAmsPolicySpec({ submissionMode: "enforce" }) hands back frozen capLimits
two paths of one function with different ownership semantics.

The file already recognises this exact hazard for its array fields and fixes it there
(packages/loopover-engine/src/ams-policy-spec.ts:249):

  // A fresh copy, not `fallback` by reference: unlike this file's number-valued fields, an array is mutable,
  // so passing through the DEFAULT_AMS_POLICY_SPEC singleton's own array here would let a caller who mutates
  // their OWN resolved spec's list (e.g. `.push`) silently corrupt every other caller's shared defaults too.
  if (value === undefined || value === null) return [...fallback];

capLimits and convergenceThresholds are mutable-shaped objects for the same reason and were missed.

Requirements

  • normalizeCapLimits must return a fresh { budget, turns, elapsedMs } object on every path, including
    the undefined/null path and the not-a-mapping path.
  • normalizeConvergenceThresholds must return a fresh
    { maxConsecutiveFailures, maxReenqueues } object on every path, including the undefined/null path
    and the not-a-mapping path.
  • No object reachable from parseAmsPolicySpec(...).spec or parseAmsPolicySpecContent(...).spec may be
    reference-identical to DEFAULT_AMS_POLICY_SPEC or to any object reachable from it, for ANY input.
  • DEFAULT_AMS_POLICY_SPEC must remain frozen (including its capLimits, convergenceThresholds and
    networkAllowlist sub-objects) and its values must be unchanged.
  • What must NOT change: the parsed VALUES for every input. parseAmsPolicySpec({ submissionMode: "enforce" })
    must still produce capLimits: { budget: 5, turns: 20, elapsedMs: 1_800_000 } and
    convergenceThresholds equal to DEFAULT_PORTFOLIO_CONVERGENCE_THRESHOLDS; the present flag,
    the warning strings, and hasConfiguredPolicyFields's outcome must all stay byte-identical. The existing
    assertions in packages/loopover-engine/test/ams-policy-spec-parser.test.ts:136-146 must keep passing
    unchanged.

⚠️ Required pattern: mirror normalizeEcosystemList /
normalizeNetworkAllowlist at packages/loopover-engine/src/ams-policy-spec.ts:249 and :293, which
already copy the fallback on every return path for exactly this reason. What does NOT satisfy this issue:
(a) deep-cloning the whole returned spec at the end of parseAmsPolicySpec instead of fixing the two
normalizers — that hides the defect behind a second mechanism and leaves the normalizers wrong for any
future caller; (b) un-freezing DEFAULT_AMS_POLICY_SPEC or its sub-objects, which removes the protection
rather than the aliasing; (c) changing ParsedAmsPolicySpec.spec's type to Readonly<...>, which is a
public-API change that pushes the problem onto every consumer; (d) a test-only PR.

Deliverables

  • packages/loopover-engine/src/ams-policy-spec.tsnormalizeCapLimits and
    normalizeConvergenceThresholds return a fresh object on every path.
  • packages/loopover-engine/test/ams-policy-spec-parser.test.ts — a regression test named for this bug
    asserting parseAmsPolicySpec({ submissionMode: "enforce" }).spec.capLimits is notStrictEqual to
    DEFAULT_AMS_POLICY_SPEC.capLimits while remaining deepEqual to it, and the same pair of assertions
    for convergenceThresholds.
  • packages/loopover-engine/test/ams-policy-spec-parser.test.ts — an assertion that the returned
    sub-objects are writable: Object.isFrozen(parseAmsPolicySpec({ submissionMode: "enforce" }).spec.capLimits)
    is false, and mutating it does not change DEFAULT_AMS_POLICY_SPEC.capLimits.
  • packages/loopover-engine/test/ams-policy-spec-parser.test.ts — the same non-aliasing assertions for
    the not-a-mapping paths: parseAmsPolicySpec({ capLimits: "nope", convergenceThresholds: [] }).
  • packages/loopover-engine/test/ams-policy-spec-parser.test.ts — an assertion that
    DEFAULT_AMS_POLICY_SPEC and all three of its sub-objects (capLimits, convergenceThresholds,
    networkAllowlist) are still Object.isFrozen.

All Deliverables above are required in a single PR. A PR that satisfies only some of them — for example one
that fixes normalizeCapLimits but leaves normalizeConvergenceThresholds returning the frozen fallback —
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/ams-policy-spec.ts IS measured.

Branches touched, each needing BOTH arms tested, in BOTH normalizers: the value === undefined || value === null
arm (absent → fresh copy of the fallback) and its else arm (present → per-field normalization); and the
typeof value !== "object" || Array.isArray(value) arm (malformed → fresh copy plus a warning) and its else
arm. That is four branch arms per function; assert all eight.

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.

Expected Outcome

Every AmsPolicySpec returned by the parser is a fully owned, mutable object graph that matches its
declared type, so an operator-side caller layering a programmatic override on a resolved spec gets an
assignment rather than a strict-mode TypeError, and can never write through into the shared frozen
defaults. The two object-valued fields now follow the same fresh-copy discipline the file's array-valued
fields already document and apply.

Links & Resources

  • packages/loopover-engine/src/ams-policy-spec.ts:121DEFAULT_AMS_POLICY_SPEC and its "clone before layering overrides" contract
  • packages/loopover-engine/src/ams-policy-spec.ts:210normalizeCapLimits returning the frozen fallback
  • packages/loopover-engine/src/ams-policy-spec.ts:224normalizeConvergenceThresholds returning the frozen fallback
  • packages/loopover-engine/src/ams-policy-spec.ts:249 — the fresh-copy precedent to mirror
  • packages/loopover-engine/src/ams-policy-spec.ts:354 — the call sites passing the singleton's sub-objects
  • packages/loopover-engine/test/ams-policy-spec-parser.test.ts:136 — the existing capLimits suite to extend

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