Skip to content

fix(managed-skills): unsupported rows on every path; failed discovery is degraded #1037

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Juliusolsson05 merged 4 commits into from
Sep 19, 2026
Merged

fix(managed-skills): unsupported rows on every path; failed discovery is degraded #1037

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3367ea0
test(managed-skills): unsupported rows on every path, degraded on fai…
Juliusolsson05 Sep 19, 2026
bd5c29b
fix(managed-skills): derive unsupported rows at snapshot time; failed…
Juliusolsson05 Sep 19, 2026
b00330a
chore: two leftover nits from the Grok review (ledger T6)
Juliusolsson05 Sep 19, 2026
c1dd488
fix(managed-skills): a discovery failure never blocks deleting a disa…
Juliusolsson05 Sep 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions src/main/agentCodeConventions/AgentCodeConventionsService.system.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -455,6 +455,43 @@ describe('AgentCodeConventionsService', () => {
]))
})

// #1017 review: the informational `unsupported:*` rows were appended on
// two paths only. Every other path (the startup audit, a target conflict,
// a disable) replaced the list without them, so Settings stopped showing
// which provider cannot take the skill as soon as anything else happened.
it('shows the unsupported provider row from the first snapshot, before any save', async () => {
const root = await temporaryDirectory()
const currentTarget = target('agents-standard-personal-skills', join(root, '.agents', 'skills'), ['codex'])
const service = new AgentCodeConventionsService({
stateFilePath: join(root, 'state', 'conventions.json'),
homeDirectory: root,
resolveTargets: async () => ({ targets: [currentTarget], unsupportedProviders: ['grok'] }),
})
await service.initialize()
expect((await service.getSnapshot()).targets).toEqual(expect.arrayContaining([
expect.objectContaining({ id: 'unsupported:grok', state: 'unsupported' }),
]))
})

it('keeps the unsupported provider row when enable stops on a target conflict', async () => {
const root = await temporaryDirectory()
const currentTarget = target('agents-standard-personal-skills', join(root, '.agents', 'skills'), ['codex'])
// A file the app does not own sits where the skill would go.
await writeFileWithParents(currentTarget.skillFile, '# Someone else\'s skill')
const service = new AgentCodeConventionsService({
stateFilePath: join(root, 'state', 'conventions.json'),
homeDirectory: root,
resolveTargets: async () => ({ targets: [currentTarget], unsupportedProviders: ['grok'] }),
})
await service.initialize()
const result = await service.save({ expectedRevision: 0, enabled: true, markdown: '# Rules' })
expect(result).toMatchObject({ ok: false, code: 'target-conflict' })
expect((await service.getSnapshot()).targets).toEqual(expect.arrayContaining([
expect.objectContaining({ id: 'unsupported:grok', state: 'unsupported' }),
expect.objectContaining({ id: 'agents-standard-personal-skills', state: 'conflict' }),
]))
})

it('still blocks enable when no registered provider supports personal skills', async () => {
const root = await temporaryDirectory()
const service = new AgentCodeConventionsService({
Expand Down
145 changes: 72 additions & 73 deletions src/main/agentCodeConventions/AgentCodeConventionsService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,10 @@ type StagedInstalledDiscovery = {
expiresAtMs: number
}

/** The status row a failed provider target discovery puts on every surface.
* It is a statement about discovery, not about any file on disk. */
const TARGET_RESOLUTION_STATUS_ID = 'provider-target-resolution'

export class AgentCodeManagedSkillsService {
private document = createEmptyAgentCodeConventionsDocument()
private recovery: AgentCodeConventionsSnapshot['recovery']
Expand Down Expand Up @@ -686,9 +690,14 @@ export class AgentCodeManagedSkillsService {
skill = this.document.installedSkills[request.skillId]!
}
const statuses = this.installedTargetStatuses.get(skill.id) ?? []
const blockers = statuses.filter(status => status.state === 'conflict'
// The discovery-failure row is not a blocker (#1037 review): it names no
// file anyone could review, and it has no fingerprint, so it could never
// be approved. Deleting a disabled skill was then impossible until
// discovery recovered. What delete must never do, forget a skill whose
// files are still on disk, is the stillOwnedKeys journal check below.
const blockers = statuses.filter(status => status.id !== TARGET_RESOLUTION_STATUS_ID && (status.state === 'conflict'
|| status.state === 'retired'
|| status.state === 'error')
|| status.state === 'error'))
const approvals = new Map((request.abandonTargets ?? []).map(value => [value.targetId, value]))
const unresolved = blockers.filter(status => {
const approval = approvals.get(status.id)
Expand Down Expand Up @@ -1147,7 +1156,8 @@ export class AgentCodeManagedSkillsService {
// targets would be a silent no-op write, so refuse it with the contract
// the Settings UI already renders.
if (request.enabled && this.targets.targets.length === 0) {
this.targetStatuses = this.unsupportedStatuses()
// The unsupported rows are added by snapshot(); see withUnsupportedRows.
this.targetStatuses = []
return { ok: false, code: 'unsupported', snapshot: this.snapshot() }
}

Expand Down Expand Up @@ -1197,10 +1207,6 @@ export class AgentCodeManagedSkillsService {
}
statuses.push(await this.publishTarget(item, rendered, desiredHash))
}
// Same informational rows the startup reconcile appends (#1014): the
// post-save snapshot must already show which providers cannot receive
// the skill, not only after a restart.
statuses.push(...this.unsupportedStatuses())
this.targetStatuses = statuses
await this.persistBestEffort(statuses)
return { ok: true, snapshot: this.snapshot() }
Expand Down Expand Up @@ -1524,9 +1530,9 @@ export class AgentCodeManagedSkillsService {
}

private async reconcileInstalledSkillLocked(skill: AgentCodeInstalledSkillRecord): Promise<void> {
// Unsupported providers no longer short-circuit (#1014); the informational
// rows are appended by applyInstalledOperationsLocked, the single funnel
// every enabled-skill status rebuild passes through.
// Unsupported providers no longer short-circuit (#1014). Their
// informational rows are added when a snapshot is built
// (withUnsupportedRows), not stored here.
const targets = this.installedTargets(skill)
if (skill.enabled) {
try {
Expand Down Expand Up @@ -1651,11 +1657,6 @@ export class AgentCodeManagedSkillsService {
conflictFingerprint: this.installedOwnershipPolicy.retiredFingerprint(key, record),
})
}
// Informational rows for providers without personal-skill support (#1014);
// installedHealth skips them when computing deployment health.
if (this.targets.unsupportedProviders.length > 0) {
statuses.push(...this.installedUnsupportedStatuses())
}
this.installedTargetStatuses.set(skill.id, statuses)
}

Expand Down Expand Up @@ -1711,7 +1712,7 @@ export class AgentCodeManagedSkillsService {
...skill,
totalBytes: skill.files.reduce((total, file) => total + file.bytes, 0),
health: this.installedHealth(skill, targets),
targets: [...targets],
targets: this.withUnsupportedRows(targets),
}
}),
unsupportedProviders: [...this.targets.unsupportedProviders],
Expand All @@ -1738,23 +1739,18 @@ export class AgentCodeManagedSkillsService {
if (deployable.some(status => status.state === 'conflict' || status.state === 'retired')) {
return 'conflict'
}
// Error BEFORE the degenerate 'unsupported', as health() and customHealth()
// already order it (#1017 review): a failed target resolution also empties
// this.targets, and "no provider can take this" is the wrong thing to say
// when the truth is "we could not look".
if (deployable.some(status => status.state === 'error')) return 'degraded'
if (this.targets.targets.length === 0) return 'unsupported'
if (deployable.length === 0 || deployable.some(status => status.state !== 'installed')) {
return 'degraded'
}
return 'active'
}

private installedUnsupportedStatuses(): AgentCodeConventionsTargetStatus[] {
return this.targets.unsupportedProviders.map(provider => ({
id: `unsupported:${provider}`,
providers: [provider],
displayPath: 'No personal skill directory',
state: 'unsupported',
message: `${provider} does not declare personal Agent Skills support.`,
}))
}

private installedStatus(
target: AgentCodeConventionsTarget,
state: AgentCodeConventionsTargetStatus['state'],
Expand Down Expand Up @@ -1883,10 +1879,6 @@ export class AgentCodeManagedSkillsService {
if (!next.pendingOperations[item.key]) statuses.push(this.customStatus(item.target, 'installed'))
else statuses.push(await this.publishCustomTarget(updated, item, rendered, desiredHash))
}
// Same informational rows the startup reconcile appends (#1014): the
// post-enable snapshot must already show providers that cannot receive
// the skill, not only after a restart.
statuses.push(...this.customUnsupportedStatuses())
this.customTargetStatuses.set(skill.id, statuses)
await this.persistBestEffort(statuses)
return { ok: true, snapshot: this.customSnapshot() }
Expand Down Expand Up @@ -2051,8 +2043,8 @@ export class AgentCodeManagedSkillsService {
}

private async reconcileEnabledLocked(): Promise<void> {
// Unsupported providers no longer short-circuit (#1014): they contribute
// informational rows at the end instead of replacing deployment rows.
// Unsupported providers no longer short-circuit (#1014). Their
// informational rows are added at snapshot time (withUnsupportedRows).
const normalized = normalizeAgentCodeConventionsMarkdown(this.document.markdown, {
requireContent: true,
})
Expand Down Expand Up @@ -2145,9 +2137,6 @@ export class AgentCodeManagedSkillsService {
// made an otherwise complete enabled reconciliation look degraded.
if (removed.state !== 'not-installed') statuses.push(removed)
}
// WHY appended, not replacing (#1014): unsupported providers are informational
// per-target rows; real deployment rows must survive so health stays truthful.
statuses.push(...this.unsupportedStatuses())
this.targetStatuses = statuses
await this.persistBestEffort(statuses)
}
Expand Down Expand Up @@ -2180,15 +2169,14 @@ export class AgentCodeManagedSkillsService {
}

private async reconcileCustomEnabledLocked(skill: AgentCodeCustomSkillRecord): Promise<void> {
// Unsupported providers no longer short-circuit (#1014): they contribute
// informational rows at the exits instead of replacing deployment rows.
// Unsupported providers no longer short-circuit (#1014). Their
// informational rows are added at snapshot time (withUnsupportedRows).
const targets = this.customTargets(skill)
const normalized = normalizeAgentCodeCustomSkill(skill, { requireContent: true })
if (!normalized.ok) {
this.customTargetStatuses.set(
skill.id,
[...targets.targets.map(target => this.customStatus(target, 'error', normalized.message)),
...this.customUnsupportedStatuses()],
targets.targets.map(target => this.customStatus(target, 'error', normalized.message)),
)
return
}
Expand Down Expand Up @@ -2284,9 +2272,6 @@ export class AgentCodeManagedSkillsService {
const removed = await this.removeCustomMaterialization(skill, key, record)
if (removed.state !== 'not-installed') statuses.push(removed)
}
// Informational rows for providers that cannot receive skills (#1014);
// deployment rows above remain the health input.
statuses.push(...this.customUnsupportedStatuses())
this.customTargetStatuses.set(skill.id, statuses)
await this.persistBestEffort(statuses)
}
Expand Down Expand Up @@ -2962,18 +2947,25 @@ export class AgentCodeManagedSkillsService {
state: 'error',
message: safeErrorMessage(error),
}]
// A previous successful audit may have left every custom target marked
// Installed. Once discovery itself fails, those paths are no longer a
// trustworthy statement about the current provider configuration; keep
// the desired definitions but invalidate deployment health together.
// A previous successful audit may have left every custom AND installed
// target marked Installed. Once discovery itself fails, those paths are
// no longer a trustworthy statement about the current provider
// configuration; keep the desired definitions but invalidate deployment
// health together. (Installed skills were missed here until the #1017
// review: they kept their stale rows, and with zero resolved targets
// their health read 'unsupported'.)
const resolutionError: AgentCodeConventionsTargetStatus = {
id: TARGET_RESOLUTION_STATUS_ID,
providers: [],
displayPath: '',
state: 'error',
message: safeErrorMessage(error),
}
for (const skill of Object.values(this.document.customSkills)) {
this.customTargetStatuses.set(skill.id, [{
id: 'provider-target-resolution',
providers: [],
displayPath: '',
state: 'error',
message: safeErrorMessage(error),
}])
this.customTargetStatuses.set(skill.id, [{ ...resolutionError }])
}
for (const skill of Object.values(this.document.installedSkills)) {
this.installedTargetStatuses.set(skill.id, [{ ...resolutionError }])
}
return false
}
Expand Down Expand Up @@ -3031,7 +3023,7 @@ export class AgentCodeManagedSkillsService {
warnings: normalized.ok ? normalized.value.warnings : [],
unsupportedProviders: this.targets.unsupportedProviders,
recovery: this.recovery,
targets: [...this.targetStatuses].sort((left, right) => left.id.localeCompare(right.id)),
targets: this.withUnsupportedRows(this.targetStatuses).sort((left, right) => left.id.localeCompare(right.id)),
}
}

Expand All @@ -3043,7 +3035,7 @@ export class AgentCodeManagedSkillsService {
skills: Object.values(this.document.customSkills)
.sort((left, right) => left.name.localeCompare(right.name))
.map(skill => {
const targets = [...(this.customTargetStatuses.get(skill.id) ?? [])]
const targets = this.withUnsupportedRows(this.customTargetStatuses.get(skill.id) ?? [])
.sort((left, right) => left.id.localeCompare(right.id))
return {
...skill,
Expand Down Expand Up @@ -3101,24 +3093,31 @@ export class AgentCodeManagedSkillsService {
return 'disabled'
}

private unsupportedStatuses(): AgentCodeConventionsTargetStatus[] {
return this.targets.unsupportedProviders.map(provider => ({
id: `unsupported:${provider}`,
providers: [provider],
displayPath: '',
state: 'unsupported',
message: 'This provider does not declare personal Agent Skill support.',
}))
}

private customUnsupportedStatuses(): AgentCodeConventionsTargetStatus[] {
return this.targets.unsupportedProviders.map(provider => ({
id: `unsupported:${provider}`,
providers: [provider],
displayPath: '',
state: 'unsupported',
message: 'This provider does not declare personal Agent Skill support.',
}))
/**
* The informational rows for registered providers that declare no personal
* Agent Skills support (#1014), added to a surface's rows when a snapshot is
* built and never stored.
*
* WHY at snapshot time (#1017 review): the rows used to be appended by the
* code paths that set statuses, and only two of roughly fifteen did. The
* startup audit, a target conflict, a disable and every custom or installed
* reconcile replaced the list without them, so Settings stopped naming the
* unsupported provider as soon as anything else happened. The rows depend
* only on `this.targets`, so deriving them here makes every path agree. The
* three near-identical builders (conventions, custom, installed) are now
* one. Health never sees these rows: it reads the stored statuses.
*/
private withUnsupportedRows(statuses: readonly AgentCodeConventionsTargetStatus[]): AgentCodeConventionsTargetStatus[] {
return [
...statuses.filter(status => status.state !== 'unsupported'),
...this.targets.unsupportedProviders.map(provider => ({
id: `unsupported:${provider}`,
providers: [provider],
displayPath: 'No personal skill directory',
state: 'unsupported' as const,
message: `${provider} does not declare personal Agent Skills support.`,
})),
]
}

private status(
Expand Down
44 changes: 43 additions & 1 deletion src/main/agentCodeConventions/AgentCodeInstalledSkillsService.system.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,8 @@ async function harness(
now?: () => Date
snapshotMaxBytes?: number
unsupportedProviders?: ResolvedAgentCodeConventionsTargets['unsupportedProviders']
/** Flip `.fail` to make provider target discovery throw from then on. */
discovery?: { fail: boolean }
} = {},
) {
const root = await temporaryDirectory()
Expand All @@ -118,7 +120,10 @@ async function harness(
installedSkillSnapshotRoot: join(root, 'state', 'managed-skill-snapshots'),
installedSkillSnapshotMaxBytes: options.snapshotMaxBytes,
homeDirectory: root,
resolveTargets: async () => resolved,
resolveTargets: async () => {
if (options.discovery?.fail) throw new Error('Could not read provider configuration')
return resolved
},
githubSkillSource,
now: options.now ?? (() => new Date('2026-08-27T00:00:00.000Z')),
operationId: (() => { let value = 0; return () => `installed-operation-${++value}` })(),
Expand Down Expand Up @@ -172,6 +177,43 @@ describe('AgentCode installed skills service', () => {
expect(snapshot.unsupportedProviders).toEqual(['grok'])
})

// #1017 review: when provider target discovery fails, the conventions
// and custom skills report 'degraded' with the error row, but an installed
// skill kept its stale rows, and with zero resolved targets its health
// read 'unsupported', which says "no provider can take this" when the
// truth is "we could not look".
it('reports an installed skill as degraded, with the error, when target discovery fails', async () => {
const discovery = { fail: false }
const { service, discoveries } = await harness({ discovery })
const staged = stagedPackage({ commit: 'a'.repeat(40), files: [{ path: 'SKILL.md', content: '# Review code' }] })
const found = await discoverOne(service, discoveries, staged)
await service.installGitHubSkills({ expectedRevision: 0, discoveryId: found.discoveryId, candidateIds: [staged.candidate.candidateId] })
discovery.fail = true
await service.audit()
const skill = (await service.getInstalledSkillsSnapshot()).skills.find(item => item.name === 'review-code')
expect(skill?.health).toBe('degraded')
expect(skill?.targets).toEqual([expect.objectContaining({ id: 'provider-target-resolution', state: 'error' })])
})

it('a disabled skill can still be deleted while target discovery is failing', async () => {
// #1037 review: the discovery-error row has no fingerprint, so as a
// delete blocker it could never be approved, and delete dead-ended with
// "External changes must be reviewed".
const discovery = { fail: false }
const { service, discoveries } = await harness({ discovery })
const staged = stagedPackage({ commit: 'a'.repeat(40), files: [{ path: 'SKILL.md', content: '# Review code' }] })
const found = await discoverOne(service, discoveries, staged)
const installed = await service.installGitHubSkills({ expectedRevision: 0, discoveryId: found.discoveryId, candidateIds: [staged.candidate.candidateId] })
if (!installed.ok) throw new Error(JSON.stringify(installed))
const skill = installed.snapshot.skills.find(item => item.name === 'review-code')!
const disabled = await service.setInstalledSkillEnabled({ expectedRevision: installed.snapshot.revision, skillId: skill.id, enabled: false })
if (!disabled.ok) throw new Error(JSON.stringify(disabled))
discovery.fail = true
await service.audit()
const deleted = await service.deleteInstalledSkill({ expectedRevision: disabled.snapshot.revision, skillId: skill.id })
expect(deleted).toMatchObject({ ok: true })
})

it('installs a reviewed package and requires a second review before updating it', async () => {
const { root, service, discoveries, skillDirectory } = await harness()
const first = stagedPackage({
Expand Down
Loading
Loading