fix(skills): stop calling the updater's own install a modified copy (#11249)

After a successful headless update the CLI installs source-repo HEAD,
which legitimately runs ahead of any shipped bundle. The scan classified
those bytes 'unrecognized', so the row went amber ('may be modified…
remove it') seconds after our own Update button ran, and the advice
looped: remove + reinstall lands the same newer content.

Scan half: a canonical/alias placement whose observed git tree sha
equals the updater lock's skillFolderHash is the CLI's own install, not
a user edit — reclassify it 'newer-known'. Display half: 'newer-known'
is recognized official content ahead of this build with nothing to fix,
so it no longer marks the copy blocked. Eligibility is deliberately
unchanged: ahead of the bundle means there is nothing this build can
update to, and offering one risks the provably-unperformable update
(#11110) when source HEAD still equals the lock.

Copies whose sha does not match the lock, copies with no lock entry,
same-name copies outside the placements the command writes, and
plugin-cache behavior all stay flagged exactly as before.
This commit is contained in:
Brennan Benson 2026-07-28 17:27:26 -07:00 committed by GitHub
parent 1d7e7656e3
commit 681c4ba458
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
8 changed files with 184 additions and 10 deletions

View File

@ -4,6 +4,7 @@
"../electron.vite.config.*",
"../build-plugins/**/*",
"../src/main/**/*",
"../src/renderer/src/lib/skill-freshness-display-status.ts",
"../src/preload/**/*",
"../src/shared/**/*",
"../src/relay/**/*",

View File

@ -1,6 +1,8 @@
import { execFile } from 'node:child_process'
import { mkdir, mkdtemp, readFile, rm, symlink, writeFile } from 'node:fs/promises'
import { tmpdir } from 'node:os'
import { join } from 'node:path'
import { promisify } from 'node:util'
import { afterEach, describe, expect, it } from 'vitest'
import type { Repo } from '../../shared/types'
import type {
@ -13,9 +15,31 @@ import {
MAXIMUM_REPOSITORY_SKILL_ROOTS
} from './skill-freshness-inventory'
import { describeObservedSkillFile, skillPackageDigest } from './skill-package-identity'
import { getSkillFreshnessDisplayStatus } from '../../renderer/src/lib/skill-freshness-display-status'
const temporaryDirectories: string[] = []
const execFileAsync = promisify(execFile)
// Why: hashed with real git, not Orca's tree-sha port — the port validating
// itself here would prove nothing about matching the updater lock's hash.
async function gitTreeShaOf(directory: string): Promise<string> {
const gitDir = await mkdtemp(join(tmpdir(), 'orca-skill-hash-'))
temporaryDirectories.push(gitDir)
const env = {
...process.env,
GIT_DIR: gitDir,
GIT_WORK_TREE: directory,
GIT_INDEX_FILE: join(gitDir, 'scratch-index'),
GIT_CONFIG_GLOBAL: join(gitDir, 'no-config'),
GIT_CONFIG_SYSTEM: join(gitDir, 'no-config')
}
await execFileAsync('git', ['init', '--quiet'], { env, cwd: directory })
await execFileAsync('git', ['add', '-A'], { env, cwd: directory })
const { stdout } = await execFileAsync('git', ['write-tree'], { env, cwd: directory })
return stdout.trim()
}
function snapshot(releaseRevision: number, markdown: string): SkillKnownSnapshot {
const observed = describeObservedSkillFile('SKILL.md', Buffer.from(markdown), false)
const file: SkillBundleFileIdentity = {
@ -115,6 +139,22 @@ async function fixture() {
}
}
async function writeSkillLockHash(homeDir: string, skillFolderHash: string): Promise<void> {
await writeFile(
join(homeDir, '.agents', '.skill-lock.json'),
`${JSON.stringify({
version: 3,
skills: {
'orca-cli': {
skillFolderHash,
skillPath: 'skills/orca-cli/SKILL.md',
source: 'stablyai/orca'
}
}
})}\n`
)
}
afterEach(async () => {
await Promise.all(temporaryDirectories.splice(0).map((root) => rm(root, { recursive: true })))
})
@ -178,6 +218,80 @@ describe('read-only skill freshness inventory', () => {
expect(inventory.eligibleUpdateNames).toEqual([])
})
it('trusts the updater lock for canonical bytes the bundle has never seen (#11220 scan half)', async () => {
// The steady state days after a release: `skills update` installed source-repo
// HEAD, wrote its lock, and no shipped bundle knows that revision yet.
const test = await fixture()
const upstreamMarkdown = `${test.newerMarkdown}\nUpstream edit no bundle has shipped.\n`
const canonical = await test.writeSkill(
join(test.homeDir, '.agents', 'skills'),
upstreamMarkdown
)
await writeSkillLockHash(test.homeDir, await gitTreeShaOf(canonical))
const inventory = await inventorySkillFreshness({
currentAppVersion: '2.0.0',
homeDir: test.homeDir,
repos: [],
resourceRoot: test.resourceRoot
})
expect(inventory.installations[0]).toMatchObject({
topology: 'canonical-copy',
status: 'newer-known'
})
// Why: ahead of the bundle means there is nothing this build can update to.
expect(inventory.eligibleUpdateNames).toEqual([])
// The user-visible verdict, across both halves of the fix: the row must read
// up to date, not amber "may be modified… remove it" over the CLI's own install.
expect(getSkillFreshnessDisplayStatus(inventory, 'orca-cli')).toBe('up-to-date')
})
it('still flags canonical bytes that do not match what the lock says was installed', async () => {
const test = await fixture()
const editedMarkdown = `${test.currentMarkdown}\nLocal edit the updater never wrote.\n`
await test.writeSkill(join(test.homeDir, '.agents', 'skills'), editedMarkdown)
const elsewhere = await test.writeSkill(join(test.root, 'elsewhere'), test.newerMarkdown)
await writeSkillLockHash(test.homeDir, await gitTreeShaOf(elsewhere))
const inventory = await inventorySkillFreshness({
currentAppVersion: '2.0.0',
homeDir: test.homeDir,
repos: [],
resourceRoot: test.resourceRoot
})
expect(inventory.installations[0]).toMatchObject({
topology: 'canonical-copy',
status: 'unrecognized'
})
expect(getSkillFreshnessDisplayStatus(inventory, 'orca-cli')).toBe('needs-attention')
})
it('does not let the lock vouch for a same-name copy outside the placements it wrote', async () => {
const test = await fixture()
await test.writeSkill(join(test.homeDir, '.agents', 'skills'), test.currentMarkdown)
const independent = await test.writeSkill(
join(test.homeDir, '.claude', 'skills'),
'---\nname: orca-cli\n---\n\nAnother tool.\n'
)
await writeSkillLockHash(test.homeDir, await gitTreeShaOf(independent))
const inventory = await inventorySkillFreshness({
currentAppVersion: '2.0.0',
homeDir: test.homeDir,
repos: [],
resourceRoot: test.resourceRoot
})
expect(inventory.installations).toEqual(
expect.arrayContaining([
expect.objectContaining({ topology: 'independent-copy', status: 'unrecognized' })
])
)
expect(getSkillFreshnessDisplayStatus(inventory, 'orca-cli')).toBe('needs-attention')
})
it('retains full-file identity without projecting unused metadata', async () => {
const test = await fixture()
const lateDescription = 'Description beyond the metadata parsing budget.'

View File

@ -1,9 +1,10 @@
import { lstat } from 'node:fs/promises'
import { join } from 'node:path'
import type { Repo } from '../../shared/types'
import type {
SkillFreshnessInstallation,
SkillFreshnessInventory
import {
SUPPORTED_GLOBAL_SKILL_TOPOLOGIES,
type SkillFreshnessInstallation,
type SkillFreshnessInventory
} from '../../shared/skill-freshness'
import { buildSkillDiscoverySources, type SkillScanRoot } from './skill-discovery-sources'
import { loadSkillBundleArtifacts } from './skill-bundle-artifacts'
@ -22,6 +23,24 @@ import { readGloballyUpdatableSkillLocks } from './skill-update-registration'
export const MAXIMUM_REPOSITORY_SKILL_ROOTS = 128
// Why: the updater installs source-repo HEAD, which legitimately runs ahead of the
// bundled registry — content the bundle has never seen is the steady state right
// after an update. Bytes whose git tree sha equals the lock's recorded hash are the
// CLI's own install, not a user edit, so calling them "unrecognized" (modified) is
// false. Judged only over the placements the update command writes; a same-name
// copy elsewhere earns no trust from someone else's lock entry.
function trustLockInstalledRevision(
installation: SkillFreshnessInstallation,
globalSkillLocks: ReadonlyMap<string, string>
): SkillFreshnessInstallation {
return installation.status === 'unrecognized' &&
SUPPORTED_GLOBAL_SKILL_TOPOLOGIES.has(installation.topology) &&
installation.observedGitTreeSha != null &&
installation.observedGitTreeSha === globalSkillLocks.get(installation.name)
? { ...installation, status: 'newer-known' }
: installation
}
export function boundRepositorySkillRoots(roots: readonly SkillScanRoot[]): {
scanned: SkillScanRoot[]
omitted: SkillScanRoot[]
@ -160,11 +179,13 @@ export async function inventorySkillFreshness(args: {
const installations = dedupeSkillFreshnessPlacements([
...homeInstallations,
...unsupportedInstallations
]).sort(
(left, right) =>
left.name.localeCompare(right.name, 'en') ||
left.unresolvedPath.localeCompare(right.unresolvedPath, 'en')
)
])
.map((installation) => trustLockInstalledRevision(installation, globalSkillLocks))
.sort(
(left, right) =>
left.name.localeCompare(right.name, 'en') ||
left.unresolvedPath.localeCompare(right.unresolvedPath, 'en')
)
return {
schemaVersion: 1,

View File

@ -223,12 +223,15 @@ describe('skillUpdateFailedNames over a real inventory', () => {
})
const locks = await readGloballyUpdatableSkillLocks({ homeDir })
// Guard the premise: this is the unrecognized path, not an accidental match.
// Guard the premise: no snapshot knows these bytes, so recognition can only
// come from the lock — the scan now reclassifies that match to 'newer-known'
// (the #11220 scan half), and the verdict accepts it either way.
const canonical = inventory.installations.filter(
(entry) => entry.name === 'orca-cli' && entry.topology === 'canonical-copy'
)
expect(canonical).toHaveLength(1)
expect(canonical[0].status).toBe('unrecognized')
expect(canonical[0].status).toBe('newer-known')
expect(canonical[0].installedReleaseRevision).toBeNull()
expect(skillUpdateFailedNames(['orca-cli'], inventory.installations, locks)).toEqual([])
})

View File

@ -99,6 +99,14 @@ describe('groupSkillFreshness', () => {
expect(groups[0]?.locations[0]?.chip).toBe('unrecognized')
})
it('raises no row for a copy that is ahead of this build', () => {
// Why: 'newer-known' is recognized official content — the updater's own install
// or a newer release's bytes. The badge stays green for it, so a row here would
// recreate the badge/dialog disagreement #11128 removed, from the other side.
const groups = groupSkillFreshness([placement('orchestration', { status: 'newer-known' })], [])
expect(groups).toEqual([])
})
it('groups a blocked skill and flags the culprit location, not the main copy', () => {
const groups = groupSkillFreshness(
[

View File

@ -149,6 +149,25 @@ describe('getSkillFreshnessDisplayStatus', () => {
)
})
it('shows recognized-newer content as up to date instead of blaming the user', () => {
// Why: 'newer-known' is official content ahead of this build — the updater's own
// install or a newer release's bytes. Amber here sent users on a remove/reinstall
// loop that lands the same newer content (#11220's scan half).
const value = inventory([placement('newer-known')])
expect(getSkillFreshnessDisplayStatus(value, SKILL_NAME)).toBe('up-to-date')
expect(hasSkillCopyNeedingAttention(value, SKILL_NAME)).toBe(false)
})
it('still reports drift beside a newer-known copy', () => {
expect(
getSkillFreshnessDisplayStatus(
inventory([placement('newer-known'), placement('unrecognized', 1)]),
SKILL_NAME
)
).toBe('needs-attention')
})
it('still reports drift in our own copy when a plugin-managed one sits alongside', () => {
expect(
getSkillFreshnessDisplayStatus(

View File

@ -24,8 +24,12 @@ export function getSkillFreshnessDisplayStatus(
continue
}
hasPlacement = true
// Why: 'newer-known' is recognized official content ahead of this build — the
// updater's own install or a newer release's bytes. There is nothing to fix and
// nothing to update to, so amber would send the user chasing a phantom edit.
if (
installation.status !== 'current' &&
installation.status !== 'newer-known' &&
!(installation.status === 'unrecognized' && installation.topology === 'plugin-cache')
) {
hasBlockedCopy = true

View File

@ -98,6 +98,10 @@ export type SkillFreshnessInstallation = {
export function isSkillCopyNeedingAttention(installation: SkillFreshnessInstallation): boolean {
return (
installation.status !== 'current' &&
// Why: 'newer-known' is recognized official content ahead of this build — the
// updater's own install or a newer release's bytes. There is nothing to fix and
// nothing to update to, so amber would send the user chasing a phantom edit.
installation.status !== 'newer-known' &&
!(installation.status === 'unrecognized' && installation.topology === 'plugin-cache') &&
!(
SUPPORTED_GLOBAL_SKILL_TOPOLOGIES.has(installation.topology) &&