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) &&