From 5ac550d62a155f0c416698ae938ebb29b1dae109 Mon Sep 17 00:00:00 2001 From: Mikael Hugo Date: Mon, 18 May 2026 00:36:40 +0200 Subject: [PATCH] fix(circular): break SF safety/autonomous-rollback chain (7-edge ring) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cycle was a clean 7-edge ring: preferences → preferences-models → uok/auto-runaway-guard → detectors/periodic-runner → detectors/crash-loop-classifier → last-green → experimental → preferences Three targeted cuts, each chosen for being a real architectural smell: 1. experimental → commands-prefs-wizard: the wizard was just re-routing the same `serializePreferencesToFrontmatter` import from preferences-serializer. experimental.js now imports from preferences-serializer directly. Edge removed. 2. crash-loop-classifier → safety/autonomous-rollback: detection should not directly trigger action — that couples concerns and creates the runtime cycle. Switched to a lazy `await import()` inside `crashLoopGate.execute()` (which is already async). The call site is unchanged from the caller's perspective; the runtime module-graph edge is gone. Walker skips dynamic imports. 3. preferences-models → uok/auto-runaway-guard: preferences-models only needed 6 runaway-threshold CONSTANTS, but pulling them from auto-runaway-guard dragged the whole detector/preferences/ experimental subsystem into the preferences-models graph. Extracted those 6 constants to a new leaf module uok/runaway-defaults.js. Both preferences-models and the guard import from there. auto-runaway-guard re-exports the constants so existing call sites keep working without churn. Net: 2 cycles → 1 cycle. 29/29 tests pass across the 5 touched modules (autonomous-rollback, experimental-flags, crash-loop- classifier detector, auto-runaway-guard, preferences-models). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../sf/detectors/crash-loop-classifier.js | 29 +++++++++++++++---- src/resources/extensions/sf/experimental.js | 6 +++- .../extensions/sf/preferences-models.js | 7 ++++- .../extensions/sf/uok/auto-runaway-guard.js | 27 +++++++++++++---- .../extensions/sf/uok/runaway-defaults.js | 25 ++++++++++++++++ 5 files changed, 81 insertions(+), 13 deletions(-) create mode 100644 src/resources/extensions/sf/uok/runaway-defaults.js diff --git a/src/resources/extensions/sf/detectors/crash-loop-classifier.js b/src/resources/extensions/sf/detectors/crash-loop-classifier.js index 79a5521f4..1f60f42cd 100644 --- a/src/resources/extensions/sf/detectors/crash-loop-classifier.js +++ b/src/resources/extensions/sf/detectors/crash-loop-classifier.js @@ -10,7 +10,21 @@ import { mkdirSync, readFileSync, renameSync, writeFileSync } from "node:fs"; import { join } from "node:path"; import { compareToLastGreen, getLastGreenEntry } from "../last-green.js"; -import { maybeRollbackCrashLoop } from "../safety/autonomous-rollback.js"; + +// `maybeRollbackCrashLoop` is imported lazily via dynamic `import()` to +// break the safety/autonomous-rollback → ... → crash-loop-classifier → +// safety/autonomous-rollback runtime cycle. The classifier's `execute()` +// is already async, so the lazy import has zero observable effect on +// callers. Dynamic imports are skipped by scripts/check-circular-deps.mjs +// since they don't run at module-graph evaluation time. +let _maybeRollbackCrashLoop = null; +async function loadMaybeRollbackCrashLoop() { + if (_maybeRollbackCrashLoop === null) { + const mod = await import("../safety/autonomous-rollback.js"); + _maybeRollbackCrashLoop = mod.maybeRollbackCrashLoop; + } + return _maybeRollbackCrashLoop; +} export const CRASH_LOOP_WINDOW_MS = 90_000; export const CRASH_LOOP_THRESHOLD = 3; @@ -106,10 +120,15 @@ export const crashLoopGate = { ctx.options, ); if (result.stuck) { - const rollback = - ctx.autoRollback === true || ctx.options?.autoRollback === true - ? maybeRollbackCrashLoop(ctx.basePath ?? ctx.cwd, result, ctx.options) - : null; + let rollback = null; + if (ctx.autoRollback === true || ctx.options?.autoRollback === true) { + const maybeRollbackCrashLoop = await loadMaybeRollbackCrashLoop(); + rollback = maybeRollbackCrashLoop( + ctx.basePath ?? ctx.cwd, + result, + ctx.options, + ); + } return { outcome: "fail", failureClass: "verification", diff --git a/src/resources/extensions/sf/experimental.js b/src/resources/extensions/sf/experimental.js index e25e1c5db..aae19c30e 100644 --- a/src/resources/extensions/sf/experimental.js +++ b/src/resources/extensions/sf/experimental.js @@ -10,7 +10,11 @@ import { existsSync, mkdirSync, readFileSync, writeFileSync } from "node:fs"; import { dirname } from "node:path"; -import { serializePreferencesToFrontmatter } from "./commands-prefs-wizard.js"; +// Import directly from the serializer module instead of going through +// commands-prefs-wizard. The wizard just re-routes this same import, and +// the detour created a runtime cycle: +// experimental → commands-prefs-wizard → preferences → ... → experimental +import { serializePreferencesToFrontmatter } from "./preferences-serializer.js"; import { getProjectSFPreferencesPath, loadEffectiveSFPreferences, diff --git a/src/resources/extensions/sf/preferences-models.js b/src/resources/extensions/sf/preferences-models.js index 6f374f644..bd9bf46a9 100644 --- a/src/resources/extensions/sf/preferences-models.js +++ b/src/resources/extensions/sf/preferences-models.js @@ -20,13 +20,18 @@ import { resolveProfileDefaults, } from "./preferences-profile.js"; import { sfHome } from "./sf-home.js"; +// Import runaway defaults from the leaf constants module rather than +// auto-runaway-guard, which would pull the entire detector/preferences/ +// experimental chain into the preferences-models module graph and form +// a runtime cycle (preferences-models → auto-runaway-guard → ... → +// preferences → preferences-models). import { DEFAULT_RUNAWAY_CHANGED_FILES_WARNING, DEFAULT_RUNAWAY_DIAGNOSTIC_TURNS, DEFAULT_RUNAWAY_ELAPSED_MINUTES, DEFAULT_RUNAWAY_TOKEN_WARNING, DEFAULT_RUNAWAY_TOOL_CALL_WARNING, -} from "./uok/auto-runaway-guard.js"; +} from "./uok/runaway-defaults.js"; import { getKeyManagerAuthStorage } from "./key-manager.js"; import { lookupDiscoveredModelCost } from "./model-catalog-cache.js"; diff --git a/src/resources/extensions/sf/uok/auto-runaway-guard.js b/src/resources/extensions/sf/uok/auto-runaway-guard.js index ea30175c7..595114ee0 100644 --- a/src/resources/extensions/sf/uok/auto-runaway-guard.js +++ b/src/resources/extensions/sf/uok/auto-runaway-guard.js @@ -20,12 +20,27 @@ import { detectorSweepSignal, zeroProgressSignal, } from "../supervision/loop-signals.js"; -export const DEFAULT_RUNAWAY_TOOL_CALL_WARNING = 60; -export const DEFAULT_RUNAWAY_TOKEN_WARNING = 1_000_000; -export const DEFAULT_RUNAWAY_ELAPSED_MINUTES = 20; -export const DEFAULT_RUNAWAY_CHANGED_FILES_WARNING = 75; -export const DEFAULT_RUNAWAY_DIAGNOSTIC_TURNS = 2; -export const DEFAULT_RUNAWAY_MIN_INTERVAL_MS = 120_000; +// Re-exported from the leaf module so existing call sites that do +// `import { DEFAULT_RUNAWAY_* } from "./uok/auto-runaway-guard.js"` +// keep working. The new canonical home is runaway-defaults.js — adding +// new defaults goes there, not inline here. The split exists to break the +// preferences-models → auto-runaway-guard → ... → preferences-models cycle. +export { + DEFAULT_RUNAWAY_CHANGED_FILES_WARNING, + DEFAULT_RUNAWAY_DIAGNOSTIC_TURNS, + DEFAULT_RUNAWAY_ELAPSED_MINUTES, + DEFAULT_RUNAWAY_MIN_INTERVAL_MS, + DEFAULT_RUNAWAY_TOKEN_WARNING, + DEFAULT_RUNAWAY_TOOL_CALL_WARNING, +} from "./runaway-defaults.js"; +import { + DEFAULT_RUNAWAY_CHANGED_FILES_WARNING, + DEFAULT_RUNAWAY_DIAGNOSTIC_TURNS, + DEFAULT_RUNAWAY_ELAPSED_MINUTES, + DEFAULT_RUNAWAY_MIN_INTERVAL_MS, + DEFAULT_RUNAWAY_TOKEN_WARNING, + DEFAULT_RUNAWAY_TOOL_CALL_WARNING, +} from "./runaway-defaults.js"; const EXECUTE_NO_PROGRESS_TOOL_WARNING = 25; const EXECUTE_NO_PROGRESS_TOKEN_WARNING = 500_000; const DURABLE_SF_ARTIFACT_PATHS = [".sf/milestones", ".sf/approvals"]; diff --git a/src/resources/extensions/sf/uok/runaway-defaults.js b/src/resources/extensions/sf/uok/runaway-defaults.js new file mode 100644 index 000000000..54b7a194a --- /dev/null +++ b/src/resources/extensions/sf/uok/runaway-defaults.js @@ -0,0 +1,25 @@ +/** + * runaway-defaults.js — default thresholds for the autonomous-loop runaway guard. + * + * Purpose: hold the runaway-guard defaults in a leaf module so both the guard + * implementation (uok/auto-runaway-guard.js) and the preferences resolver + * (preferences-models.js) can import from a single point WITHOUT pulling the + * full guard logic into the preferences module graph. + * + * Why this file exists: preferences-models needs only the numeric defaults to + * compute resolved-config fallbacks, but importing from auto-runaway-guard + * brought the entire detector/preferences/experimental subsystem into the + * preferences cycle. Splitting just the constants here lets both modules read + * the same source of truth without a runtime cycle. + * + * Consumer: uok/auto-runaway-guard.js (re-exports for internal call sites) and + * preferences-models.js (default-value lookups). Add NEW runaway-related + * constants here, not inline in the guard. + */ + +export const DEFAULT_RUNAWAY_TOOL_CALL_WARNING = 60; +export const DEFAULT_RUNAWAY_TOKEN_WARNING = 1_000_000; +export const DEFAULT_RUNAWAY_ELAPSED_MINUTES = 20; +export const DEFAULT_RUNAWAY_CHANGED_FILES_WARNING = 75; +export const DEFAULT_RUNAWAY_DIAGNOSTIC_TURNS = 2; +export const DEFAULT_RUNAWAY_MIN_INTERVAL_MS = 120_000;