diff --git a/CHANGELOG.md b/CHANGELOG.md index ba1e78b..1a5b394 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,19 @@ major version, and each entry says so when it happens. ## [Unreleased] +### Fixed + +- A release that added a generated file (as rc.11 did with + `submit/answers.schema.json` and `submit/agent-instructions.txt`) could not be + applied to a deployment without two hand-made pull requests. The updater + refused the new ownership rules until the deployment had migrated, and the + deployment's ownership check refused the migration until it had the new + generator. The updater now adopts rules that only protect new generated + files the deployment does not have, and `npm run generate` writes them. Every + other ownership change still fails closed. This takes effect from the update + after the one that ships it, because a deployment runs the updater it + already has. + ## [1.11.0-rc.13] — 2026-10-17 Thirteenth candidate. It carries rc.12 plus one fix to the upload service, so a diff --git a/docs/upgrading.md b/docs/upgrading.md index 31d173e..aa1b11f 100644 --- a/docs/upgrading.md +++ b/docs/upgrading.md @@ -36,6 +36,15 @@ that changes owner should be handled, and then rerun the update. This prevents a path from being overwritten and a path reclaimed by PHCT from silently retaining stale deployment content. +One change needs no migration: a release that only adds new generated files, such as a new file +`npm run generate` writes from your configuration. The updater adopts those rules itself when every +added rule is a single file (not a pattern) marked `merge=ours`, listed under `ownership.generated` +in the release's `.phct/ownership.yml`, and absent from your working tree (committed or not), and when every existing +rule is kept in order. It leaves those files alone, `npm run generate` writes them from your data, +and the run's log names them. Anything else, including a new pattern, a path PHCT takes back, or a +file you already have, still fails closed. The updater that runs is the one your deployment already +has, so this applies from the update after the one that ships it. + ## Optional expert merge setup ```sh diff --git a/scripts/apply_phct_update.mjs b/scripts/apply_phct_update.mjs index 0c9d4cd..dec6b69 100644 --- a/scripts/apply_phct_update.mjs +++ b/scripts/apply_phct_update.mjs @@ -7,6 +7,8 @@ import process from 'node:process'; import { execFileSync } from 'node:child_process'; import { fileURLToPath } from 'node:url'; +import * as yaml from 'js-yaml'; + import { forkOwnershipRules, isForkOwned, isImmutableUpdateRef } from './upgrade_check.mjs'; const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); @@ -107,26 +109,117 @@ export function planReconciliation(targetFiles, currentFiles, rules) { return { take, remove, preserve }; } +const CONTRACT_CHANGED = + 'Target release changes the deployment ownership contract. Apply and review the ' + + '.gitattributes and .phct/ownership.yml migration separately before rerunning this update.'; + +const sameRule = (a, b) => a.pattern === b.pattern && a.owned === b.owned; + +/** + * The target's ownership rules, when they are the current rules plus only new + * generator outputs the deployment does not have yet. Such a path cannot hold + * deployment content, so protecting it overwrites nothing and reclaims + * nothing: the update leaves it alone and `npm run generate` writes it from + * the deployment's own data. Each added rule must be a literal `merge=ours` + * path, listed in the target's `ownership.generated`, absent from the + * deployment (tracked or not), and still protected after every target rule; the existing rules + * must survive in order. Anything else returns null. + * + * @param {{pattern: string, owned: boolean}[]} currentRules + * @param {{pattern: string, owned: boolean}[]} targetRules + * @param {{generated?: string[], currentFiles?: string[], exists?: (file: string) => boolean}} context + * `exists` reports a path on disk, so an untracked file there counts as present + * @returns {string[]|null} the adopted paths, or null when the change is not of that kind + */ +export function generatedOutputAdditions( + currentRules, + targetRules, + { generated = [], currentFiles = [], exists = () => false } = {} +) { + const added = []; + let next = 0; + for (const rule of targetRules) { + if (next < currentRules.length && sameRule(rule, currentRules[next])) next += 1; + else added.push(rule); + } + if (next !== currentRules.length || added.length === 0) return null; + + const present = new Set(currentFiles); + const adoptable = (rule) => + rule.owned && + !/[*?[\]\\]/.test(rule.pattern) && + !rule.pattern.endsWith('/') && + generated.includes(rule.pattern) && + !present.has(rule.pattern) && + !exists(rule.pattern) && + isForkOwned(targetRules, rule.pattern); + return added.every(adoptable) ? added.map((rule) => rule.pattern) : null; +} + /** * Refuse to guess when a release moves paths across the ownership boundary. * Ownership migrations need a separate, human-reviewed downstream change so * newly protected files cannot be overwritten and reclaimed files cannot keep - * stale deployment content. + * stale deployment content. The one exception is a release that only adds + * generator outputs the deployment does not have (generatedOutputAdditions): + * the update adopts those rules itself. * * @param {string} currentAttributes * @param {string} targetAttributes - * @returns {{pattern: string, owned: boolean}[]} + * @param {{generated?: string[], currentFiles?: string[]}} [context] the target's + * `ownership.generated` and the deployment's tracked files; without them only an + * unchanged contract passes + * @returns {{rules: {pattern: string, owned: boolean}[], adopted: string[]}} */ -export function assertStableOwnershipContract(currentAttributes, targetAttributes) { +export function resolveOwnershipContract(currentAttributes, targetAttributes, context = {}) { const currentRules = forkOwnershipRules(currentAttributes); const targetRules = forkOwnershipRules(targetAttributes); - if (JSON.stringify(currentRules) !== JSON.stringify(targetRules)) { - throw new Error( - 'Target release changes the deployment ownership contract. Apply and review the ' + - '.gitattributes and .phct/ownership.yml migration separately before rerunning this update.' - ); + if (JSON.stringify(currentRules) === JSON.stringify(targetRules)) { + return { rules: currentRules, adopted: [] }; + } + const adopted = generatedOutputAdditions(currentRules, targetRules, context); + if (!adopted) throw new Error(CONTRACT_CHANGED); + return { rules: targetRules, adopted }; +} + +/** + * The strict form: the target must keep the current contract exactly. + * + * @param {string} currentAttributes + * @param {string} targetAttributes + * @returns {{pattern: string, owned: boolean}[]} + */ +export function assertStableOwnershipContract(currentAttributes, targetAttributes) { + return resolveOwnershipContract(currentAttributes, targetAttributes).rules; +} + +/** + * `ownership.generated` from the target release's manifest; empty when the + * release has none, which leaves only the strict contract check. + * + * @param {string} root + * @param {string} to + * @returns {string[]} + */ +function targetGeneratedPaths(root, to) { + let text; + try { + text = git(root, ['show', `${to}:.phct/ownership.yml`]); + } catch { + return []; + } + const generated = yaml.load(text)?.ownership?.generated; + return Array.isArray(generated) ? generated.filter((item) => typeof item === 'string') : []; +} + +/** Anything at the path, including a dangling symlink. */ +function pathExists(file) { + try { + fs.lstatSync(file); + return true; + } catch { + return false; } - return currentRules; } function nulList(output) { @@ -162,12 +255,16 @@ export function applyUpdate({ root = ROOT, from, to }) { const currentAttributes = fs.readFileSync(path.join(root, '.gitattributes'), 'utf8'); const targetAttributes = git(root, ['show', `${to}:.gitattributes`]); - const rules = assertStableOwnershipContract(currentAttributes, targetAttributes); + const targetFiles = nulList(git(root, ['ls-tree', '-r', '-z', '--name-only', to, '--'])); + const currentFiles = nulList(git(root, ['ls-files', '-z'])); + const { rules, adopted } = resolveOwnershipContract(currentAttributes, targetAttributes, { + generated: targetGeneratedPaths(root, to), + currentFiles, + exists: (file) => pathExists(path.join(root, file)), + }); const changes = parseNameStatusZ( git(root, ['diff', '--name-status', '-z', '--find-renames', from, to, '--']) ); - const targetFiles = nulList(git(root, ['ls-tree', '-r', '-z', '--name-only', to, '--'])); - const currentFiles = nulList(git(root, ['ls-files', '-z'])); const plan = planReconciliation(targetFiles, currentFiles, rules); for (const batch of batches(plan.remove)) { @@ -180,7 +277,7 @@ export function applyUpdate({ root = ROOT, from, to }) { const unmerged = git(root, ['diff', '--name-only', '--diff-filter=U', '--']).trim(); if (unmerged) throw new Error(`Update left unresolved paths:\n${unmerged}`); - return { changes, ...plan }; + return { changes, adopted, ...plan }; } function parseArgs(argv) { @@ -205,6 +302,11 @@ function main(argv) { console.log( `Reconciled ${result.take.length + result.remove.length} template-owned paths to ${to} from ${result.changes.length} parent changes; preserved ${result.preserve.length} deployment-owned paths.` ); + if (result.adopted.length > 0) { + console.log( + `Adopted the release's new generated outputs as deployment-owned; npm run generate writes them: ${result.adopted.join(', ')}.` + ); + } return 0; } catch (error) { console.error(error instanceof Error ? error.message : String(error)); diff --git a/test/scripts/apply_phct_update.test.mjs b/test/scripts/apply_phct_update.test.mjs index b259ca2..df49d1e 100644 --- a/test/scripts/apply_phct_update.test.mjs +++ b/test/scripts/apply_phct_update.test.mjs @@ -9,9 +9,11 @@ import { fileURLToPath } from 'node:url'; import { applyUpdate, assertStableOwnershipContract, + generatedOutputAdditions, parseNameStatusZ, planReconciliation, planUpdate, + resolveOwnershipContract, } from '../../scripts/apply_phct_update.mjs'; import { forkOwnershipRules } from '../../scripts/upgrade_check.mjs'; @@ -113,6 +115,124 @@ test('an ownership migration stops before changing the downstream tree', () => { assert.equal(git(root, 'status', '--short'), ''); }); +test('a release that only adds absent generator outputs is adopted, not refused', () => { + const current = 'config/** merge=ours\nconfig/template/** !merge\ngen/a.json merge=ours\n'; + const target = `${current}gen/b.json merge=ours\ngen/c.txt merge=ours\n`; + const context = { + generated: ['gen/a.json', 'gen/b.json', 'gen/c.txt'], + currentFiles: ['gen/a.json', 'app.js'], + }; + const { rules, adopted } = resolveOwnershipContract(current, target, context); + assert.deepEqual(adopted, ['gen/b.json', 'gen/c.txt']); + assert.deepEqual(rules, forkOwnershipRules(target)); + + // Inserted between existing rules is still a pure addition. + const between = + 'config/** merge=ours\ngen/b.json merge=ours\nconfig/template/** !merge\ngen/a.json merge=ours\n'; + assert.deepEqual(resolveOwnershipContract(current, between, context).adopted, ['gen/b.json']); + + // An unchanged contract adopts nothing. + assert.deepEqual(resolveOwnershipContract(current, `# target\n${current}`, context).adopted, []); +}); + +test('every other ownership change still fails closed', () => { + const current = 'config/** merge=ours\nconfig/template/** !merge\n'; + const context = { generated: ['gen/b.json', 'config/template/out.json'], currentFiles: ['app.js'] }; + const refused = (target, ctx = context) => + assert.throws( + () => resolveOwnershipContract(current, target, ctx), + /changes the deployment ownership contract/ + ); + + refused(`${current}gen/b.json merge=ours\n`, { ...context, currentFiles: ['gen/b.json'] }); // the deployment has the file + refused(`${current}gen/b.json merge=ours\n`, { ...context, exists: (file) => file === 'gen/b.json' }); // untracked on disk + refused(`${current}gen/b.json merge=ours\n`, { ...context, generated: [] }); // not a generator output + refused(`${current}gen/b.json merge=ours\n`, {}); // no context: strict + refused(`${current}gen/** merge=ours\n`, { ...context, generated: ['gen/**'] }); // a glob + refused(`${current}gen/b?.json merge=ours\n`, { ...context, generated: ['gen/b?.json'] }); + refused(`${current}gen/b.json merge=ours\ngen/b.json !merge\n`); // a later rule unprotects it + refused(`${current}gen/b.json merge=ours\nconfig/reclaimed/** !merge\n`); // a reclaim rides along + refused('config/template/** !merge\nconfig/** merge=ours\ngen/b.json merge=ours\n'); // reordered + refused('config/** merge=ours\ngen/b.json merge=ours\n'); // an existing rule removed + // Added before an existing exception glob that unprotects it: still template-owned. + refused('config/** merge=ours\nconfig/template/out.json merge=ours\nconfig/template/** !merge\n'); + assert.equal( + generatedOutputAdditions(forkOwnershipRules(current), forkOwnershipRules(current), context), + null + ); +}); + +test('the updater adopts a new generated output end to end and leaves it for generate', () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'phct-generated-addition-')); + git(root, 'init', '--quiet', '--initial-branch=main'); + git(root, 'config', 'user.name', 'PHCT test'); + git(root, 'config', 'user.email', 'test@example.invalid'); + fs.mkdirSync(path.join(root, '.phct')); + fs.mkdirSync(path.join(root, 'config')); + fs.writeFileSync(path.join(root, '.gitattributes'), 'config/** merge=ours\n'); + fs.writeFileSync(path.join(root, '.phct/ownership.yml'), 'ownership:\n generated: []\n'); + fs.writeFileSync(path.join(root, 'config/site.yml'), 'name: parent\n'); + fs.writeFileSync(path.join(root, 'app.txt'), 'parent v1\n'); + git(root, 'add', '.'); + git(root, 'commit', '--quiet', '-m', 'parent v1'); + git(root, 'tag', 'v1.0.0'); + + const targetAttributes = 'config/** merge=ours\nout/answers.json merge=ours\n'; + fs.writeFileSync(path.join(root, '.gitattributes'), targetAttributes); + fs.writeFileSync( + path.join(root, '.phct/ownership.yml'), + 'ownership:\n generated:\n - out/answers.json\n' + ); + fs.mkdirSync(path.join(root, 'out')); + fs.writeFileSync(path.join(root, 'out/answers.json'), '{"from":"the template demo"}\n'); + fs.writeFileSync(path.join(root, 'app.txt'), 'parent v2\n'); + git(root, 'add', '.'); + git(root, 'commit', '--quiet', '-m', 'parent v2'); + git(root, 'tag', 'v1.1.0'); + + git(root, 'checkout', '--quiet', '-b', 'downstream', 'v1.0.0'); + fs.writeFileSync(path.join(root, 'config/site.yml'), 'name: downstream\n'); + git(root, 'add', '.'); + git(root, 'commit', '--quiet', '-m', 'customize downstream'); + + const result = applyUpdate({ root, from: 'v1.0.0', to: 'v1.1.0' }); + assert.deepEqual(result.adopted, ['out/answers.json']); + assert.equal(fs.readFileSync(path.join(root, '.gitattributes'), 'utf8'), targetAttributes); + assert.equal(fs.readFileSync(path.join(root, 'app.txt'), 'utf8'), 'parent v2\n'); + assert.equal(fs.readFileSync(path.join(root, 'config/site.yml'), 'utf8'), 'name: downstream\n'); + assert.equal(fs.existsSync(path.join(root, 'out/answers.json')), false, 'left for npm run generate'); + assert.ok(result.preserve.includes('out/answers.json')); +}); + +test('an untracked file at a new generated path stops the update before it writes', () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'phct-generated-untracked-')); + git(root, 'init', '--quiet', '--initial-branch=main'); + git(root, 'config', 'user.name', 'PHCT test'); + git(root, 'config', 'user.email', 'test@example.invalid'); + fs.mkdirSync(path.join(root, '.phct')); + fs.writeFileSync(path.join(root, '.gitattributes'), 'config/** merge=ours\n'); + fs.writeFileSync(path.join(root, '.phct/ownership.yml'), 'ownership:\n generated: []\n'); + fs.writeFileSync(path.join(root, 'app.txt'), 'parent v1\n'); + git(root, 'add', '.'); + git(root, 'commit', '--quiet', '-m', 'parent v1'); + git(root, 'tag', 'v1.0.0'); + fs.writeFileSync(path.join(root, '.gitattributes'), 'config/** merge=ours\nout.json merge=ours\n'); + fs.writeFileSync(path.join(root, '.phct/ownership.yml'), 'ownership:\n generated:\n - out.json\n'); + fs.writeFileSync(path.join(root, 'app.txt'), 'parent v2\n'); + git(root, 'add', '.'); + git(root, 'commit', '--quiet', '-m', 'parent v2'); + git(root, 'tag', 'v1.1.0'); + git(root, 'checkout', '--quiet', '-b', 'downstream', 'v1.0.0'); + fs.writeFileSync(path.join(root, 'out.json'), 'hand-made, never committed\n'); + + assert.throws( + () => applyUpdate({ root, from: 'v1.0.0', to: 'v1.1.0' }), + /changes the deployment ownership contract/ + ); + assert.equal(fs.readFileSync(path.join(root, 'app.txt'), 'utf8'), 'parent v1\n'); + assert.equal(fs.readFileSync(path.join(root, 'out.json'), 'utf8'), 'hand-made, never committed\n'); +}); + test('the CLI runs when invoked through a symlinked path', () => { const root = fs.mkdtempSync(path.join(os.tmpdir(), 'phct-update-cli-')); const link = path.join(root, 'apply-update.mjs');