fix(upgrade): report which files a step could not upgrade, and recover

[imorland]

The problem

A single unparseable file anywhere in an extension ended the whole upgrade:2.0 run:

Error occurred, and could not complete:
Unexpected token (2:25)
SyntaxError: Unexpected token (2:25)
at constructor (/…/node_modules/@babel/parser/lib/index.js:351:19)
at V8IntrinsicMixin.raise (/…/node_modules/@babel/parser/lib/index.js:3233:19)
… 8 more frames of our internals

Nothing there names the file. Preceding it, the terminal was handed a dump of the file's entire contents from a leftover debug console.log. And because the step's partial writes stayed on disk, the uncommitted-changes guard then refused to let the command be run again — so the author had to work out what to revert before they could even retry.

What it does now

Each step collects every file it could not handle, rolls back its own writes, and aborts with the file names and the reasons:

 => FAILED
js/src/forum/alsobroken.js
Could not parse: Unexpected token (2:11)
js/src/forum/broken.js
Could not parse: Unexpected token (2:25)
=> ERROR 2 files could not be upgraded by this step, so it made no changes.
Fix the problems above and run the command again — steps that already
completed are skipped, so it will resume from here.

Two decisions worth calling out:

Abort rather than skip the file and continue. A step that half-applied its transforms would leave the extension in a state nobody could reason about — some files migrated, some not, all committed together. Steps stay all-or-nothing.

Roll back the step's own writes. This is what makes the advice in the message true. Since every step starts behind the dirty-tree guard and each successful step commits its own work, anything uncommitted at the point of failure was written by the failing step — so checkout -- . is bounded to that step and cannot discard the author's work.

Collect all failures before aborting, so the author fixes everything the step found in one pass instead of rediscovering problems one re-run at a time.

Crash paths closed

Five places could kill the run. Four were found by reading; the fifth (the before() pre-pass) only showed up when driving the real binary.

Location	Was	Now
advancedContent JSON parse	command.error() — exits the process	throws with the parse reason
advancedContent JS/TS parse	no handling at all	throws with the parse reason
re-parse after transform	warned with a full file dump, rethrew	throws, names what failed
generateCode	warned with a full file dump, rethrew	throws, names what failed
before() pre-pass	unprotected	collected like the transform pass

Also

• Removed a leftover debug console.log(code) in parseCode that printed whole files on any parse failure. The file name is now in the report, which is what's actually needed.
• backend/phpunit.ts pointed authors at http://localhost:3000/extend/testing#model-factories — someone's dev server. Now https://docs.flarum.org/2.x/extend/testing#model-factories.

Testing

5 new unit tests for the failure collector, written red first. 181 tests pass; Prettier, ESLint and tsc clean.

Unit tests can't tell you whether the command behaves, so I drove the real binary against a fixture extension for each case: one broken file, two broken files, fix-and-resume (proving the run goes on through all 16 steps, which is what the error message promises), and a clean extension to confirm the normal path still formats and commits as before.

[imorland]

The problem

A single unparseable file anywhere in an extension ended the whole upgrade:2.0 run:

Error occurred, and could not complete:
Unexpected token (2:25)
SyntaxError: Unexpected token (2:25)
at constructor (/…/node_modules/@babel/parser/lib/index.js:351:19)
at V8IntrinsicMixin.raise (/…/node_modules/@babel/parser/lib/index.js:3233:19)
… 8 more frames of our internals

Nothing there names the file. Preceding it, the terminal was handed a dump of the file's entire contents from a leftover debug console.log. And because the step's partial writes stayed on disk, the uncommitted-changes guard then refused to let the command be run again — so the author had to work out what to revert before they could even retry.

What it does now

Each step collects every file it could not handle, rolls back its own writes, and aborts with the file names and the reasons:

 => FAILED
js/src/forum/alsobroken.js
Could not parse: Unexpected token (2:11)
js/src/forum/broken.js
Could not parse: Unexpected token (2:25)
=> ERROR 2 files could not be upgraded by this step, so it made no changes.
Fix the problems above and run the command again — steps that already
completed are skipped, so it will resume from here.

Two decisions worth calling out:

Abort rather than skip the file and continue. A step that half-applied its transforms would leave the extension in a state nobody could reason about — some files migrated, some not, all committed together. Steps stay all-or-nothing.

Roll back the step's own writes. This is what makes the advice in the message true. Since every step starts behind the dirty-tree guard and each successful step commits its own work, anything uncommitted at the point of failure was written by the failing step — so checkout -- . is bounded to that step and cannot discard the author's work.

Collect all failures before aborting, so the author fixes everything the step found in one pass instead of rediscovering problems one re-run at a time.

Crash paths closed

Five places could kill the run. Four were found by reading; the fifth (the before() pre-pass) only showed up when driving the real binary.

Location	Was	Now
advancedContent JSON parse	command.error() — exits the process	throws with the parse reason
advancedContent JS/TS parse	no handling at all	throws with the parse reason
re-parse after transform	warned with a full file dump, rethrew	throws, names what failed
generateCode	warned with a full file dump, rethrew	throws, names what failed
before() pre-pass	unprotected	collected like the transform pass

Also

• Removed a leftover debug console.log(code) in parseCode that printed whole files on any parse failure. The file name is now in the report, which is what's actually needed.
• backend/phpunit.ts pointed authors at http://localhost:3000/extend/testing#model-factories — someone's dev server. Now https://docs.flarum.org/2.x/extend/testing#model-factories.

Testing

5 new unit tests for the failure collector, written red first. 181 tests pass; Prettier, ESLint and tsc clean.

Unit tests can't tell you whether the command behaves, so I drove the real binary against a fixture extension for each case: one broken file, two broken files, fix-and-resume (proving the run goes on through all 16 steps, which is what the error message promises), and a clean extension to confirm the normal path still formats and commits as before.

[imorland]

The problem

A single unparseable file anywhere in an extension ended the whole upgrade:2.0 run:

Error occurred, and could not complete:
Unexpected token (2:25)
SyntaxError: Unexpected token (2:25)
at constructor (/…/node_modules/@babel/parser/lib/index.js:351:19)
at V8IntrinsicMixin.raise (/…/node_modules/@babel/parser/lib/index.js:3233:19)
… 8 more frames of our internals

Nothing there names the file. Preceding it, the terminal was handed a dump of the file's entire contents from a leftover debug console.log. And because the step's partial writes stayed on disk, the uncommitted-changes guard then refused to let the command be run again — so the author had to work out what to revert before they could even retry.

What it does now

Each step collects every file it could not handle, rolls back its own writes, and aborts with the file names and the reasons:

 => FAILED
js/src/forum/alsobroken.js
Could not parse: Unexpected token (2:11)
js/src/forum/broken.js
Could not parse: Unexpected token (2:25)
=> ERROR 2 files could not be upgraded by this step, so it made no changes.
Fix the problems above and run the command again — steps that already
completed are skipped, so it will resume from here.

Two decisions worth calling out:

Abort rather than skip the file and continue. A step that half-applied its transforms would leave the extension in a state nobody could reason about — some files migrated, some not, all committed together. Steps stay all-or-nothing.

Roll back the step's own writes. This is what makes the advice in the message true. Since every step starts behind the dirty-tree guard and each successful step commits its own work, anything uncommitted at the point of failure was written by the failing step — so checkout -- . is bounded to that step and cannot discard the author's work.

Collect all failures before aborting, so the author fixes everything the step found in one pass instead of rediscovering problems one re-run at a time.

Crash paths closed

Five places could kill the run. Four were found by reading; the fifth (the before() pre-pass) only showed up when driving the real binary.

Location	Was	Now
advancedContent JSON parse	command.error() — exits the process	throws with the parse reason
advancedContent JS/TS parse	no handling at all	throws with the parse reason
re-parse after transform	warned with a full file dump, rethrew	throws, names what failed
generateCode	warned with a full file dump, rethrew	throws, names what failed
before() pre-pass	unprotected	collected like the transform pass

Also

• Removed a leftover debug console.log(code) in parseCode that printed whole files on any parse failure. The file name is now in the report, which is what's actually needed.
• backend/phpunit.ts pointed authors at http://localhost:3000/extend/testing#model-factories — someone's dev server. Now https://docs.flarum.org/2.x/extend/testing#model-factories.

Testing

5 new unit tests for the failure collector, written red first. 181 tests pass; Prettier, ESLint and tsc clean.

Unit tests can't tell you whether the command behaves, so I drove the real binary against a fixture extension for each case: one broken file, two broken files, fix-and-resume (proving the run goes on through all 16 steps, which is what the error message promises), and a clean extension to confirm the normal path still formats and commits as before.

[imorland]

The problem

A single unparseable file anywhere in an extension ended the whole upgrade:2.0 run:

Error occurred, and could not complete:
Unexpected token (2:25)
SyntaxError: Unexpected token (2:25)
at constructor (/…/node_modules/@babel/parser/lib/index.js:351:19)
at V8IntrinsicMixin.raise (/…/node_modules/@babel/parser/lib/index.js:3233:19)
… 8 more frames of our internals

Nothing there names the file. Preceding it, the terminal was handed a dump of the file's entire contents from a leftover debug console.log. And because the step's partial writes stayed on disk, the uncommitted-changes guard then refused to let the command be run again — so the author had to work out what to revert before they could even retry.

What it does now

Each step collects every file it could not handle, rolls back its own writes, and aborts with the file names and the reasons:

 => FAILED
js/src/forum/alsobroken.js
Could not parse: Unexpected token (2:11)
js/src/forum/broken.js
Could not parse: Unexpected token (2:25)
=> ERROR 2 files could not be upgraded by this step, so it made no changes.
Fix the problems above and run the command again — steps that already
completed are skipped, so it will resume from here.

Two decisions worth calling out:

Abort rather than skip the file and continue. A step that half-applied its transforms would leave the extension in a state nobody could reason about — some files migrated, some not, all committed together. Steps stay all-or-nothing.

Roll back the step's own writes. This is what makes the advice in the message true. Since every step starts behind the dirty-tree guard and each successful step commits its own work, anything uncommitted at the point of failure was written by the failing step — so checkout -- . is bounded to that step and cannot discard the author's work.

Collect all failures before aborting, so the author fixes everything the step found in one pass instead of rediscovering problems one re-run at a time.

Crash paths closed

Five places could kill the run. Four were found by reading; the fifth (the before() pre-pass) only showed up when driving the real binary.

Location	Was	Now
advancedContent JSON parse	command.error() — exits the process	throws with the parse reason
advancedContent JS/TS parse	no handling at all	throws with the parse reason
re-parse after transform	warned with a full file dump, rethrew	throws, names what failed
generateCode	warned with a full file dump, rethrew	throws, names what failed
before() pre-pass	unprotected	collected like the transform pass

Also

• Removed a leftover debug console.log(code) in parseCode that printed whole files on any parse failure. The file name is now in the report, which is what's actually needed.
• backend/phpunit.ts pointed authors at http://localhost:3000/extend/testing#model-factories — someone's dev server. Now https://docs.flarum.org/2.x/extend/testing#model-factories.

Testing

5 new unit tests for the failure collector, written red first. 181 tests pass; Prettier, ESLint and tsc clean.

Unit tests can't tell you whether the command behaves, so I drove the real binary against a fixture extension for each case: one broken file, two broken files, fix-and-resume (proving the run goes on through all 16 steps, which is what the error message promises), and a clean extension to confirm the normal path still formats and commits as before.

[imorland]

The problem

A single unparseable file anywhere in an extension ended the whole upgrade:2.0 run:

Error occurred, and could not complete:
Unexpected token (2:25)
SyntaxError: Unexpected token (2:25)
at constructor (/…/node_modules/@babel/parser/lib/index.js:351:19)
at V8IntrinsicMixin.raise (/…/node_modules/@babel/parser/lib/index.js:3233:19)
… 8 more frames of our internals

Nothing there names the file. Preceding it, the terminal was handed a dump of the file's entire contents from a leftover debug console.log. And because the step's partial writes stayed on disk, the uncommitted-changes guard then refused to let the command be run again — so the author had to work out what to revert before they could even retry.

What it does now

Each step collects every file it could not handle, rolls back its own writes, and aborts with the file names and the reasons:

 => FAILED
js/src/forum/alsobroken.js
Could not parse: Unexpected token (2:11)
js/src/forum/broken.js
Could not parse: Unexpected token (2:25)
=> ERROR 2 files could not be upgraded by this step, so it made no changes.
Fix the problems above and run the command again — steps that already
completed are skipped, so it will resume from here.

Two decisions worth calling out:

Abort rather than skip the file and continue. A step that half-applied its transforms would leave the extension in a state nobody could reason about — some files migrated, some not, all committed together. Steps stay all-or-nothing.

Roll back the step's own writes. This is what makes the advice in the message true. Since every step starts behind the dirty-tree guard and each successful step commits its own work, anything uncommitted at the point of failure was written by the failing step — so checkout -- . is bounded to that step and cannot discard the author's work.

Collect all failures before aborting, so the author fixes everything the step found in one pass instead of rediscovering problems one re-run at a time.

Crash paths closed

Five places could kill the run. Four were found by reading; the fifth (the before() pre-pass) only showed up when driving the real binary.

Location	Was	Now
advancedContent JSON parse	command.error() — exits the process	throws with the parse reason
advancedContent JS/TS parse	no handling at all	throws with the parse reason
re-parse after transform	warned with a full file dump, rethrew	throws, names what failed
generateCode	warned with a full file dump, rethrew	throws, names what failed
before() pre-pass	unprotected	collected like the transform pass

Also

• Removed a leftover debug console.log(code) in parseCode that printed whole files on any parse failure. The file name is now in the report, which is what's actually needed.
• backend/phpunit.ts pointed authors at http://localhost:3000/extend/testing#model-factories — someone's dev server. Now https://docs.flarum.org/2.x/extend/testing#model-factories.

Testing

5 new unit tests for the failure collector, written red first. 181 tests pass; Prettier, ESLint and tsc clean.

Unit tests can't tell you whether the command behaves, so I drove the real binary against a fixture extension for each case: one broken file, two broken files, fix-and-resume (proving the run goes on through all 16 steps, which is what the error message promises), and a clean extension to confirm the normal path still formats and commits as before.

[imorland]

The problem

A single unparseable file anywhere in an extension ended the whole upgrade:2.0 run:

Error occurred, and could not complete:
Unexpected token (2:25)
SyntaxError: Unexpected token (2:25)
at constructor (/…/node_modules/@babel/parser/lib/index.js:351:19)
at V8IntrinsicMixin.raise (/…/node_modules/@babel/parser/lib/index.js:3233:19)
… 8 more frames of our internals

Nothing there names the file. Preceding it, the terminal was handed a dump of the file's entire contents from a leftover debug console.log. And because the step's partial writes stayed on disk, the uncommitted-changes guard then refused to let the command be run again — so the author had to work out what to revert before they could even retry.

What it does now

Each step collects every file it could not handle, rolls back its own writes, and aborts with the file names and the reasons:

 => FAILED
js/src/forum/alsobroken.js
Could not parse: Unexpected token (2:11)
js/src/forum/broken.js
Could not parse: Unexpected token (2:25)
=> ERROR 2 files could not be upgraded by this step, so it made no changes.
Fix the problems above and run the command again — steps that already
completed are skipped, so it will resume from here.

Two decisions worth calling out:

Abort rather than skip the file and continue. A step that half-applied its transforms would leave the extension in a state nobody could reason about — some files migrated, some not, all committed together. Steps stay all-or-nothing.

Roll back the step's own writes. This is what makes the advice in the message true. Since every step starts behind the dirty-tree guard and each successful step commits its own work, anything uncommitted at the point of failure was written by the failing step — so checkout -- . is bounded to that step and cannot discard the author's work.

Collect all failures before aborting, so the author fixes everything the step found in one pass instead of rediscovering problems one re-run at a time.

Crash paths closed

Five places could kill the run. Four were found by reading; the fifth (the before() pre-pass) only showed up when driving the real binary.

Location	Was	Now
advancedContent JSON parse	command.error() — exits the process	throws with the parse reason
advancedContent JS/TS parse	no handling at all	throws with the parse reason
re-parse after transform	warned with a full file dump, rethrew	throws, names what failed
generateCode	warned with a full file dump, rethrew	throws, names what failed
before() pre-pass	unprotected	collected like the transform pass

Also

• Removed a leftover debug console.log(code) in parseCode that printed whole files on any parse failure. The file name is now in the report, which is what's actually needed.
• backend/phpunit.ts pointed authors at http://localhost:3000/extend/testing#model-factories — someone's dev server. Now https://docs.flarum.org/2.x/extend/testing#model-factories.

Testing

5 new unit tests for the failure collector, written red first. 181 tests pass; Prettier, ESLint and tsc clean.

Unit tests can't tell you whether the command behaves, so I drove the real binary against a fixture extension for each case: one broken file, two broken files, fix-and-resume (proving the run goes on through all 16 steps, which is what the error message promises), and a clean extension to confirm the normal path still formats and commits as before.

[imorland]

The problem

A single unparseable file anywhere in an extension ended the whole upgrade:2.0 run:

Error occurred, and could not complete:
Unexpected token (2:25)
SyntaxError: Unexpected token (2:25)
at constructor (/…/node_modules/@babel/parser/lib/index.js:351:19)
at V8IntrinsicMixin.raise (/…/node_modules/@babel/parser/lib/index.js:3233:19)
… 8 more frames of our internals

Nothing there names the file. Preceding it, the terminal was handed a dump of the file's entire contents from a leftover debug console.log. And because the step's partial writes stayed on disk, the uncommitted-changes guard then refused to let the command be run again — so the author had to work out what to revert before they could even retry.

What it does now

Each step collects every file it could not handle, rolls back its own writes, and aborts with the file names and the reasons:

 => FAILED
js/src/forum/alsobroken.js
Could not parse: Unexpected token (2:11)
js/src/forum/broken.js
Could not parse: Unexpected token (2:25)
=> ERROR 2 files could not be upgraded by this step, so it made no changes.
Fix the problems above and run the command again — steps that already
completed are skipped, so it will resume from here.

Two decisions worth calling out:

Abort rather than skip the file and continue. A step that half-applied its transforms would leave the extension in a state nobody could reason about — some files migrated, some not, all committed together. Steps stay all-or-nothing.

Roll back the step's own writes. This is what makes the advice in the message true. Since every step starts behind the dirty-tree guard and each successful step commits its own work, anything uncommitted at the point of failure was written by the failing step — so checkout -- . is bounded to that step and cannot discard the author's work.

Collect all failures before aborting, so the author fixes everything the step found in one pass instead of rediscovering problems one re-run at a time.

Crash paths closed

Five places could kill the run. Four were found by reading; the fifth (the before() pre-pass) only showed up when driving the real binary.

Location	Was	Now
advancedContent JSON parse	command.error() — exits the process	throws with the parse reason
advancedContent JS/TS parse	no handling at all	throws with the parse reason
re-parse after transform	warned with a full file dump, rethrew	throws, names what failed
generateCode	warned with a full file dump, rethrew	throws, names what failed
before() pre-pass	unprotected	collected like the transform pass

Also

• Removed a leftover debug console.log(code) in parseCode that printed whole files on any parse failure. The file name is now in the report, which is what's actually needed.
• backend/phpunit.ts pointed authors at http://localhost:3000/extend/testing#model-factories — someone's dev server. Now https://docs.flarum.org/2.x/extend/testing#model-factories.

Testing

5 new unit tests for the failure collector, written red first. 181 tests pass; Prettier, ESLint and tsc clean.

Unit tests can't tell you whether the command behaves, so I drove the real binary against a fixture extension for each case: one broken file, two broken files, fix-and-resume (proving the run goes on through all 16 steps, which is what the error message promises), and a clean extension to confirm the normal path still formats and commits as before.