From 67d68e2684ff8c2d3f7bae3167b839ddfe1e44af Mon Sep 17 00:00:00 2001 From: Iouri Goussev Date: Mon, 13 Apr 2026 08:13:43 -0400 Subject: [PATCH] fix(gsd): break 3 circular dependencies in extension modules (#3730) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1 of #3631 — eliminate circular imports before screaming arch reorg. Cycle 1 (auto.ts ↔ auto-direct-dispatch.ts): Remove redundant re-export of dispatchDirectPhase from auto.ts. No consumer imported it through auto.ts. Cycle 2 (context-injector.ts ↔ custom-workflow-engine.ts): Extract readFrozenDefinition to new definition-io.ts. context-injector now imports from definition-io directly. Cycle 3 (preferences.ts ↔ preferences-skills.ts): Move formatSkillRef to preferences-types.ts (pure fn, depends only on SkillResolution which is already there). Move resolveSkillDiscoveryMode + resolveSkillStalenessDays into preferences.ts (trivial wrappers over loadEffectiveGSDPreferences). Tests: new definition-io.test.ts (3 tests), preferences-formatting.test.ts (6 tests covering all formatSkillRef branches). Co-authored-by: Claude Opus 4.6 (1M context) --- src/resources/extensions/gsd/auto.ts | 3 - .../extensions/gsd/context-injector.ts | 2 +- .../extensions/gsd/custom-workflow-engine.ts | 12 +-- src/resources/extensions/gsd/definition-io.ts | 18 ++++ .../extensions/gsd/preferences-skills.ts | 38 +------- .../extensions/gsd/preferences-types.ts | 16 ++++ src/resources/extensions/gsd/preferences.ts | 22 +++-- .../gsd/tests/definition-io.test.ts | 57 ++++++++++++ .../gsd/tests/preferences-formatting.test.ts | 87 +++++++++++++++++++ 9 files changed, 201 insertions(+), 54 deletions(-) create mode 100644 src/resources/extensions/gsd/definition-io.ts create mode 100644 src/resources/extensions/gsd/tests/definition-io.test.ts create mode 100644 src/resources/extensions/gsd/tests/preferences-formatting.test.ts diff --git a/src/resources/extensions/gsd/auto.ts b/src/resources/extensions/gsd/auto.ts index 59b84f362..f2e353f1f 100644 --- a/src/resources/extensions/gsd/auto.ts +++ b/src/resources/extensions/gsd/auto.ts @@ -1717,9 +1717,6 @@ export async function dispatchHookUnit( return true; } -// Direct phase dispatch → auto-direct-dispatch.ts -export { dispatchDirectPhase } from "./auto-direct-dispatch.js"; - // Re-export recovery functions for external consumers export { buildLoopRemediationSteps, diff --git a/src/resources/extensions/gsd/context-injector.ts b/src/resources/extensions/gsd/context-injector.ts index 00dcae2c3..c5b90b752 100644 --- a/src/resources/extensions/gsd/context-injector.ts +++ b/src/resources/extensions/gsd/context-injector.ts @@ -16,7 +16,7 @@ import { readFileSync, existsSync } from "node:fs"; import { join, resolve, sep } from "node:path"; import type { StepDefinition } from "./definition-loader.js"; -import { readFrozenDefinition } from "./custom-workflow-engine.js"; +import { readFrozenDefinition } from "./definition-io.js"; /** Maximum characters per artifact to prevent context window blowout. */ const MAX_CONTEXT_CHARS = 10_000; diff --git a/src/resources/extensions/gsd/custom-workflow-engine.ts b/src/resources/extensions/gsd/custom-workflow-engine.ts index 0b1266326..53d520cb9 100644 --- a/src/resources/extensions/gsd/custom-workflow-engine.ts +++ b/src/resources/extensions/gsd/custom-workflow-engine.ts @@ -22,7 +22,6 @@ import type { } from "./engine-types.js"; import { readFileSync } from "node:fs"; import { join } from "node:path"; -import { parse } from "yaml"; import { readGraph, writeGraph, @@ -32,16 +31,13 @@ import { type WorkflowGraph, } from "./graph.js"; import { injectContext } from "./context-injector.js"; -import type { WorkflowDefinition, StepDefinition } from "./definition-loader.js"; +import type { StepDefinition } from "./definition-loader.js"; +import { readFrozenDefinition } from "./definition-io.js"; import { parseUnitId } from "./unit-id.js"; import { withFileLock } from "./file-lock.js"; -/** Read and parse the frozen DEFINITION.yaml from a run directory. */ -export function readFrozenDefinition(runDir: string): WorkflowDefinition { - const defPath = join(runDir, "DEFINITION.yaml"); - const raw = readFileSync(defPath, "utf-8"); - return parse(raw, { schema: "core" }) as WorkflowDefinition; -} +// Re-export for downstream consumers +export { readFrozenDefinition } from "./definition-io.js"; export class CustomWorkflowEngine implements WorkflowEngine { readonly engineId = "custom"; diff --git a/src/resources/extensions/gsd/definition-io.ts b/src/resources/extensions/gsd/definition-io.ts new file mode 100644 index 000000000..ac0ed9a42 --- /dev/null +++ b/src/resources/extensions/gsd/definition-io.ts @@ -0,0 +1,18 @@ +/** + * definition-io.ts — Read frozen DEFINITION.yaml from a run directory. + * + * Extracted from custom-workflow-engine.ts to break the circular dependency + * between context-injector.ts and custom-workflow-engine.ts. + */ + +import { readFileSync } from "node:fs"; +import { join } from "node:path"; +import { parse } from "yaml"; +import type { WorkflowDefinition } from "./definition-loader.js"; + +/** Read and parse the frozen DEFINITION.yaml from a run directory. */ +export function readFrozenDefinition(runDir: string): WorkflowDefinition { + const defPath = join(runDir, "DEFINITION.yaml"); + const raw = readFileSync(defPath, "utf-8"); + return parse(raw, { schema: "core" }) as WorkflowDefinition; +} diff --git a/src/resources/extensions/gsd/preferences-skills.ts b/src/resources/extensions/gsd/preferences-skills.ts index d930ba0b4..30b286d4c 100644 --- a/src/resources/extensions/gsd/preferences-skills.ts +++ b/src/resources/extensions/gsd/preferences-skills.ts @@ -17,7 +17,6 @@ import type { SkillResolutionReport, } from "./preferences-types.js"; import { validatePreferences } from "./preferences-validation.js"; -import { loadEffectiveGSDPreferences } from "./preferences.js"; // Re-export types so existing consumers of ./preferences-skills.js keep working export type { GSDSkillRule, SkillDiscoveryMode, SkillResolution, SkillResolutionReport } from "./preferences-types.js"; @@ -143,38 +142,5 @@ export function resolveAllSkillReferences(preferences: GSDPreferences, cwd: stri return { resolutions, warnings }; } -/** - * Format a skill reference for the system prompt. - * If resolved, shows the path so the agent knows exactly where to read. - * If unresolved, marks it clearly. - */ -export function formatSkillRef(ref: string, resolutions: Map): string { - const resolution = resolutions.get(ref); - if (!resolution || resolution.method === "unresolved") { - return `${ref} (⚠ not found — check skill name or path)`; - } - // For absolute paths where SKILL.md is just appended, don't clutter the output - if (resolution.method === "absolute-path" || resolution.method === "absolute-dir") { - return ref; - } - // For bare names resolved from skill directories, show the resolved path - return `${ref} → \`${resolution.resolvedPath}\``; -} - -/** - * Resolve the skill discovery mode from effective preferences. - * Defaults to "suggest" -- skills are identified during research but not installed automatically. - */ -export function resolveSkillDiscoveryMode(): SkillDiscoveryMode { - const prefs = loadEffectiveGSDPreferences(); - return prefs?.preferences.skill_discovery ?? "suggest"; -} - -/** - * Resolve the skill staleness threshold in days. - * Returns 0 if disabled, default 60 if not configured. - */ -export function resolveSkillStalenessDays(): number { - const prefs = loadEffectiveGSDPreferences(); - return prefs?.preferences.skill_staleness_days ?? 60; -} +// resolveSkillDiscoveryMode and resolveSkillStalenessDays moved to +// preferences.ts to break circular dependency (they need loadEffectiveGSDPreferences). diff --git a/src/resources/extensions/gsd/preferences-types.ts b/src/resources/extensions/gsd/preferences-types.ts index 47ed0c12b..75aac4a0c 100644 --- a/src/resources/extensions/gsd/preferences-types.ts +++ b/src/resources/extensions/gsd/preferences-types.ts @@ -384,3 +384,19 @@ export interface SkillResolutionReport { /** References that could not be resolved. */ warnings: string[]; } + +/** + * Format a skill reference for the system prompt. + * If resolved, shows the path so the agent knows exactly where to read. + * If unresolved, marks it clearly. + */ +export function formatSkillRef(ref: string, resolutions: Map): string { + const resolution = resolutions.get(ref); + if (!resolution || resolution.method === "unresolved") { + return `${ref} (⚠ not found — check skill name or path)`; + } + if (resolution.method === "absolute-path" || resolution.method === "absolute-dir") { + return ref; + } + return `${ref} → \`${resolution.resolvedPath}\``; +} diff --git a/src/resources/extensions/gsd/preferences.ts b/src/resources/extensions/gsd/preferences.ts index 77d658fdd..7a7ac6751 100644 --- a/src/resources/extensions/gsd/preferences.ts +++ b/src/resources/extensions/gsd/preferences.ts @@ -29,9 +29,10 @@ import { type GSDPreferences, type LoadedGSDPreferences, type SkillResolution, + type SkillDiscoveryMode, + formatSkillRef, } from "./preferences-types.js"; import { validatePreferences } from "./preferences-validation.js"; -import { formatSkillRef } from "./preferences-skills.js"; // ─── Re-exports: types ────────────────────────────────────────────────────── // Every type/interface that was previously exported from this file is @@ -60,11 +61,20 @@ export type { export { validatePreferences } from "./preferences-validation.js"; // ─── Re-exports: skills ───────────────────────────────────────────────────── -export { - resolveAllSkillReferences, - resolveSkillDiscoveryMode, - resolveSkillStalenessDays, -} from "./preferences-skills.js"; +export { resolveAllSkillReferences } from "./preferences-skills.js"; + +// These lived in preferences-skills.ts but imported loadEffectiveGSDPreferences +// back from this file, creating a circular dependency. Moved here since they +// are trivial wrappers over loadEffectiveGSDPreferences. +export function resolveSkillDiscoveryMode(): SkillDiscoveryMode { + const prefs = loadEffectiveGSDPreferences(); + return prefs?.preferences.skill_discovery ?? "suggest"; +} + +export function resolveSkillStalenessDays(): number { + const prefs = loadEffectiveGSDPreferences(); + return prefs?.preferences.skill_staleness_days ?? 60; +} // ─── Re-exports: models ───────────────────────────────────────────────────── export { diff --git a/src/resources/extensions/gsd/tests/definition-io.test.ts b/src/resources/extensions/gsd/tests/definition-io.test.ts new file mode 100644 index 000000000..bbf9b793f --- /dev/null +++ b/src/resources/extensions/gsd/tests/definition-io.test.ts @@ -0,0 +1,57 @@ +/** + * definition-io.ts — unit tests for readFrozenDefinition. + */ + +import { describe, test, beforeEach, afterEach } from "node:test"; +import assert from "node:assert/strict"; +import { mkdtempSync, mkdirSync, writeFileSync, rmSync, realpathSync } from "node:fs"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; + +import { readFrozenDefinition } from "../definition-io.ts"; + +function createTmpDir(): string { + return realpathSync(mkdtempSync(join(tmpdir(), "gsd-defio-test-"))); +} + +describe("readFrozenDefinition", () => { + let runDir: string; + + beforeEach(() => { + runDir = createTmpDir(); + }); + + afterEach(() => { + rmSync(runDir, { recursive: true, force: true }); + }); + + test("parses a valid DEFINITION.yaml", () => { + const yaml = [ + "version: 1", + "name: test-workflow", + "description: A test workflow", + "steps:", + " - id: step-1", + " prompt: do the thing", + ].join("\n"); + writeFileSync(join(runDir, "DEFINITION.yaml"), yaml, "utf-8"); + + const def = readFrozenDefinition(runDir); + assert.equal(def.version, 1); + assert.equal(def.name, "test-workflow"); + assert.equal(def.description, "A test workflow"); + assert.equal(def.steps.length, 1); + assert.equal(def.steps[0].id, "step-1"); + }); + + test("throws when DEFINITION.yaml is missing", () => { + assert.throws(() => readFrozenDefinition(runDir), { + code: "ENOENT", + }); + }); + + test("throws on malformed YAML", () => { + writeFileSync(join(runDir, "DEFINITION.yaml"), ": : : not valid yaml [", "utf-8"); + assert.throws(() => readFrozenDefinition(runDir)); + }); +}); diff --git a/src/resources/extensions/gsd/tests/preferences-formatting.test.ts b/src/resources/extensions/gsd/tests/preferences-formatting.test.ts new file mode 100644 index 000000000..f14a7a16e --- /dev/null +++ b/src/resources/extensions/gsd/tests/preferences-formatting.test.ts @@ -0,0 +1,87 @@ +/** + * Tests for formatSkillRef — pure formatting logic for skill references + * in the system prompt. Moved from preferences-skills.ts to preferences-types.ts + * to break the preferences ↔ preferences-skills circular dependency. + */ + +import { describe, test } from "node:test"; +import assert from "node:assert/strict"; + +import { formatSkillRef } from "../preferences-types.ts"; +import type { SkillResolution } from "../preferences-types.ts"; + +function makeResolutions(entries: [string, Partial][]): Map { + const map = new Map(); + for (const [key, partial] of entries) { + map.set(key, { + original: partial.original ?? key, + resolvedPath: partial.resolvedPath ?? null, + method: partial.method ?? "unresolved", + }); + } + return map; +} + +describe("formatSkillRef", () => { + test("marks unresolved references with a warning", () => { + const resolutions = makeResolutions([ + ["my-skill", { method: "unresolved" }], + ]); + const result = formatSkillRef("my-skill", resolutions); + assert.match(result, /my-skill/); + assert.match(result, /not found/); + }); + + test("marks unknown references (not in map) with a warning", () => { + const resolutions = new Map(); + const result = formatSkillRef("unknown-skill", resolutions); + assert.match(result, /unknown-skill/); + assert.match(result, /not found/); + }); + + test("returns bare ref for absolute-path resolution", () => { + const resolutions = makeResolutions([ + ["/home/user/skills/SKILL.md", { + method: "absolute-path", + resolvedPath: "/home/user/skills/SKILL.md", + }], + ]); + const result = formatSkillRef("/home/user/skills/SKILL.md", resolutions); + assert.equal(result, "/home/user/skills/SKILL.md"); + }); + + test("returns bare ref for absolute-dir resolution", () => { + const resolutions = makeResolutions([ + ["/home/user/skills/my-skill", { + method: "absolute-dir", + resolvedPath: "/home/user/skills/my-skill/SKILL.md", + }], + ]); + const result = formatSkillRef("/home/user/skills/my-skill", resolutions); + assert.equal(result, "/home/user/skills/my-skill"); + }); + + test("shows resolved path for user-skill resolution", () => { + const resolutions = makeResolutions([ + ["code-review", { + method: "user-skill", + resolvedPath: "/home/user/.claude/skills/code-review/SKILL.md", + }], + ]); + const result = formatSkillRef("code-review", resolutions); + assert.match(result, /code-review/); + assert.match(result, /\.claude\/skills\/code-review\/SKILL\.md/); + }); + + test("shows resolved path for project-skill resolution", () => { + const resolutions = makeResolutions([ + ["lint-fix", { + method: "project-skill", + resolvedPath: "/repo/.gsd/skills/lint-fix/SKILL.md", + }], + ]); + const result = formatSkillRef("lint-fix", resolutions); + assert.match(result, /lint-fix/); + assert.match(result, /\.gsd\/skills\/lint-fix\/SKILL\.md/); + }); +});