perf(lifecycle): cut install time by ~21% via script reuse, builder import caching, and phase-end migration optimization - #125
Conversation
- Merge install planning duration into a single module operation plan info log while keeping plan details at debug level. - Downgrade install finalizing step completion logs to debug and fold finalizing duration fields into the final module operation completed info log. - Align uninstall and upgrade plan summary logs with install by appending planning duration to the plan info attributes.
- Add unified pipeline progress events for module, app, bundles, and web stages via OnProgress callbacks. - Wire install, uninstall, and upgrade flows to the shared progress events so spinner updates stay consistent across operations. - Use global-stage spinner copy for bundles and web builds to avoid misleading root-module prefixes while keeping existing log fields unchanged.
- Add resolver options to attach module and application labels to cumulative esm resolver metrics logs. - Wire backend and web builders to pass module and application names when constructing resolver plugins. - Extend resolver logger tests to assert module and application fields are emitted in metrics output.
- Prefer loading already-built runtime scripts before falling back to rebuilding module entry scripts in migration runner resolution. - Add per-module phase-end completion timing logs during install and upgrade finalization for hotspot diagnosis.
- Add scripts.RunOptions.ReuseExecutorScripts and skip redundant executor reloads when script set is unchanged. - Make migration wrapper script module-agnostic and pass app/module selectors via request args to keep script content stable. - Reuse loaded executor scripts throughout install finalizing phase_end loop and restore once after the loop in module manager. - Update migration runner tests and add coverage proving script reuse avoids repeated reloads across module runs.
- Add hooks.RunOptions.ReuseExecutorScripts and generic hook wrapper args to reduce repeated script reloads during install hook phases. - Restore js executor scripts once for the full install operation and enable hook script reuse in installer pre_init/post_init runs. - Cache backend builder entryPointImports per builder run so prebuild/build passes avoid duplicate DB and filesystem scans. - Update hook runner tests with reuse-path coverage and generic wrapper assertions.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds resolver metadata labels, unified pipeline progress events, executor script reuse for hook and migration runners, and lifecycle wiring for progress, spinner, and phase-end execution. ChangesModule observability and execution reuse
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several enhancements, including adding module and application name metadata to the ESM resolver metrics, caching entry-point imports in the backend module builder, implementing a unified pipeline progress reporting mechanism with spinner updates, and optimizing JS script execution by allowing script reuse across module loops. The review feedback highlights several critical issues where b or b.module is dereferenced before a nil check in builder.go and webBuilder.go, which could lead to panics. Additionally, it is recommended to extend the script reuse optimization to the module upgrade flow in modulemanager.go to ensure consistency with the install flow.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/module/artifact/pipeline/pipeline.go (1)
958-976: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDelay the unified install-completed event until post-install generation succeeds.
Line 958 emits
module.install.completedbeforegenerateModulesForApp(app)can fail. If that generation fails, legacyOnInstallProgressreports failed, but unifiedOnProgresshas already reported success and never emitsmodule.install.failed.Proposed fix
- emitProgress(ProgressEvent{Stage: ProgressStageModuleInstallCompleted, Current: index + 1, Total: totalModules, Module: moduleName, Duration: installDuration}) logStep(slog.LevelInfo, "module installed", "installed_module", mod.Name, "duration_ms", installDuration.Milliseconds(), ) @@ if err := generateModulesForApp(app); err != nil { + emitProgress(ProgressEvent{Stage: ProgressStageModuleInstallFailed, Current: index + 1, Total: totalModules, Module: moduleName, Duration: installDuration, Err: err}) if cb.OnInstallProgress != nil { cb.OnInstallProgress(ModuleInstallProgress{ @@ return err } generated[app] = true } + emitProgress(ProgressEvent{Stage: ProgressStageModuleInstallCompleted, Current: index + 1, Total: totalModules, Module: moduleName, Duration: installDuration})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/module/artifact/pipeline/pipeline.go` around lines 958 - 976, The unified module install completion event is emitted too early in the install flow, before generateModulesForApp can still fail. Update the install path around the emitProgress call in pipeline.go so module.install.completed is only sent after generateModulesForApp(app) succeeds, and if that generation fails, emit the matching unified failure event for the same module/install step instead of success. Keep the existing legacy OnInstallProgress behavior in sync with the same success/failure outcome.
🧹 Nitpick comments (3)
internal/module/artifact/build/backend/builder.go (1)
195-199: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant/misleading nil check on
b.module.Line 195 (and 215) dereferences
b.module.Pathunconditionally right before the newif b.module != nilguard on the next line. Ifb.modulecan genuinely be nil,WithModulePath(b.module.Path)already panics before the guard is reached; if it can't be nil (as implied by other unguarded uses likeb.module.ApplicationStrat Line 117/164), the guard is dead code. Either fix the ordering so the nil-guard actually protects allb.modulefield accesses, or drop the guard sinceModuleBuilder.moduleis apparently never nil in this path.Example fix if module can be nil
- resolverOpts := []esmresolver.Option{ - esmresolver.WithCacheDir(runtimeOptions.defaultChoysumPath), - esmresolver.WithTarget("es2020"), - esmresolver.WithModulePath(b.module.Path), - } - if b.module != nil { - resolverOpts = append(resolverOpts, esmresolver.WithModuleName(b.module.Name), esmresolver.WithApplicationName(b.module.ApplicationStr)) - } + resolverOpts := []esmresolver.Option{ + esmresolver.WithCacheDir(runtimeOptions.defaultChoysumPath), + esmresolver.WithTarget("es2020"), + } + if b.module != nil { + resolverOpts = append(resolverOpts, + esmresolver.WithModulePath(b.module.Path), + esmresolver.WithModuleName(b.module.Name), + esmresolver.WithApplicationName(b.module.ApplicationStr)) + }Also applies to: 215-219
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/module/artifact/build/backend/builder.go` around lines 195 - 199, The nil check around b.module in builder.go is redundant and misleading because b.module.Path is already accessed before the guard, so either reorder the b.module checks in the affected builder setup to protect every b.module field access (including WithModulePath, WithModuleName, and WithApplicationName), or remove the guard entirely if ModuleBuilder.module is guaranteed non-nil in this flow. Use the existing resolverOpts construction in the builder method as the place to make the change so the handling of b.module is consistent.internal/module/lifecycle/modulemanager.go (1)
1521-1522: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winApply phase-end script reuse to upgrade as well.
Install phase-end passes
ReuseExecutorScripts: m.jsExecutor != nil, but upgrade still reloads per module. Mirror the install path here, including an executor snapshot/restore around the upgrade operation if reuse is enabled, so multi-module upgrades benefit from the same migration-script reuse.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/module/lifecycle/modulemanager.go` around lines 1521 - 1522, Phase-end script reuse is only applied for install, not upgrade. Update the upgrade path in ModuleManager’s phase-end handling to mirror the install flow by passing ReuseExecutorScripts when invoking the runner and, when reuse is enabled, taking an executor snapshot before the upgrade and restoring it afterward so multi-module upgrades reuse migration scripts across modules.internal/module/artifact/pipeline/pipeline_test.go (1)
278-288: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the app build/generate completion stages too.
The new API includes
ProgressStageAppBuildCompletedandProgressStageAppGenerateCompleted, but this test only requires their started counterparts. Add both completion stages so regressions in duration/error completion events are caught.Proposed fix
ProgressStageAppStageStarted, ProgressStageAppBuildStarted, + ProgressStageAppBuildCompleted, ProgressStageAppGenerateStarted, + ProgressStageAppGenerateCompleted, ProgressStageBundlesBuildStarted,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/module/artifact/pipeline/pipeline_test.go` around lines 278 - 288, The stage coverage in the pipeline progress test is incomplete: it asserts only the started events for app build and generate, so regressions in the completion events can slip through. Update the stage list in the pipeline test that iterates over ProgressStage values to also include ProgressStageAppBuildCompleted and ProgressStageAppGenerateCompleted, keeping the existing ProgressStageAppBuildStarted and ProgressStageAppGenerateStarted checks intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/module/evolution/hooks/runner.go`:
- Around line 363-370: In executeWithScripts on Runner, restore the previous
jsExecutor script set if r.jsExecutor.Reload(scripts...) fails after
SetJsScripts has already been called. Update the changedScripts branch to save
prevScripts, revert via r.jsExecutor.SetJsScripts(prevScripts) before returning
the reload error, and keep the behavior localized to this reload path so
equivalentScripts continues to reflect the actual runtime state.
In `@internal/module/evolution/scripts/runner_decorators.go`:
- Around line 321-327: The script update path in r.jsExecutor should not leave
metadata mutated if Reload fails. In runner_decorators.go, around the
changedScripts block, capture the previous script list from GetJsScripts before
calling SetJsScripts and Reload, and if Reload returns an error restore the old
scripts on the executor before returning the error. Use the existing jsExecutor
methods (GetJsScripts, SetJsScripts, Reload) in the migration runner flow so
later calls don’t incorrectly skip reloading based on stale equivalentScripts
state.
In `@internal/module/lifecycle/modulemanager.go`:
- Around line 902-905: The deferred restore in modulemanager.go’s lifecycle
cleanup ignores failures from m.jsExecutor.Reload, so restore problems are
hidden and the executor may remain in a bad state. Update the defer in the
modulemanager restore path to handle and log any Reload error, using the
existing m.jsExecutor and previousExecutorScripts context so failures are
visible during later lifecycle runs.
---
Outside diff comments:
In `@internal/module/artifact/pipeline/pipeline.go`:
- Around line 958-976: The unified module install completion event is emitted
too early in the install flow, before generateModulesForApp can still fail.
Update the install path around the emitProgress call in pipeline.go so
module.install.completed is only sent after generateModulesForApp(app) succeeds,
and if that generation fails, emit the matching unified failure event for the
same module/install step instead of success. Keep the existing legacy
OnInstallProgress behavior in sync with the same success/failure outcome.
---
Nitpick comments:
In `@internal/module/artifact/build/backend/builder.go`:
- Around line 195-199: The nil check around b.module in builder.go is redundant
and misleading because b.module.Path is already accessed before the guard, so
either reorder the b.module checks in the affected builder setup to protect
every b.module field access (including WithModulePath, WithModuleName, and
WithApplicationName), or remove the guard entirely if ModuleBuilder.module is
guaranteed non-nil in this flow. Use the existing resolverOpts construction in
the builder method as the place to make the change so the handling of b.module
is consistent.
In `@internal/module/artifact/pipeline/pipeline_test.go`:
- Around line 278-288: The stage coverage in the pipeline progress test is
incomplete: it asserts only the started events for app build and generate, so
regressions in the completion events can slip through. Update the stage list in
the pipeline test that iterates over ProgressStage values to also include
ProgressStageAppBuildCompleted and ProgressStageAppGenerateCompleted, keeping
the existing ProgressStageAppBuildStarted and ProgressStageAppGenerateStarted
checks intact.
In `@internal/module/lifecycle/modulemanager.go`:
- Around line 1521-1522: Phase-end script reuse is only applied for install, not
upgrade. Update the upgrade path in ModuleManager’s phase-end handling to mirror
the install flow by passing ReuseExecutorScripts when invoking the runner and,
when reuse is enabled, taking an executor snapshot before the upgrade and
restoring it afterward so multi-module upgrades reuse migration scripts across
modules.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 00748f30-d641-4dc0-9bbc-648bfd20993d
📒 Files selected for processing (12)
internal/esmresolver/resolver.gointernal/esmresolver/resolver_test.gointernal/module/artifact/build/backend/builder.gointernal/module/artifact/build/web/webBuilder.gointernal/module/artifact/pipeline/pipeline.gointernal/module/artifact/pipeline/pipeline_test.gointernal/module/evolution/hooks/runner.gointernal/module/evolution/hooks/runner_test.gointernal/module/evolution/scripts/runner_decorators.gointernal/module/evolution/scripts/runner_decorators_test.gointernal/module/lifecycle/installer.gointernal/module/lifecycle/modulemanager.go
- Restore previous executor scripts when Reload fails in hooks and migration runners. - Delay module.install.completed emission until per-app generation succeeds and emit module.install.failed on generation errors. - Align upgrade phase-end script execution with ReuseExecutorScripts and log js executor restore reload failures. - Guard resolver module options and nil receiver access ordering in backend/web builders.
|
/gemini review |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces several performance and observability enhancements, including caching entry-point imports in the module builder, adding module and application labels to the ESM resolver metrics, and implementing a unified progress event system to report detailed pipeline execution stages. Additionally, it optimizes JS hook and migration runners by allowing scripts to be reused on the shared executor across runs. The review feedback correctly highlights two important issues: first, in resolveScripts, compilation errors from buildModuleEntryScript are swallowed when LoadRuntimeScripts fails; second, the Upgrade flow in ModuleManager lacks the script restoration deferred block that was added to the Install flow, which could lead to script leakage on the shared executor.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
- Preserve buildModuleEntryScript errors in resolveScripts when runtime scripts are missing. - Add upgrade-flow js executor script restoration with reload-failure warning, matching install flow cleanup. - Add regression test for resolveScripts fallback error precedence.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces observability labels (module and application names) to the ESM resolver, implements caching for entry-point imports in the backend builder, and adds a unified progress event system to the artifact pipeline to update user-facing progress views. Additionally, it optimizes JS script execution by allowing loaded scripts to be reused across module loops to avoid redundant reloads. The feedback points out that while script restoration blocks were added to the Install and Uninstall operations in the module manager, the Upgrade operation is missing this restoration logic, which could leave the shared executor in an inconsistent state.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
- Add missing js executor script restoration defer in Upgrade flow. - Log reload restore failures consistently with Install/Uninstall.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces unified progress reporting across pipeline execution stages, adds caching for entry-point imports in the backend builder, and implements a mechanism to reuse loaded JS scripts on the shared executor to avoid redundant reloads during module lifecycle operations. Feedback on these changes highlights potential nil pointer dereference panics in builder.go and webBuilder.go due to inconsistent nil-checking of b.module. Additionally, in modulemanager.go, the spinner state should be cleared if runner.RunPhase fails during the finalizing phase of the upgrade operation to prevent a stuck terminal UI.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
- Clear spinner state before returning when Upgrade phase-end RunPhase fails.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces unified progress reporting across pipeline execution stages (install, uninstall, upgrade, and builds) and optimizes JS execution by allowing loaded scripts to be reused across module loops. It also adds module and application labels to the ESM resolver for better observability. However, a critical bug was identified in the migration runner where prioritizing cached runtime scripts over building the latest source code during upgrades can cause new migration scripts to be skipped.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
- Resolve migration scripts source-first for Pre/Post/Validate and keep runtime-first only for PhaseEnd. - Add regression coverage for phase-aware script resolution preference and update runner tests accordingly.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a unified progress tracking system across pipeline execution stages, adds observability labels to the ESM resolver, implements caching for entry-point imports, and optimizes JS hook and migration execution by allowing script reuse on the shared executor. Feedback on these changes suggests extracting duplicated progress-handling logic in ModuleManager into a helper method, removing the now-obsolete buildHookEnvScript and its tests, and resolving inconsistent nil checks on b.module in both builder.go and webBuilder.go to prevent potential panics.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
- Remove obsolete hook env script helper and related unit-test assertions. - Restore consistent module non-nil invariant usage in backend/web resolver option assembly by removing partial nil guards.
- Add handlePipelineSharedProgress to centralize shared app/bundles/web spinner stage updates. - Reuse the helper in install, uninstall, and upgrade pipeline OnProgress callbacks while keeping operation-specific module stage handling in each callback. - Preserve existing user-facing progress messages and fallback behavior for unknown app names and total app counts.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces performance optimizations and observability improvements to the module lifecycle and build pipeline. Key changes include caching entry-point imports in the module builder, implementing a unified progress reporting mechanism (OnProgress) integrated with CLI spinner updates, and optimizing JS execution by making hook/migration wrappers generic and allowing script reuse on the shared JS executor to avoid redundant reloads. Additionally, module and application labels have been added to the ESM resolver for enhanced observability.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/module/evolution/scripts/runner_decorators_test.go (1)
154-172: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the new
prepareRunnerModuleSourcehelper instead of duplicating setup.This test manually re-implements the same modules-path/tsconfig/entry-point setup that
prepareRunnerModuleSource(lines 38-60) was just extracted to encapsulate. Using the helper here keeps the fixture logic in one place and avoids future drift between the two copies.♻️ Proposed refactor
- if err := os.MkdirAll(testRuntimeScope.cfg.ModulesPath, 0o755); err != nil { - t.Fatalf("mkdir modules path: %v", err) - } - if err := os.WriteFile(filepath.Join(testRuntimeScope.cfg.ModulesPath, "tsconfig.json"), []byte(`{"compilerOptions":{"baseUrl":".","paths":{"`@/`*":["./*"]}}}`), 0o644); err != nil { - t.Fatalf("write tsconfig: %v", err) - } - - entryPoint := filepath.Join(testRuntimeScope.cfg.ModulesPath, "base", "service", "index.ts") - if err := os.MkdirAll(filepath.Dir(entryPoint), 0o755); err != nil { - t.Fatalf("mkdir entry dir: %v", err) - } - if err := os.WriteFile(entryPoint, []byte("export const migration = {}\n"), 0o644); err != nil { - t.Fatalf("write entry point: %v", err) - } + prepareRunnerModuleSource(t, testRuntimeScope, "base", "service/index.ts", "export const migration = {}\n")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/module/evolution/scripts/runner_decorators_test.go` around lines 154 - 172, The test setup in TestResolveScripts_SourceFirstUnlessPhaseEnd is duplicating the modules-path, tsconfig, and entry-point fixture logic that prepareRunnerModuleSource already encapsulates. Replace the manual os.MkdirAll/os.WriteFile setup with a call to prepareRunnerModuleSource in runner_decorators_test.go, then use its returned entry-point/module source values when building the test case so the fixture logic stays centralized and consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/module/evolution/scripts/runner_decorators_test.go`:
- Around line 154-172: The test setup in
TestResolveScripts_SourceFirstUnlessPhaseEnd is duplicating the modules-path,
tsconfig, and entry-point fixture logic that prepareRunnerModuleSource already
encapsulates. Replace the manual os.MkdirAll/os.WriteFile setup with a call to
prepareRunnerModuleSource in runner_decorators_test.go, then use its returned
entry-point/module source values when building the test case so the fixture
logic stays centralized and consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4aa91f66-2522-4cba-8856-1257a47aa70b
📒 Files selected for processing (7)
internal/module/artifact/build/backend/builder.gointernal/module/artifact/build/web/webBuilder.gointernal/module/evolution/hooks/runner.gointernal/module/evolution/hooks/runner_test.gointernal/module/evolution/scripts/runner_decorators.gointernal/module/evolution/scripts/runner_decorators_test.gointernal/module/lifecycle/modulemanager.go
💤 Files with no reviewable changes (2)
- internal/module/evolution/hooks/runner.go
- internal/module/evolution/hooks/runner_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/module/artifact/build/web/webBuilder.go
- internal/module/artifact/build/backend/builder.go
- internal/module/lifecycle/modulemanager.go
- internal/module/evolution/scripts/runner_decorators.go
- Reuse prepareRunnerModuleSource in TestResolveScripts_SourceFirstUnlessPhaseEnd instead of duplicating setup. - Add TestHandlePipelineSharedProgress covering all shared stage branches and edge cases (app stage, build, generate, bundles, web, unknown stage). - Add TestEquivalentScripts unit test for nil, different lengths, different content, and nil-element comparisons. - Add TestExecuteWithScriptsReloadFailureRollback for both scripts and hooks packages, verifying script restoration and double-reload on failure. - Add TestRunPhaseNonRequiredHookUnavailable covering non-required phase resolution and execution failure warning paths. - Add TestSummarizeInfoNames for empty, dedup, empty-string skip, all-empty, and over-limit cases.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
internal/module/evolution/scripts/runner_decorators_test.go (1)
434-465: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame rollback-assertion concern as
internal/module/evolution/hooks/runner_test.go.
failingReloadExecutor.Reloadreturns the configured error without ever callinge.inner.Reload(...), sobaseExecutor's stored scripts (set once at Line 521) are never actually mutated by either reload attempt. The "verify executor scripts were restored to previous" check at Lines 541-544 therefore doesn't prove thatexecuteWithScriptsperforms a real rollback against a stateful executor — only the reload call-order/argument assertions (Lines 547-555) meaningfully validate the rollback path.Also applies to: 514-556
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/module/evolution/scripts/runner_decorators_test.go` around lines 434 - 465, The rollback test in failingReloadExecutor.Reload is only stubbing the error path and never mutating the wrapped executor state, so the restore assertion in executeWithScripts is not proving a real rollback. Update failingReloadExecutor so it can simulate a stateful Reload by delegating to the inner executor before/while returning the configured error, and keep the reloaded capture so the assertions in runner_decorators_test still verify both the rollback call order and that the baseExecutor scripts were actually restored.internal/module/artifact/pipeline/pipeline_test.go (1)
2094-2143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid coverage; consider adding an exact-boundary case.
Tests cover nil, single, dedup, empty-skip, all-empty, and over-limit (
infoSummaryNameListLimit+1), but not the exact boundary (infoSummaryNameListLimititems) where the>comparison insummarizeInfoNamesshould still return the compact list rather thannil. Adding that case would guard against an off-by-one regression in the>vs>=check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/module/artifact/pipeline/pipeline_test.go` around lines 2094 - 2143, Add a test case in TestSummarizeInfoNames for the exact boundary of infoSummaryNameListLimit, since summarizeInfoNames should still return the compact name list when the input size equals the limit. Use the existing summarizeInfoNames helper and compare against the over-limit case to verify the > check does not accidentally behave like >=, and keep the assertion aligned with the other subtests.internal/module/evolution/hooks/runner_test.go (1)
636-666: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTighten the rollback test coverage The restored-scripts check only exercises the explicit
SetJsScripts(prevScripts)restore; becausehooksFailingReloadExecutor.Reloadshort-circuits onreloadErr, it doesn’t prove the rollbackReload(prevScripts...)call has any effect. If the intent is to cover rollback behavior, make the failure wrapper delegate the second reload or drop this assertion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/module/evolution/hooks/runner_test.go` around lines 636 - 666, The rollback test is only verifying the direct SetJsScripts restore path, while hooksFailingReloadExecutor.Reload currently returns early on reloadErr and never exercises the rollback Reload(prevScripts...) behavior. Update the test around hooksFailingReloadExecutor and the rollback flow so the failure wrapper still delegates the rollback reload call, or remove the restored-scripts assertion if that path is not meant to be covered. Keep the coverage focused on the actual rollback behavior in the failing reload scenario.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/module/artifact/pipeline/pipeline_test.go`:
- Around line 2094-2143: Add a test case in TestSummarizeInfoNames for the exact
boundary of infoSummaryNameListLimit, since summarizeInfoNames should still
return the compact name list when the input size equals the limit. Use the
existing summarizeInfoNames helper and compare against the over-limit case to
verify the > check does not accidentally behave like >=, and keep the assertion
aligned with the other subtests.
In `@internal/module/evolution/hooks/runner_test.go`:
- Around line 636-666: The rollback test is only verifying the direct
SetJsScripts restore path, while hooksFailingReloadExecutor.Reload currently
returns early on reloadErr and never exercises the rollback
Reload(prevScripts...) behavior. Update the test around
hooksFailingReloadExecutor and the rollback flow so the failure wrapper still
delegates the rollback reload call, or remove the restored-scripts assertion if
that path is not meant to be covered. Keep the coverage focused on the actual
rollback behavior in the failing reload scenario.
In `@internal/module/evolution/scripts/runner_decorators_test.go`:
- Around line 434-465: The rollback test in failingReloadExecutor.Reload is only
stubbing the error path and never mutating the wrapped executor state, so the
restore assertion in executeWithScripts is not proving a real rollback. Update
failingReloadExecutor so it can simulate a stateful Reload by delegating to the
inner executor before/while returning the configured error, and keep the
reloaded capture so the assertions in runner_decorators_test still verify both
the rollback call order and that the baseExecutor scripts were actually
restored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ff8766f-11a9-4045-880a-848bb41f2705
📒 Files selected for processing (4)
internal/module/artifact/pipeline/pipeline_test.gointernal/module/evolution/hooks/runner_test.gointernal/module/evolution/scripts/runner_decorators_test.gointernal/module/lifecycle/modulemanager_logging_test.go
1 participant
Summary
This branch reduces
install documenttotal wall time by ~21.5% (115s → 90s in local benchmarks), primarily by eliminating redundant hook/migration script runs and caching builder imports across modules.Commits
chore(lifecycle): reduce module operation log noise— drop verbose heartbeat logs in TTY mode to reduce output noise.feat(lifecycle): unify pipeline-driven spinner progress— consolidate spinner updates into the pipeline callback layer for consistent progress reporting.feat(esmresolver): annotate metrics with build context— add module/application labels to ESM resolver metrics for better observability.perf(lifecycle): reduce phase-end migration overhead— trim unnecessary migration work during the finalizing phase.perf(lifecycle): reuse migration scripts across phase-end modules— share compiled migration script runners across modules rather than re-initializing per module.perf(lifecycle): reuse hook scripts and cache install builder imports— primary performance gain: cache JsScripts across install steps and share hook runners; responsible for ~21% install-time reduction across all modules.Benchmark
install documenttotalBenchmark logs:
.choysum/tmp/regression/install-document-breakdown-{baseline,optimized}-*.summary.logChanged Files
internal/module/lifecycle/modulemanager.gointernal/module/evolution/hooks/runner.gointernal/module/evolution/scripts/runner_decorators.gointernal/module/artifact/build/backend/builder.gointernal/module/artifact/pipeline/pipeline.gointernal/esmresolver/resolver.goTesting
go test ./internal/module/lifecycle ./internal/module/artifact/build/backend ./internal/module/evolution/... ./internal/esmresolver/...— all pass.By submitting this pull request, you agree to the Contributor License Agreement (CLA) of this project. If you have not yet signed it, please follow the instructions provided by @cla-bot below.
Summary by CodeRabbit
New Features
Bug Fixes
Tests