fix(#1575): address code review M1+M2+L1
M1: gate skills:'*' sentinel on unmodified-full profile (base profile must be
'full' AND no surface mods) so tiered profiles (core/standard) don't over-stage.
M2: thread resolveAttribution through capability-writer materialize opts; add
parity test variant with non-undefined Co-Authored-By attribution.
L1: remove redundant .agent.md filter condition (.endsWith('.md') already covers it).
This commit is contained in:
@@ -92,7 +92,7 @@ interface DesiredCapability {
|
||||
}
|
||||
|
||||
interface SetCapabilityStateOptions {
|
||||
materialize?: { runtime: string; scope: string };
|
||||
materialize?: { runtime: string; scope: string; resolveAttribution?: (runtime: string) => string | null | undefined };
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -352,8 +352,18 @@ function setCapabilityState(
|
||||
const layout = runtimeArtifactLayout.resolveRuntimeArtifactLayout(runtime, resolvedConfigDir, scope);
|
||||
const commandsGsdDir = _resolveCommandsGsdDir();
|
||||
const manifest = _resolveManifest(commandsGsdDir, resolvedConfigDir);
|
||||
// #1575: applySurface now accepts opts.resolveAttribution so surface-path
|
||||
// agents get the same Co-Authored-By trailer as the install path. The
|
||||
// resolver is not threaded here yet — the CLI command handler does not have
|
||||
// access to getCommitAttribution (which lives in bin/install.js). Until that
|
||||
// is refactored into a shared module, surface-path agents for descriptor-
|
||||
// driven runtimes will lack the Co-Authored-By trailer that install adds.
|
||||
// Parity is proven when resolveAttribution IS provided (see
|
||||
// tests/issue-1575-agent-descriptor-parity.test.cjs).
|
||||
// eslint-disable-next-line @typescript-eslint/no-unsafe-argument
|
||||
applySurface(resolvedConfigDir, layout, manifest, undefined, registry);
|
||||
applySurface(resolvedConfigDir, layout, manifest, undefined, registry, opts?.materialize?.resolveAttribution
|
||||
? { resolveAttribution: opts.materialize.resolveAttribution }
|
||||
: undefined);
|
||||
} catch (err: unknown) {
|
||||
const msg = err instanceof Error ? err.message : String(err);
|
||||
// Fix C: materialise was explicitly requested — a failure is an error (non-zero exit),
|
||||
|
||||
@@ -337,22 +337,27 @@ function applySurface(runtimeConfigDir: string, layout: Layout, manifest: Map<st
|
||||
const agentCtx: AgentCtx = { runtime: layout.runtime, pathPrefix: _pathPrefix, attribution: _attribution };
|
||||
|
||||
const tempDirsToClean: string[] = [];
|
||||
// #1575: When the surface has no state modifications (no .gsd-surface.json or
|
||||
// it has no disabled clusters / explicit changes), pass the '*' sentinel for
|
||||
// agents staging so ALL agents are staged — matching the install path which
|
||||
// uses { skills: '*' }. Without this, agents not referenced by any skill's
|
||||
// _calls_agents_ manifest entry would be silently dropped from the surface path.
|
||||
// #1575: When the surface has no state modifications AND the base profile is
|
||||
// 'full', pass the '*' sentinel for agents staging so ALL agents are staged —
|
||||
// matching the install path which uses { skills: '*' }. Without this, agents
|
||||
// not referenced by any skill's _calls_agents_ manifest entry would be silently
|
||||
// dropped from the surface path. For tiered profiles (core/standard) or when
|
||||
// surface mods exist, pass the resolved set so only the filtered subset stages.
|
||||
const _surfaceState = readSurface(layout.configDir);
|
||||
const _baseProfileName = (_surfaceState && _surfaceState.baseProfile)
|
||||
? _surfaceState.baseProfile
|
||||
: (readActiveProfile(layout.configDir) || 'full');
|
||||
const _hasSurfaceMods = !!_surfaceState && (
|
||||
_surfaceState.disabledClusters.length > 0 ||
|
||||
_surfaceState.explicitAdds.length > 0 ||
|
||||
_surfaceState.explicitRemoves.length > 0
|
||||
);
|
||||
const _isUnmodifiedFull = _baseProfileName === 'full' && !_hasSurfaceMods;
|
||||
try {
|
||||
for (const kind of layout.kinds) {
|
||||
let staged: string;
|
||||
if (kind.kind === 'agents') {
|
||||
const agentProfile = _hasSurfaceMods ? resolved : { ...resolved, skills: '*' as const };
|
||||
const agentProfile = _isUnmodifiedFull ? { ...resolved, skills: '*' as const } : resolved;
|
||||
staged = kind.stage(agentProfile, agentCtx);
|
||||
} else {
|
||||
staged = kind.stage(resolved);
|
||||
@@ -548,7 +553,7 @@ function _syncGsdDir(stagedDir: string, destDir: string, kind: ArtifactKind | st
|
||||
// - namespaced command dirs: the whole dir is GSD-owned
|
||||
const shouldPruneAgents = !(kindName === 'agents' && (!manifest || manifest.size === 0));
|
||||
if (shouldPruneAgents) {
|
||||
for (const file of fs.readdirSync(destDir).filter(f => f.endsWith('.md') || (isCopilotAgents && f.endsWith('.agent.md')))) {
|
||||
for (const file of fs.readdirSync(destDir).filter(f => f.endsWith('.md'))) {
|
||||
if (kindName === 'agents' && !file.startsWith('gsd-')) continue;
|
||||
if (kindName === 'commands' && !namespacedByDir && kindPrefix && !file.startsWith(kindPrefix)) continue;
|
||||
if (!stagedDestNames.has(file)) {
|
||||
|
||||
@@ -98,7 +98,35 @@ describe('#1575 — golden-parity: surface path matches install path for descrip
|
||||
installContent,
|
||||
`${runtime}/${fileName}: surface content must be byte-identical to install content`,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
test('cursor with non-undefined attribution: surface agents byte-identical to install agents (M2 coverage)', (t) => {
|
||||
// M2 regression guard: verify parity holds when resolveAttribution returns
|
||||
// a real Co-Authored-By value, not just undefined. Proves the agentCtx
|
||||
// threading is correct for both paths.
|
||||
const attrResolver = () => 'Test Bot <test@example.com>';
|
||||
const configDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-1575-attr-'));
|
||||
t.after(() => { try { cleanup(configDir); } catch { /* best-effort */ } });
|
||||
|
||||
installRuntimeArtifacts('cursor', configDir, 'global', profile, attrResolver);
|
||||
|
||||
const agentsDir = path.join(configDir, 'agents');
|
||||
const installSnap = snapshotAgents(agentsDir);
|
||||
assert.ok(installSnap.size > 0, 'install must produce agents');
|
||||
|
||||
// Verify attribution was actually applied by install path
|
||||
const firstContent = [...installSnap.values()][0];
|
||||
assert.ok(firstContent.includes('Co-Authored-By: Test Bot'), 'install must apply Co-Authored-By');
|
||||
|
||||
const layout = resolveRuntimeArtifactLayout('cursor', configDir, 'global');
|
||||
applySurface(configDir, layout, manifest, undefined, undefined, { resolveAttribution: attrResolver });
|
||||
|
||||
const surfaceSnap = snapshotAgents(agentsDir);
|
||||
for (const [fileName, installContent] of installSnap) {
|
||||
assert.strictEqual(surfaceSnap.get(fileName), installContent,
|
||||
`cursor/${fileName}: content must be byte-identical with attribution`);
|
||||
}
|
||||
});
|
||||
});
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user