diff --git a/code/cli/src/cli/commands/SyncCommand.ts b/code/cli/src/cli/commands/SyncCommand.ts index ae550d63..e2e0c32e 100644 --- a/code/cli/src/cli/commands/SyncCommand.ts +++ b/code/cli/src/cli/commands/SyncCommand.ts @@ -5,6 +5,7 @@ import { Command } from 'commander'; import { remoteSourceLayer } from '../../resolver/Layer.js'; import { resolveResources } from '../../resolver/Resolver.js'; +import { resolveEffectiveSet } from '../../resolver/ResolverContext.js'; import { validateEffectiveSet } from '../../resolver/ResolverValidation.js'; import { createSettingsLoadPlan, @@ -350,7 +351,9 @@ export const executeSyncCommand = ( directRemoteSources, sourcePhase, }); - return finishSync(merged.issues, remoteSettingsPhase, sourcePhase, transitiveClosure); + const result = finishSync(merged.issues, remoteSettingsPhase, sourcePhase, transitiveClosure); + const ambiguityWarnings = resolveEffectiveSet(input).ambiguityWarnings.map((warning) => `warning: ${warning}`); + return { ...result, messages: [...result.messages, ...ambiguityWarnings] }; }; export const createSyncCommand = (dependencies: SyncCommandDependencies = {}): CommandObject => ({ diff --git a/code/cli/src/resolver/AmbiguityWarnings.ts b/code/cli/src/resolver/AmbiguityWarnings.ts new file mode 100644 index 00000000..075dab00 --- /dev/null +++ b/code/cli/src/resolver/AmbiguityWarnings.ts @@ -0,0 +1,137 @@ +// Reports source-ref and resource-slug disagreements without changing deterministic precedence. +import type { LoadedSettingsFile } from '../settings/SettingsLoader.js'; +import type { SourceReference } from '../settings/Settings.js'; +import { + encodeRemoteSourceSelection, + isRemoteSource, + normalizeGitUri, + normalizeRemoteSourceUri, + redactSourceUriCredentials, +} from '../sources/SourceCache.js'; +import type { RemoteSourceReference } from '../sources/SourceCache.js'; +import type { DeclaredRemoteSource } from '../sources/TransitiveSources.js'; +import type { EffectiveResourceSet, ResolvedResource, ResourceKind } from './Resource.js'; + +interface SourceDeclaration { + readonly source: RemoteSourceReference; + readonly declaredBy: string; +} + +const repositoryKey = (source: RemoteSourceReference): string => + redactSourceUriCredentials(normalizeGitUri(normalizeRemoteSourceUri(source))); + +const repositoryDisplay = (source: RemoteSourceReference): string => { + if (source.github !== undefined) return `github:${source.github}`; + return redactSourceUriCredentials(normalizeGitUri(source.uri)); +}; + +const refDisplay = (source: RemoteSourceReference): string => source.ref ?? '(default)'; + +const directDeclarations = (files: readonly LoadedSettingsFile[]): readonly SourceDeclaration[] => + [...files] + .reverse() + .flatMap((file) => + (file.settings.sources ?? []) + .filter(isRemoteSource) + .map((source) => ({ source, declaredBy: file.location.path })), + ); + +const replacedSourceListWarnings = ( + files: readonly LoadedSettingsFile[], + effectiveSources: readonly SourceReference[], +): readonly string[] => { + const replacingIndex = files.findLastIndex((file) => file.settings.sources !== undefined); + if (replacingIndex < 1) return []; + + const replacingFile = files[replacingIndex]; + const effectiveRepositories = new Set(effectiveSources.filter(isRemoteSource).map(repositoryKey)); + const reportedRepositories = new Set(); + const warnings: string[] = []; + + for (const file of files.slice(0, replacingIndex).reverse()) { + for (const source of (file.settings.sources ?? []).filter(isRemoteSource)) { + const key = repositoryKey(source); + if (effectiveRepositories.has(key) || reportedRepositories.has(key)) continue; + reportedRepositories.add(key); + warnings.push( + `source '${repositoryDisplay(source)}' declared by '${file.location.path}' was replaced by '${replacingFile.location.path}' and is not in the effective configuration`, + ); + } + } + + return warnings; +}; + +const selectedDeclarations = ( + effectiveSources: readonly SourceReference[], + direct: readonly SourceDeclaration[], + transitive: readonly DeclaredRemoteSource[], +): readonly SourceDeclaration[] => { + const selectedDirect = effectiveSources.filter(isRemoteSource).map((source) => { + const selection = encodeRemoteSourceSelection(source); + // Every effective direct source came from one of the loaded files used to produce settings. + return direct.find((entry) => encodeRemoteSourceSelection(entry.source) === selection)!; + }); + return [...selectedDirect, ...transitive]; +}; + +export const sourceRefAmbiguityWarnings = ( + files: readonly LoadedSettingsFile[], + effectiveSources: readonly SourceReference[], + transitiveDeclarations: readonly DeclaredRemoteSource[], +): readonly string[] => { + const direct = directDeclarations(files); + const declarations: readonly SourceDeclaration[] = [...direct, ...transitiveDeclarations]; + const selected = selectedDeclarations(effectiveSources, direct, transitiveDeclarations); + const byRepository = new Map(); + + for (const declaration of declarations) { + const key = repositoryKey(declaration.source); + const entries = byRepository.get(key) ?? []; + entries.push(declaration); + byRepository.set(key, entries); + } + + const warnings: string[] = [...replacedSourceListWarnings(files, effectiveSources)]; + for (const entries of byRepository.values()) { + if (new Set(entries.map((entry) => entry.source.ref)).size < 2) continue; + const key = repositoryKey(entries[0].source); + const winner = selected.find((entry) => repositoryKey(entry.source) === key); + if (winner === undefined) continue; + const declarationsText = entries + .map((entry) => `'${entry.declaredBy}' declares ref '${refDisplay(entry.source)}'`) + .join('; '); + warnings.push( + `Ambiguous source repository '${repositoryDisplay(entries[0].source)}': ${declarationsText}; declaration from '${winner.declaredBy}' at ref '${refDisplay(winner.source)}' won.`, + ); + } + + return warnings; +}; + +const slugWarning = (kind: ResourceKind, resource: ResolvedResource, context?: string): string | undefined => { + const definitions = [resource.winner, ...resource.shadowed]; + const labels = [...new Set(definitions.map((definition) => definition.layer.label))]; + if (labels.length < 2) return undefined; + return `Ambiguous ${kind} slug '${resource.slug}'${context ?? ''} is supplied by ${labels.map((label) => `'${label}'`).join(', ')}; '${resource.winner.layer.label}' won.`; +}; + +const resourceWarnings = ( + kind: ResourceKind, + resources: Iterable | undefined, + context?: string, +): readonly string[] => + resources === undefined + ? [] + : [...resources].flatMap((resource) => { + const warning = slugWarning(kind, resource, context); + return warning === undefined ? [] : [warning]; + }); + +export const slugAmbiguityWarnings = (set: EffectiveResourceSet): readonly string[] => [ + ...resourceWarnings('agent', set.resources.get('agent')?.values()), + ...resourceWarnings('skill', set.resources.get('skill')?.values()), + ...[...set.agentResources].flatMap(([agent, kinds]) => + resourceWarnings('skill', kinds.get('skill')?.values(), ` for agent '${agent}'`), + ), +]; diff --git a/code/cli/src/resolver/Layer.ts b/code/cli/src/resolver/Layer.ts index 68612d46..708075a3 100644 --- a/code/cli/src/resolver/Layer.ts +++ b/code/cli/src/resolver/Layer.ts @@ -11,6 +11,7 @@ import { } from '../sources/SourceCache.js'; import type { RemoteSourceReference } from '../sources/SourceCache.js'; import { expandTransitiveSources } from '../sources/TransitiveSources.js'; +import type { DeclaredRemoteSource } from '../sources/TransitiveSources.js'; import type { Settings, SourceReference } from '../settings/Settings.js'; import type { Layer } from './Resource.js'; @@ -42,6 +43,8 @@ const sourceLayer = (input: LayerDiscoveryInput, source: SourceReference): Layer export interface LayerDiscoveryResult { readonly layers: readonly Layer[]; + /** Accepted transitive declarations, including duplicates, retained for ambiguity diagnostics. */ + readonly transitiveDeclarations: readonly DeclaredRemoteSource[]; /** * Configured remote sources whose cache is absent, reported with `outfitter sync` guidance rather * than silently dropped (OFTR-004.2.18). Reported, never fatal: a private catalog the enterprise @@ -113,6 +116,7 @@ export const discoverLayers = (input: LayerDiscoveryInput): LayerDiscoveryResult return { layers: candidates.filter((layer) => existsSync(layer.root)), + transitiveDeclarations: expansion.declarations, unsynchronized, warnings: [...invalid, ...expansion.warnings], }; diff --git a/code/cli/src/resolver/ResolverContext.ts b/code/cli/src/resolver/ResolverContext.ts index 390bf611..eb376b79 100644 --- a/code/cli/src/resolver/ResolverContext.ts +++ b/code/cli/src/resolver/ResolverContext.ts @@ -2,6 +2,7 @@ import { loadSettingsWithCachedRemoteSettings } from '../settings/SettingsLoader.js'; import type { SettingsLoadIssue } from '../settings/SettingsLoader.js'; import type { Settings } from '../settings/Settings.js'; +import { slugAmbiguityWarnings, sourceRefAmbiguityWarnings } from './AmbiguityWarnings.js'; import { discoverLayers } from './Layer.js'; import type { EffectiveResourceSet } from './Resource.js'; import { resolveResources } from './Resolver.js'; @@ -18,17 +19,30 @@ export interface ResolveResult { readonly settingsIssues: readonly SettingsLoadIssue[]; /** Non-fatal guidance: uncached remote sources plus transitive-source skip warnings. */ readonly warnings: readonly string[]; + /** Ambiguity-only subset, exposed so sync can report shared resolution diagnostics after fetching. */ + readonly ambiguityWarnings: readonly string[]; } /** The single shared resolution path used by list, validate, run, and dump. */ export const resolveEffectiveSet = (input: ResolveInput): ResolveResult => { const loadedSettings = loadSettingsWithCachedRemoteSettings(input); const discovered = discoverLayers({ ...input, settings: loadedSettings.settings }); + const set = resolveResources(discovered.layers); + const ambiguityWarnings = [ + ...sourceRefAmbiguityWarnings( + loadedSettings.files, + // Settings merging always materializes the default empty source list. + loadedSettings.settings.sources!, + discovered.transitiveDeclarations, + ), + ...slugAmbiguityWarnings(set), + ]; return { - set: resolveResources(discovered.layers), + set, settings: loadedSettings.settings, settingsIssues: loadedSettings.issues, - warnings: [...discovered.unsynchronized, ...discovered.warnings], + warnings: [...discovered.unsynchronized, ...discovered.warnings, ...ambiguityWarnings], + ambiguityWarnings, }; }; diff --git a/code/cli/src/sources/TransitiveSources.ts b/code/cli/src/sources/TransitiveSources.ts index dfc57454..9b6526d0 100644 --- a/code/cli/src/sources/TransitiveSources.ts +++ b/code/cli/src/sources/TransitiveSources.ts @@ -158,6 +158,8 @@ export interface TransitiveExpansionInput { export interface TransitiveExpansionResult { /** Newly discovered remote sources, breadth-first by depth then declaration order (OFTR-004.6.3). */ readonly sources: readonly DeclaredRemoteSource[]; + /** Every accepted declaration, including duplicates suppressed from the resolved source graph. */ + readonly declarations: readonly DeclaredRemoteSource[]; readonly warnings: readonly string[]; } @@ -190,6 +192,7 @@ const declarationsFromParent = ( export const expandTransitiveSources = (input: TransitiveExpansionInput): TransitiveExpansionResult => { const sources: DeclaredRemoteSource[] = []; + const declarations: DeclaredRemoteSource[] = []; const warnings: string[] = []; const visited = new Set(); @@ -214,6 +217,7 @@ export const expandTransitiveSources = (input: TransitiveExpansionInput): Transi warnings.push(...declared.warnings); for (const entry of declared.sources) { + declarations.push(entry); const key = encodeRemoteSourceSelection(entry.source); if (visited.has(key)) continue; visited.add(key); @@ -225,5 +229,5 @@ export const expandTransitiveSources = (input: TransitiveExpansionInput): Transi frontier = next; } - return { sources, warnings }; + return { sources, declarations, warnings }; }; diff --git a/code/cli/tests/unit/ambiguity-warnings.test.ts b/code/cli/tests/unit/ambiguity-warnings.test.ts new file mode 100644 index 00000000..2703c92d --- /dev/null +++ b/code/cli/tests/unit/ambiguity-warnings.test.ts @@ -0,0 +1,174 @@ +import { mkdtempSync, mkdirSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { dirname, join } from 'node:path'; + +import { afterEach, describe, expect, it } from 'vitest'; + +import { executeListCommand } from '../../src/cli/commands/ListCommand.js'; +import { executeRunAgentCommand } from '../../src/cli/commands/RunAgentCommand.js'; +import { executeSyncCommand } from '../../src/cli/commands/SyncCommand.js'; +import { executeValidateCommand } from '../../src/cli/commands/ValidateCommand.js'; +import { findResource } from '../../src/resolver/Resource.js'; +import { resolveEffectiveSet } from '../../src/resolver/ResolverContext.js'; + +const temporaryRoots: string[] = []; + +const createTemporaryRoot = (): string => { + const root = mkdtempSync(join(tmpdir(), 'outfitter-ambiguity-')); + temporaryRoots.push(root); + return root; +}; + +const write = (path: string, content: string): void => { + mkdirSync(dirname(path), { recursive: true }); + writeFileSync(path, content); +}; + +const resource = (root: string, kind: 'agents' | 'skills', slug: string): void => { + const filename = kind === 'agents' ? 'agent.md' : 'SKILL.md'; + write(join(root, kind, slug, filename), `---\nname: ${slug}\n---\n\n${root}\n`); +}; + +const resolve = (root: string) => + resolveEffectiveSet({ homeDirectory: join(root, 'home'), projectDirectory: join(root, 'project') }); + +afterEach(() => { + for (const root of temporaryRoots.splice(0)) { + rmSync(root, { recursive: true, force: true }); + } +}); + +describe('ambiguous source resolution warnings', () => { + // THIS TEST VALIDATES A HARD REQUIREMENT (OFTR-004.7.1, OFTR-004.7.5). + // YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES. + it('names both declaring settings layers and refs, the winner, and preserves source precedence', () => { + const root = createTemporaryRoot(); + write( + join(root, 'home', '.agents', 'settings.yml'), + 'sources:\n - github: ai-outfitter/community-profiles\n ref: v1.2.1\n', + ); + write( + join(root, 'project', '.agents', 'settings.yml'), + 'sources:\n - github: ai-outfitter/community-profiles\n ref: v1.2.0\n', + ); + + const result = resolve(root); + const warning = result.ambiguityWarnings.find((message) => message.includes('community-profiles')); + + expect(warning).toContain(join(root, 'home', '.agents', 'settings.yml')); + expect(warning).toContain('v1.2.1'); + expect(warning).toContain(join(root, 'project', '.agents', 'settings.yml')); + expect(warning).toContain('v1.2.0'); + expect(warning).toContain(`'${join(root, 'project', '.agents', 'settings.yml')}' at ref 'v1.2.0' won`); + expect(result.settings.sources).toEqual([{ github: 'ai-outfitter/community-profiles', ref: 'v1.2.0' }]); + }); + + // THIS TEST VALIDATES A HARD REQUIREMENT (OFTR-004.7.2, OFTR-004.7.5). + // YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES. + it('warns when a project source list drops a different repository declared by the user scope', () => { + const root = createTemporaryRoot(); + const userSettings = join(root, 'home', '.agents', 'settings.yml'); + const projectSettings = join(root, 'project', '.agents', 'settings.yml'); + write(userSettings, 'sources:\n - github: ai-outfitter/.agents\n'); + write(projectSettings, 'sources:\n - github: ai-outfitter/community-profiles\n ref: v1.2.0\n'); + + const result = resolve(root); + const warning = result.ambiguityWarnings.find((message) => message.includes('github:ai-outfitter/.agents')); + + expect(warning).toContain(`declared by '${userSettings}'`); + expect(warning).toContain(`replaced by '${projectSettings}'`); + expect(warning).toContain('is not in the effective configuration'); + expect(result.settings.sources).toEqual([{ github: 'ai-outfitter/community-profiles', ref: 'v1.2.0' }]); + expect(result.settings.sources).not.toContainEqual({ github: 'ai-outfitter/.agents' }); + }); + + // THIS TEST VALIDATES A HARD REQUIREMENT (OFTR-004.7.3, OFTR-004.7.5). + // YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES. + it('warns when two sources supply an agent slug, names both sources and the unchanged winner', () => { + const root = createTemporaryRoot(); + const winner = join(root, 'winner'); + const shadowed = join(root, 'shadowed'); + resource(winner, 'agents', 'actions-agent'); + resource(shadowed, 'agents', 'actions-agent'); + write(join(root, 'project', '.agents', 'settings.yml'), `sources:\n - path: ${winner}\n - path: ${shadowed}\n`); + + const result = resolve(root); + const warning = result.warnings.find((message) => message.includes("agent slug 'actions-agent'")); + + expect(warning).toContain(winner); + expect(warning).toContain(shadowed); + expect(warning).toContain(`'${winner}' won`); + expect(findResource(result.set, 'agent', 'actions-agent')?.winner.layer.label).toBe(winner); + }); + + // THIS TEST VALIDATES A HARD REQUIREMENT (OFTR-004.7.3). + // YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES. + it('warns when two sources supply a skill slug and names the winner', () => { + const root = createTemporaryRoot(); + const winner = join(root, 'skills-one'); + const shadowed = join(root, 'skills-two'); + resource(winner, 'skills', 'triage'); + resource(shadowed, 'skills', 'triage'); + write(join(root, 'project', '.agents', 'settings.yml'), `sources:\n - path: ${winner}\n - path: ${shadowed}\n`); + + const warning = resolve(root).warnings.find((message) => message.includes("skill slug 'triage'")); + + expect(warning).toContain(winner); + expect(warning).toContain(shadowed); + expect(warning).toContain(`'${winner}' won`); + }); + + // THIS TEST VALIDATES A HARD REQUIREMENT (OFTR-004.7.4). + // YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES. + it('surfaces ambiguity warnings from sync, validate, list agents, and run', async () => { + const root = createTemporaryRoot(); + const winner = join(root, 'catalog-one'); + const shadowed = join(root, 'catalog-two'); + resource(winner, 'agents', 'actions-agent'); + resource(shadowed, 'agents', 'actions-agent'); + write(join(root, 'project', '.agents', 'settings.yml'), `sources:\n - path: ${winner}\n - path: ${shadowed}\n`); + const input = { homeDirectory: join(root, 'home'), projectDirectory: join(root, 'project') }; + const isAmbiguityWarning = (message: string): boolean => message.includes("Ambiguous agent slug 'actions-agent'"); + + expect(executeSyncCommand(input).messages.some(isAmbiguityWarning)).toBe(true); + expect(executeValidateCommand(input).messages.some(isAmbiguityWarning)).toBe(true); + expect(executeListCommand({ ...input, kind: 'agents' }).messages.some(isAmbiguityWarning)).toBe(true); + const run = await executeRunAgentCommand({ + ...input, + agent: 'actions-agent', + launcher: () => Promise.resolve(0), + }); + expect(run.messages.some(isAmbiguityWarning)).toBe(true); + }); + + // THIS TEST VALIDATES A HARD REQUIREMENT (OFTR-004.7.1, OFTR-004.7.3). + // YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES. + it('does not report ambiguity for a clean effective configuration', () => { + const root = createTemporaryRoot(); + const first = join(root, 'first'); + const second = join(root, 'second'); + resource(first, 'agents', 'engineer'); + resource(second, 'skills', 'research'); + write(join(root, 'project', '.agents', 'settings.yml'), `sources:\n - path: ${first}\n - path: ${second}\n`); + + expect(resolve(root).warnings.filter((message) => message.includes('Ambiguous'))).toEqual([]); + }); + + // THIS TEST VALIDATES A HARD REQUIREMENT (OFTR-004.7.2). + // YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES. + it('reports a dropped repository once when a higher-precedence empty list excludes duplicate declarations', () => { + const root = createTemporaryRoot(); + write( + join(root, 'home', '.agents', 'settings.yml'), + 'sources:\n - github: acme/catalog\n ref: v1.0.0\n - github: acme/catalog\n ref: v2.0.0\n', + ); + write(join(root, 'project', '.agents', 'settings.yml'), 'sources: []\n'); + + const warnings = resolve(root).ambiguityWarnings; + + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain("source 'github:acme/catalog'"); + expect(warnings[0]).toContain(join(root, 'home', '.agents', 'settings.yml')); + expect(warnings[0]).toContain(join(root, 'project', '.agents', 'settings.yml')); + }); +}); diff --git a/code/cli/tests/unit/resolver-cli.test.ts b/code/cli/tests/unit/resolver-cli.test.ts index b4febd53..85cced70 100644 --- a/code/cli/tests/unit/resolver-cli.test.ts +++ b/code/cli/tests/unit/resolver-cli.test.ts @@ -66,6 +66,12 @@ describe('resolver command objects', () => { expect(lines.join('\n')).toContain('engineer [workspace]'); }); + it('list prints (none) for a kind with no resources', async () => { + const lines: string[] = []; + await buildProgram(project(), lines).parseAsync(['node', 'outfitter', 'list', 'skills']); + expect(lines).toEqual(['skills:', ' (none)']); + }); + it('list forwards --agent and marks local resources', async () => { const lines: string[] = []; await buildProgram(project(), lines).parseAsync(['node', 'outfitter', 'list', 'skills', '--agent', 'engineer']); diff --git a/docs/requirements/OFTR-004-sync-and-setup.md b/docs/requirements/OFTR-004-sync-and-setup.md index 6a195bab..936f6846 100644 --- a/docs/requirements/OFTR-004-sync-and-setup.md +++ b/docs/requirements/OFTR-004-sync-and-setup.md @@ -159,3 +159,23 @@ bootstrap is the one gate exemption — see OFTR-004.6.10.) Consistent with OFTR-005.3.4, the run-time composer surfaces an unresolved loadout reference as a non-fatal warning (fatal only under `outfitter run --strict`); `outfitter validate` is the command that fails on it. + +### OFTR-004.7: Ambiguous Source Resolution + +Resolution precedence exists to compose layers, not to hide disagreement. When two declarations +disagree about the same thing, the selected declaration must be visible. + +1. Resolution MUST detect when the same source repository is declared more than once with different + refs across all declared configuration — every loaded settings scope plus the transitive + declarations of effective sources — MUST report a warning naming each declaring layer and its + ref, and MUST name the declaration that won. +2. Resolution MUST detect when a settings scope's `sources` list replaces a lower-precedence scope's + list and drops a declared repository entirely, and MUST report a warning naming the dropped + source, its declaring scope, and the replacing scope. +3. Resolution MUST detect when the same agent or skill slug is supplied by more than one source, + MUST report a warning naming each supplying source, and MUST name the definition that won. A slug + intentionally overridden by a higher-precedence layer is still reported; visibility, not + prohibition, is the requirement. +4. These warnings MUST be surfaced by every command that resolves the effective set, including + `sync`, `validate`, `list agents`, and `run`, not only by a dedicated diagnostic. +5. Detection MUST NOT change which declaration wins; precedence rules are unchanged.