Skip to content

Commit be29d66

Browse files
committed
fix(cli): keep the --write hunks in meta.ts small; pin the failure exits
The human report keeps its indentation (a --write failure exits from inside the try, and the catch rethrows an oclif exit), so a later merge of main reconciles cleanly. The pins hold that --write writes exactly the applied set, that semantic TODOs are listed as the dry run lists them (not a pinned listing), and that both failure exits leave no file half-written. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RWZbGvPFcRKvUqASZtunCU
1 parent b1799bf commit be29d66

3 files changed

Lines changed: 108 additions & 38 deletions

File tree

‎.changeset/9591-migrate-meta-write.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
Clause-②: yes (widening)
88

99
- A new flag on the authored-source mode: `os migrate meta --from N --write`. Without it nothing changes: the dry run, its report and its `--json` payload are what they were, and `--out` still writes its snapshot.
10-
- What it writes: each mechanical change the chain applied (`applied`), at a site it traces to one object or array literal in one project file — through `define*` calls and `<spec export>.create(…)` factories, module-level `const` bindings, relative imports and re-exports, and `Object.values()` over a namespace import — when the loaded value matches that literal and nothing else references the bindings on the way. Only that site's bytes change: a renamed key keeps its value and its comments, a removed key takes its own line(s), and every other byte (comments, formatting, key order) stays as it was.
10+
- What it writes: each mechanical change the chain applied (`applied`), at a site it traces to one object or array literal in one project file — through `define*` calls and the `.create(…)` factories `@objectstack/spec` exports, module-level `const` bindings, relative imports and re-exports, and `Object.values()` over a namespace import — when the loaded value matches that literal and nothing else references the bindings on the way. Only that site's bytes change: a renamed key keeps its value and its comments, a removed key takes its own line(s), and every other byte (comments, formatting, key order) stays as it was.
1111
- What it refuses, each change listed with the reason (`--json`: `write.manual[].kind`): `computed`, `helper`, `spread`, `shared`, `outside-project`, `mismatch`, `injected`, `unspellable`, `layout` and `unattributed`; and `entangled`, because a conversion's edits are written whole or not at all.
1212
- What it never writes: the semantic changes (`todos`), which stay listed exactly as before, and a site a conversion declines, for which no mechanical change exists.
1313
- After writing it re-runs the chain over the written sources. Unless the re-run applies exactly the changes it left, it restores every file it wrote and exits 1.

‎packages/cli/src/commands/migrate/meta.ts‎

Lines changed: 33 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -741,7 +741,6 @@ export default class MigrateMeta extends Command {
741741

742742
if (!flags.json) printHeader('Migrate · meta');
743743

744-
let exitCode = 0;
745744
try {
746745
if (!flags.json) printStep('Loading configuration…');
747746
// `authoredSource`: read the config as the author WROTE it, not as the
@@ -789,7 +788,6 @@ export default class MigrateMeta extends Command {
789788
json: Boolean(flags.json),
790789
})
791790
: undefined;
792-
if (write && write.status !== 'written') exitCode = 1;
793791

794792
if (flags.json) {
795793
await emitJson({
@@ -826,35 +824,41 @@ export default class MigrateMeta extends Command {
826824
duration: timer.elapsed(),
827825
});
828826
if (flags.out) writeFileSync(resolve(flags.out), JSON.stringify(result.stack, null, 2));
829-
} else {
830-
printInfo(`Config: ${chalk.white(absolutePath)}`);
831-
// State this build's protocol major in the protocol's own units.
832-
// `PROTOCOL_VERSION` is that major padded to a semver ('17.0.0'), never
833-
// the installed package version -- printed as a bare semver under the
834-
// word "runtime" it read as one, so on a 17.3.0 install the operator saw
835-
// an apparent downgrade next to the real package versions of the same
836-
// upgrade session. The fact itself is worth keeping: with `--to` below
837-
// this build's major it is the only line saying where the runtime
838-
// actually stands. So it is relabelled and de-padded, not dropped.
839-
printInfo(`Chain: protocol ${fromMajor} → ${toMajor} (this runtime implements protocol ${PROTOCOL_MAJOR})`);
840-
console.log('');
841-
842-
// The verdict and the refusals lead, then the applied edits, then the
843-
// semantic notices — see printMigrationReport for why, and for what it
844-
// must never do to a notice.
845-
printMigrationReport({
846-
result,
847-
normalized,
848-
schemaValid: parsed.success,
849-
refusals: parsed.success ? [] : parsed.error.issues,
850-
dataMigrations,
851-
step: flags.step,
852-
...(flags.out ? { out: resolve(flags.out) } : {}),
853-
...(write ? { write } : {}),
854-
elapsed: timer.display(),
855-
});
827+
if (write && write.status !== 'written') this.exit(1);
828+
return;
856829
}
830+
831+
printInfo(`Config: ${chalk.white(absolutePath)}`);
832+
// State this build's protocol major in the protocol's own units.
833+
// `PROTOCOL_VERSION` is that major padded to a semver ('17.0.0'), never
834+
// the installed package version -- printed as a bare semver under the
835+
// word "runtime" it read as one, so on a 17.3.0 install the operator saw
836+
// an apparent downgrade next to the real package versions of the same
837+
// upgrade session. The fact itself is worth keeping: with `--to` below
838+
// this build's major it is the only line saying where the runtime
839+
// actually stands. So it is relabelled and de-padded, not dropped.
840+
printInfo(`Chain: protocol ${fromMajor} → ${toMajor} (this runtime implements protocol ${PROTOCOL_MAJOR})`);
841+
console.log('');
842+
843+
// The verdict and the refusals lead, then the applied edits, then the
844+
// semantic notices — see printMigrationReport for why, and for what it
845+
// must never do to a notice.
846+
printMigrationReport({
847+
result,
848+
normalized,
849+
schemaValid: parsed.success,
850+
refusals: parsed.success ? [] : parsed.error.issues,
851+
dataMigrations,
852+
step: flags.step,
853+
...(flags.out ? { out: resolve(flags.out) } : {}),
854+
...(write ? { write } : {}),
855+
elapsed: timer.display(),
856+
});
857+
// A write refused or undone is a failed run, reported above.
858+
if (write && write.status !== 'written') this.exit(1);
857859
} catch (error: any) {
860+
// `this.exit()` throws; a `--write` failure exits from inside the try.
861+
if (typeof error?.oclif?.exit === 'number') throw error;
858862
if (error instanceof MigrationFloorError) {
859863
if (flags.json) {
860864
await emitJson({ error: 'unsupported_from_major', message: error.message }, 0, { compact: true });
@@ -874,9 +878,6 @@ export default class MigrateMeta extends Command {
874878
if (!isReportedError(error)) printError(error.message || String(error));
875879
this.exit(1);
876880
}
877-
// Decided inside, exited outside: `this.exit()` throws, and the catch above
878-
// would report that throw as a bare "EEXIT: 1".
879-
if (exitCode !== 0) this.exit(exitCode);
880881
}
881882

882883
/**

‎packages/cli/test/migrate-meta-write.test.ts‎

Lines changed: 74 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@
3131
*/
3232

3333
import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest';
34-
import { mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync, statSync, symlinkSync, unlinkSync, writeFileSync } from 'node:fs';
34+
import { mkdirSync, mkdtempSync, readFileSync, readdirSync, realpathSync, rmSync, statSync, symlinkSync, unlinkSync, writeFileSync } from 'node:fs';
3535
import { tmpdir } from 'node:os';
3636
import { dirname, join, relative, resolve } from 'node:path';
3737
import { createRequire } from 'node:module';
@@ -42,9 +42,27 @@ import MigrateMeta from '../src/commands/migrate/meta.js';
4242
import {
4343
planAuthoredSourceWrite,
4444
verifyAuthoredSourceWrite,
45+
type AuthoredSourceWritePlan,
4546
type CodemodRefusalKind,
4647
} from '../src/utils/authored-source-codemod.js';
4748

49+
/**
50+
* A seam between planning and writing, for the two failure exits: the command
51+
* runs the real planner, and a test may act on the plan before the write.
52+
*/
53+
const hooks = vi.hoisted(() => ({ afterPlan: undefined as undefined | ((plan: AuthoredSourceWritePlan) => void) }));
54+
vi.mock('../src/utils/authored-source-codemod.js', async (importActual) => {
55+
const actual = await importActual<typeof import('../src/utils/authored-source-codemod.js')>();
56+
return {
57+
...actual,
58+
planAuthoredSourceWrite: async (input: Parameters<typeof actual.planAuthoredSourceWrite>[0]) => {
59+
const plan = await actual.planAuthoredSourceWrite(input);
60+
hooks.afterPlan?.(plan);
61+
return plan;
62+
},
63+
};
64+
});
65+
4866
const CLI_ROOT = resolve(fileURLToPath(import.meta.url), '..', '..');
4967
const CODEMOD_SOURCE = resolve(fileURLToPath(import.meta.url), '..', '..', 'src', 'utils', 'authored-source-codemod.ts');
5068
const RUN_TIMEOUT = 120_000;
@@ -352,12 +370,20 @@ describe('os migrate meta --write over per-artifact modules', () => {
352370
expect(at['views[0].list.bordered']).toBe('src/views/ticket.view.ts:10');
353371
});
354372

355-
it('never writes a semantic TODO: the manual list is the dry run\'s, unchanged', () => {
373+
it('writes the applied set and nothing else: every site it reports is an applied entry', () => {
374+
// What `--write` writes or leaves is exactly the chain's `applied` set —
375+
// never a semantic TODO, which the planner is not even handed.
376+
expect(sites([...firstJson.write.written, ...firstJson.write.manual])).toEqual(sites(firstJson.applied));
377+
});
378+
379+
it('never writes a semantic TODO, and lists them as the dry run does', () => {
380+
// Relative to the dry run of the same build, not a pinned listing: which
381+
// notices the default list carries is the chain's business, not --write's.
356382
expect(firstJson.todos.map((t: any) => t.id)).toEqual(dry.todos.map((t: any) => t.id));
357-
expect(firstJson.todos.length).toBeGreaterThan(0);
358-
// The declined `compareTo` arm keeps its bytes and its schema refusal.
383+
// The `compareTo` arm the conversion declines has no mechanical change, so
384+
// its bytes stay — and the schema still refuses it, as before the write.
359385
expect(readFileSync(join(dir, 'src/dashboards/kpi.dashboard.ts'), 'utf8')).toContain("compareTo: { offset: '7d' }");
360-
expect(firstJson.todos.map((t: any) => t.id)).toContain('dashboard-widget-compareto-offset');
386+
expect(firstJson.schemaValid).toBe(false);
361387
});
362388

363389
it('is idempotent: a second run applies nothing and writes nothing', async () => {
@@ -461,6 +487,49 @@ describe('os migrate meta --write adds a key where the conversion adds one', ()
461487
}, RUN_TIMEOUT);
462488
});
463489

490+
// ── the two failure exits: nothing is left half-written ─────────────────────
491+
492+
describe('os migrate meta --write fails closed', () => {
493+
it('writes nothing, and exits 1, when a file changed after it was read', async () => {
494+
const dir = writeProject(PROJECT);
495+
let touched = '';
496+
hooks.afterPlan = (plan) => {
497+
touched = plan.rewrites[0]!.path;
498+
writeFileSync(touched, `${readFileSync(touched, 'utf8')}// edited meanwhile\n`);
499+
};
500+
let run: Run;
501+
try {
502+
run = await runMeta(dir, ['--write', '--json']);
503+
} finally {
504+
hooks.afterPlan = undefined;
505+
}
506+
expect(run.exitCode).toBe(1);
507+
const payload = json(run);
508+
expect(payload.write.status).toBe('unwritten');
509+
expect(payload.write.error).toMatch(/changed on disk/);
510+
const rel = relative(realpathSync(dir), touched).split('\\').join('/');
511+
expect(snapshot(dir)).toEqual({ ...PROJECT, [rel]: `${PROJECT[rel]}// edited meanwhile\n` });
512+
}, RUN_TIMEOUT);
513+
514+
it('restores every file, and exits 1, when the re-run disagrees with its report', async () => {
515+
const dir = writeProject(PROJECT);
516+
// A report claiming one more site left by hand than the chain will find.
517+
hooks.afterPlan = (plan) => {
518+
plan.manual.push({ application: applied('nowhere.at.all', 'probe-phantom'), refusal: { kind: 'computed', reason: 'r' } });
519+
};
520+
let run: Run;
521+
try {
522+
run = await runMeta(dir, ['--write']);
523+
} finally {
524+
hooks.afterPlan = undefined;
525+
}
526+
expect(run.exitCode).toBe(1);
527+
expect(run.stdout).toContain('every one was restored to its previous bytes');
528+
expect(run.stdout).toContain('no longer converted: nowhere.at.all (probe-phantom)');
529+
expect(snapshot(dir)).toEqual(PROJECT);
530+
}, RUN_TIMEOUT);
531+
});
532+
464533
// ── 5: the refusals a real project produces ─────────────────────────────────
465534

466535
const REFUSALS: Record<string, string> = {

0 commit comments

Comments
 (0)