From dc746ce328cda02af298f10d22da31159a7978fe Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Mon, 6 Jul 2026 11:32:28 -0400 Subject: [PATCH] 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). --- src/capability-writer.cts | 14 +++++++-- src/surface.cts | 19 +++++++----- ...ssue-1575-agent-descriptor-parity.test.cjs | 30 ++++++++++++++++++- 3 files changed, 53 insertions(+), 10 deletions(-) diff --git a/src/capability-writer.cts b/src/capability-writer.cts index f9a51579c..7605614f8 100644 --- a/src/capability-writer.cts +++ b/src/capability-writer.cts @@ -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), diff --git a/src/surface.cts b/src/surface.cts index 03facdb82..6c93c61d0 100644 --- a/src/surface.cts +++ b/src/surface.cts @@ -337,22 +337,27 @@ function applySurface(runtimeConfigDir: string, layout: Layout, manifest: Map 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)) { diff --git a/tests/issue-1575-agent-descriptor-parity.test.cjs b/tests/issue-1575-agent-descriptor-parity.test.cjs index af7507088..23a3a94c6 100644 --- a/tests/issue-1575-agent-descriptor-parity.test.cjs +++ b/tests/issue-1575-agent-descriptor-parity.test.cjs @@ -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 '; + 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`); + } + }); }); }