Skip to content

review(auto-apply): give shadow overrides the same expired-clear_at handling as live overrides #10291

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

src/review/auto-apply.ts maintains two parallel tables of tunable-override rows: the LIVE table
(tunables_overrides) and the SHADOW table (tunables_overrides_shadow, a soak-gated staging area
that gets promoted to live once it passes validation). Both tables carry an operator-settable
clear_at column — a temporary override's own expiration timestamp.

The LIVE-side read/write pair correctly treats an already-lapsed clear_at as "cleared":

/** PURE: is a row's clear_at in the past relative to nowIso? (Skipped — not expired — when either is unset.)
 *  Extracted so writeLiveOverride's clear_at-preservation logic uses the exact same rule as rowToOverride. */
function clearAtIsExpired(clearAt: string | null, nowIso?: string): boolean {
  return !!(clearAt && nowIso && clearAt <= nowIso);
}

export async function loadOverride(env: StorageEnv, project: string, nowIso?: string): Promise<TunableOverride | null> {
  return rowToOverride(await loadOverrideRow(env, project), nowIso);
}

export async function writeLiveOverride(env: StorageEnv, project: string, o: TunableOverride, nowIso?: string): Promise<void> {
  const existingRow = await loadOverrideRow(env, project);
  const merged = mergeOverride(rowToOverride(existingRow, nowIso), o);
  const clearAt = existingRow && !clearAtIsExpired(existingRow.clear_at, nowIso) ? existingRow.clear_at : null;
  // ... INSERT OR REPLACE with `clearAt` (dropped when expired)
}

writeLiveOverride's own doc comment states this explicitly: it "[p]reserves any existing clear_at ...
UNLESS that clear_at has itself already lapsed, in which case it is dropped rather than resurrected".
This is covered by an existing test in test/unit/auto-apply.test.ts (line 574):
"does NOT resurrect an ALREADY-EXPIRED override (or its stale clear_at) when nowIso is passed".

The SHADOW-side sibling functions do not carry the same protection:

async function loadShadowOverrideRow(env: StorageEnv, project: string): Promise<(OverrideRow & { validated_until: string | null }) | null> {
  // ...
}

export async function writeShadowOverride(env: StorageEnv, project: string, o: TunableOverride, validatedUntilIso: string): Promise<void> {
  const existingRow = await loadShadowOverrideRow(env, project);
  const merged = mergeOverride(existingRow ? rowToOverride(existingRow) : null, o);
  const clearAt = existingRow?.clear_at ?? null;
  // ... INSERT OR REPLACE with `clearAt` (never dropped, regardless of expiry)
}

export async function loadShadowOverride(env: StorageEnv, project: string): Promise<ShadowOverride | null> {
  const row = await loadShadowOverrideRow(env, project);
  if (!row) return null;
  const override = rowToOverride(row);
  return override ? { override, validatedUntil: row.validated_until } : null;
}

Two concrete gaps versus the live-side pair:

  1. writeShadowOverride has no nowIso parameter at all, and calls rowToOverride(existingRow) with
    no second argument — so clearAtIsExpired always short-circuits to false inside rowToOverride
    (its nowIso && guard fails), meaning a shadow row's clear_at can never be treated as expired when
    merging in a new recommendation.
  2. writeShadowOverride's clearAt computation (existingRow?.clear_at ?? null) unconditionally
    re-persists whatever clear_at was already on the row, with no clearAtIsExpired check at all —
    unlike writeLiveOverride's explicit expiry check.
  3. loadShadowOverride also has no nowIso parameter and calls rowToOverride(row) with no second
    argument, so a shadow override with a lapsed clear_at is still read back as active.

writeShadowOverride's own doc comment even claims live-side parity — "Preserves any existing clear_at
rather than silently nulling it via INSERT OR REPLACE (#stale-clear-at-fix)" — but only ported the
"preserve the column" half of that fix, not the "drop it once expired" half. The existing shadow test
at test/unit/auto-apply.test.ts line 609 ("PRESERVES an existing clear_at across a shadow write
instead of silently nulling it") only exercises a FUTURE clear_at, so it does not defend against — or
even reveal — the missing expiry check.

Impact: loadShadowOverride is read by runAutoApplyRecommendations's promotion step
(src/review/auto-apply.ts) and by the gate-config/effective / live-gate-thresholds API and MCP
routes (src/api/routes.ts, src/mcp/server.ts). A shadow override whose clear_at has already
lapsed is treated as still active indefinitely, and a stale shadow tightening can still be promoted to
LIVE after its soak window even though its own operator-set expiration has already passed — silently
defeating the "operator's temporary-override expiration" semantics the code's own comments describe for
the live side.

Requirements

  • loadShadowOverride must accept an optional nowIso parameter (mirroring loadOverride's existing
    signature exactly) and thread it into rowToOverride so an expired clear_at on a shadow row causes
    the same "treated as cleared" behavior loadOverride already gives the live table.
  • writeShadowOverride must accept an optional nowIso parameter (mirroring writeLiveOverride's
    existing signature) and use it in exactly the same way writeLiveOverride does: compute
    clearAt as existingRow && !clearAtIsExpired(existingRow.clear_at, nowIso) ? existingRow.clear_at : null
    instead of the current unconditional existingRow?.clear_at ?? null, and pass nowIso into its own
    rowToOverride(existingRow, nowIso) call when merging.
  • Every existing caller of loadShadowOverride and writeShadowOverride must continue to compile and
    behave exactly as before when nowIso is omitted (both parameters are optional, matching the live
    side's own optional nowIso convention) — this is an additive signature change, not a breaking one.
  • Do not change any other behavior of the live-side loadOverride/writeLiveOverride pair, and do not
    change the promotion/soak-gate decision logic itself (runAutoApplyRecommendations or any function
    that decides WHETHER a shadow override gets promoted) — this issue is scoped strictly to making the
    shadow-side read/write pair's clear_at-expiry handling match the live-side pair's already-correct,
    already-tested behavior.

Deliverables

  • loadShadowOverride(env, project, nowIso?) treats an expired clear_at on a shadow row as
    cleared (returns null for that field the same way rowToOverride already does for the live
    table), verified by a new test mirroring test/unit/auto-apply.test.ts's existing "does NOT
    resurrect an ALREADY-EXPIRED override" test (line 574) but calling loadShadowOverride with a
    row whose clear_at is in the past and a nowIso after it, asserting the returned override is
    null (or has the expired tunable field dropped, matching rowToOverride's exact contract).
  • writeShadowOverride(env, project, o, validatedUntilIso, nowIso?) drops an already-expired
    clear_at instead of re-persisting it, verified by a new test mirroring the existing shadow
    clear_at-preservation test (line 609) but for an EXPIRED clear_at: write once with a
    clear_at in the past, write again with a nowIso after it, and assert the row's clear_at is
    null afterward (not the stale value).
  • The existing "PRESERVES an existing clear_at across a shadow write" test (line 609, a FUTURE
    clear_at) continues to pass unmodified — the fix must not regress the already-correct
    preserve-when-not-expired case.

All three Deliverables are required in the same PR — this is one parity gap (the shadow path missing
both halves of the live path's already-shipped fix) and a partial fix (e.g. only loadShadowOverride
or only writeShadowOverride) leaves the promotion path still able to read a stale override.

Test Coverage Requirements

This repo's Codecov patch gate requires 99%+ patch coverage on every changed line and branch under
src/**. src/review/auto-apply.ts is inside src/**, so this is fully gated. Both new tests above
must cover the actual expired-clear_at branch (not just the not-expired branch, which is already
covered) — a nowIso-omitted call path must also stay covered by the pre-existing tests that don't
pass nowIso, so both the with-nowIso and without-nowIso branches of the new optional-parameter
logic have real test coverage, matching the existing coverage shape for the live-side loadOverride/
writeLiveOverride pair.

Expected Outcome

loadShadowOverride and writeShadowOverride handle an expired clear_at identically to
loadOverride and writeLiveOverride — an operator's temporary shadow-queued override expires on
schedule and is neither read back as active nor silently re-persisted past its own clear_at, closing
the gap where a stale shadow tightening could still be promoted to live after its intended expiration.

Links & Resources

  • src/review/auto-apply.tsrowToOverride/clearAtIsExpired (around line 64), loadOverride/
    writeLiveOverride (around lines 208-225, the already-correct pair to mirror),
    loadShadowOverrideRow/writeShadowOverride/loadShadowOverride (around lines 262-297, the pair to
    fix).
  • test/unit/auto-apply.test.ts — the existing live-side expiry test (line 574) and the existing
    shadow-side preserve-on-write test (line 609), both to use as the template for the two new tests.

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