fix: normalize oclif v2 plugin hooks for v4 compatibility#922
fix: normalize oclif v2 plugin hooks for v4 compatibility#922
Conversation
When the global aio-cli (oclif v2) runs commands from this plugin
(oclif v4), v4's Config.load re-uses the v2 Plugin objects as-is.
Those objects store hooks as plain string arrays, but v4's runHook
expects {identifier, target} objects — causing hook.target to be
undefined and crashing path.extname() with ERR_INVALID_ARG_TYPE.
Normalize all plugin hooks to v4 format once in BaseCommand.init()
so every command that calls this.config.runHook works correctly.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![github-actions[bot]](https://avatars.gh.lumenfield.work/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 PR Reviewer
The diff adds a normalization step for oclif v2 plugin hooks in the init method. The logic is straightforward and well-commented, but there are a few concerns around safety and maintainability.
📝 2 suggestion(s) - Please review inline comments below.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When the global aio-cli (oclif v2) runs commands from this plugin
(oclif v4), v4's Config.load re-uses the v2 Plugin objects as-is.
Those objects store hooks as plain string arrays, but v4's runHook
expects {identifier, target} objects — causing hook.target to be
undefined and crashing path.extname() with ERR_INVALID_ARG_TYPE.
Normalize all plugin hooks to v4 format in BaseCommand.init(), but
only when string hooks are actually present to avoid unnecessary
mutation of already-normalized plugin objects.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When the global aio-cli (oclif v2) runs commands from this plugin
(oclif v4), v4's Config.load re-uses the v2 Plugin objects as-is.
Those objects store hooks as plain string arrays, but v4's runHook
expects {identifier, target} objects — causing hook.target to be
undefined and crashing path.extname() with ERR_INVALID_ARG_TYPE.
Normalize all plugin hooks to v4 format in BaseCommand.init(), but
only when string hooks are actually present to avoid unnecessary
mutation of already-normalized plugin objects. Uses getPluginsList()
if available, falling back to this.config.plugins Map.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🤖 PR Reviewer
The previous suggestions have been addressed: a guard for getPluginsList is now in place, and a check prevents unnecessary mutation when no string hooks are present. The implementation is clean and well-tested with good coverage of edge cases including fallback paths and frozen/already-normalized hooks.
✅ LGTM! This PR looks good to merge.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
There was a problem hiding this comment.
🤖 PR Reviewer
The implementation is clean and well-reasoned, correctly normalizing oclif v2 string hooks to v4 object format with appropriate guards. Tests are thorough and cover edge cases including the fallback path, frozen-object concerns, and mixed formats. One minor concern: the code silently mutates plugin objects that may be shared/frozen references, though the comment acknowledges this, and the actual freeze guard is not implemented.
✅ LGTM! This PR looks good to merge.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This reverts commit cab6c55.
There was a problem hiding this comment.
🤖 PR Reviewer
The hook normalization logic is well-structured and the tests provide good coverage. However, the previously raised concern about mutating potentially frozen/sealed plugin hooks objects remains unaddressed — the comment says 'guard against frozen plugin references' but no actual guard is implemented, which will throw a TypeError in strict mode if a plugin's hooks object is frozen or sealed.
🔄 1 re-raised suggestion(s) from previous review
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
When the global aio-cli (oclif v2) runs commands from this plugin
(oclif v4), v4's Config.load re-uses the v2 Plugin objects as-is.
Those objects store hooks as plain string arrays, but v4's runHook
expects {identifier, target} objects — causing hook.target to be
undefined and crashing path.extname() with ERR_INVALID_ARG_TYPE.
Normalize all plugin hooks to v4 format in BaseCommand.init(), but
only when string hooks are actually present to avoid unnecessary
mutation of already-normalized plugin objects. Uses getPluginsList()
if available, falling back to this.config.plugins Map. Wraps
assignment in try/catch to handle frozen plugin hook objects.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🤖 PR Reviewer
The re-raised frozen-object concern has been addressed with a try/catch block. The implementation is clean and well-tested, covering string normalization, frozen hooks, fallback plugin iteration, and edge cases. No significant issues remain.
✅ LGTM! This PR looks good to merge.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
2 participants
Fixes #921
Summary
aio-cli(oclif v2) invokes commands from this plugin (oclif v4), v4'sConfig.loaddetects the incoming v2Configand re-uses itsPluginobjects directly['./src/hooks/foo.js']); oclif v4'srunHookexpects{identifier, target}objects — sohook.targetwasundefined, crashingpath.extname(undefined)withERR_INVALID_ARG_TYPE{identifier, target}format once inBaseCommand.init(), covering every command that callsthis.config.runHookTest plan
aio app deployin a project with@adobe/aio-cli-plugin-eventslinked — confirm thepre-deploy-event-reghook runs withoutERR_INVALID_ARG_TYPEaio app undeploy— confirmpre-undeploy-event-reghook worksaio app pack— confirmpre-packhook worksnpm test🤖 Generated with Claude Code