Found while fixing #1610, deliberately not grown into that PR: it is a pre-existing coverage hole in the READABLE case, not a readability-guard problem, and closing it changes what the migration does.
The shape
removeMacroIndirection collects its work with flattenChoices (src/migrations/removeMacroIndirection.ts):
const allChoices = flattenChoices(settings.choices);
flattenChoices descends Multi.choices and nothing else. A Macro choice living inside another macro's command list - a NestedChoice command, or one inside a Conditional branch - is invisible to it, even when every container in the vault is a perfectly good array.
So for such a choice:
- its
macroId is never resolved, and the macro it points at is classified as ORPHANED
- the orphan is duplicated as a fresh root-level Macro choice
settings.macros is then deleted, and the migration is flagged complete forever- the nested choice keeps a dangling
macroId that nothing at runtime resolves, so it is dead:
MacroChoiceEngine reports "No commands in the macro for choice 'X'"
Why it was not fixed in #1610
#1610 made the migration guards match each migration's own traversal, and deliberately left this one asking the narrow, folders-only question - precisely because widening the guard without widening the traversal would block the migration on data it cannot act on. Widening the TRAVERSAL is the real fix and changes behaviour for readable vaults, which deserves its own diff and its own before/after evidence.
Suggested shape
Raise the migration to the shared walk (walkAllChoices / the walkSettings traversal in src/migrations/helpers/choice-traversal.ts) instead of flattenChoices, collecting into an array first so the orphan push does not mutate the tree mid-walk. Then the guard can honestly become settingsTreeHasUnreadableData, and the two stay matched.
Worth checking how many vaults can still have this migration pending - it predates the embedded-macro format, so the blast radius is old vaults only.
Related
Found while fixing #1610, deliberately not grown into that PR: it is a pre-existing coverage hole in the READABLE case, not a readability-guard problem, and closing it changes what the migration does.
The shape
removeMacroIndirectioncollects its work withflattenChoices(src/migrations/removeMacroIndirection.ts):flattenChoicesdescendsMulti.choicesand nothing else. A Macro choice living inside another macro's command list - aNestedChoicecommand, or one inside a Conditional branch - is invisible to it, even when every container in the vault is a perfectly good array.So for such a choice:
macroIdis never resolved, and the macro it points at is classified as ORPHANEDsettings.macrosis then deleted, and the migration is flagged complete forevermacroIdthat nothing at runtime resolves, so it is dead:MacroChoiceEnginereports "No commands in the macro for choice 'X'"Why it was not fixed in #1610
#1610 made the migration guards match each migration's own traversal, and deliberately left this one asking the narrow, folders-only question - precisely because widening the guard without widening the traversal would block the migration on data it cannot act on. Widening the TRAVERSAL is the real fix and changes behaviour for readable vaults, which deserves its own diff and its own before/after evidence.
Suggested shape
Raise the migration to the shared walk (
walkAllChoices/ thewalkSettingstraversal insrc/migrations/helpers/choice-traversal.ts) instead offlattenChoices, collecting into an array first so the orphanpushdoes not mutate the tree mid-walk. Then the guard can honestly becomesettingsTreeHasUnreadableData, and the two stay matched.Worth checking how many vaults can still have this migration pending - it predates the embedded-macro format, so the blast radius is old vaults only.
Related