From 10ccc84c456919d1bb1c55cc8f83ee1313d54029 Mon Sep 17 00:00:00 2001 From: Christopher Collins Date: Sat, 17 Oct 2026 12:26:46 -0500 Subject: [PATCH 1/3] fix: let the updater adopt a release's new generated outputs A release that added a generator output deadlocked downstream updates: the updater refused the new merge=ours rules until the deployment had migrated, and the deployment's own ownership:check refused the migration until it had the new generated_paths.mjs. rc.11 needed two hand-made pull requests in BCHC (#143, #144) to get through. The updater now accepts a target contract that is the current one plus only literal merge=ours paths listed in the target's ownership.generated, absent from the deployment, still protected after every target rule, with every existing rule kept in order. It leaves those paths for npm run generate. Every other ownership change still fails closed. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 13 +++ docs/upgrading.md | 9 ++ scripts/apply_phct_update.mjs | 115 +++++++++++++++++++++--- test/scripts/apply_phct_update.test.mjs | 88 ++++++++++++++++++ 4 files changed, 212 insertions(+), 13 deletions(-) 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..5bc1bb7 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 repository, 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..ea131c1 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,105 @@ 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, 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[]}} context + * @returns {string[]|null} the adopted paths, or null when the change is not of that kind + */ +export function generatedOutputAdditions( + currentRules, + targetRules, + { generated = [], currentFiles = [] } = {} +) { + 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) && + 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 []; } - return currentRules; + const generated = yaml.load(text)?.ownership?.generated; + return Array.isArray(generated) ? generated.filter((item) => typeof item === 'string') : []; } function nulList(output) { @@ -162,12 +243,15 @@ 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, + }); 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 +264,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 +289,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..9ce3c5f 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,92 @@ 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, 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 + 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('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'); From 9a46234dff2d568088bfc4f61844584b512bf043 Mon Sep 17 00:00:00 2001 From: Christopher Collins Date: Sat, 17 Oct 2026 12:31:29 -0500 Subject: [PATCH 2/3] test: refuse an added generated output that an existing exception unprotects Co-Authored-By: Claude Opus 5.5 --- test/scripts/apply_phct_update.test.mjs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/scripts/apply_phct_update.test.mjs b/test/scripts/apply_phct_update.test.mjs index 9ce3c5f..11d7b65 100644 --- a/test/scripts/apply_phct_update.test.mjs +++ b/test/scripts/apply_phct_update.test.mjs @@ -153,6 +153,8 @@ test('every other ownership change still fails closed', () => { 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 From 6a255e6fd7767c27e88e461a70833ac89f53aa7e Mon Sep 17 00:00:00 2001 From: Christopher Collins Date: Sat, 17 Oct 2026 12:39:40 -0500 Subject: [PATCH 3/3] fix: count an untracked file at a new generated path as present In the manual update flow an untracked file is missing from git ls-files, so the updater would adopt the rule and npm run generate would replace the file. Any path on disk now refuses the adoption. Co-Authored-By: Claude Opus 5.5 --- docs/upgrading.md | 2 +- scripts/apply_phct_update.mjs | 19 +++++++++++++--- test/scripts/apply_phct_update.test.mjs | 30 +++++++++++++++++++++++++ 3 files changed, 47 insertions(+), 4 deletions(-) diff --git a/docs/upgrading.md b/docs/upgrading.md index 5bc1bb7..aa1b11f 100644 --- a/docs/upgrading.md +++ b/docs/upgrading.md @@ -39,7 +39,7 @@ 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 repository, and when every existing +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 diff --git a/scripts/apply_phct_update.mjs b/scripts/apply_phct_update.mjs index ea131c1..dec6b69 100644 --- a/scripts/apply_phct_update.mjs +++ b/scripts/apply_phct_update.mjs @@ -122,18 +122,19 @@ const sameRule = (a, b) => a.pattern === b.pattern && a.owned === b.owned; * 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, and still protected after every target rule; the existing rules + * 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[]}} context + * @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 = [] } = {} + { generated = [], currentFiles = [], exists = () => false } = {} ) { const added = []; let next = 0; @@ -150,6 +151,7 @@ export function generatedOutputAdditions( !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; } @@ -210,6 +212,16 @@ function targetGeneratedPaths(root, to) { 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; + } +} + function nulList(output) { const values = String(output ?? '').split('\0'); if (values.at(-1) === '') values.pop(); @@ -248,6 +260,7 @@ export function applyUpdate({ root = ROOT, from, to }) { 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, '--']) diff --git a/test/scripts/apply_phct_update.test.mjs b/test/scripts/apply_phct_update.test.mjs index 11d7b65..df49d1e 100644 --- a/test/scripts/apply_phct_update.test.mjs +++ b/test/scripts/apply_phct_update.test.mjs @@ -145,6 +145,7 @@ test('every other ownership change still fails closed', () => { ); 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 @@ -203,6 +204,35 @@ test('the updater adopts a new generated output end to end and leaves it for gen 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');