fix(#4086): resolve skills/ manifest keys at the runtime's actual skills root (#4311)

* fix(#4086): resolve skills/ manifest keys at the runtime's actual skills root

Codex installs skills to ~/.agents/skills (skills-kind home override), but
saveLocalPatches() and verify-reapply-patches.cjs resolved every manifest key
config-dir-relative only — every skills/ key missed, so user modifications to
Codex skills were never hash-compared, never backed up, and silently
overwritten on update; the reapply verifier false-failed the same keys with
fail_installed_missing.

configDir stays first (non-override runtimes byte-identical); the skills root
(same _resolveSkillsRootDir / skillsManifestPrefix seams the write side uses)
is a containment-guarded fallback when the config-dir path is absent.

* fix(#4086): drop unused test param; add changeset fragment

* chore(#4086): backfill PR number in changeset fragment

---------

Co-authored-by: agent-4086 <agent-4086@local>
This commit is contained in:
Tom Boucher
2026-09-05 08:06:19 -04:00
committed by GitHub
parent 70f22e4643
commit b327331747
5 changed files with 428 additions and 6 deletions

View File

@@ -0,0 +1,5 @@
---
type: Fixed
pr: 4311
---
**Codex skill edits are backed up on update** — `gsd-file-manifest.json` skills paths now resolve at the runtime's real skills root (`~/.agents/skills`), so user modifications to Codex skills are detected, backed up to `gsd-local-patches/`, and verified by the reapply gate instead of being silently overwritten. (#4086)

View File

@@ -10170,19 +10170,57 @@ function saveLocalPatches(configDir, pristineCtx) {
const modified = [];
const pristineHashes = {};
// #4086: skills/ manifest keys may live OUTSIDE configDir at the runtime's
// ACTUAL skills root — codex global installs to $HOME/.agents/skills (the
// ADR-1239 skills-kind `home` override), which writeManifest() already
// hashes from via _resolveSkillsRootDir (#2088/#3738). Resolving every key
// against configDir alone made every skills/ key miss here, so user
// modifications to Codex skills were never hash-compared, never backed up,
// and were silently overwritten by the next update. configDir stays FIRST
// (Postel: runtimes whose skills genuinely live under configDir resolve
// byte-identically to before); the skills root is a fallback, only for keys
// under the SAME descriptor-driven manifest prefix writeManifest uses, and
// only when that root resolves outside configDir. Containment + symlink
// guards apply to the alternate root too (resolveInstallRelativePath).
const patchRuntime = (pristineCtx && pristineCtx.runtime) || manifest.runtime || null;
const patchScope = manifest.scope === 'local' ? 'local' : 'global';
let skillsRedirect = null;
if (patchRuntime) {
const skillsRoot = _resolveSkillsRootDir(patchRuntime, configDir, patchScope);
const resolvedConfig = path.resolve(configDir);
if (
skillsRoot &&
skillsRoot !== resolvedConfig &&
!skillsRoot.startsWith(resolvedConfig + path.sep)
) {
const prefix = _hostBehaviors(patchRuntime).skillsManifestPrefix || 'skills/';
skillsRedirect = { root: skillsRoot, prefix };
}
}
for (const [relPath, originalHash] of Object.entries(manifest.files || {})) {
const safeRef = resolveInstallRelativePath(configDir, relPath);
if (!safeRef) continue;
const { relPath: safeRelPath, fullPath } = safeRef;
if (!fs.existsSync(fullPath)) continue;
const currentHash = fileHash(fullPath);
let installedPath = fullPath;
if (!fs.existsSync(installedPath) && skillsRedirect && safeRelPath.startsWith(skillsRedirect.prefix)) {
const altRef = resolveInstallRelativePath(
skillsRedirect.root,
safeRelPath.slice(skillsRedirect.prefix.length)
);
if (altRef && fs.existsSync(altRef.fullPath)) {
installedPath = altRef.fullPath;
}
}
if (!fs.existsSync(installedPath)) continue;
const currentHash = fileHash(installedPath);
if (currentHash !== originalHash) {
// Back up the user's modified version
const backupRef = resolveInstallRelativePath(patchesDir, safeRelPath);
if (!backupRef) continue;
const backupPath = backupRef.fullPath;
fs.mkdirSync(path.dirname(backupPath), { recursive: true });
fs.copyFileSync(fullPath, backupPath);
fs.copyFileSync(installedPath, backupPath);
modified.push(safeRelPath);
pristineHashes[safeRelPath] = originalHash;
}

View File

@@ -184,9 +184,72 @@ const REASON = Object.freeze({
FAIL_USER_LINES_MISSING: 'fail_user_lines_missing',
});
function verifyFile({ relPath, patchesDir, configDir, pristineDir, pristineHashes }) {
/**
* #4086: resolve where a backed-up file's INSTALLED counterpart lives.
* Primary is the config-dir-relative join (the manifest key's native form).
* For skills/ keys of a runtime whose skills kind declares a `home` override
* (codex global → $HOME/.agents/skills), the file lives OUTSIDE configDir, so
* when the primary path is absent we fall back to the runtime's ACTUAL skills
* root — derived from the same `resolveRuntimeArtifactLayout` seam the
* installer writes through. Returns the primary path unchanged whenever the
* fallback is impossible (no manifest, no runtime, no override, no file).
*/
function resolveInstalledPath(configDir, relPath, skillsRedirect) {
const primary = path.join(configDir, relPath);
if (!skillsRedirect) return primary;
const hashKey = relPath.replace(/\\/g, '/');
if (!hashKey.startsWith(skillsRedirect.prefix)) return primary;
if (fs.existsSync(primary)) return primary;
const alt = path.join(skillsRedirect.root, hashKey.slice(skillsRedirect.prefix.length));
return fs.existsSync(alt) ? alt : primary;
}
/**
* #4086: build the skills-root fallback descriptor for a config dir from its
* own gsd-file-manifest.json (runtime + scope) and the runtime-artifact
* layout seam. Returns null whenever any step is unavailable — the verifier
* then behaves exactly as before (config-dir-relative only). Lazy + guarded
* require: runtime-artifact-layout.cjs is a built lib; a layout-unaware
* invocation must never crash the gate.
*/
function resolveSkillsRedirect(configDir) {
try {
const manifest = JSON.parse(
fs.readFileSync(path.join(configDir, 'gsd-file-manifest.json'), 'utf8'),
);
if (!manifest || typeof manifest.runtime !== 'string' || !manifest.runtime) return null;
const scope = manifest.scope === 'local' ? 'local' : 'global';
const { resolveRuntimeArtifactLayout } = require('./lib/runtime-artifact-layout.cjs');
const layout = resolveRuntimeArtifactLayout(manifest.runtime, configDir, scope);
const kind = layout.kinds.find((k) => k.kind === 'skills');
if (!kind) return null;
const root = path.resolve(path.join(kind.home || configDir, kind.destSubpath));
const resolvedConfig = path.resolve(configDir);
if (root === resolvedConfig || root.startsWith(resolvedConfig + path.sep)) return null;
// Same descriptor-driven manifest prefix writeManifest uses for skills
// keys (hermes nests under 'skills/gsd/', everyone else 'skills/'), read
// from the same shipped registry the installer's _resolveHostBehaviors
// consults — never a re-derived literal (generative-fix-divergence guard).
let prefix = 'skills/';
try {
const { runtimes: registryRuntimes } = require('./lib/capability-registry.cjs');
const declared = registryRuntimes
&& registryRuntimes[manifest.runtime]
&& registryRuntimes[manifest.runtime].runtime
&& registryRuntimes[manifest.runtime].runtime.hostBehaviors;
if (declared && typeof declared.skillsManifestPrefix === 'string') {
prefix = declared.skillsManifestPrefix;
}
} catch { /* keep default prefix */ }
return { root, prefix };
} catch {
return null;
}
}
function verifyFile({ relPath, patchesDir, configDir, pristineDir, pristineHashes, skillsRedirect }) {
const backupPath = path.join(patchesDir, relPath);
const installedPath = path.join(configDir, relPath);
const installedPath = resolveInstalledPath(configDir, relPath, skillsRedirect);
const result = { file: relPath, status: 'ok', missing: [], reason: null };
if (!fs.existsSync(backupPath) || !fs.statSync(backupPath).isFile()) {
@@ -336,6 +399,9 @@ function main() {
// Bug #3657: read pristine_hashes from backup-meta.json once and share
// across all per-file verifications so each can detect drift independently.
const pristineHashes = readPristineHashes(opts.patchesDir);
// #4086: skills-root fallback for runtimes whose skills kind declares a
// `home` override outside configDir (codex global → $HOME/.agents/skills).
const skillsRedirect = resolveSkillsRedirect(opts.configDir);
const results = files.map((relPath) =>
verifyFile({
relPath,
@@ -343,6 +409,7 @@ function main() {
configDir: opts.configDir,
pristineDir: opts.pristineDir,
pristineHashes,
skillsRedirect,
}),
);
@@ -396,4 +463,4 @@ if (require.main === module) {
runMain(main);
}
module.exports = { computeUserAddedLines, isSignificantLine, verifyFile, walk, REASON, readPristineHashes, sha256 };
module.exports = { computeUserAddedLines, isSignificantLine, verifyFile, walk, REASON, readPristineHashes, sha256, resolveInstalledPath, resolveSkillsRedirect };

View File

@@ -3811,3 +3811,205 @@ describe('#3712 in-process home confinement', () => {
`codex skills must NOT resolve inside the ambient home ${ambientHome}, got ${dest}`);
});
});
// ────────────────────────────────────────────────────────────────────────
// Folded regression block — #4086 (Codex skills manifest keys never resolve)
// Codex installs skills to the skills-kind `home` override (~/.agents/skills),
// outside configDir (~/.codex). writeManifest() hashes skills from the real
// location, but saveLocalPatches() resolved every manifest key against
// configDir only — every skills/ key missed, so user modifications to Codex
// skills were never hash-compared, never backed up, silently overwritten.
// ────────────────────────────────────────────────────────────────────────
{
const { describe: __foldDescribe } = require('node:test');
__foldDescribe('folded:bug-4086-codex-skills-manifest-paths', () => {
'use strict';
const { test, beforeEach } = require('node:test');
const assert = require('node:assert/strict');
const fs = require('node:fs');
const os = require('node:os');
const path = require('node:path');
const crypto = require('node:crypto');
const ROOT = path.join(__dirname, '..');
const INSTALL = require(path.join(ROOT, 'bin', 'install.js'));
const { cleanup, sandboxHome, scrubConfigLocationEnv } = require('./helpers.cjs');
const MANIFEST_NAME = 'gsd-file-manifest.json';
const PATCHES_DIR_NAME = 'gsd-local-patches';
function sha256(content) {
return crypto.createHash('sha256').update(content, 'utf8').digest('hex');
}
function writeManifestFile(configDir, files, extra = {}) {
fs.mkdirSync(configDir, { recursive: true });
fs.writeFileSync(
path.join(configDir, MANIFEST_NAME),
JSON.stringify({ version: '1.12.0', timestamp: new Date().toISOString(), files, ...extra }, null, 2),
);
}
describe('Bug #4086: saveLocalPatches resolves skills/ manifest keys at the runtime skills root', () => {
let tmpDir;
let homeDir;
let configDir;
let skillsRoot;
let restoreConfigEnv;
let fakeSrcDir;
beforeEach((t) => {
tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-4086-'));
homeDir = path.join(tmpDir, 'home');
fs.mkdirSync(homeDir, { recursive: true });
sandboxHome(t, homeDir);
restoreConfigEnv = scrubConfigLocationEnv();
configDir = path.join(homeDir, '.codex');
// Same join the layout uses for codex's skills-kind home override
// (#3712 block above pins that this is $HOME/.agents/skills).
skillsRoot = path.join(homeDir, '.agents', 'skills');
fakeSrcDir = path.join(tmpDir, 'pkg-src');
fs.mkdirSync(fakeSrcDir, { recursive: true });
t.after(() => {
restoreConfigEnv();
cleanup(tmpDir);
});
});
test('saveLocalPatches backs up a modified Codex skill installed under ~/.agents/skills (#4086)', () => {
const pristine = '---\nname: gsd-x\ndescription: stock skill body line for hashing\n---\nstock\n';
const modified = pristine + '<!-- user edit 4086: a substantial marker line -->\n';
const relKey = 'skills/gsd-x/SKILL.md';
fs.mkdirSync(path.join(skillsRoot, 'gsd-x'), { recursive: true });
fs.writeFileSync(path.join(skillsRoot, 'gsd-x', 'SKILL.md'), modified);
writeManifestFile(configDir, { [relKey]: sha256(pristine) }, { runtime: 'codex', scope: 'global' });
const modifiedList = INSTALL.saveLocalPatches(configDir, {
packageSrc: fakeSrcDir,
runtime: 'codex',
pathPrefix: '',
isGlobal: true,
});
assert.deepEqual(modifiedList, [relKey], 'the modified skill must be detected via the skills root');
const backup = path.join(configDir, PATCHES_DIR_NAME, relKey);
assert.ok(fs.existsSync(backup), `backup must exist at ${PATCHES_DIR_NAME}/${relKey}`);
assert.equal(fs.readFileSync(backup, 'utf8'), modified, 'backup must hold the user-modified bytes');
const meta = JSON.parse(fs.readFileSync(path.join(configDir, PATCHES_DIR_NAME, 'backup-meta.json'), 'utf8'));
assert.deepEqual(meta.files, [relKey]);
});
test('unmodified Codex skill under ~/.agents/skills produces no patch (#4086)', () => {
const pristine = '---\nname: gsd-x\ndescription: stock skill body line for hashing\n---\nstock\n';
const relKey = 'skills/gsd-x/SKILL.md';
fs.mkdirSync(path.join(skillsRoot, 'gsd-x'), { recursive: true });
fs.writeFileSync(path.join(skillsRoot, 'gsd-x', 'SKILL.md'), pristine);
writeManifestFile(configDir, { [relKey]: sha256(pristine) }, { runtime: 'codex', scope: 'global' });
const modifiedList = INSTALL.saveLocalPatches(configDir, {
packageSrc: fakeSrcDir,
runtime: 'codex',
pathPrefix: '',
isGlobal: true,
});
assert.deepEqual(modifiedList, [], 'no false positive for an unmodified skill at the skills root');
});
test('config-dir-relative skills keep resolving against configDir first (#4086)', () => {
const pristine = '---\nname: gsd-x\ndescription: stock claude skill body line\n---\nstock\n';
const modified = pristine + '<!-- user edit 4086: claude skill marker line -->\n';
const relKey = 'skills/gsd-x/SKILL.md';
// claude has NO skills home override — skills live under configDir itself.
const claudeConfig = path.join(homeDir, '.claude');
fs.mkdirSync(path.join(claudeConfig, 'skills', 'gsd-x'), { recursive: true });
fs.writeFileSync(path.join(claudeConfig, 'skills', 'gsd-x', 'SKILL.md'), modified);
writeManifestFile(claudeConfig, { [relKey]: sha256(pristine) }, { runtime: 'claude', scope: 'global' });
const modifiedList = INSTALL.saveLocalPatches(claudeConfig, {
packageSrc: fakeSrcDir,
runtime: 'claude',
pathPrefix: '',
isGlobal: true,
});
assert.deepEqual(modifiedList, [relKey], 'config-dir-relative skills are detected exactly as before');
});
test('configDir copy wins when both locations exist (#4086)', () => {
const pristine = '---\nname: gsd-x\ndescription: stock skill body line for hashing\n---\nstock\n';
const configCopy = pristine + '<!-- user edit at configDir copy 4086 -->\n';
const rootCopy = pristine + '<!-- DIFFERENT user edit at skills root 4086 -->\n';
const relKey = 'skills/gsd-x/SKILL.md';
fs.mkdirSync(path.join(configDir, 'skills', 'gsd-x'), { recursive: true });
fs.writeFileSync(path.join(configDir, 'skills', 'gsd-x', 'SKILL.md'), configCopy);
fs.mkdirSync(path.join(skillsRoot, 'gsd-x'), { recursive: true });
fs.writeFileSync(path.join(skillsRoot, 'gsd-x', 'SKILL.md'), rootCopy);
writeManifestFile(configDir, { [relKey]: sha256(pristine) }, { runtime: 'codex', scope: 'global' });
const modifiedList = INSTALL.saveLocalPatches(configDir, {
packageSrc: fakeSrcDir,
runtime: 'codex',
pathPrefix: '',
isGlobal: true,
});
assert.deepEqual(modifiedList, [relKey]);
const backup = path.join(configDir, PATCHES_DIR_NAME, relKey);
assert.equal(fs.readFileSync(backup, 'utf8'), configCopy, 'configDir copy must be hashed/backed up, not the skills-root copy');
});
test('legacy runtime-less manifest is tolerated (#4086)', () => {
const pristine = '---\nname: gsd-x\ndescription: stock skill body line for hashing\n---\nstock\n';
const relKey = 'skills/gsd-x/SKILL.md';
fs.mkdirSync(path.join(skillsRoot, 'gsd-x'), { recursive: true });
fs.writeFileSync(path.join(skillsRoot, 'gsd-x', 'SKILL.md'), pristine + 'user line\n');
// No runtime field, and the caller passes no pristineCtx.runtime either
// (legacy callers) — the redirect is impossible; behave as before (skip).
writeManifestFile(configDir, { [relKey]: sha256(pristine) });
let modifiedList;
assert.doesNotThrow(() => {
modifiedList = INSTALL.saveLocalPatches(configDir, {});
}, 'a runtime-less manifest must not crash saveLocalPatches');
assert.deepEqual(modifiedList, [], 'without a runtime the old skip behavior applies');
});
test('end-to-end codex reinstall backs up the modified skill (#4086)', { timeout: 120_000 }, () => {
const origLog = console.log;
const origWarn = console.warn;
console.log = () => {};
console.warn = () => {};
try {
INSTALL.install(true, 'codex');
} finally {
console.log = origLog;
console.warn = origWarn;
}
const manifest = JSON.parse(fs.readFileSync(path.join(configDir, MANIFEST_NAME), 'utf8'));
const skillKeys = Object.keys(manifest.files).filter((k) => k.startsWith('skills/'));
assert.ok(skillKeys.length > 0, 'codex global install must record skills/ manifest keys');
const skillAbs = skillKeys.map((k) => path.join(homeDir, '.agents', k));
assert.ok(
skillKeys.every((k, i) => !fs.existsSync(path.join(configDir, k)) && fs.existsSync(skillAbs[i])),
'installed skills must live under ~/.agents, not under configDir (the #4086 premise)',
);
// User modifies one installed skill, then reinstalls.
fs.appendFileSync(skillAbs[0], '\n<!-- user edit 4086 e2e marker line -->\n');
try {
console.log = () => {};
console.warn = () => {};
INSTALL.install(true, 'codex');
} finally {
console.log = origLog;
console.warn = origWarn;
}
const backup = path.join(configDir, PATCHES_DIR_NAME, skillKeys[0]);
assert.ok(fs.existsSync(backup), `reinstall must back the modified skill up at ${PATCHES_DIR_NAME}/${skillKeys[0]}`);
assert.match(fs.readFileSync(backup, 'utf8'), /user edit 4086 e2e marker line/);
});
});
});
}

View File

@@ -1060,3 +1060,113 @@ describe('Bug #934: OK_NO_BASELINE when recordedHash present but pristine file a
});
});
}
// ────────────────────────────────────────────────────────────────────────
// Folded regression block — #4086 (verifier resolves skills/ entries at the
// runtime's ACTUAL skills root). Codex installs skills to ~/.agents/skills;
// verifyFile() joined every relPath against configDir only, so legacy
// Codex skills/ patch entries reported fail_installed_missing even though
// the file existed at its real location.
// ────────────────────────────────────────────────────────────────────────
{
const { describe: __foldDescribe } = require('node:test');
__foldDescribe('folded:bug-4086-verify-reapply-skills-root', () => {
'use strict';
const { test, describe, beforeEach, afterEach } = require('node:test');
const assert = require('node:assert/strict');
const fs = require('node:fs');
const os = require('node:os');
const path = require('node:path');
const { cleanup, scrubConfigLocationEnv } = require('./helpers.cjs');
const { runNode } = require('./helpers/process-seam.cjs');
const ROOT = path.join(__dirname, '..');
const SCRIPT = path.join(ROOT, 'gsd-core', 'bin', 'verify-reapply-patches.cjs');
const { REASON } = require(SCRIPT);
let tmpRoot;
let patchesDir;
let configDir;
let savedHome;
let savedUserProfile;
let restoreConfigEnv;
function writeFile(absPath, content) {
fs.mkdirSync(path.dirname(absPath), { recursive: true });
fs.writeFileSync(absPath, content);
}
function runVerifier() {
const r = runNode([
SCRIPT,
'--patches-dir', patchesDir,
'--config-dir', configDir,
'--json',
], { timeoutMs: 60_000 });
return {
status: r.exitCode,
report: r.stdout && r.stdout.length ? JSON.parse(r.stdout) : null,
};
}
beforeEach(() => {
tmpRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-4086-vfy-'));
patchesDir = path.join(tmpRoot, 'patches');
configDir = path.join(tmpRoot, 'home', '.codex');
fs.mkdirSync(patchesDir, { recursive: true });
fs.mkdirSync(configDir, { recursive: true });
// Sandbox HOME so the codex skills-kind home override resolves inside the
// fixture (~/.agents/skills), not the developer's real home.
const home = path.join(tmpRoot, 'home');
savedHome = process.env.HOME;
savedUserProfile = process.env.USERPROFILE;
process.env.HOME = home;
process.env.USERPROFILE = home;
restoreConfigEnv = scrubConfigLocationEnv();
});
afterEach(() => {
restoreConfigEnv();
process.env.HOME = savedHome;
if (savedUserProfile === undefined) delete process.env.USERPROFILE;
else process.env.USERPROFILE = savedUserProfile;
cleanup(tmpRoot);
});
describe('Bug #4086: verifyFile resolves skills entries at the runtime skills root', () => {
test('verifyFile resolves skills entries at the runtime\'s skills root (#4086)', () => {
const key = 'skills/gsd-x/SKILL.md';
writeFile(path.join(patchesDir, key), 'stock body line for the patch backup\nuser-added line that must survive merges\n');
writeFile(path.join(tmpRoot, 'home', '.agents', key), 'stock body line for the patch backup\nuser-added line that must survive merges\n');
// Manifest tells the verifier which runtime/scope owns this configDir.
writeFile(path.join(configDir, 'gsd-file-manifest.json'), JSON.stringify({
version: '1.12.0', runtime: 'codex', scope: 'global', files: {},
}));
const { status, report } = runVerifier();
assert.equal(status, 0, `gate must pass; got report ${JSON.stringify(report)}`);
assert.equal(report.failures, 0);
const r0 = report.results[0];
assert.equal(r0.status, 'ok');
assert.notEqual(r0.reason, REASON.FAIL_INSTALLED_MISSING);
});
test('verifyFile still fails when the skill file is genuinely missing (#4086)', () => {
const key = 'skills/gsd-gone/SKILL.md';
writeFile(path.join(patchesDir, key), 'stock body line for the patch backup\nuser-added line that must survive merges\n');
writeFile(path.join(configDir, 'gsd-file-manifest.json'), JSON.stringify({
version: '1.12.0', runtime: 'codex', scope: 'global', files: {},
}));
const { status, report } = runVerifier();
assert.equal(status, 1);
const r0 = report.results[0];
assert.equal(r0.file.replace(/\\/g, '/'), key);
assert.equal(r0.status, 'fail');
assert.equal(r0.reason, REASON.FAIL_INSTALLED_MISSING);
});
});
});
}