From 681c4ba458d28f2f9b527db1cb6b246baf7bf478 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Tue, 28 Jul 2026 17:27:26 -0700 Subject: [PATCH] fix(skills): stop calling the updater's own install a modified copy (#11249) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- config/tsconfig.node.json | 1 + .../skills/skill-freshness-inventory.test.ts | 114 ++++++++++++++++++ src/main/skills/skill-freshness-inventory.ts | 37 ++++-- src/main/skills/skill-update-outcome.test.ts | 7 +- .../skills/skill-freshness-grouping.test.ts | 8 ++ .../skill-freshness-display-status.test.ts | 19 +++ .../src/lib/skill-freshness-display-status.ts | 4 + src/shared/skill-freshness.ts | 4 + 8 files changed, 184 insertions(+), 10 deletions(-) diff --git a/config/tsconfig.node.json b/config/tsconfig.node.json index 817b00c9c..e641dda15 100644 --- a/config/tsconfig.node.json +++ b/config/tsconfig.node.json @@ -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/**/*", diff --git a/src/main/skills/skill-freshness-inventory.test.ts b/src/main/skills/skill-freshness-inventory.test.ts index cdf60c642..415e052ad 100644 --- a/src/main/skills/skill-freshness-inventory.test.ts +++ b/src/main/skills/skill-freshness-inventory.test.ts @@ -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 { + 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 { + 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.' diff --git a/src/main/skills/skill-freshness-inventory.ts b/src/main/skills/skill-freshness-inventory.ts index 41467b11e..a37474518 100644 --- a/src/main/skills/skill-freshness-inventory.ts +++ b/src/main/skills/skill-freshness-inventory.ts @@ -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 +): 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, diff --git a/src/main/skills/skill-update-outcome.test.ts b/src/main/skills/skill-update-outcome.test.ts index 4fdf8a4ff..ad04c835b 100644 --- a/src/main/skills/skill-update-outcome.test.ts +++ b/src/main/skills/skill-update-outcome.test.ts @@ -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([]) }) diff --git a/src/renderer/src/components/skills/skill-freshness-grouping.test.ts b/src/renderer/src/components/skills/skill-freshness-grouping.test.ts index 39bb58cc4..63c2243b7 100644 --- a/src/renderer/src/components/skills/skill-freshness-grouping.test.ts +++ b/src/renderer/src/components/skills/skill-freshness-grouping.test.ts @@ -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( [ diff --git a/src/renderer/src/lib/skill-freshness-display-status.test.ts b/src/renderer/src/lib/skill-freshness-display-status.test.ts index f8bdf54d1..55a68e774 100644 --- a/src/renderer/src/lib/skill-freshness-display-status.test.ts +++ b/src/renderer/src/lib/skill-freshness-display-status.test.ts @@ -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( diff --git a/src/renderer/src/lib/skill-freshness-display-status.ts b/src/renderer/src/lib/skill-freshness-display-status.ts index 029725106..ed983e88c 100644 --- a/src/renderer/src/lib/skill-freshness-display-status.ts +++ b/src/renderer/src/lib/skill-freshness-display-status.ts @@ -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 diff --git a/src/shared/skill-freshness.ts b/src/shared/skill-freshness.ts index 847988ca4..fe3c94272 100644 --- a/src/shared/skill-freshness.ts +++ b/src/shared/skill-freshness.ts @@ -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) &&