refactor(modules): extract approvals + interactive as registry-based modules#1840
Merged
refactor(modules): extract approvals + interactive as registry-based modules#1840
Conversation
…modules Phase 2 / PR #3 of the module refactor. Moves the approval and interactive- question flows out of core and into src/modules/, wired through the response dispatcher and delivery action registries. New modules: - src/modules/interactive/ — registers a response handler that claims pending_questions rows, writes question_response to the session DB, wakes the container. createPendingQuestion call stays inline in delivery.ts (guarded by hasTable) per plan. - src/modules/approvals/ — registers 3 delivery actions (install_packages, request_rebuild, add_mcp_server), a response handler for pending_approvals (including OneCLI action fall-through), an adapter-ready hook that boots the OneCLI manual-approval handler, and a shutdown hook that stops it. OneCLI implementation (src/onecli-approvals.ts) moves into the module. Core lifecycle hooks added (narrow, not registries): - onDeliveryAdapterReady(cb) in delivery.ts — fires when setDeliveryAdapter runs (or immediately if already set). Used by approvals for OneCLI boot. - onShutdown(cb) in index.ts — fires on SIGTERM/SIGINT. Used by approvals for OneCLI teardown. - getDeliveryAdapter() getter in delivery.ts — for live-flow adapter access in registered delivery actions. Core shrinks: delivery.ts 911 → 665 lines, index.ts 405 → 224 lines. dispatchResponse now logs "Unclaimed response" instead of falling through to an inline handler — the inline fallback moved into the two modules. Migration files renamed to the module-<name>-<short>.ts convention: - 003-pending-approvals.ts → module-approvals-pending-approvals.ts - 007-pending-approvals-title-options.ts → module-approvals-title-options.ts Migration.name fields unchanged so existing DBs treat them as already-applied. Degradation verified: emptying the modules barrel builds clean and 137/137 tests pass. Actions would log "Unknown system action"; button clicks would log "Unclaimed response". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR #3 introduced a circular-import temporal-dead-zone bug that didn't surface in unit tests but crashed the service at startup: src/index.ts imports './modules/index.js' for side effects → src/modules/interactive/index.ts calls registerResponseHandler() → that function is declared in src/index.ts → but src/index.ts's const responseHandlers = [] hasn't been initialized yet (we're in the middle of its module-init) → ReferenceError: Cannot access 'responseHandlers' before initialization Same issue for registerResponseHandler itself (the function reference resolves to undefined) and for onShutdown in the approvals module. Caught when the operator started the service and systemd flagged the process as crashing in auto-restart loop. Fix: extract responseHandlers + registerResponseHandler + shutdownCallbacks + onShutdown into src/response-registry.ts, which has no dependencies on src/index.ts or on modules. index.ts re-exports the same surface for any existing consumers; modules import directly from response-registry.js. The bug was latent because: - Unit tests import pieces, never src/index.ts's main() flow. - Host builds clean because TypeScript doesn't catch runtime circular init order. - Only surfaces when the ES module loader actually executes src/index.ts as the entry point. Verified: service boots on Linux host with approvals + interactive loaded; OneCLI handler starts via onDeliveryAdapterReady callback. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 2 / PR #3 of the module refactor. Moves the approval and interactive-question flows out of core and into
src/modules/, wired through the response dispatcher and delivery action registries added in PR #2.New modules
src/modules/interactive/— response handler claimspending_questionsrows, writesquestion_responsesystem message, wakes container.createPendingQuestioncall stays inline indelivery.ts(guarded byhasTable) per plan.src/modules/approvals/— 3 delivery actions (install_packages,request_rebuild,add_mcp_server), response handler forpending_approvals(with OneCLI fall-through), adapter-ready hook boots OneCLI, shutdown hook stops it.src/onecli-approvals.tsmoved into the module.Core lifecycle hooks added (narrow, not registries)
onDeliveryAdapterReady(cb)indelivery.ts— fires whensetDeliveryAdapterruns.onShutdown(cb)inindex.ts— fires on SIGTERM/SIGINT.getDeliveryAdapter()getter indelivery.ts— for live-flow adapter access.Core shrinks
delivery.ts911 → 665 linesindex.ts405 → 224 linesdispatchResponsenow logs "Unclaimed response" instead of falling through to an inline handler.Migration renames
003-pending-approvals.ts→module-approvals-pending-approvals.ts007-pending-approvals-title-options.ts→module-approvals-title-options.tsMigration.namefields unchanged so existing DBs treat them as already-applied.Test plan
pnpm run buildcleansrc/modules/index.ts→ build clean, 137/137 tests still pass. In that state: delivery actions log "Unknown system action", button clicks log "Unclaimed response".install_packagesflow end-to-end (admin card → approve → image rebuild → container restart → follow-up prompt).ask_user_questionflow end-to-end (card delivered → click → response returned to agent).🤖 Generated with Claude Code