From b003cdc52f8fa4c46a6ec121b9526c44a28af484 Mon Sep 17 00:00:00 2001 From: Stan Lewis Date: Wed, 23 Sep 2026 13:51:31 -0400 Subject: [PATCH 01/11] feat(plugin-dev): add watch mode, readiness polling, and phased start UX (RHIDP-16673) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit plugin dev update --watch ---------------------- - Add chokidar file watcher on src/ and package.json with 500ms debounce - Extract runUpdateCycle() so one-shot and watch share the same implementation - Serialize cycles: a change arriving during an active cycle queues exactly one follow-up rather than running concurrently - Add waitForContainerEvent(tool, service, action, timeoutMs): subscribe to container runtime event stream via `podman|docker events --stream`, resolve when the target service emits the target action. Shared primitive used by both cleanup settling and start phase progress. - Add waitForContainerCleanup(tool, timeoutMs): waits for both rhdh and install-dynamic-plugins to emit their terminal event before the next back-to-back cycle starts, preventing the crun exec.fifo race: Podman: cleanup (after crun tears down exec.fifo) Docker: die (terminal event; no equivalent cleanup step) - Print '[watch] Refresh your browser at ' when RHDH is ready - Fix isIgnoredWatchPath: chokidar v4+ dropped glob-string support for `ignored` (function/regex/literal path only). The previous glob array (e.g. double-star dist/node_modules patterns) silently matched nothing, verified by reproduction against the vendored chokidar. Replaced with a path-segment predicate that actually excludes output directories. plugin dev start — phased UX and --watch ----------------------------------------- - Show four labeled phases: build/export, start runtime, install plugins, wait for readiness - [3/4] Installing dynamic plugins uses waitForContainerEvent to watch for the installer container's died event rather than blocking on a compose call - waitForInstallerToFinish checks the installer's current Compose state before subscribing to the events stream: compose up -d won't recreate an already-exited one-shot container, so a re-entrant `start` against an already-running runtime previously stalled for the full 60s waitForContainerEvent timeout waiting on a `died` event that already fired in a prior invocation. Skips straight through when already exited. - [4/4] polls HTTP until RHDH responds, then prints the URL - Add --watch to `start`: after readiness, fall straight into watchUpdate() so a user can go from a cold start into continuous watch mode in one command. --configure stays independent — no implicit behavior change. plugin dev update / restart — runtime pre-flight checks --------------------------------------------------------- - Add ensureRuntimeRunning(tool, runtimeDir): inspects Compose ps output for the rhdh service and fails fast with an actionable message ("RHDH Local is not running. Run `rhdh-cli plugin dev start` first.") instead of a raw compose/container subprocess error. Applied to both runUpdateCycle (covers update and update --watch) and restart, the only two subcommands with a real "runtime must already exist" precondition — confirmed via a full precondition/action/postcondition pass over all six plugin dev subcommands (start, update, restart, stop, logs, status). - Refactored formatRuntimeStatus's inline service-lookup closures into shared findComposeService/composeServiceState/composeServiceExitCode helpers, reused by ensureRuntimeRunning, waitForInstallerToFinish, and formatRuntimeStatus itself. - Fixed ensureGeneratedConfigIncluded's error message: it told users to "rerun with --configure", but that flag only exists on `start`, not on `update` — a dead end for a user hitting this on `update --watch`. Now points at `rhdh-cli plugin dev start --configure` directly. Shared helpers -------------- - resolveRhdhUrl(runtimeDir): reads BASE_URL from default.env then .env (optional override), falls back to http://localhost:7007 - waitForRhdhReady(url, timeoutMs, pollIntervalMs): fetch-polls with dots, resolves with the URL, throws with an actionable message on timeout Testing ------- - waitForContainerEvent: resolves on match, ignores wrong service, times out, passes correct filter to spawn - waitForContainerCleanup: podman uses cleanup, docker uses die - resolveRhdhUrl: fallback, default.env, .env override, commented lines - waitForRhdhReady: resolves on 200, retries on ECONNREFUSED, throws on timeout - watchUpdate: cycle fires, debounce coalesces, recovery after failure, SIGINT, fails cleanly and keeps watching when RHDH Local is not running - isIgnoredWatchPath: excludes output/dependency directories, does not exclude ordinary watched paths (regression test for the chokidar v4 glob behavior change — exercises the real predicate, not a mocked watcher) - start: enters watch mode on --watch, does not enter it otherwise, skips the installer event wait when it already exited (re-entrant start) - update / restart: reject with an actionable message when RHDH Local is not running or stopped; restart succeeds and issues both compose calls when running Signed-off-by: Stan Lewis Assisted-by: claude-sonnet-4-6@default rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED --- CHANGELOG.md | 4 + package.json | 1 + src/commands/dev/command.test.ts | 768 ++++++++++++++++++++++++++++++- src/commands/dev/command.ts | 477 ++++++++++++++++++- src/commands/dev/index.ts | 14 +- src/commands/index.ts | 8 + yarn.lock | 17 + 7 files changed, 1258 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0cd7baf..2b1f3ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] +### Added + +- **`plugin dev`:** Add `--watch` to `rhdh-cli plugin dev start`, and a standalone `rhdh-cli plugin dev update --watch`, for continuous re-export/re-stage/restart on source changes ([RHIDP-16673](https://redhat.atlassian.net/browse/RHIDP-16673)). Watches `src/` and `package.json` with a 500ms debounce and serializes cycles so a change arriving mid-cycle queues exactly one follow-up; prints a refresh URL once RHDH responds. `plugin dev start` now also shows phased `[1/4]`–`[4/4]` progress through build/export, runtime start, plugin install, and readiness polling. `plugin dev update` and `plugin dev restart` fail fast with an actionable message when RHDH Local isn't running yet, instead of surfacing a raw compose/container error. + ## 2.1.1 - 2026-09-21 ### Added diff --git a/package.json b/package.json index 769f2a2..c4038ec 100644 --- a/package.json +++ b/package.json @@ -56,6 +56,7 @@ "@yarnpkg/lockfile": "^1.1.0", "@yarnpkg/parsers": "^3.0.0-rc.4", "chalk": "^4.0.0", + "chokidar": "^5.0.0", "codeowners": "^5.1.1", "commander": "^9.1.0", "eslint": "8.57.1", diff --git a/src/commands/dev/command.test.ts b/src/commands/dev/command.test.ts index 75d4a72..dd33ea6 100644 --- a/src/commands/dev/command.test.ts +++ b/src/commands/dev/command.test.ts @@ -1,6 +1,8 @@ import fs from 'fs-extra'; import os from 'node:os'; import path from 'node:path'; +import EventEmitter from 'node:events'; +import chokidar from 'chokidar'; jest.mock('../../lib/paths', () => ({ paths: { @@ -40,6 +42,40 @@ jest.mock('../export-dynamic-plugin', () => ({ command: jest.fn(), })); +// --------------------------------------------------------------------------- +// Chokidar mock used by watchUpdate tests +// --------------------------------------------------------------------------- +class FakeWatcher extends EventEmitter { + close = jest.fn().mockResolvedValue(undefined); +} +let fakeWatcher: FakeWatcher; + +jest.mock('chokidar', () => ({ + __esModule: true, + default: { + watch: jest.fn(() => { + fakeWatcher = new FakeWatcher(); + return fakeWatcher; + }), + }, +})); + +// --------------------------------------------------------------------------- +// node:child_process spawn mock used by waitForContainerCleanup tests +// --------------------------------------------------------------------------- +class FakeChildProcess extends EventEmitter { + stdout = new EventEmitter(); + kill = jest.fn(); +} +let fakeChild: FakeChildProcess; + +jest.mock('node:child_process', () => ({ + spawn: jest.fn(() => { + fakeChild = new FakeChildProcess(); + return fakeChild; + }), +})); + import { composeArgs, composeStatusArgs, @@ -55,6 +91,15 @@ import { updateGeneratedConfig, stop, status, + start, + update, + restart, + watchUpdate, + waitForContainerEvent, + waitForContainerCleanup, + resolveRhdhUrl, + waitForRhdhReady, + isIgnoredWatchPath, } from './command'; import { Task } from '../../lib/tasks'; import { run, execFile } from '../../lib/run'; @@ -233,16 +278,19 @@ describe('plugin dev', () => { }); it('rejects a container tool that is not on PATH', async () => { - const taskMock = Task as jest.Mocked; - taskMock.forCommand.mockRejectedValueOnce(new Error('command not found')); + const execFileMock = execFile as jest.MockedFunction; + execFileMock.mockRejectedValueOnce(new Error('command not found')); await expect(validateContainerTool('docker')).rejects.toThrow( 'Unable to find docker on PATH', ); }); it('accepts a container tool that is on PATH', async () => { - const taskMock = Task as jest.Mocked; - taskMock.forCommand.mockResolvedValueOnce(undefined); + const execFileMock = execFile as jest.MockedFunction; + execFileMock.mockResolvedValueOnce({ + stdout: 'podman version 5.0.0', + stderr: '', + }); await expect(validateContainerTool('podman')).resolves.toBe('podman'); }); @@ -284,6 +332,12 @@ describe('plugin dev', () => { } mockRun.mockReset(); mockExecFile.mockReset(); + // validateContainerTool now uses execFile for the silent version check. + // Default to resolving so subcommand tests don't need to set it up themselves. + mockExecFile.mockResolvedValue({ + stdout: 'podman version 5.0.0', + stderr: '', + }); mockTask.forCommand.mockResolvedValue(undefined); mockTask.log.mockReset(); }); @@ -302,11 +356,69 @@ describe('plugin dev', () => { it('status rejects when the compose child process fails', async () => { const err = new ExitCodeError(1, 'podman compose ps'); - mockExecFile.mockRejectedValueOnce(err); + // First call: version check (passes); second call: compose ps (fails). + mockExecFile + .mockResolvedValueOnce({ stdout: 'podman version 5.0.0', stderr: '' }) + .mockRejectedValueOnce(err); await expect( status({ rhdhLocalDir: runtimeDir, containerTool: 'podman' }), ).rejects.toThrow(ExitCodeError); }); + + it('update rejects with an actionable message when RHDH Local is not running', async () => { + // First call: version check (passes); second call: compose ps reports + // no services at all (RHDH Local was never started). + mockExecFile + .mockResolvedValueOnce({ stdout: 'podman version 5.0.0', stderr: '' }) + .mockResolvedValueOnce({ stdout: '[]', stderr: '' }); + await expect( + update({ rhdhLocalDir: runtimeDir, containerTool: 'podman' }), + ).rejects.toThrow('RHDH Local is not running'); + }); + + it('update rejects when RHDH Local is stopped (not just absent)', async () => { + mockExecFile + .mockResolvedValueOnce({ stdout: 'podman version 5.0.0', stderr: '' }) + .mockResolvedValueOnce({ + stdout: JSON.stringify([ + { Service: 'rhdh', State: 'exited', ExitCode: 0 }, + ]), + stderr: '', + }); + await expect( + update({ rhdhLocalDir: runtimeDir, containerTool: 'podman' }), + ).rejects.toThrow('RHDH Local is not running'); + }); + + it('restart rejects with an actionable message when RHDH Local is not running', async () => { + mockExecFile + .mockResolvedValueOnce({ stdout: 'podman version 5.0.0', stderr: '' }) + .mockResolvedValueOnce({ stdout: '[]', stderr: '' }); + await expect( + restart({ rhdhLocalDir: runtimeDir, containerTool: 'podman' }), + ).rejects.toThrow('RHDH Local is not running'); + expect(mockRun).not.toHaveBeenCalled(); + }); + + it('restart stops and starts rhdh when RHDH Local is running', async () => { + // version check, then two compose-ps calls: the pre-flight check, then + // the final getRuntimeStatus call after stop-rhdh/start-rhdh. + mockExecFile + .mockResolvedValueOnce({ stdout: 'podman version 5.0.0', stderr: '' }) + .mockResolvedValueOnce({ + stdout: JSON.stringify([{ Service: 'rhdh', State: 'running' }]), + stderr: '', + }) + .mockResolvedValueOnce({ + stdout: JSON.stringify([{ Service: 'rhdh', State: 'running' }]), + stderr: '', + }); + mockRun.mockResolvedValue(undefined); + await expect( + restart({ rhdhLocalDir: runtimeDir, containerTool: 'podman' }), + ).resolves.toBeUndefined(); + expect(mockRun).toHaveBeenCalledTimes(2); + }); }); it('reports the RHDH Local files missing from an incompatible directory', async () => { @@ -333,7 +445,7 @@ describe('plugin dev', () => { ); await expect( ensureGeneratedConfigIncluded(directory, false), - ).rejects.toThrow('rerun with --configure'); + ).rejects.toThrow('plugin dev start --configure'); await ensureGeneratedConfigIncluded(directory, true); await expect(fs.readFile(override, 'utf8')).resolves.toContain( 'rhdh-cli.generated.local.yaml', @@ -516,3 +628,647 @@ describe('plugin dev', () => { } }); }); + +// --------------------------------------------------------------------------- +// isIgnoredWatchPath — chokidar v4+ dropped glob-string support for `ignored`; +// this is a regression test for that (a glob array previously matched nothing). +// --------------------------------------------------------------------------- + +describe('isIgnoredWatchPath', () => { + it('ignores paths under output/dependency directories', () => { + expect(isIgnoredWatchPath(path.join('plugin', 'dist', 'output.js'))).toBe( + true, + ); + expect( + isIgnoredWatchPath(path.join('plugin', 'dist-dynamic', 'package.json')), + ).toBe(true); + expect( + isIgnoredWatchPath(path.join('plugin', 'dist-types', 'index.d.ts')), + ).toBe(true); + expect( + isIgnoredWatchPath( + path.join('plugin', 'node_modules', 'pkg', 'index.js'), + ), + ).toBe(true); + }); + + it('does not ignore ordinary watched paths', () => { + expect(isIgnoredWatchPath(path.join('plugin', 'src', 'index.ts'))).toBe( + false, + ); + expect(isIgnoredWatchPath(path.join('plugin', 'package.json'))).toBe(false); + }); +}); + +// --------------------------------------------------------------------------- +// watchUpdate — file-watching behaviour +// --------------------------------------------------------------------------- + +describe('watchUpdate', () => { + let runtimeDir: string; + const mockRun = run as jest.MockedFunction; + const mockExecFile = execFile as jest.MockedFunction; + const mockTask = Task as jest.Mocked; + const mockExport = jest.requireMock('../export-dynamic-plugin') + .command as jest.MockedFunction<() => Promise>; + + let pluginDir: string; + + beforeEach(async () => { + runtimeDir = await fs.mkdtemp( + path.join(os.tmpdir(), 'plugin-dev-watch-runtime-'), + ); + pluginDir = await fs.mkdtemp( + path.join(os.tmpdir(), 'plugin-dev-watch-plugin-'), + ); + (global as any).__pluginDevTestDir = pluginDir; + + for (const f of [ + 'compose.yaml', + 'compose-dynamic-plugins-root.yaml', + 'prepare-and-install-dynamic-plugins.sh', + 'wait-for-plugins-and-start.sh', + ]) { + await fs.writeFile(path.join(runtimeDir, f), ''); + } + + // Write the generated config include so ensureGeneratedConfigIncluded passes + const override = path.join( + runtimeDir, + 'configs/dynamic-plugins/dynamic-plugins.override.yaml', + ); + await fs.outputFile( + override, + `includes:\n - configs/dynamic-plugins/rhdh-cli.generated.local.yaml\n`, + ); + + // Write a minimal frontend plugin package.json so validateProjectFiles passes + // (frontend plugins do not require dist-types). + await fs.writeJson(path.join(pluginDir, 'package.json'), { + name: '@internal/my-watch-plugin', + version: '0.1.0', + backstage: { role: 'frontend-plugin' }, + }); + + mockRun.mockReset(); + mockExecFile.mockReset(); + mockTask.forCommand.mockResolvedValue(undefined); + mockTask.log.mockReset(); + mockExport.mockReset(); + + // Default: export + compose succeed; compose ps reports rhdh running so + // ensureRuntimeRunning's pre-flight check passes. + mockExport.mockResolvedValue(undefined); + mockRun.mockResolvedValue(undefined); + mockExecFile.mockResolvedValue({ + stdout: JSON.stringify([{ Service: 'rhdh', State: 'running' }]), + stderr: '', + }); + }); + + afterEach(async () => { + delete (global as any).__pluginDevTestDir; + // Remove signal listeners added by watchUpdate to avoid accumulation + process.removeAllListeners('SIGINT'); + process.removeAllListeners('SIGTERM'); + await fs.remove(runtimeDir); + await fs.remove(pluginDir); + }); + + /** + * Wait until mockExport has been called at least `count` times by polling + * with setImmediate-based yields. We pass debounceMs=0 to watchUpdate so the + * setTimeout fires on the very next event loop tick; real timers mean fs-extra + * and other async operations resolve normally. + */ + async function waitForExportCalls(count: number, timeoutMs = 5000) { + const deadline = Date.now() + timeoutMs; + while (mockExport.mock.calls.length < count) { + if (Date.now() > deadline) { + throw new Error( + `Timed out waiting for mockExport to be called ${count} time(s) ` + + `(called ${mockExport.mock.calls.length} time(s))`, + ); + } + await new Promise(resolve => setImmediate(resolve)); + } + } + + it('runs an update cycle when a file change event fires', async () => { + // debounceMs=0 so the timer fires on the next event-loop tick. + const watchPromise = watchUpdate(runtimeDir, 'podman', 0, 0); + + fakeWatcher.emit('all', 'change', 'src/index.ts'); + await waitForExportCalls(1); + + expect(mockExport).toHaveBeenCalledTimes(1); + + fakeWatcher.emit('error', new Error('done')); + await expect(watchPromise).rejects.toThrow('done'); + }); + + it('debounces rapid consecutive file events into a single cycle', async () => { + // Use debounceMs=20 so rapid events within that window coalesce, but the + // cycle still completes quickly in real-timer mode. + const watchPromise = watchUpdate(runtimeDir, 'podman', 20, 0); + + // Three events fired rapidly — only the first one should schedule a timer + // (the guard `if (debounceTimer !== undefined) return` drops the rest). + fakeWatcher.emit('all', 'change', 'src/a.ts'); + fakeWatcher.emit('all', 'change', 'src/b.ts'); + fakeWatcher.emit('all', 'change', 'src/c.ts'); + await waitForExportCalls(1); + + expect(mockExport).toHaveBeenCalledTimes(1); + + fakeWatcher.emit('error', new Error('done')); + await expect(watchPromise).rejects.toThrow('done'); + }); + + it('continues watching after a failed update cycle', async () => { + const watchPromise = watchUpdate(runtimeDir, 'podman', 0, 0); + + mockExport.mockRejectedValueOnce(new Error('build exploded')); + + fakeWatcher.emit('all', 'change', 'src/fail.ts'); + await waitForExportCalls(1); + + expect(mockExport).toHaveBeenCalledTimes(1); + expect(mockTask.log).toHaveBeenCalledWith( + expect.stringContaining('build exploded'), + ); + + // A second event after the failed cycle should still trigger a new cycle. + mockExport.mockResolvedValueOnce(undefined); + fakeWatcher.emit('all', 'change', 'src/fixed.ts'); + await waitForExportCalls(2); + + expect(mockExport).toHaveBeenCalledTimes(2); + + fakeWatcher.emit('error', new Error('done')); + await expect(watchPromise).rejects.toThrow('done'); + }); + + async function waitForTaskLogContaining(substring: string, timeoutMs = 5000) { + const deadline = Date.now() + timeoutMs; + while ( + !mockTask.log.mock.calls.some(call => String(call[0]).includes(substring)) + ) { + if (Date.now() > deadline) { + throw new Error( + `Timed out waiting for a Task.log call containing "${substring}"`, + ); + } + await new Promise(resolve => setImmediate(resolve)); + } + } + + it('fails a cycle cleanly and keeps watching when RHDH Local is not running', async () => { + const watchPromise = watchUpdate(runtimeDir, 'podman', 0, 0); + + // Simulate RHDH Local not running for the first triggered cycle — + // ensureRuntimeRunning should reject before exportCommand is ever called. + mockExecFile.mockResolvedValueOnce({ stdout: '[]', stderr: '' }); + + fakeWatcher.emit('all', 'change', 'src/index.ts'); + await waitForTaskLogContaining('RHDH Local is not running'); + expect(mockExport).not.toHaveBeenCalled(); + + // A subsequent change, with RHDH running again (the default mock), should + // succeed and the watcher should still be alive to pick it up. + fakeWatcher.emit('all', 'change', 'src/fixed.ts'); + await waitForExportCalls(1); + expect(mockExport).toHaveBeenCalledTimes(1); + + fakeWatcher.emit('error', new Error('done')); + await expect(watchPromise).rejects.toThrow('done'); + }); + + it('closes the watcher on SIGINT', async () => { + watchUpdate(runtimeDir, 'podman', 0, 0); + + const exitSpy = jest + .spyOn(process, 'exit') + .mockImplementation((() => {}) as () => never); + + process.emit('SIGINT'); + + // Allow the shutdown async chain to run. + await new Promise(resolve => setImmediate(resolve)); + await Promise.resolve(); + + expect(fakeWatcher.close).toHaveBeenCalled(); + expect(exitSpy).toHaveBeenCalledWith(0); + + exitSpy.mockRestore(); + }); +}); + +// --------------------------------------------------------------------------- +// start — --watch wiring +// --------------------------------------------------------------------------- + +describe('start', () => { + let runtimeDir: string; + let pluginDir: string; + const mockRun = run as jest.MockedFunction; + const mockExecFile = execFile as jest.MockedFunction; + const mockTask = Task as jest.Mocked; + const mockExport = jest.requireMock('../export-dynamic-plugin') + .command as jest.MockedFunction<() => Promise>; + const mockFetch = jest.fn< + ReturnType, + Parameters + >(); + const mockChokidarWatch = chokidar.watch as jest.MockedFunction< + typeof chokidar.watch + >; + + beforeEach(async () => { + runtimeDir = await fs.mkdtemp( + path.join(os.tmpdir(), 'plugin-dev-start-runtime-'), + ); + pluginDir = await fs.mkdtemp( + path.join(os.tmpdir(), 'plugin-dev-start-plugin-'), + ); + (global as any).__pluginDevTestDir = pluginDir; + + for (const f of [ + 'compose.yaml', + 'compose-dynamic-plugins-root.yaml', + 'prepare-and-install-dynamic-plugins.sh', + 'wait-for-plugins-and-start.sh', + ]) { + await fs.writeFile(path.join(runtimeDir, f), ''); + } + + const override = path.join( + runtimeDir, + 'configs/dynamic-plugins/dynamic-plugins.override.yaml', + ); + await fs.outputFile( + override, + `includes:\n - configs/dynamic-plugins/rhdh-cli.generated.local.yaml\n`, + ); + + await fs.writeJson(path.join(pluginDir, 'package.json'), { + name: '@internal/my-start-plugin', + version: '0.1.0', + backstage: { role: 'frontend-plugin' }, + }); + // stagePlugin requires dist-dynamic to already exist (export is mocked). + await fs.ensureDir(path.join(pluginDir, 'dist-dynamic')); + await fs.writeJson(path.join(pluginDir, 'dist-dynamic', 'package.json'), { + name: '@internal/my-start-plugin', + version: '0.1.0', + }); + + mockRun.mockReset(); + mockExecFile.mockReset(); + mockTask.forCommand.mockResolvedValue(undefined); + mockTask.log.mockReset(); + mockExport.mockReset(); + mockChokidarWatch.mockClear(); + (jest.requireMock('node:child_process').spawn as jest.Mock).mockClear(); + + mockExport.mockResolvedValue(undefined); + mockRun.mockResolvedValue(undefined); + mockExecFile.mockResolvedValue({ + stdout: 'podman version 5.0.0', + stderr: '', + }); + + mockFetch.mockReset(); + (global as any).fetch = mockFetch; + mockFetch.mockResolvedValue( + new Response(null, { status: 200 }) as Response, + ); + jest.spyOn(process.stdout, 'write').mockImplementation(() => true); + }); + + afterEach(async () => { + delete (global as any).__pluginDevTestDir; + (process.stdout.write as jest.Mock).mockRestore(); + (global as any).fetch = undefined; + process.removeAllListeners('SIGINT'); + process.removeAllListeners('SIGTERM'); + await fs.remove(runtimeDir); + await fs.remove(pluginDir); + }); + + const { spawn: spawnMock } = jest.requireMock('node:child_process') as { + spawn: jest.Mock; + }; + + function emitInstallerDied() { + const event = JSON.stringify({ + Action: 'died', + Attributes: { 'com.docker.compose.service': 'install-dynamic-plugins' }, + }); + fakeChild.stdout.emit('data', Buffer.from(`${event}\n`)); + } + + async function waitForCondition( + check: () => boolean, + description: string, + timeoutMs = 5000, + ) { + const deadline = Date.now() + timeoutMs; + while (!check()) { + if (Date.now() > deadline) { + throw new Error(`Timed out waiting for: ${description}`); + } + await new Promise(resolve => setImmediate(resolve)); + } + } + + it('enters watch mode after a successful start when --watch is passed', async () => { + const startPromise = start({ + rhdhLocalDir: runtimeDir, + containerTool: 'podman', + watch: true, + }); + + // Phases 1-2 (export/stage/compose-start) involve real fs I/O, so poll + // rather than assume a fixed number of ticks — wait until phase 3 + // actually spawns the container-events subscription, then satisfy it. + await waitForCondition( + () => spawnMock.mock.calls.length > 0, + 'waitForContainerEvent to spawn the events subscription', + ); + emitInstallerDied(); + + // Phase 4's readiness poll resolves on the first mocked fetch; once + // start() proceeds into watchUpdate, chokidar.watch is called synchronously. + await waitForCondition( + () => mockChokidarWatch.mock.calls.length > 0, + 'watchUpdate to call chokidar.watch', + ); + + expect(mockTask.log).toHaveBeenCalledWith( + expect.stringContaining('RHDH is ready at'), + ); + + fakeWatcher.emit('error', new Error('done')); + await expect(startPromise).rejects.toThrow('done'); + }); + + it('does not enter watch mode when --watch is not passed', async () => { + const startPromise = start({ + rhdhLocalDir: runtimeDir, + containerTool: 'podman', + }); + + await waitForCondition( + () => spawnMock.mock.calls.length > 0, + 'waitForContainerEvent to spawn the events subscription', + ); + emitInstallerDied(); + + await expect(startPromise).resolves.toBeUndefined(); + expect(mockChokidarWatch).not.toHaveBeenCalled(); + }); + + it('skips waiting for the installer event when it already exited (re-entrant start)', async () => { + // version check, then compose ps reporting the installer already exited + // from a previous `start` — compose up -d won't restart it, so its + // `died` event will never fire again for a fresh event-stream subscriber. + mockExecFile + .mockResolvedValueOnce({ stdout: 'podman version 5.0.0', stderr: '' }) + .mockResolvedValueOnce({ + stdout: JSON.stringify([ + { Service: 'install-dynamic-plugins', State: 'exited', ExitCode: 0 }, + ]), + stderr: '', + }); + + const startPromise = start({ + rhdhLocalDir: runtimeDir, + containerTool: 'podman', + }); + + await expect(startPromise).resolves.toBeUndefined(); + // Never needed to subscribe to the events stream at all. + expect(spawnMock).not.toHaveBeenCalled(); + }); +}); + +// --------------------------------------------------------------------------- +// waitForContainerEvent — single-service event subscription +// --------------------------------------------------------------------------- + +describe('waitForContainerEvent', () => { + function emitEvent(svc: string, action: string) { + const event = JSON.stringify({ + Action: action, + Attributes: { 'com.docker.compose.service': svc }, + }); + fakeChild.stdout.emit('data', Buffer.from(`${event}\n`)); + } + + it('resolves when the target service emits the target action', async () => { + const p = waitForContainerEvent('podman', 'rhdh', 'died', 5000); + await new Promise(resolve => setImmediate(resolve)); + emitEvent('rhdh', 'died'); + await new Promise(resolve => setImmediate(resolve)); + await expect(p).resolves.toBeUndefined(); + expect(fakeChild.kill).toHaveBeenCalled(); + }); + + it('ignores events for other services', async () => { + const p = waitForContainerEvent('podman', 'rhdh', 'died', 5000); + await new Promise(resolve => setImmediate(resolve)); + // wrong service — should not resolve + emitEvent('install-dynamic-plugins', 'died'); + await new Promise(resolve => setImmediate(resolve)); + // correct service + emitEvent('rhdh', 'died'); + await new Promise(resolve => setImmediate(resolve)); + await expect(p).resolves.toBeUndefined(); + }); + + it('resolves on timeout when the event does not arrive', async () => { + jest.useFakeTimers(); + const p = waitForContainerEvent('podman', 'rhdh', 'died', 1000); + await Promise.resolve(); + jest.advanceTimersByTime(1100); + for (let i = 0; i < 5; i++) await Promise.resolve(); + await expect(p).resolves.toBeUndefined(); + expect(fakeChild.kill).toHaveBeenCalled(); + jest.useRealTimers(); + }); + + it('passes the correct event filter to spawn', async () => { + const { spawn: spawnMock } = jest.requireMock('node:child_process') as { + spawn: jest.Mock; + }; + spawnMock.mockClear(); + + waitForContainerEvent('docker', 'install-dynamic-plugins', 'die', 5000); + await new Promise(resolve => setImmediate(resolve)); + + expect(spawnMock).toHaveBeenCalledWith( + 'docker', + expect.arrayContaining([ + 'event=die', + 'label=com.docker.compose.service=install-dynamic-plugins', + ]), + expect.anything(), + ); + }); +}); + +// --------------------------------------------------------------------------- +// waitForContainerCleanup — dual-service settle (delegates to waitForContainerEvent) +// --------------------------------------------------------------------------- + +describe('waitForContainerCleanup', () => { + it('uses cleanup for podman and die for docker', async () => { + const { spawn: spawnMock } = jest.requireMock('node:child_process') as { + spawn: jest.Mock; + }; + spawnMock.mockClear(); + + // waitForContainerCleanup spawns two parallel waitForContainerEvent calls. + // Capture each FakeChildProcess instance as spawn is called. + const children: FakeChildProcess[] = []; + spawnMock.mockImplementation(() => { + const child = new FakeChildProcess(); + children.push(child); + return child; + }); + + const p = waitForContainerCleanup('podman', 5000); + // Allow both spawns to register their stdout listeners. + await new Promise(resolve => setImmediate(resolve)); + await new Promise(resolve => setImmediate(resolve)); + + const calls = spawnMock.mock.calls as Array<[string, string[]]>; + expect(calls.every(([tool]) => tool === 'podman')).toBe(true); + expect(calls.some(([, args]) => args.includes('event=cleanup'))).toBe(true); + expect(calls.some(([, args]) => args.includes('event=die'))).toBe(false); + + // Emit the cleanup event to each respective child. + function emitTo(child: FakeChildProcess, svc: string, action = 'cleanup') { + const event = JSON.stringify({ + Action: action, + Attributes: { 'com.docker.compose.service': svc }, + }); + child.stdout.emit('data', Buffer.from(`${event}\n`)); + } + // children[0] watches rhdh, children[1] watches install-dynamic-plugins + // (or vice-versa depending on Promise.all order — emit to both). + for (const child of children) { + emitTo(child, 'rhdh'); + emitTo(child, 'install-dynamic-plugins'); + } + await new Promise(resolve => setImmediate(resolve)); + await expect(p).resolves.toBeUndefined(); + + // Restore the default mock implementation for subsequent tests. + spawnMock.mockImplementation(() => { + fakeChild = new FakeChildProcess(); + return fakeChild; + }); + }); +}); + +// --------------------------------------------------------------------------- +// resolveRhdhUrl — env file parsing +// --------------------------------------------------------------------------- + +describe('resolveRhdhUrl', () => { + let dir: string; + + beforeEach(async () => { + dir = await fs.mkdtemp(path.join(os.tmpdir(), 'plugin-dev-url-')); + }); + afterEach(() => fs.remove(dir)); + + it('returns the fallback URL when no env files exist', async () => { + await expect(resolveRhdhUrl(dir)).resolves.toBe('http://localhost:7007'); + }); + + it('reads BASE_URL from default.env', async () => { + await fs.writeFile( + path.join(dir, 'default.env'), + 'BASE_URL=http://localhost:9999\n', + ); + await expect(resolveRhdhUrl(dir)).resolves.toBe('http://localhost:9999'); + }); + + it('.env overrides default.env', async () => { + await fs.writeFile( + path.join(dir, 'default.env'), + 'BASE_URL=http://localhost:9999\n', + ); + await fs.writeFile( + path.join(dir, '.env'), + '# comment\nBASE_URL=http://my-host:7007\n', + ); + await expect(resolveRhdhUrl(dir)).resolves.toBe('http://my-host:7007'); + }); + + it('ignores commented-out BASE_URL lines', async () => { + await fs.writeFile( + path.join(dir, 'default.env'), + 'BASE_URL=http://localhost:7007\n', + ); + await fs.writeFile( + path.join(dir, '.env'), + '# BASE_URL=http://other:1234\n', + ); + await expect(resolveRhdhUrl(dir)).resolves.toBe('http://localhost:7007'); + }); +}); + +// --------------------------------------------------------------------------- +// waitForRhdhReady — HTTP readiness polling +// --------------------------------------------------------------------------- + +describe('waitForRhdhReady', () => { + const mockFetch = jest.fn< + ReturnType, + Parameters + >(); + + beforeEach(() => { + mockFetch.mockReset(); + // Replace global fetch with the mock for the duration of each test. + (global as any).fetch = mockFetch; + // Suppress the dot output during tests. + jest.spyOn(process.stdout, 'write').mockImplementation(() => true); + }); + + afterEach(() => { + (process.stdout.write as jest.Mock).mockRestore(); + (global as any).fetch = undefined; + }); + + it('resolves with the URL when the first poll returns 200', async () => { + mockFetch.mockResolvedValueOnce( + new Response(null, { status: 200 }) as Response, + ); + await expect( + waitForRhdhReady('http://localhost:7007', 5000, 0), + ).resolves.toBe('http://localhost:7007'); + expect(mockFetch).toHaveBeenCalledTimes(1); + }); + + it('retries on connection failure and resolves when it succeeds', async () => { + mockFetch + .mockRejectedValueOnce(new Error('ECONNREFUSED')) + .mockResolvedValueOnce(new Response(null, { status: 200 }) as Response); + await expect( + waitForRhdhReady('http://localhost:7007', 5000, 0), + ).resolves.toBe('http://localhost:7007'); + expect(mockFetch).toHaveBeenCalledTimes(2); + }); + + it('throws after timeout when never ready', async () => { + mockFetch.mockRejectedValue(new Error('ECONNREFUSED')); + await expect( + waitForRhdhReady('http://localhost:7007', 10, 0), + ).rejects.toThrow('did not become ready'); + }); +}); diff --git a/src/commands/dev/command.ts b/src/commands/dev/command.ts index 149d70b..e068ab7 100644 --- a/src/commands/dev/command.ts +++ b/src/commands/dev/command.ts @@ -18,12 +18,71 @@ import { OptionValues } from 'commander'; import fs from 'fs-extra'; import YAML from 'yaml'; import path from 'node:path'; +import { spawn } from 'node:child_process'; +import chokidar from 'chokidar'; import { execFile, run } from '../../lib/run'; import { Task } from '../../lib/tasks'; import { paths } from '../../lib/paths'; import { command as exportCommand } from '../export-dynamic-plugin'; +/** Default readiness poll timeout for `waitForRhdhReady`. */ +const READY_TIMEOUT_MS = 120_000; +/** Interval between readiness poll attempts. */ +const READY_POLL_INTERVAL_MS = 2_000; + +/** + * Read `BASE_URL` from the rhdh-local env files (`default.env`, then `.env`). + * Falls back to `http://localhost:7007` if neither file defines the key. + * The `.env` file is optional and may override `default.env` values. + */ +export async function resolveRhdhUrl(runtimeDir: string): Promise { + const fallback = 'http://localhost:7007'; + let url = fallback; + for (const name of ['default.env', '.env']) { + const file = path.join(runtimeDir, name); + if (!(await fs.pathExists(file))) continue; + const content = await fs.readFile(file, 'utf8'); + for (const line of content.split('\n')) { + const match = line.match(/^\s*BASE_URL\s*=\s*(.+?)\s*$/); + if (match) url = match[1].replace(/^["']|["']$/g, ''); + } + } + return url; +} + +/** + * Poll `url` until it returns HTTP 2xx, then return the URL. + * Prints a dot to stdout every `pollIntervalMs` while waiting. + * Throws if `timeoutMs` elapses without a successful response. + */ +export async function waitForRhdhReady( + url: string, + timeoutMs = READY_TIMEOUT_MS, + pollIntervalMs = READY_POLL_INTERVAL_MS, +): Promise { + const deadline = Date.now() + timeoutMs; + process.stdout.write(`Waiting for ${url} `); + while (Date.now() < deadline) { + try { + const res = await fetch(url, { signal: AbortSignal.timeout(5_000) }); + if (res.ok || (res.status >= 200 && res.status < 400)) { + process.stdout.write(' ready.\n'); + return url; + } + } catch { + // Connection refused, timeout, etc. — keep polling. + } + process.stdout.write('.'); + await new Promise(resolve => setTimeout(resolve, pollIntervalMs)); + } + process.stdout.write('\n'); + throw new Error( + `RHDH did not become ready at ${url} within ${timeoutMs / 1000}s. ` + + `Run \`rhdh-cli plugin dev logs --rhdh\` to check for startup errors.`, + ); +} + const requiredRuntimeFiles = [ 'compose.yaml', 'compose-dynamic-plugins-root.yaml', @@ -69,8 +128,9 @@ export async function validateContainerTool( ); } // Verify the tool is actually on PATH before attempting any compose operation. + // Use execFile silently — a version check produces no useful output for the user. try { - await Task.forCommand(`${containerTool} --version`); + await execFile(containerTool, ['--version'], { shell: false }); } catch { throw new Error( `Unable to find ${containerTool} on PATH. Make sure ${containerTool} is installed and available, or pass --container-tool docker if you use Docker instead.`, @@ -87,10 +147,10 @@ async function resolveAndValidate(opts: OptionValues) { return { runtimeDir, containerTool }; } -async function getRuntimeStatus( +async function getComposeServices( containerTool: string, runtimeDir: string, -): Promise { +): Promise { const { stdout } = await execFile( containerTool, composeStatusArgs(containerTool), @@ -99,7 +159,38 @@ async function getRuntimeStatus( shell: false, }, ); - return formatRuntimeStatus(parseComposeStatus(stdout)); + return parseComposeStatus(stdout); +} + +async function getRuntimeStatus( + containerTool: string, + runtimeDir: string, +): Promise { + return formatRuntimeStatus( + await getComposeServices(containerTool, runtimeDir), + ); +} + +/** + * Throws unless the `rhdh` Compose service is currently running. + * + * `update`'s round-trip (re-export, re-stage, `compose start rhdh`) assumes + * the runtime was already brought up by `start`. Without this check, running + * `update`/`update --watch` against a runtime that was never started (or was + * stopped in another terminal) fails deep inside a raw `compose`/`container` + * subprocess error instead of a clear, actionable message. + */ +async function ensureRuntimeRunning( + containerTool: string, + runtimeDir: string, +): Promise { + const services = await getComposeServices(containerTool, runtimeDir); + const rhdh = findComposeService(services, 'rhdh'); + if (!composeServiceState(rhdh).includes('running')) { + throw new Error( + 'RHDH Local is not running. Run `rhdh-cli plugin dev start` first.', + ); + } } async function compose( @@ -110,18 +201,71 @@ async function compose( await run(containerTool, args, { cwd: runtimeDir, shell: false }); } +/** + * Wait for the `install-dynamic-plugins` container to finish, without + * needlessly stalling on a re-entrant `start` call. + * + * `waitForContainerEvent` subscribes to the container events stream starting + * *now* — it only sees events emitted after the subscription begins. `compose + * up -d` is idempotent and won't recreate/restart an already-exited one-shot + * container, so on a re-entrant `start` against an already-`Running` runtime, + * the installer's `died` event already happened in a prior invocation and + * will never be seen again. Without this check, that stalls `start` for the + * full `waitForContainerEvent` timeout (60s) before its timeout-resolve path + * lets it proceed anyway. + * + * Checking the installer's current Compose state first avoids that stall: + * if it already reports `exited`, the install already finished (in this run + * or a previous one) and there's nothing left to wait for. + */ +async function waitForInstallerToFinish( + containerTool: string, + runtimeDir: string, +): Promise { + const services = await getComposeServices(containerTool, runtimeDir); + const installer = findComposeService( + services, + 'install-dynamic-plugins', + 'rhdh-plugins-installer', + ); + if (composeServiceState(installer).includes('exited')) { + return; + } + await waitForContainerEvent(containerTool, 'install-dynamic-plugins', 'died'); +} + export async function start(opts: OptionValues) { const { runtimeDir, containerTool } = await resolveAndValidate(opts); await validateProjectFiles(); await ensureGeneratedConfigIncluded(runtimeDir, opts.configure); + + Task.log('[1/4] Building and exporting plugin...'); await exportCommand({ build: true, install: true }); await stagePlugin(runtimeDir); + + Task.log('[2/4] Starting RHDH Local runtime...'); await compose(containerTool, runtimeDir, composeArgs('start')); - Task.log(await getRuntimeStatus(containerTool, runtimeDir)); + + Task.log('[3/4] Installing dynamic plugins...'); + await waitForInstallerToFinish(containerTool, runtimeDir); + + Task.log('[4/4] Waiting for RHDH to be ready...'); + const url = await resolveRhdhUrl(runtimeDir); + await waitForRhdhReady(url); + + Task.log(`\nRHDH is ready at ${url}`); + + if (opts.watch) { + await watchUpdate(runtimeDir, containerTool); + } } -export async function update(opts: OptionValues) { - const { runtimeDir, containerTool } = await resolveAndValidate(opts); +async function runUpdateCycle( + runtimeDir: string, + containerTool: string, + prefix = '', +): Promise { + await ensureRuntimeRunning(containerTool, runtimeDir); await validateProjectFiles(); // Fail fast if the generated config include is missing — without it, an // update re-stages the plugin but RHDH never loads it. Use configure: false @@ -132,7 +276,272 @@ export async function update(opts: OptionValues) { for (const action of ['install-dynamic-plugins', 'stop-rhdh', 'start-rhdh']) { await compose(containerTool, runtimeDir, composeArgs(action)); } - Task.log(await getRuntimeStatus(containerTool, runtimeDir)); + const url = await resolveRhdhUrl(runtimeDir); + await waitForRhdhReady(url); + Task.log(`${prefix}Refresh your browser at ${url}`); +} + +export async function update(opts: OptionValues) { + const { runtimeDir, containerTool } = await resolveAndValidate(opts); + if (opts.watch) { + await watchUpdate(runtimeDir, containerTool); + } else { + await runUpdateCycle(runtimeDir, containerTool); + } +} + +/** + * Subscribe to the container runtime event stream and resolve once a specific + * service emits a specific event action, or when `timeoutMs` elapses. + * + * Both Podman and Docker support `--format json` and `--filter label=`. + * Uses the `com.docker.compose.service` label set by Compose on all containers. + * + * @param containerTool - 'podman' or 'docker' + * @param service - Compose service name (e.g. 'install-dynamic-plugins') + * @param eventAction - Event action to wait for (e.g. 'died', 'cleanup', 'start') + * @param timeoutMs - Max wait in ms. 0 means no timeout (wait indefinitely). + */ +export async function waitForContainerEvent( + containerTool: string, + service: string, + eventAction: string, + timeoutMs = 60_000, +): Promise { + await new Promise(resolve => { + const child = spawn( + containerTool, + [ + 'events', + '--stream', + '--format', + 'json', + '--filter', + `event=${eventAction}`, + '--filter', + `label=com.docker.compose.service=${service}`, + ], + { stdio: ['ignore', 'pipe', 'ignore'] }, + ); + + const timer = + timeoutMs > 0 + ? setTimeout(() => { + child.kill(); + resolve(); + }, timeoutMs) + : undefined; + + let buf = ''; + child.stdout.on('data', (chunk: Buffer) => { + buf += chunk.toString(); + const lines = buf.split('\n'); + buf = lines.pop() ?? ''; + for (const line of lines) { + const trimmed = line.trim(); + if (!trimmed) continue; + try { + const event = JSON.parse(trimmed) as { + Action?: string; + Status?: string; + Attributes?: Record; + }; + const action = event.Action ?? event.Status ?? ''; + const svc = event.Attributes?.['com.docker.compose.service'] ?? ''; + if (action === eventAction && svc === service) { + if (timer !== undefined) clearTimeout(timer); + child.kill(); + resolve(); + } + } catch { + // non-JSON line — ignore + } + } + }); + + child.on('error', () => { + if (timer !== undefined) clearTimeout(timer); + resolve(); + }); + + child.on('close', () => { + if (timer !== undefined) clearTimeout(timer); + resolve(); + }); + }); +} + +/** + * Wait until both the `rhdh` and `install-dynamic-plugins` containers have + * emitted their terminal cleanup event, or until `timeoutMs` elapses. + * + * Podman: wait for `cleanup` (fires after `died`, once crun tears down exec.fifo). + * Docker: wait for `die` (terminal event; no equivalent cleanup step). + * + * Only used between back-to-back watch cycles to prevent the crun exec.fifo + * race condition. Single one-shot `plugin dev update` runs are unaffected. + */ +export async function waitForContainerCleanup( + containerTool: string, + timeoutMs: number, +): Promise { + // Podman emits `cleanup` after `died` once crun has torn down exec.fifo. + // Docker emits `die` as its terminal container event (no equivalent cleanup). + const settleEvent = containerTool === 'podman' ? 'cleanup' : 'die'; + + await Promise.all([ + waitForContainerEvent(containerTool, 'rhdh', settleEvent, timeoutMs), + waitForContainerEvent( + containerTool, + 'install-dynamic-plugins', + settleEvent, + timeoutMs, + ), + ]); +} + +const ignoredWatchSegments = new Set([ + 'node_modules', + 'dist', + 'dist-dynamic', + 'dist-types', +]); + +/** + * Predicate for chokidar's `ignored` option. + * + * Chokidar v4+ dropped glob-string support for `ignored` — it now only + * accepts a function, a regex, or a literal path. A glob-style array (e.g. + * double-star dist/node_modules patterns) is silently accepted but matches + * nothing, since chokidar no longer interprets `*` as a wildcard there. This + * checks path segments directly instead, so output directories are actually + * excluded rather than just documented as excluded. + */ +export function isIgnoredWatchPath(filePath: string): boolean { + return filePath + .split(path.sep) + .some(segment => ignoredWatchSegments.has(segment)); +} + +/** + * Watch source files and run a full update cycle on changes. + * + * Design: + * - Watches src/, package.json, tsconfig*.json, and *.config.* files. + * - Excludes output directories (node_modules, dist, dist-dynamic, + * dist-types) to prevent output-loop triggering. + * - Events are debounced: the first event in a `debounceMs` window triggers + * the cycle; subsequent events within the window are coalesced. + * - Cycles are serialized: if a cycle is already running when the debounce + * fires, the pending change is deferred until the active cycle finishes. + * - A failed cycle logs the error and continues watching; it does not exit. + * - SIGINT/SIGTERM cleanly close the watcher and exit. + * + * @param debounceMs - Milliseconds to wait after a change before triggering a + * cycle. Defaults to 500ms; callers may pass 0 for tests. + * @param settleTimeoutMs - Maximum milliseconds to wait for container cleanup + * events between back-to-back cycles. If the events don't arrive within this + * window the next cycle starts anyway. Defaults to 15000ms; callers may pass + * 0 to skip event-based settling entirely (used in tests). + */ +export async function watchUpdate( + runtimeDir: string, + containerTool: string, + debounceMs = 500, + settleTimeoutMs = 15_000, +): Promise { + const watchPaths = [ + paths.resolveTarget('src'), + paths.resolveTarget('package.json'), + ]; + + Task.log( + 'Watching for changes. Press Ctrl+C to stop.\n' + + ` Watching: src/, package.json\n` + + ` Ignored: node_modules/, dist/, dist-dynamic/, dist-types/`, + ); + + const watcher = chokidar.watch(watchPaths, { + ignored: isIgnoredWatchPath, + ignoreInitial: true, + persistent: true, + }); + + let running = false; + let pendingChange = false; + let debounceTimer: ReturnType | undefined; + + const drainCycles = async () => { + let keepGoing = true; + while (keepGoing) { + running = true; + pendingChange = false; + const cycleStart = Date.now(); + try { + Task.log(`\n[watch] Change detected — starting update cycle...`); + await runUpdateCycle(runtimeDir, containerTool, '[watch] '); + Task.log(`[watch] Update complete in ${Date.now() - cycleStart}ms.`); + } catch (err: unknown) { + const message = err instanceof Error ? err.message : String(err); + Task.log( + `[watch] Update failed after ${Date.now() - cycleStart}ms: ${message}`, + ); + Task.log('[watch] Watching for further changes...'); + } finally { + running = false; + } + keepGoing = pendingChange; + if (keepGoing) { + Task.log( + `[watch] Change received during cycle — waiting for runtime to settle...`, + ); + if (settleTimeoutMs > 0) { + await waitForContainerCleanup(containerTool, settleTimeoutMs); + } + } + } + }; + + const scheduleUpdate = () => { + if (debounceTimer !== undefined) return; // already scheduled + debounceTimer = setTimeout(async () => { + debounceTimer = undefined; + if (running) { + // A cycle is active — record the intent and let the cycle's finally + // block start another one when it finishes. + pendingChange = true; + return; + } + await drainCycles(); + }, debounceMs); + }; + + watcher.on('all', (_event, filePath) => { + Task.log(`[watch] ${filePath} changed`); + scheduleUpdate(); + }); + + watcher.on('error', (err: unknown) => { + const message = err instanceof Error ? err.message : String(err); + Task.log(`[watch] Watcher error: ${message}`); + }); + + const shutdown = async () => { + Task.log('\n[watch] Shutting down...'); + if (debounceTimer !== undefined) { + clearTimeout(debounceTimer); + } + await watcher.close(); + process.exit(0); + }; + + process.on('SIGINT', shutdown); + process.on('SIGTERM', shutdown); + + // Keep the process alive while the watcher is active. + await new Promise((_resolve, reject) => { + watcher.on('error', reject); + }); } export async function stop(opts: OptionValues) { @@ -148,6 +557,7 @@ export async function stop(opts: OptionValues) { export async function restart(opts: OptionValues) { const { runtimeDir, containerTool } = await resolveAndValidate(opts); + await ensureRuntimeRunning(containerTool, runtimeDir); await compose(containerTool, runtimeDir, composeArgs('stop-rhdh')); await compose(containerTool, runtimeDir, composeArgs('start-rhdh')); Task.log(await getRuntimeStatus(containerTool, runtimeDir)); @@ -299,7 +709,8 @@ export async function ensureGeneratedConfigIncluded( if (alreadyIncluded) return; if (!configure) { throw new Error( - `Add ${generatedConfig} to ${override}'s includes list, or rerun with --configure.`, + `Add ${generatedConfig} to ${override}'s includes list, or run ` + + '`rhdh-cli plugin dev start --configure`.', ); } if (includes !== undefined && !YAML.isSeq(includes)) { @@ -410,25 +821,43 @@ export function parseComposeStatus(output: string): ComposeService[] { }); } +function composeServiceNames(item: ComposeService): string[] { + return Array.isArray(item.Names) + ? item.Names + : (item.Names?.split(',') ?? []); +} + +function findComposeService( + services: ComposeService[], + ...serviceNames: string[] +): ComposeService | undefined { + return services.find( + item => + serviceNames.includes(item.Service ?? '') || + serviceNames.includes(item.Name ?? '') || + composeServiceNames(item).some(name => serviceNames.includes(name)), + ); +} + +function composeServiceState(item: ComposeService | undefined): string { + return (item?.State ?? item?.Status ?? '').toLowerCase(); +} + +function composeServiceExitCode( + item: ComposeService | undefined, +): string | undefined { + return item?.ExitCode === undefined ? undefined : String(item.ExitCode); +} + export function formatRuntimeStatus(services: ComposeService[]): string { - const names = (item: ComposeService) => - Array.isArray(item.Names) ? item.Names : (item.Names?.split(',') ?? []); - const service = (...serviceNames: string[]) => - services.find( - item => - serviceNames.includes(item.Service ?? '') || - serviceNames.includes(item.Name ?? '') || - names(item).some(name => serviceNames.includes(name)), - ); - const rhdh = service('rhdh'); - const installer = service( + const rhdh = findComposeService(services, 'rhdh'); + const installer = findComposeService( + services, 'install-dynamic-plugins', 'rhdh-plugins-installer', ); - const state = (item: ComposeService | undefined) => - (item?.State ?? item?.Status ?? '').toLowerCase(); - const exitCode = (item: ComposeService | undefined) => - item?.ExitCode === undefined ? undefined : String(item.ExitCode); + const state = composeServiceState; + const exitCode = composeServiceExitCode; if ( state(installer).includes('exited') && diff --git a/src/commands/dev/index.ts b/src/commands/dev/index.ts index 495e57b..08f2712 100644 --- a/src/commands/dev/index.ts +++ b/src/commands/dev/index.ts @@ -14,4 +14,16 @@ * limitations under the License. */ -export { start, update, restart, stop, logs, status } from './command'; +export { + start, + update, + watchUpdate, + waitForContainerEvent, + waitForContainerCleanup, + resolveRhdhUrl, + waitForRhdhReady, + restart, + stop, + logs, + status, +} from './command'; diff --git a/src/commands/index.ts b/src/commands/index.ts index 272446d..1cb64d2 100644 --- a/src/commands/index.ts +++ b/src/commands/index.ts @@ -227,12 +227,20 @@ export function registerPluginCommand(program: Command) { '--configure', 'Add the CLI-managed plugin configuration include to RHDH Local on first use', ) + .option( + '--watch', + 'After startup, watch source files and re-run the update cycle automatically on change', + ) .action(lazy(() => import('./dev').then(m => m.start))); devSharedOptions(dev.command('update')) .description( 'Re-export and re-stage the plugin, then restart the RHDH service', ) + .option( + '--watch', + 'Watch source files and re-run the update cycle automatically on change', + ) .action(lazy(() => import('./dev').then(m => m.update))); devSharedOptions(dev.command('stop')) diff --git a/yarn.lock b/yarn.lock index 4a1f447..91bdfcd 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3418,6 +3418,7 @@ __metadata: "@yarnpkg/parsers": "npm:^3.0.0-rc.4" axios: "npm:^1.9.0" chalk: "npm:^4.0.0" + chokidar: "npm:^5.0.0" codeowners: "npm:^5.1.1" commander: "npm:^9.1.0" eslint: "npm:8.57.1" @@ -6900,6 +6901,15 @@ __metadata: languageName: node linkType: hard +"chokidar@npm:^5.0.0": + version: 5.0.0 + resolution: "chokidar@npm:5.0.0" + dependencies: + readdirp: "npm:^5.0.0" + checksum: 10/a1c2a4ee6ee81ba6409712c295a47be055fb9de1186dfbab33c1e82f28619de962ba02fc5f9d433daaedc96c35747460d8b2079ac2907de2c95e3f7cce913113 + languageName: node + linkType: hard + "chownr@npm:^1.1.1": version: 1.1.4 resolution: "chownr@npm:1.1.4" @@ -15185,6 +15195,13 @@ __metadata: languageName: node linkType: hard +"readdirp@npm:^5.0.0": + version: 5.1.1 + resolution: "readdirp@npm:5.1.1" + checksum: 10/4a6a61272c6db332db361166b42be34ce8480542073061f04f405ed2f703e517e90dfafe33843f647e91d20090c575aee04a823e9ed9d9d9a01ec3603ca45a8b + languageName: node + linkType: hard + "readdirp@npm:~3.6.0": version: 3.6.0 resolution: "readdirp@npm:3.6.0" From 7792901d732cada69c0d7a7aba3098aea0d722b1 Mon Sep 17 00:00:00 2001 From: Stan Lewis Date: Fri, 25 Sep 2026 07:18:35 -0400 Subject: [PATCH 02/11] docs(changelog): add PR link for RHIDP-16673 watch mode entry Signed-off-by: Stan Lewis Assisted-By: opencode rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2b1f3ec..a25d0ff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Added -- **`plugin dev`:** Add `--watch` to `rhdh-cli plugin dev start`, and a standalone `rhdh-cli plugin dev update --watch`, for continuous re-export/re-stage/restart on source changes ([RHIDP-16673](https://redhat.atlassian.net/browse/RHIDP-16673)). Watches `src/` and `package.json` with a 500ms debounce and serializes cycles so a change arriving mid-cycle queues exactly one follow-up; prints a refresh URL once RHDH responds. `plugin dev start` now also shows phased `[1/4]`–`[4/4]` progress through build/export, runtime start, plugin install, and readiness polling. `plugin dev update` and `plugin dev restart` fail fast with an actionable message when RHDH Local isn't running yet, instead of surfacing a raw compose/container error. +- **`plugin dev`:** Add `--watch` to `rhdh-cli plugin dev start`, and a standalone `rhdh-cli plugin dev update --watch`, for continuous re-export/re-stage/restart on source changes ([RHIDP-16673](https://redhat.atlassian.net/browse/RHIDP-16673), [#222](https://github.com/redhat-developer/rhdh-cli/pull/222)). Watches `src/` and `package.json` with a 500ms debounce and serializes cycles so a change arriving mid-cycle queues exactly one follow-up; prints a refresh URL once RHDH responds. `plugin dev start` now also shows phased `[1/4]`–`[4/4]` progress through build/export, runtime start, plugin install, and readiness polling. `plugin dev update` and `plugin dev restart` fail fast with an actionable message when RHDH Local isn't running yet, instead of surfacing a raw compose/container error. ## 2.1.1 - 2026-09-21 From c41f8a3889aac02db74cfc1e20c525715ec6b0d1 Mon Sep 17 00:00:00 2001 From: Stan Lewis Date: Fri, 25 Sep 2026 07:25:32 -0400 Subject: [PATCH 03/11] fix(plugin-dev): address SonarCloud findings on watch mode - Simplify the BASE_URL regex in resolveRhdhUrl to remove a superlinear backtracking pattern (typescript:S8786): capture the rest of the line greedily and trim() afterward instead of combining a lazy capture with a trailing \s*$ anchor. - Add describeWatchError(err) to stringify caught unknown values in watchUpdate's cycle-failure and watcher-error handlers, instead of String(err), which falls back to Object.prototype.toString() ('[object Object]') for plain objects (typescript:S6551 x2). Signed-off-by: Stan Lewis Assisted-By: opencode rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED --- src/commands/dev/command.ts | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-) diff --git a/src/commands/dev/command.ts b/src/commands/dev/command.ts index e068ab7..2ebaa79 100644 --- a/src/commands/dev/command.ts +++ b/src/commands/dev/command.ts @@ -44,8 +44,9 @@ export async function resolveRhdhUrl(runtimeDir: string): Promise { if (!(await fs.pathExists(file))) continue; const content = await fs.readFile(file, 'utf8'); for (const line of content.split('\n')) { - const match = line.match(/^\s*BASE_URL\s*=\s*(.+?)\s*$/); - if (match) url = match[1].replace(/^["']|["']$/g, ''); + const match = line.match(/^\s*BASE_URL\s*=\s*(.*)$/); + const value = match?.[1].trim(); + if (value) url = value.replace(/^["']|["']$/g, ''); } } return url; @@ -423,6 +424,30 @@ export function isIgnoredWatchPath(filePath: string): boolean { .some(segment => ignoredWatchSegments.has(segment)); } +/** + * Render a caught `unknown` value as a human-readable string for log output. + * + * Avoids `String(value)` on non-primitive values, which falls back to + * `Object.prototype.toString()` (`[object Object]`) for plain objects that + * don't define their own `toString()`. + */ +function describeWatchError(err: unknown): string { + if (err instanceof Error) return err.message; + if (typeof err === 'string') return err; + if ( + typeof err === 'number' || + typeof err === 'boolean' || + typeof err === 'bigint' + ) { + return String(err); + } + try { + return JSON.stringify(err) ?? 'Unknown error'; + } catch { + return 'Unknown error'; + } +} + /** * Watch source files and run a full update cycle on changes. * @@ -482,7 +507,7 @@ export async function watchUpdate( await runUpdateCycle(runtimeDir, containerTool, '[watch] '); Task.log(`[watch] Update complete in ${Date.now() - cycleStart}ms.`); } catch (err: unknown) { - const message = err instanceof Error ? err.message : String(err); + const message = describeWatchError(err); Task.log( `[watch] Update failed after ${Date.now() - cycleStart}ms: ${message}`, ); @@ -522,7 +547,7 @@ export async function watchUpdate( }); watcher.on('error', (err: unknown) => { - const message = err instanceof Error ? err.message : String(err); + const message = describeWatchError(err); Task.log(`[watch] Watcher error: ${message}`); }); From 2b18afbb7ef12fc540970e5766049bc49f85d729 Mon Sep 17 00:00:00 2001 From: Stan Lewis Date: Fri, 25 Sep 2026 07:29:23 -0400 Subject: [PATCH 04/11] fix(plugin-dev): drop remaining regex to fully clear S8786 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The prior fix still combined two adjacent quantifiers over overlapping character classes (\s* then .*), which SonarCloud continued to flag as superlinear (typescript:S8786). Replace the whole BASE_URL match with a plain indexOf('=')/slice() split — no regex needed for the key/value parse, only the existing anchored quote-strip regex remains. Signed-off-by: Stan Lewis Assisted-By: opencode rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED --- src/commands/dev/command.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/commands/dev/command.ts b/src/commands/dev/command.ts index 2ebaa79..6f4a600 100644 --- a/src/commands/dev/command.ts +++ b/src/commands/dev/command.ts @@ -44,9 +44,13 @@ export async function resolveRhdhUrl(runtimeDir: string): Promise { if (!(await fs.pathExists(file))) continue; const content = await fs.readFile(file, 'utf8'); for (const line of content.split('\n')) { - const match = line.match(/^\s*BASE_URL\s*=\s*(.*)$/); - const value = match?.[1].trim(); - if (value) url = value.replace(/^["']|["']$/g, ''); + const eq = line.indexOf('='); + if (eq === -1 || line.slice(0, eq).trim() !== 'BASE_URL') continue; + const value = line + .slice(eq + 1) + .trim() + .replace(/^["']|["']$/g, ''); + if (value) url = value; } } return url; From a06a9fcdf446370569113b4b4b439add3077cc21 Mon Sep 17 00:00:00 2001 From: Stan Lewis Date: Fri, 25 Sep 2026 07:34:26 -0400 Subject: [PATCH 05/11] test(plugin-dev): dedupe runtime scaffolding to clear Sonar CPD gate The watchUpdate and start suites each carried a near-identical block that mkdtemp'd a runtime/plugin dir, wrote the RHDH Local marker files, and wrote a minimal frontend plugin package.json. Extract scaffoldPluginDevRuntime() and have both beforeEach hooks call it. Also collapse the repeated fs.writeFile(path.join(dir, name), content) calls in the resolveRhdhUrl suite into a small writeEnvFile() closure. Pulled new_duplicated_lines_density back under the 3% quality-gate threshold (was 7.7%, all in this file); no behavior change, same 49 tests still pass. Signed-off-by: Stan Lewis Assisted-By: opencode rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED --- src/commands/dev/command.test.ts | 149 +++++++++++++------------------ 1 file changed, 64 insertions(+), 85 deletions(-) diff --git a/src/commands/dev/command.test.ts b/src/commands/dev/command.test.ts index dd33ea6..91d3b02 100644 --- a/src/commands/dev/command.test.ts +++ b/src/commands/dev/command.test.ts @@ -664,6 +664,52 @@ describe('isIgnoredWatchPath', () => { // watchUpdate — file-watching behaviour // --------------------------------------------------------------------------- +/** + * Scaffold a temp RHDH Local runtime dir (with the files + * `validateProjectFiles`/Compose checks require) and a temp plugin dir with a + * minimal frontend plugin `package.json`. Shared by the `watchUpdate` and + * `start` suites, which both drive a real (mocked-at-the-edges) `plugin dev` + * cycle against these directories. + */ +async function scaffoldPluginDevRuntime( + runtimePrefix: string, + pluginPrefix: string, + pluginPackageName: string, +): Promise<{ runtimeDir: string; pluginDir: string }> { + const runtimeDir = await fs.mkdtemp(path.join(os.tmpdir(), runtimePrefix)); + const pluginDir = await fs.mkdtemp(path.join(os.tmpdir(), pluginPrefix)); + (global as any).__pluginDevTestDir = pluginDir; + + for (const f of [ + 'compose.yaml', + 'compose-dynamic-plugins-root.yaml', + 'prepare-and-install-dynamic-plugins.sh', + 'wait-for-plugins-and-start.sh', + ]) { + await fs.writeFile(path.join(runtimeDir, f), ''); + } + + // Write the generated config include so ensureGeneratedConfigIncluded passes + const override = path.join( + runtimeDir, + 'configs/dynamic-plugins/dynamic-plugins.override.yaml', + ); + await fs.outputFile( + override, + `includes:\n - configs/dynamic-plugins/rhdh-cli.generated.local.yaml\n`, + ); + + // Write a minimal frontend plugin package.json so validateProjectFiles passes + // (frontend plugins do not require dist-types). + await fs.writeJson(path.join(pluginDir, 'package.json'), { + name: pluginPackageName, + version: '0.1.0', + backstage: { role: 'frontend-plugin' }, + }); + + return { runtimeDir, pluginDir }; +} + describe('watchUpdate', () => { let runtimeDir: string; const mockRun = run as jest.MockedFunction; @@ -675,40 +721,11 @@ describe('watchUpdate', () => { let pluginDir: string; beforeEach(async () => { - runtimeDir = await fs.mkdtemp( - path.join(os.tmpdir(), 'plugin-dev-watch-runtime-'), - ); - pluginDir = await fs.mkdtemp( - path.join(os.tmpdir(), 'plugin-dev-watch-plugin-'), - ); - (global as any).__pluginDevTestDir = pluginDir; - - for (const f of [ - 'compose.yaml', - 'compose-dynamic-plugins-root.yaml', - 'prepare-and-install-dynamic-plugins.sh', - 'wait-for-plugins-and-start.sh', - ]) { - await fs.writeFile(path.join(runtimeDir, f), ''); - } - - // Write the generated config include so ensureGeneratedConfigIncluded passes - const override = path.join( - runtimeDir, - 'configs/dynamic-plugins/dynamic-plugins.override.yaml', - ); - await fs.outputFile( - override, - `includes:\n - configs/dynamic-plugins/rhdh-cli.generated.local.yaml\n`, - ); - - // Write a minimal frontend plugin package.json so validateProjectFiles passes - // (frontend plugins do not require dist-types). - await fs.writeJson(path.join(pluginDir, 'package.json'), { - name: '@internal/my-watch-plugin', - version: '0.1.0', - backstage: { role: 'frontend-plugin' }, - }); + ({ runtimeDir, pluginDir } = await scaffoldPluginDevRuntime( + 'plugin-dev-watch-runtime-', + 'plugin-dev-watch-plugin-', + '@internal/my-watch-plugin', + )); mockRun.mockReset(); mockExecFile.mockReset(); @@ -885,37 +902,11 @@ describe('start', () => { >; beforeEach(async () => { - runtimeDir = await fs.mkdtemp( - path.join(os.tmpdir(), 'plugin-dev-start-runtime-'), - ); - pluginDir = await fs.mkdtemp( - path.join(os.tmpdir(), 'plugin-dev-start-plugin-'), - ); - (global as any).__pluginDevTestDir = pluginDir; - - for (const f of [ - 'compose.yaml', - 'compose-dynamic-plugins-root.yaml', - 'prepare-and-install-dynamic-plugins.sh', - 'wait-for-plugins-and-start.sh', - ]) { - await fs.writeFile(path.join(runtimeDir, f), ''); - } - - const override = path.join( - runtimeDir, - 'configs/dynamic-plugins/dynamic-plugins.override.yaml', - ); - await fs.outputFile( - override, - `includes:\n - configs/dynamic-plugins/rhdh-cli.generated.local.yaml\n`, - ); - - await fs.writeJson(path.join(pluginDir, 'package.json'), { - name: '@internal/my-start-plugin', - version: '0.1.0', - backstage: { role: 'frontend-plugin' }, - }); + ({ runtimeDir, pluginDir } = await scaffoldPluginDevRuntime( + 'plugin-dev-start-runtime-', + 'plugin-dev-start-plugin-', + '@internal/my-start-plugin', + )); // stagePlugin requires dist-dynamic to already exist (export is mocked). await fs.ensureDir(path.join(pluginDir, 'dist-dynamic')); await fs.writeJson(path.join(pluginDir, 'dist-dynamic', 'package.json'), { @@ -1185,39 +1176,27 @@ describe('resolveRhdhUrl', () => { }); afterEach(() => fs.remove(dir)); + const writeEnvFile = (name: string, content: string) => + fs.writeFile(path.join(dir, name), content); + it('returns the fallback URL when no env files exist', async () => { await expect(resolveRhdhUrl(dir)).resolves.toBe('http://localhost:7007'); }); it('reads BASE_URL from default.env', async () => { - await fs.writeFile( - path.join(dir, 'default.env'), - 'BASE_URL=http://localhost:9999\n', - ); + await writeEnvFile('default.env', 'BASE_URL=http://localhost:9999\n'); await expect(resolveRhdhUrl(dir)).resolves.toBe('http://localhost:9999'); }); it('.env overrides default.env', async () => { - await fs.writeFile( - path.join(dir, 'default.env'), - 'BASE_URL=http://localhost:9999\n', - ); - await fs.writeFile( - path.join(dir, '.env'), - '# comment\nBASE_URL=http://my-host:7007\n', - ); + await writeEnvFile('default.env', 'BASE_URL=http://localhost:9999\n'); + await writeEnvFile('.env', '# comment\nBASE_URL=http://my-host:7007\n'); await expect(resolveRhdhUrl(dir)).resolves.toBe('http://my-host:7007'); }); it('ignores commented-out BASE_URL lines', async () => { - await fs.writeFile( - path.join(dir, 'default.env'), - 'BASE_URL=http://localhost:7007\n', - ); - await fs.writeFile( - path.join(dir, '.env'), - '# BASE_URL=http://other:1234\n', - ); + await writeEnvFile('default.env', 'BASE_URL=http://localhost:7007\n'); + await writeEnvFile('.env', '# BASE_URL=http://other:1234\n'); await expect(resolveRhdhUrl(dir)).resolves.toBe('http://localhost:7007'); }); }); From dbc174801092f3c44d82f143337a7d0868c3c87b Mon Sep 17 00:00:00 2001 From: Stan Lewis Date: Fri, 25 Sep 2026 08:44:25 -0400 Subject: [PATCH 06/11] fix(plugin-dev): address fullsend review findings on watch mode PR - Fix waitForInstallerToFinish hardcoding Podman's 'died' event action unconditionally: on Docker (--container-tool docker), the terminal container-death event is 'die', not 'died', so the --filter event=died argument to 'docker events' never matched anything and phase 3 of plugin dev start stalled for the full 60s waitForContainerEvent timeout on every Docker-backed run. Branch on containerTool the same way waitForContainerCleanup already does. Add a regression test that asserts the docker branch subscribes with event=die, not event=died. - Trim src/commands/dev/index.ts back down to the six CLI action handlers (start, update, restart, stop, logs, status) per this repo's barrel-file convention (AGENTS.md). watchUpdate, waitForContainerEvent, waitForContainerCleanup, resolveRhdhUrl, and waitForRhdhReady have no consumer outside command.ts/command.test.ts (tests already import them directly from ./command) and were widening the module's public surface without a driving need. - Fix stale watchUpdate JSDoc claiming it watches tsconfig*.json and *.config.* files; watchPaths only ever contained src/ and package.json. - README: document --watch on start/update, start's phased [1/4]-[4/4] output and blocking-until-ready behavior, update's readiness-poll blocking (up to 2 minutes) and URL output, and that update/restart require the runtime to already be running. - AGENTS.md: document --watch, the readiness-poll blocking behavior on start/update, and the ensureRuntimeRunning pre-flight check on update/restart. Addresses fullsend-ai-review findings on PR #222 (inline comments on command.ts:239, dev/index.ts:20, command.ts:459, and the PR-level summary comment covering README.md/AGENTS.md staleness). The 'authorization-non-github' note and the 'ssrf' note on waitForRhdhReady (informational only, no remediation suggested; the poll target is local developer configuration, not an external input) require no code change. Signed-off-by: Stan Lewis Assisted-By: opencode rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED --- AGENTS.md | 13 ++++++++++++- README.md | 14 +++++++++++++- src/commands/dev/command.test.ts | 27 +++++++++++++++++++++++++++ src/commands/dev/command.ts | 12 ++++++++++-- src/commands/dev/index.ts | 14 +------------- 5 files changed, 63 insertions(+), 17 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index ebf5435..c0440fe 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -125,6 +125,13 @@ registered in `src/commands/index.ts` and lazy-load their handlers from re-staging) — use it when changing RHDH Local configuration without touching plugin code. `update` re-exports, re-stages, and then restarts the RHDH service. +`start` and `update` both block on `waitForRhdhReady` (poll-based, 120s default +timeout) before returning, printing the RHDH URL once it responds. Both also +accept `--watch`, which hands off into `watchUpdate`: a chokidar watcher on +`src/` and `package.json` (500ms debounce, serialized cycles) that repeats the +same export/stage/restart cycle as a one-shot `update` on every source change, +so a source edit doesn't require re-running the command by hand. + Key files: - `command.ts` — per-subcommand handlers (`start`, `update`, `restart`, `stop`, @@ -152,7 +159,11 @@ RHDH Local and will not appear in `git status` after a successful `start`. **Pre-flight check:** `start` and `update` call `validateProjectFiles()` before invoking `plugin export`. For backend plugins this checks that `dist-types/` exists, failing fast with a clear `yarn tsc` instruction rather than letting -`yarn build` fail deep in the export process. +`yarn build` fail deep in the export process. `update` and `restart` also call +`ensureRuntimeRunning()` first, which inspects Compose `ps` for the `rhdh` +service and fails fast with an actionable message ("RHDH Local is not running. +Run `rhdh-cli plugin dev start` first.") instead of a raw compose/container +subprocess error. **Symlink handling:** `stagePlugin` uses `fs.remove` + `fs.copy` with `dereference: false` so relative symlinks in `node_modules/.bin/` are preserved diff --git a/README.md b/README.md index da3f4a5..5f3c7b3 100644 --- a/README.md +++ b/README.md @@ -88,12 +88,24 @@ Use `plugin dev` from a generated or existing dynamic plugin project to export i rhdh-cli plugin dev start --configure --rhdh-local-dir /path/to/rhdh-local ``` -`--configure` adds the CLI-managed plugin configuration include without replacing existing user configuration. Set `RHDH_LOCAL_DIR` to avoid repeating the path. After changing plugin source, refresh the staged plugin and RHDH service with: +`--configure` adds the CLI-managed plugin configuration include without replacing existing user configuration. Set `RHDH_LOCAL_DIR` to avoid repeating the path. `start` prints four labeled phases (`[1/4]` build/export, `[2/4]` start the runtime, `[3/4]` install plugins, `[4/4]` wait for readiness) and blocks until RHDH responds, printing the URL to open once it's ready. + +After changing plugin source, refresh the staged plugin and RHDH service with: ```bash rhdh-cli plugin dev update ``` +`update` and `restart` require the runtime to already be running — start it first with `plugin dev start`, or they fail fast with an actionable message instead of a raw compose/container error. Like `start`, `update` blocks until RHDH is reachable again (up to two minutes) and prints the URL on success. + +Pass `--watch` to `start` or `update` to keep the CLI running: it watches `src/` and `package.json` and automatically re-exports, re-stages, and restarts the runtime on every change, so you don't have to re-run `update` by hand. + +```bash +rhdh-cli plugin dev start --watch +# or, once the runtime is already up: +rhdh-cli plugin dev update --watch +``` + Use `rhdh-cli plugin dev status` for the interpreted runtime state, `rhdh-cli plugin dev logs` for application logs, and `rhdh-cli plugin dev logs --installer` to diagnose installation failures. To restart the RHDH service after changing RHDH Local configuration (without re-deploying the plugin), use `rhdh-cli plugin dev restart`. Stop the runtime with `rhdh-cli plugin dev stop`; add `--clean` to remove containers and networks while retaining volumes, configuration, and plugin artifacts. The default container tool is `podman`; pass `--container-tool docker` if your environment uses Docker instead. The CLI manages a single plugin entry in `configs/dynamic-plugins/rhdh-cli.generated.local.yaml`. Each `start` or `update` run overwrites this file with the current plugin's package path, disabled flag, and pull policy. Extra `pluginConfig` for the plugin (such as app-config keys) belongs in `dynamic-plugins.override.yaml` under a `plugins:` entry for the same package, not in the generated file. diff --git a/src/commands/dev/command.test.ts b/src/commands/dev/command.test.ts index 91d3b02..b51b60c 100644 --- a/src/commands/dev/command.test.ts +++ b/src/commands/dev/command.test.ts @@ -1042,6 +1042,33 @@ describe('start', () => { // Never needed to subscribe to the events stream at all. expect(spawnMock).not.toHaveBeenCalled(); }); + + it("waits for docker's 'die' event, not podman's 'died', when --container-tool=docker", async () => { + // Regression test: Docker's terminal container-death event is 'die', not + // 'died'. Subscribing to phase 3's events stream with the wrong filter + // means it never matches anything and stalls for the full + // waitForContainerEvent timeout on every Docker-backed `start`. + const startPromise = start({ + rhdhLocalDir: runtimeDir, + containerTool: 'docker', + }); + + await waitForCondition( + () => spawnMock.mock.calls.length > 0, + 'waitForContainerEvent to spawn the events subscription', + ); + const [, spawnArgs] = spawnMock.mock.calls[0] as [string, string[]]; + expect(spawnArgs).toContain('event=die'); + expect(spawnArgs).not.toContain('event=died'); + + const event = JSON.stringify({ + Action: 'die', + Attributes: { 'com.docker.compose.service': 'install-dynamic-plugins' }, + }); + fakeChild.stdout.emit('data', Buffer.from(`${event}\n`)); + + await expect(startPromise).resolves.toBeUndefined(); + }); }); // --------------------------------------------------------------------------- diff --git a/src/commands/dev/command.ts b/src/commands/dev/command.ts index 6f4a600..7aad45e 100644 --- a/src/commands/dev/command.ts +++ b/src/commands/dev/command.ts @@ -236,7 +236,15 @@ async function waitForInstallerToFinish( if (composeServiceState(installer).includes('exited')) { return; } - await waitForContainerEvent(containerTool, 'install-dynamic-plugins', 'died'); + // Podman's terminal container-death event is 'died'; Docker's is 'die'. + // Subscribing with the wrong filter means the event stream never matches + // anything and this stalls for the full waitForContainerEvent timeout. + const diedEvent = containerTool === 'podman' ? 'died' : 'die'; + await waitForContainerEvent( + containerTool, + 'install-dynamic-plugins', + diedEvent, + ); } export async function start(opts: OptionValues) { @@ -456,7 +464,7 @@ function describeWatchError(err: unknown): string { * Watch source files and run a full update cycle on changes. * * Design: - * - Watches src/, package.json, tsconfig*.json, and *.config.* files. + * - Watches src/ and package.json. * - Excludes output directories (node_modules, dist, dist-dynamic, * dist-types) to prevent output-loop triggering. * - Events are debounced: the first event in a `debounceMs` window triggers diff --git a/src/commands/dev/index.ts b/src/commands/dev/index.ts index 08f2712..495e57b 100644 --- a/src/commands/dev/index.ts +++ b/src/commands/dev/index.ts @@ -14,16 +14,4 @@ * limitations under the License. */ -export { - start, - update, - watchUpdate, - waitForContainerEvent, - waitForContainerCleanup, - resolveRhdhUrl, - waitForRhdhReady, - restart, - stop, - logs, - status, -} from './command'; +export { start, update, restart, stop, logs, status } from './command'; From f4ae23e7a34f1d3b19e1c6e5eca78531cade7d97 Mon Sep 17 00:00:00 2001 From: Stan Lewis Date: Fri, 25 Sep 2026 09:29:54 -0400 Subject: [PATCH 07/11] fix(plugin-dev): address second round of fullsend review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix api-contract-violation: waitForContainerEvent passed '--stream' unconditionally, but Docker's 'events' command has no such flag (it always streams) and rejects unknown flags, exiting immediately. child.on('close') then resolved the promise before any event was seen, making phase 3 of 'plugin dev start' (waitForInstallerToFinish) a silent no-op on Docker. Only pass --stream for Podman. Added a regression test asserting docker args omit --stream and podman args include it; confirmed it fails against the unfixed code. - Fix edge-case: 'update --watch' entered watchUpdate without calling ensureRuntimeRunning first, so a stopped runtime only surfaced an error on the first change-triggered cycle instead of failing fast like the one-shot 'update' path. Call ensureRuntimeRunning before entering watch mode. Added a regression test. - Fix logic-error: the readiness check 'res.ok || (res.status >= 200 && res.status < 400)' was a tautology (res.ok's [200,300) is already covered by [200,400)), obscuring the intent to also accept 3xx redirects. Simplified to the range check alone with a clarifying comment. - Fix api-shape: runUpdateCycle and watchUpdate led with runtimeDir, while every other multi-param helper in the file (getComposeServices, ensureRuntimeRunning, compose, waitForInstallerToFinish, waitForContainerCleanup) leads with containerTool. Reordered both to (containerTool, runtimeDir, ...) and updated all call sites (command.ts and command.test.ts). - Fix code-organization: watchUpdate registered two separate watcher.on('error', ...) listeners (one logging, one rejecting). Consolidated into a single handler that does both. Addresses fullsend-ai-review findings on PR #222 at commit dbc1748. The 'protected-path' note on AGENTS.md is acknowledged — that edit was made in the prior commit at the PR author's explicit direction to document the RHIDP-16673 behaviors landing in this same PR. Signed-off-by: Stan Lewis Assisted-By: opencode rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED --- src/commands/dev/command.test.ts | 54 +++++++++++++++++++++++++++++--- src/commands/dev/command.ts | 41 +++++++++++++++--------- 2 files changed, 75 insertions(+), 20 deletions(-) diff --git a/src/commands/dev/command.test.ts b/src/commands/dev/command.test.ts index b51b60c..0be2746 100644 --- a/src/commands/dev/command.test.ts +++ b/src/commands/dev/command.test.ts @@ -390,6 +390,28 @@ describe('plugin dev', () => { ).rejects.toThrow('RHDH Local is not running'); }); + it('update --watch rejects immediately when RHDH Local is not running, without entering watch mode', async () => { + // Regression test: --watch must fail fast the same way the one-shot + // path does. Without an explicit ensureRuntimeRunning call before + // watchUpdate, this would instead resolve into watch mode and only + // surface the error on the first change-triggered cycle. + const mockChokidarWatch = chokidar.watch as jest.MockedFunction< + typeof chokidar.watch + >; + mockChokidarWatch.mockClear(); + mockExecFile + .mockResolvedValueOnce({ stdout: 'podman version 5.0.0', stderr: '' }) + .mockResolvedValueOnce({ stdout: '[]', stderr: '' }); + await expect( + update({ + rhdhLocalDir: runtimeDir, + containerTool: 'podman', + watch: true, + }), + ).rejects.toThrow('RHDH Local is not running'); + expect(mockChokidarWatch).not.toHaveBeenCalled(); + }); + it('restart rejects with an actionable message when RHDH Local is not running', async () => { mockExecFile .mockResolvedValueOnce({ stdout: 'podman version 5.0.0', stderr: '' }) @@ -773,7 +795,7 @@ describe('watchUpdate', () => { it('runs an update cycle when a file change event fires', async () => { // debounceMs=0 so the timer fires on the next event-loop tick. - const watchPromise = watchUpdate(runtimeDir, 'podman', 0, 0); + const watchPromise = watchUpdate('podman', runtimeDir, 0, 0); fakeWatcher.emit('all', 'change', 'src/index.ts'); await waitForExportCalls(1); @@ -787,7 +809,7 @@ describe('watchUpdate', () => { it('debounces rapid consecutive file events into a single cycle', async () => { // Use debounceMs=20 so rapid events within that window coalesce, but the // cycle still completes quickly in real-timer mode. - const watchPromise = watchUpdate(runtimeDir, 'podman', 20, 0); + const watchPromise = watchUpdate('podman', runtimeDir, 20, 0); // Three events fired rapidly — only the first one should schedule a timer // (the guard `if (debounceTimer !== undefined) return` drops the rest). @@ -803,7 +825,7 @@ describe('watchUpdate', () => { }); it('continues watching after a failed update cycle', async () => { - const watchPromise = watchUpdate(runtimeDir, 'podman', 0, 0); + const watchPromise = watchUpdate('podman', runtimeDir, 0, 0); mockExport.mockRejectedValueOnce(new Error('build exploded')); @@ -841,7 +863,7 @@ describe('watchUpdate', () => { } it('fails a cycle cleanly and keeps watching when RHDH Local is not running', async () => { - const watchPromise = watchUpdate(runtimeDir, 'podman', 0, 0); + const watchPromise = watchUpdate('podman', runtimeDir, 0, 0); // Simulate RHDH Local not running for the first triggered cycle — // ensureRuntimeRunning should reject before exportCommand is ever called. @@ -862,7 +884,7 @@ describe('watchUpdate', () => { }); it('closes the watcher on SIGINT', async () => { - watchUpdate(runtimeDir, 'podman', 0, 0); + watchUpdate('podman', runtimeDir, 0, 0); const exitSpy = jest .spyOn(process, 'exit') @@ -1134,6 +1156,28 @@ describe('waitForContainerEvent', () => { expect.anything(), ); }); + + it('omits --stream for docker, which has no such flag and always streams', async () => { + // Regression test: docker's `events` command has no --stream flag and + // rejects it, exiting immediately (child.on('close') would then resolve + // this promise before any event is ever seen). Podman's `events` + // supports (and per its own docs, expects) an explicit --stream. + const { spawn: spawnMock } = jest.requireMock('node:child_process') as { + spawn: jest.Mock; + }; + spawnMock.mockClear(); + + waitForContainerEvent('docker', 'rhdh', 'die', 5000); + await new Promise(resolve => setImmediate(resolve)); + const [, dockerArgs] = spawnMock.mock.calls[0] as [string, string[]]; + expect(dockerArgs).not.toContain('--stream'); + + spawnMock.mockClear(); + waitForContainerEvent('podman', 'rhdh', 'died', 5000); + await new Promise(resolve => setImmediate(resolve)); + const [, podmanArgs] = spawnMock.mock.calls[0] as [string, string[]]; + expect(podmanArgs).toContain('--stream'); + }); }); // --------------------------------------------------------------------------- diff --git a/src/commands/dev/command.ts b/src/commands/dev/command.ts index 7aad45e..7fe4a07 100644 --- a/src/commands/dev/command.ts +++ b/src/commands/dev/command.ts @@ -71,7 +71,8 @@ export async function waitForRhdhReady( while (Date.now() < deadline) { try { const res = await fetch(url, { signal: AbortSignal.timeout(5_000) }); - if (res.ok || (res.status >= 200 && res.status < 400)) { + // 3xx redirects are treated as ready too, not just res.ok's [200, 300). + if (res.status >= 200 && res.status < 400) { process.stdout.write(' ready.\n'); return url; } @@ -269,13 +270,13 @@ export async function start(opts: OptionValues) { Task.log(`\nRHDH is ready at ${url}`); if (opts.watch) { - await watchUpdate(runtimeDir, containerTool); + await watchUpdate(containerTool, runtimeDir); } } async function runUpdateCycle( - runtimeDir: string, containerTool: string, + runtimeDir: string, prefix = '', ): Promise { await ensureRuntimeRunning(containerTool, runtimeDir); @@ -297,9 +298,14 @@ async function runUpdateCycle( export async function update(opts: OptionValues) { const { runtimeDir, containerTool } = await resolveAndValidate(opts); if (opts.watch) { - await watchUpdate(runtimeDir, containerTool); + // Fail fast here rather than relying on the first change-triggered cycle + // inside watchUpdate to discover this — a user starting `update --watch` + // against a stopped runtime should see the actionable error immediately, + // not only after they edit a source file. + await ensureRuntimeRunning(containerTool, runtimeDir); + await watchUpdate(containerTool, runtimeDir); } else { - await runUpdateCycle(runtimeDir, containerTool); + await runUpdateCycle(containerTool, runtimeDir); } } @@ -326,7 +332,12 @@ export async function waitForContainerEvent( containerTool, [ 'events', - '--stream', + // Podman defaults to streaming but accepts (and needs, per its own + // docs) an explicit --stream. Docker's `events` has no such flag and + // always streams — passing it there makes Docker reject the command + // and exit immediately, so `close` would resolve this promise before + // any event is ever seen. + ...(containerTool === 'podman' ? ['--stream'] : []), '--format', 'json', '--filter', @@ -482,8 +493,8 @@ function describeWatchError(err: unknown): string { * 0 to skip event-based settling entirely (used in tests). */ export async function watchUpdate( - runtimeDir: string, containerTool: string, + runtimeDir: string, debounceMs = 500, settleTimeoutMs = 15_000, ): Promise { @@ -516,7 +527,7 @@ export async function watchUpdate( const cycleStart = Date.now(); try { Task.log(`\n[watch] Change detected — starting update cycle...`); - await runUpdateCycle(runtimeDir, containerTool, '[watch] '); + await runUpdateCycle(containerTool, runtimeDir, '[watch] '); Task.log(`[watch] Update complete in ${Date.now() - cycleStart}ms.`); } catch (err: unknown) { const message = describeWatchError(err); @@ -558,11 +569,6 @@ export async function watchUpdate( scheduleUpdate(); }); - watcher.on('error', (err: unknown) => { - const message = describeWatchError(err); - Task.log(`[watch] Watcher error: ${message}`); - }); - const shutdown = async () => { Task.log('\n[watch] Shutting down...'); if (debounceTimer !== undefined) { @@ -575,9 +581,14 @@ export async function watchUpdate( process.on('SIGINT', shutdown); process.on('SIGTERM', shutdown); - // Keep the process alive while the watcher is active. + // Keep the process alive while the watcher is active. A fatal watcher + // error logs and ends the keep-alive promise. await new Promise((_resolve, reject) => { - watcher.on('error', reject); + watcher.on('error', (err: unknown) => { + const message = describeWatchError(err); + Task.log(`[watch] Watcher error: ${message}`); + reject(err); + }); }); } From 650e2a40419b2cd98df5641b3c5addaf88726514 Mon Sep 17 00:00:00 2001 From: Stan Lewis Date: Fri, 25 Sep 2026 15:30:42 -0400 Subject: [PATCH 08/11] fix(plugin-dev): address human review findings on watch mode PR Seven findings from PatAKnight's review round: - Hold running through the settle-wait instead of clearing it in a finally block right after the cycle. A change arriving during the wait previously saw running === false and started a second, concurrent drainCycles() loop instead of being coalesced into the active one. Also move the settle-wait's event subscription to the top of each cycle (before runUpdateCycle's own compose stop/start calls run), since subscribing afterward almost always misses the event and burns the full settleTimeoutMs. - Fix Docker's events JSON shape: the compose-service label lives under Actor.Attributes for Docker, not the top-level Attributes field Podman uses (verified against real podman events --format json output and Docker's documented schema). Reading only the top-level field meant the filter never matched on Docker, and the events-shape bug was masked by a prior regression test that emitted Podman's shape for a Docker scenario -- fixed that test too. - Watch tsconfig.json in addition to src/ and package.json -- RHIDP-16673's acceptance criteria require watching "relevant build configuration files"; tsconfig.json is the only one plugin new scaffolds. - Scope isIgnoredWatchPath's segment matching to the plugin root (chokidar always passes absolute paths) instead of checking every segment of the full absolute path, which ignored every file when a checkout merely lived under an ancestor directory named e.g. dist or node_modules. - Kill in-flight container-events child processes on shutdown. spawn() shares the parent's process group by default, so a terminal Ctrl+C's whole-group SIGINT takes them along for free, but a direct SIGTERM to just this PID does not, and process.exit() prevents waitForContainerEvent's own JS-side timeout from ever running to kill them itself. Track spawned children in a module-level set; shutdown() kills everything still in it. Added a SIGTERM counterpart to the existing SIGINT test. - Log a warning instead of silently proceeding when the events subprocess errors or exits with an unexpected non-zero code, instead of treating it the same as a successful match. - Run update --watch's first cycle immediately instead of leaving the current tree undeployed until the first save, matching start --watch's build-then-watch structure. This also subsumes the previous round's standalone ensureRuntimeRunning pre-flight call. Manually verified end to end: scaffolded a backend plugin with plugin new, ran plugin dev start and plugin dev update --watch against a real RHDH Local checkout (podman). Confirmed the initial --watch deploy happens before entering watch mode, a source edit triggers a full re-export/re-stage/restart cycle, tsconfig.json is listed in the watch log, and SIGTERM sent mid-cycle kills the in-flight settle-wait's podman events children with no orphans left behind (verified via ps). The new close-code warning also fired for real during this session against a live podman events subprocess, confirming it surfaces genuine environment hiccups rather than only synthetic test scenarios. Signed-off-by: Stan Lewis Assisted-By: opencode rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED --- src/commands/dev/command.test.ts | 372 ++++++++++++++++++++++++++++++- src/commands/dev/command.ts | 124 ++++++++--- 2 files changed, 463 insertions(+), 33 deletions(-) diff --git a/src/commands/dev/command.test.ts b/src/commands/dev/command.test.ts index 0be2746..99f4211 100644 --- a/src/commands/dev/command.test.ts +++ b/src/commands/dev/command.test.ts @@ -657,28 +657,62 @@ describe('plugin dev', () => { // --------------------------------------------------------------------------- describe('isIgnoredWatchPath', () => { - it('ignores paths under output/dependency directories', () => { - expect(isIgnoredWatchPath(path.join('plugin', 'dist', 'output.js'))).toBe( + const root = path.join(path.sep, 'home', 'user', 'checkout', 'plugin'); + + it('ignores paths under output/dependency directories inside the plugin root', () => { + expect(isIgnoredWatchPath(path.join(root, 'dist', 'output.js'), root)).toBe( true, ); expect( - isIgnoredWatchPath(path.join('plugin', 'dist-dynamic', 'package.json')), + isIgnoredWatchPath(path.join(root, 'dist-dynamic', 'package.json'), root), ).toBe(true); expect( - isIgnoredWatchPath(path.join('plugin', 'dist-types', 'index.d.ts')), + isIgnoredWatchPath(path.join(root, 'dist-types', 'index.d.ts'), root), ).toBe(true); expect( isIgnoredWatchPath( - path.join('plugin', 'node_modules', 'pkg', 'index.js'), + path.join(root, 'node_modules', 'pkg', 'index.js'), + root, ), ).toBe(true); }); it('does not ignore ordinary watched paths', () => { - expect(isIgnoredWatchPath(path.join('plugin', 'src', 'index.ts'))).toBe( + expect(isIgnoredWatchPath(path.join(root, 'src', 'index.ts'), root)).toBe( + false, + ); + expect(isIgnoredWatchPath(path.join(root, 'package.json'), root)).toBe( false, ); - expect(isIgnoredWatchPath(path.join('plugin', 'package.json'))).toBe(false); + }); + + it('does not ignore a path just because an ancestor outside the root is named dist or node_modules', () => { + // Regression test: chokidar always passes absolute paths. Checking every + // segment of the *full* absolute path (rather than only segments inside + // the plugin root) meant a checkout that merely lived under e.g. + // /home/user/dist/my-plugin or /home/user/node_modules-backup/my-plugin + // had every file ignored, while `watchUpdate` still printed "Watching...". + const weirdRoot = path.join(path.sep, 'home', 'user', 'dist', 'plugin'); + expect( + isIgnoredWatchPath(path.join(weirdRoot, 'src', 'index.ts'), weirdRoot), + ).toBe(false); + + const weirdRoot2 = path.join( + path.sep, + 'home', + 'user', + 'node_modules', + 'plugin', + ); + expect( + isIgnoredWatchPath(path.join(weirdRoot2, 'package.json'), weirdRoot2), + ).toBe(false); + }); + + it('does not ignore paths outside the root (defensive; chokidar should not call us with these)', () => { + expect( + isIgnoredWatchPath(path.join(path.sep, 'other', 'dist', 'file.js'), root), + ).toBe(false); }); }); @@ -806,6 +840,28 @@ describe('watchUpdate', () => { await expect(watchPromise).rejects.toThrow('done'); }); + it('watches tsconfig.json in addition to src/ and package.json', async () => { + // Regression test: RHIDP-16673's acceptance criteria require watching + // "relevant build configuration files" alongside src/ and package.json. + // tsconfig.json is the only one scaffolded projects actually have + // (plugin new's adaptStandaloneProject). + const mockChokidarWatch = chokidar.watch as jest.MockedFunction< + typeof chokidar.watch + >; + mockChokidarWatch.mockClear(); + + const watchPromise = watchUpdate('podman', runtimeDir, 0, 0); + await new Promise(resolve => setImmediate(resolve)); + + expect(mockChokidarWatch).toHaveBeenCalledWith( + expect.arrayContaining([expect.stringContaining('tsconfig.json')]), + expect.anything(), + ); + + fakeWatcher.emit('error', new Error('done')); + await expect(watchPromise).rejects.toThrow('done'); + }); + it('debounces rapid consecutive file events into a single cycle', async () => { // Use debounceMs=20 so rapid events within that window coalesce, but the // cycle still completes quickly in real-timer mode. @@ -824,6 +880,101 @@ describe('watchUpdate', () => { await expect(watchPromise).rejects.toThrow('done'); }); + it('holds running through the settle wait, so a change during it does not start a concurrent cycle', async () => { + // Regression test: `running` was previously cleared in a `finally` block + // right after the cycle itself, before the optional settle-wait ran. A + // change arriving during that wait saw `running === false` and called + // drainCycles() again — a second, concurrent loop rather than a + // coalesced follow-up. Also verifies the settle-wait subscribes to the + // events stream *before* runUpdateCycle's own compose stop/start calls + // run (not after), by capturing spawned children and confirming both + // exist before cycle 2 begins. + const { spawn: spawnMock } = jest.requireMock('node:child_process') as { + spawn: jest.Mock; + }; + const settleChildren: FakeChildProcess[] = []; + spawnMock.mockImplementation(() => { + const child = new FakeChildProcess(); + settleChildren.push(child); + return child; + }); + + // Hold cycle 1 open so we can reliably inject a second change while + // `running` is still true. + let releaseCycle1: () => void = () => {}; + mockExport.mockImplementationOnce( + () => new Promise(resolve => (releaseCycle1 = resolve)), + ); + + const watchPromise = watchUpdate('podman', runtimeDir, 0, 5000); + + fakeWatcher.emit('all', 'change', 'src/a.ts'); + await waitForExportCalls(1); // cycle 1 started; still in-flight + + fakeWatcher.emit('all', 'change', 'src/b.ts'); + // Let the second change's own (0ms) debounce timer fire and observe + // running === true, setting pendingChange instead of starting a second + // drainCycles(). + await new Promise(resolve => setTimeout(resolve, 10)); + + releaseCycle1(); // cycle 1 finishes; keepGoing becomes true + + // The settle-wait's two subscriptions (rhdh + install-dynamic-plugins) + // should spawn before cycle 2 starts. + const spawnDeadline = Date.now() + 5000; + while (settleChildren.length < 2) { + if (Date.now() > spawnDeadline) { + throw new Error('Timed out waiting for settle-wait children to spawn'); + } + await new Promise(resolve => setImmediate(resolve)); + } + + // While the settle-wait is unresolved, cycle 2 must not have started yet + // — proving `running` stayed true and the pending change was coalesced + // into this same drainCycles() loop rather than a concurrent one. + await new Promise(resolve => setImmediate(resolve)); + expect(mockExport).toHaveBeenCalledTimes(1); + + // A third change arriving *during* the settle-wait is the critical case: + // with the bug (running cleared before the wait), this would see + // running === false and call drainCycles() again — a second, concurrent + // cycle starting immediately, incrementing mockExport to 2 before the + // settle-wait ever resolves. With the fix, it's coalesced into the same + // pendingChange the already-running loop will pick up once the wait ends. + fakeWatcher.emit('all', 'change', 'src/c.ts'); + await new Promise(resolve => setTimeout(resolve, 10)); + expect(mockExport).toHaveBeenCalledTimes(1); + + // Resolve the settle-wait by emitting the expected event to each child. + for (const child of settleChildren) { + const event = JSON.stringify({ + Status: 'cleanup', + Attributes: { 'com.docker.compose.service': 'rhdh' }, + }); + child.stdout.emit('data', Buffer.from(`${event}\n`)); + const event2 = JSON.stringify({ + Status: 'cleanup', + Attributes: { + 'com.docker.compose.service': 'install-dynamic-plugins', + }, + }); + child.stdout.emit('data', Buffer.from(`${event2}\n`)); + } + + // Cycle 2 now runs, absorbing the coalesced pending change. + await waitForExportCalls(2); + expect(mockExport).toHaveBeenCalledTimes(2); + + fakeWatcher.emit('error', new Error('done')); + await expect(watchPromise).rejects.toThrow('done'); + + // Restore the default mock implementation for subsequent tests. + spawnMock.mockImplementation(() => { + fakeChild = new FakeChildProcess(); + return fakeChild; + }); + }); + it('continues watching after a failed update cycle', async () => { const watchPromise = watchUpdate('podman', runtimeDir, 0, 0); @@ -901,6 +1052,139 @@ describe('watchUpdate', () => { exitSpy.mockRestore(); }); + + it('closes the watcher on SIGTERM', async () => { + // Regression coverage alongside the SIGINT test above: a terminal Ctrl+C + // signals the whole foreground process group for free, but a direct + // SIGTERM to just this process's PID does not reach any spawned + // children — shutdown() must still run the same way on either signal. + watchUpdate('podman', runtimeDir, 0, 0); + + const exitSpy = jest + .spyOn(process, 'exit') + .mockImplementation((() => {}) as () => never); + + process.emit('SIGTERM'); + + await new Promise(resolve => setImmediate(resolve)); + await Promise.resolve(); + + expect(fakeWatcher.close).toHaveBeenCalled(); + expect(exitSpy).toHaveBeenCalledWith(0); + + exitSpy.mockRestore(); + }); + + it('kills an in-flight settle-wait events subscription on shutdown', async () => { + // Regression test: spawn() puts children in the same process group as + // the parent by default, so SIGINT (terminal-wide) takes them along for + // free, but SIGTERM to just this PID does not — and process.exit() + // prevents waitForContainerEvent's own JS-side timeout from ever running + // to kill them itself. shutdown() must kill any tracked child directly. + const { spawn: spawnMock } = jest.requireMock('node:child_process') as { + spawn: jest.Mock; + }; + const settleChildren: FakeChildProcess[] = []; + spawnMock.mockImplementation(() => { + const child = new FakeChildProcess(); + settleChildren.push(child); + return child; + }); + + // Hold cycle 1 open (mockExport won't resolve) so we can reliably inject + // a second change while `running` is still true, forcing pendingChange + // and, once cycle 1 is released, the settle-wait phase. + let releaseCycle1: () => void = () => {}; + mockExport.mockImplementationOnce( + () => new Promise(resolve => (releaseCycle1 = resolve)), + ); + + // settleTimeoutMs > 0 so drainCycles spawns the settle-wait listeners. + const watchPromise = watchUpdate('podman', runtimeDir, 0, 5000); + + fakeWatcher.emit('all', 'change', 'src/a.ts'); + await waitForExportCalls(1); // cycle 1 started; still in-flight (export pending) + + fakeWatcher.emit('all', 'change', 'src/b.ts'); + // Let the second change's own (0ms) debounce timer fire and observe + // running === true, setting pendingChange rather than starting a + // concurrent drainCycles(). + await new Promise(resolve => setTimeout(resolve, 10)); + + releaseCycle1(); // let cycle 1 finish; keepGoing becomes true + + // Wait for the settle-wait's two events subscriptions (rhdh + + // install-dynamic-plugins) to spawn. + const deadline = Date.now() + 5000; + while (settleChildren.length < 2) { + if (Date.now() > deadline) { + throw new Error('Timed out waiting for settle-wait children to spawn'); + } + await new Promise(resolve => setImmediate(resolve)); + } + + const exitSpy = jest + .spyOn(process, 'exit') + .mockImplementation((() => {}) as () => never); + + process.emit('SIGTERM'); + await new Promise(resolve => setImmediate(resolve)); + await Promise.resolve(); + + for (const child of settleChildren) { + expect(child.kill).toHaveBeenCalled(); + } + + exitSpy.mockRestore(); + void watchPromise; + + // Restore the default mock implementation for subsequent tests. + spawnMock.mockImplementation(() => { + fakeChild = new FakeChildProcess(); + return fakeChild; + }); + }); + + it('update --watch deploys the current tree immediately, before entering watch mode', async () => { + // Regression test: unlike `start --watch` (which runs its full 4-phase + // cycle before conditionally entering watchUpdate), `update --watch` + // previously skipped straight into watchUpdate() with no initial + // deploy — whatever was already on disk sat undeployed until the first + // change-triggered cycle. + // stagePlugin requires dist-dynamic to already exist (export is mocked + // and doesn't create it itself). + await fs.ensureDir(path.join(pluginDir, 'dist-dynamic')); + await fs.writeJson(path.join(pluginDir, 'dist-dynamic', 'package.json'), { + name: '@internal/my-watch-plugin', + version: '0.1.0', + }); + + const mockFetch = jest + .fn() + .mockResolvedValue(new Response(null, { status: 200 }) as Response); + (global as any).fetch = mockFetch; + + const updatePromise = update({ + rhdhLocalDir: runtimeDir, + containerTool: 'podman', + watch: true, + }); + + // The initial cycle runs immediately, without waiting for any watcher + // event. + await waitForExportCalls(1); + await waitForTaskLogContaining('Refresh your browser at'); + + // A subsequent file change triggers a second cycle via watchUpdate. + fakeWatcher.emit('all', 'change', 'src/index.ts'); + await waitForExportCalls(2); + expect(mockExport).toHaveBeenCalledTimes(2); + + fakeWatcher.emit('error', new Error('done')); + await expect(updatePromise).rejects.toThrow('done'); + + (global as any).fetch = undefined; + }); }); // --------------------------------------------------------------------------- @@ -1083,9 +1367,15 @@ describe('start', () => { expect(spawnArgs).toContain('event=die'); expect(spawnArgs).not.toContain('event=died'); + // Real Docker `events --format json` shape: the compose-service label + // lives under Actor.Attributes, not a top-level Attributes field (that's + // Podman's shape — using it here would silently pass this test without + // actually exercising Docker's real event parsing). const event = JSON.stringify({ Action: 'die', - Attributes: { 'com.docker.compose.service': 'install-dynamic-plugins' }, + Actor: { + Attributes: { 'com.docker.compose.service': 'install-dynamic-plugins' }, + }, }); fakeChild.stdout.emit('data', Buffer.from(`${event}\n`)); @@ -1178,6 +1468,72 @@ describe('waitForContainerEvent', () => { const [, podmanArgs] = spawnMock.mock.calls[0] as [string, string[]]; expect(podmanArgs).toContain('--stream'); }); + + it("matches Docker's Actor.Attributes event shape, not just Podman's top-level Attributes", async () => { + // Regression test: real `docker events --format json` output nests the + // compose-service label under Actor.Attributes, not a top-level + // Attributes field. Podman puts it top-level. Reading only the top-level + // field means svc is always '' for real Docker events, so the filter + // never matches and this always burns the full timeout on Docker. + const p = waitForContainerEvent('docker', 'rhdh', 'die', 5000); + await new Promise(resolve => setImmediate(resolve)); + const event = JSON.stringify({ + Action: 'die', + Actor: { Attributes: { 'com.docker.compose.service': 'rhdh' } }, + }); + fakeChild.stdout.emit('data', Buffer.from(`${event}\n`)); + await new Promise(resolve => setImmediate(resolve)); + await expect(p).resolves.toBeUndefined(); + expect(fakeChild.kill).toHaveBeenCalled(); + }); + + it('still matches events using the top-level Attributes shape (Podman)', async () => { + const p = waitForContainerEvent('podman', 'rhdh', 'died', 5000); + await new Promise(resolve => setImmediate(resolve)); + emitEvent('rhdh', 'died'); + await new Promise(resolve => setImmediate(resolve)); + await expect(p).resolves.toBeUndefined(); + }); + + it('logs a warning and still resolves when the events command fails to spawn', async () => { + const mockTask = Task as jest.Mocked; + mockTask.log.mockClear(); + const p = waitForContainerEvent('podman', 'rhdh', 'died', 5000); + await new Promise(resolve => setImmediate(resolve)); + fakeChild.emit('error', new Error('spawn podman ENOENT')); + await expect(p).resolves.toBeUndefined(); + expect(mockTask.log).toHaveBeenCalledWith( + expect.stringContaining('spawn podman ENOENT'), + ); + }); + + it('logs a warning when the events command exits unexpectedly on its own', async () => { + const mockTask = Task as jest.Mocked; + mockTask.log.mockClear(); + const p = waitForContainerEvent('podman', 'rhdh', 'died', 5000); + await new Promise(resolve => setImmediate(resolve)); + fakeChild.emit('close', 1, null); + await expect(p).resolves.toBeUndefined(); + expect(mockTask.log).toHaveBeenCalledWith( + expect.stringContaining('exited unexpectedly (code 1)'), + ); + }); + + it('does not log a warning when close follows our own kill() after a match', async () => { + const mockTask = Task as jest.Mocked; + mockTask.log.mockClear(); + const p = waitForContainerEvent('podman', 'rhdh', 'died', 5000); + await new Promise(resolve => setImmediate(resolve)); + emitEvent('rhdh', 'died'); + await new Promise(resolve => setImmediate(resolve)); + await p; + // Simulate the 'close' event that naturally follows our own kill() — + // code is null (terminated by signal), not a non-zero exit code. + fakeChild.emit('close', null, 'SIGTERM'); + expect(mockTask.log).not.toHaveBeenCalledWith( + expect.stringContaining('exited unexpectedly'), + ); + }); }); // --------------------------------------------------------------------------- diff --git a/src/commands/dev/command.ts b/src/commands/dev/command.ts index 7fe4a07..b5e42a3 100644 --- a/src/commands/dev/command.ts +++ b/src/commands/dev/command.ts @@ -18,7 +18,7 @@ import { OptionValues } from 'commander'; import fs from 'fs-extra'; import YAML from 'yaml'; import path from 'node:path'; -import { spawn } from 'node:child_process'; +import { spawn, ChildProcess } from 'node:child_process'; import chokidar from 'chokidar'; import { execFile, run } from '../../lib/run'; @@ -297,18 +297,30 @@ async function runUpdateCycle( export async function update(opts: OptionValues) { const { runtimeDir, containerTool } = await resolveAndValidate(opts); + // Always deploy the current tree immediately, matching `start`'s + // build-then-optionally-watch structure — `--watch` must not leave + // whatever's already on disk undeployed until the first change arrives. + // This also covers the ensureRuntimeRunning pre-flight check, so a + // stopped runtime fails fast here rather than only on the first + // change-triggered cycle inside watchUpdate. + await runUpdateCycle(containerTool, runtimeDir); if (opts.watch) { - // Fail fast here rather than relying on the first change-triggered cycle - // inside watchUpdate to discover this — a user starting `update --watch` - // against a stopped runtime should see the actionable error immediately, - // not only after they edit a source file. - await ensureRuntimeRunning(containerTool, runtimeDir); await watchUpdate(containerTool, runtimeDir); - } else { - await runUpdateCycle(containerTool, runtimeDir); } } +/** + * Container-events child processes (spawned by `waitForContainerEvent`) that + * are still running. Tracked so `watchUpdate`'s shutdown handler can kill + * them on SIGINT/SIGTERM instead of leaving them orphaned: `spawn()` puts + * children in the same process group as the parent by default, so a + * terminal Ctrl+C (which signals the whole foreground process group) takes + * them with it, but a direct `SIGTERM` to just this process's PID does not. + * Once `process.exit()` runs, this file's own JS-side timeout-driven + * `child.kill()` never gets a chance to fire either. + */ +const activeEventSubscriptions = new Set(); + /** * Subscribe to the container runtime event stream and resolve once a specific * service emits a specific event action, or when `timeoutMs` elapses. @@ -347,12 +359,17 @@ export async function waitForContainerEvent( ], { stdio: ['ignore', 'pipe', 'ignore'] }, ); + activeEventSubscriptions.add(child); + const finish = () => { + activeEventSubscriptions.delete(child); + resolve(); + }; const timer = timeoutMs > 0 ? setTimeout(() => { child.kill(); - resolve(); + finish(); }, timeoutMs) : undefined; @@ -369,13 +386,21 @@ export async function waitForContainerEvent( Action?: string; Status?: string; Attributes?: Record; + // Docker nests the compose-service label here instead of at the + // top level (verified against Docker's documented events JSON + // schema); Podman uses the top-level `Attributes` above + // (verified against real `podman events --format json` output). + Actor?: { Attributes?: Record }; }; const action = event.Action ?? event.Status ?? ''; - const svc = event.Attributes?.['com.docker.compose.service'] ?? ''; + const svc = + event.Attributes?.['com.docker.compose.service'] ?? + event.Actor?.Attributes?.['com.docker.compose.service'] ?? + ''; if (action === eventAction && svc === service) { if (timer !== undefined) clearTimeout(timer); child.kill(); - resolve(); + finish(); } } catch { // non-JSON line — ignore @@ -383,14 +408,24 @@ export async function waitForContainerEvent( } }); - child.on('error', () => { + child.on('error', err => { if (timer !== undefined) clearTimeout(timer); - resolve(); + Task.log( + `Warning: could not watch for '${containerTool} events' (${describeWatchError(err)}). Proceeding without confirming ${service}'s '${eventAction}' event.`, + ); + finish(); }); - child.on('close', () => { + child.on('close', code => { if (timer !== undefined) clearTimeout(timer); - resolve(); + // A null code means we killed it ourselves (SIGTERM) after already + // matching the event or hitting our own timeout — not a failure. + if (code !== null && code !== 0) { + Task.log( + `Warning: '${containerTool} events' exited unexpectedly (code ${code}). Proceeding without confirming ${service}'s '${eventAction}' event.`, + ); + } + finish(); }); }); } @@ -440,9 +475,20 @@ const ignoredWatchSegments = new Set([ * nothing, since chokidar no longer interprets `*` as a wildcard there. This * checks path segments directly instead, so output directories are actually * excluded rather than just documented as excluded. + * + * Chokidar always calls this with an absolute path. Segments are checked only + * within `root` (the plugin directory) — not the full absolute path — so a + * checkout that merely happens to live under an ancestor directory named + * e.g. `dist` or `node_modules` doesn't have every file ignored while still + * printing "Watching...". */ -export function isIgnoredWatchPath(filePath: string): boolean { - return filePath +export function isIgnoredWatchPath(filePath: string, root: string): boolean { + const relative = path.relative(root, filePath); + // Outside root entirely — chokidar shouldn't call us with these for our + // watched paths, but stay conservative and never ignore what we can't + // place relative to the plugin. + if (relative.startsWith('..') || path.isAbsolute(relative)) return false; + return relative .split(path.sep) .some(segment => ignoredWatchSegments.has(segment)); } @@ -475,7 +521,7 @@ function describeWatchError(err: unknown): string { * Watch source files and run a full update cycle on changes. * * Design: - * - Watches src/ and package.json. + * - Watches src/, package.json, and tsconfig.json. * - Excludes output directories (node_modules, dist, dist-dynamic, * dist-types) to prevent output-loop triggering. * - Events are debounced: the first event in a `debounceMs` window triggers @@ -498,19 +544,24 @@ export async function watchUpdate( debounceMs = 500, settleTimeoutMs = 15_000, ): Promise { + const root = paths.targetDir; const watchPaths = [ paths.resolveTarget('src'), paths.resolveTarget('package.json'), + // The only build-configuration file `plugin new` scaffolds (see + // adaptStandaloneProject); chokidar is fine watching a path that + // doesn't exist yet, so this is safe for projects without one. + paths.resolveTarget('tsconfig.json'), ]; Task.log( 'Watching for changes. Press Ctrl+C to stop.\n' + - ` Watching: src/, package.json\n` + + ` Watching: src/, package.json, tsconfig.json\n` + ` Ignored: node_modules/, dist/, dist-dynamic/, dist-types/`, ); const watcher = chokidar.watch(watchPaths, { - ignored: isIgnoredWatchPath, + ignored: filePath => isIgnoredWatchPath(filePath, root), ignoreInitial: true, persistent: true, }); @@ -525,6 +576,19 @@ export async function watchUpdate( running = true; pendingChange = false; const cycleStart = Date.now(); + // Start listening for this cycle's own compose stop/install actions' + // terminal event *before* runUpdateCycle runs them below — + // waitForContainerEvent only sees events emitted after the + // subscription begins, so subscribing afterward (once the actions have + // already run) almost always misses the event and burns the full + // settleTimeoutMs on every queued follow-up cycle. Only awaited below + // if a follow-up cycle turns out to be needed; otherwise left to + // resolve on its own (killed on shutdown via activeEventSubscriptions + // if the process exits first). + const settlePromise = + settleTimeoutMs > 0 + ? waitForContainerCleanup(containerTool, settleTimeoutMs) + : undefined; try { Task.log(`\n[watch] Change detected — starting update cycle...`); await runUpdateCycle(containerTool, runtimeDir, '[watch] '); @@ -535,18 +599,20 @@ export async function watchUpdate( `[watch] Update failed after ${Date.now() - cycleStart}ms: ${message}`, ); Task.log('[watch] Watching for further changes...'); - } finally { - running = false; } keepGoing = pendingChange; if (keepGoing) { Task.log( `[watch] Change received during cycle — waiting for runtime to settle...`, ); - if (settleTimeoutMs > 0) { - await waitForContainerCleanup(containerTool, settleTimeoutMs); - } + if (settlePromise) await settlePromise; } + // Cleared only now, after any settle-wait completes — not in a + // `finally` right after the cycle itself. Clearing it earlier let a + // change arriving during the settle-wait see `running === false` and + // start a second, concurrent drainCycles() instead of being coalesced + // into pendingChange for this same loop. + running = false; } }; @@ -574,6 +640,14 @@ export async function watchUpdate( if (debounceTimer !== undefined) { clearTimeout(debounceTimer); } + // Kill any in-flight `events` subscriptions (e.g. a pending settle-wait) + // explicitly. SIGINT gets these for free via the terminal's + // whole-process-group signal, but a direct SIGTERM to just this + // process's PID does not, and process.exit() below would otherwise + // leave them orphaned before their own JS-side timeout ever fires. + for (const child of activeEventSubscriptions) { + child.kill(); + } await watcher.close(); process.exit(0); }; From e8b503ff7fd19d1c17c50ee096840674c175ff28 Mon Sep 17 00:00:00 2001 From: Stan Lewis Date: Fri, 25 Sep 2026 15:36:40 -0400 Subject: [PATCH 09/11] fix(plugin-dev): address SonarCloud findings on settle-wait fix - typescript:S6544: `if (settlePromise) await settlePromise;` used a Promise value directly as a boolean condition. Changed to an explicit `!== undefined` check. - Duplication gate: the two new settle-wait tests (holds running through the settle wait / kills an in-flight settle-wait subscription on shutdown) shared a near-identical block that put watchUpdate into the settle-wait phase (capture spawned children, hold cycle 1 open, inject a second change, release, wait for both events subscriptions to spawn). Extracted enterSettleWait(), resolveSettleWait(), and restoreDefaultSpawnMock() helpers; both tests now call them instead of repeating the setup. Signed-off-by: Stan Lewis Assisted-By: opencode rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED --- src/commands/dev/command.test.ts | 200 ++++++++++++++----------------- src/commands/dev/command.ts | 2 +- 2 files changed, 92 insertions(+), 110 deletions(-) diff --git a/src/commands/dev/command.test.ts b/src/commands/dev/command.test.ts index 99f4211..12577e2 100644 --- a/src/commands/dev/command.test.ts +++ b/src/commands/dev/command.test.ts @@ -827,6 +827,86 @@ describe('watchUpdate', () => { } } + /** + * Start watchUpdate and drive it into the settle-wait phase: cycle 1 is + * held open, a second change arrives while it's still in-flight (setting + * pendingChange), cycle 1 is then released, and this resolves once the + * settle-wait's two events subscriptions (rhdh + install-dynamic-plugins) + * have spawned. Shared by tests that need to interact with that window — + * injecting further changes, resolving it, or triggering shutdown. + */ + async function enterSettleWait(settleTimeoutMs: number): Promise<{ + watchPromise: Promise; + settleChildren: FakeChildProcess[]; + spawnMock: jest.Mock; + }> { + const { spawn: spawnMock } = jest.requireMock('node:child_process') as { + spawn: jest.Mock; + }; + const settleChildren: FakeChildProcess[] = []; + spawnMock.mockImplementation(() => { + const child = new FakeChildProcess(); + settleChildren.push(child); + return child; + }); + + // Hold cycle 1 open so we can reliably inject a second change while + // `running` is still true. + let releaseCycle1: () => void = () => {}; + mockExport.mockImplementationOnce( + () => new Promise(resolve => (releaseCycle1 = resolve)), + ); + + const watchPromise = watchUpdate('podman', runtimeDir, 0, settleTimeoutMs); + + fakeWatcher.emit('all', 'change', 'src/a.ts'); + await waitForExportCalls(1); // cycle 1 started; still in-flight + + fakeWatcher.emit('all', 'change', 'src/b.ts'); + // Let the second change's own (0ms) debounce timer fire and observe + // running === true, setting pendingChange instead of starting a second + // drainCycles(). + await new Promise(resolve => setTimeout(resolve, 10)); + + releaseCycle1(); // cycle 1 finishes; keepGoing becomes true + + const spawnDeadline = Date.now() + 5000; + while (settleChildren.length < 2) { + if (Date.now() > spawnDeadline) { + throw new Error('Timed out waiting for settle-wait children to spawn'); + } + await new Promise(resolve => setImmediate(resolve)); + } + + return { watchPromise, settleChildren, spawnMock }; + } + + /** Emit the settle-wait's expected 'cleanup' event to each captured child. */ + function resolveSettleWait(settleChildren: FakeChildProcess[]) { + for (const child of settleChildren) { + const rhdhEvent = JSON.stringify({ + Status: 'cleanup', + Attributes: { 'com.docker.compose.service': 'rhdh' }, + }); + child.stdout.emit('data', Buffer.from(`${rhdhEvent}\n`)); + const installerEvent = JSON.stringify({ + Status: 'cleanup', + Attributes: { + 'com.docker.compose.service': 'install-dynamic-plugins', + }, + }); + child.stdout.emit('data', Buffer.from(`${installerEvent}\n`)); + } + } + + /** Restore spawn's default mock (tracking the shared `fakeChild`). */ + function restoreDefaultSpawnMock(spawnMock: jest.Mock) { + spawnMock.mockImplementation(() => { + fakeChild = new FakeChildProcess(); + return fakeChild; + }); + } + it('runs an update cycle when a file change event fires', async () => { // debounceMs=0 so the timer fires on the next event-loop tick. const watchPromise = watchUpdate('podman', runtimeDir, 0, 0); @@ -885,49 +965,12 @@ describe('watchUpdate', () => { // right after the cycle itself, before the optional settle-wait ran. A // change arriving during that wait saw `running === false` and called // drainCycles() again — a second, concurrent loop rather than a - // coalesced follow-up. Also verifies the settle-wait subscribes to the - // events stream *before* runUpdateCycle's own compose stop/start calls - // run (not after), by capturing spawned children and confirming both - // exist before cycle 2 begins. - const { spawn: spawnMock } = jest.requireMock('node:child_process') as { - spawn: jest.Mock; - }; - const settleChildren: FakeChildProcess[] = []; - spawnMock.mockImplementation(() => { - const child = new FakeChildProcess(); - settleChildren.push(child); - return child; - }); - - // Hold cycle 1 open so we can reliably inject a second change while - // `running` is still true. - let releaseCycle1: () => void = () => {}; - mockExport.mockImplementationOnce( - () => new Promise(resolve => (releaseCycle1 = resolve)), - ); - - const watchPromise = watchUpdate('podman', runtimeDir, 0, 5000); - - fakeWatcher.emit('all', 'change', 'src/a.ts'); - await waitForExportCalls(1); // cycle 1 started; still in-flight - - fakeWatcher.emit('all', 'change', 'src/b.ts'); - // Let the second change's own (0ms) debounce timer fire and observe - // running === true, setting pendingChange instead of starting a second - // drainCycles(). - await new Promise(resolve => setTimeout(resolve, 10)); - - releaseCycle1(); // cycle 1 finishes; keepGoing becomes true - - // The settle-wait's two subscriptions (rhdh + install-dynamic-plugins) - // should spawn before cycle 2 starts. - const spawnDeadline = Date.now() + 5000; - while (settleChildren.length < 2) { - if (Date.now() > spawnDeadline) { - throw new Error('Timed out waiting for settle-wait children to spawn'); - } - await new Promise(resolve => setImmediate(resolve)); - } + // coalesced follow-up. enterSettleWait() already proves the settle-wait + // subscribes to the events stream *before* runUpdateCycle's own compose + // stop/start calls run (not after), by capturing spawned children and + // confirming both exist before cycle 2 begins. + const { watchPromise, settleChildren, spawnMock } = + await enterSettleWait(5000); // While the settle-wait is unresolved, cycle 2 must not have started yet // — proving `running` stayed true and the pending change was coalesced @@ -945,21 +988,7 @@ describe('watchUpdate', () => { await new Promise(resolve => setTimeout(resolve, 10)); expect(mockExport).toHaveBeenCalledTimes(1); - // Resolve the settle-wait by emitting the expected event to each child. - for (const child of settleChildren) { - const event = JSON.stringify({ - Status: 'cleanup', - Attributes: { 'com.docker.compose.service': 'rhdh' }, - }); - child.stdout.emit('data', Buffer.from(`${event}\n`)); - const event2 = JSON.stringify({ - Status: 'cleanup', - Attributes: { - 'com.docker.compose.service': 'install-dynamic-plugins', - }, - }); - child.stdout.emit('data', Buffer.from(`${event2}\n`)); - } + resolveSettleWait(settleChildren); // Cycle 2 now runs, absorbing the coalesced pending change. await waitForExportCalls(2); @@ -968,11 +997,7 @@ describe('watchUpdate', () => { fakeWatcher.emit('error', new Error('done')); await expect(watchPromise).rejects.toThrow('done'); - // Restore the default mock implementation for subsequent tests. - spawnMock.mockImplementation(() => { - fakeChild = new FakeChildProcess(); - return fakeChild; - }); + restoreDefaultSpawnMock(spawnMock); }); it('continues watching after a failed update cycle', async () => { @@ -1081,47 +1106,8 @@ describe('watchUpdate', () => { // free, but SIGTERM to just this PID does not — and process.exit() // prevents waitForContainerEvent's own JS-side timeout from ever running // to kill them itself. shutdown() must kill any tracked child directly. - const { spawn: spawnMock } = jest.requireMock('node:child_process') as { - spawn: jest.Mock; - }; - const settleChildren: FakeChildProcess[] = []; - spawnMock.mockImplementation(() => { - const child = new FakeChildProcess(); - settleChildren.push(child); - return child; - }); - - // Hold cycle 1 open (mockExport won't resolve) so we can reliably inject - // a second change while `running` is still true, forcing pendingChange - // and, once cycle 1 is released, the settle-wait phase. - let releaseCycle1: () => void = () => {}; - mockExport.mockImplementationOnce( - () => new Promise(resolve => (releaseCycle1 = resolve)), - ); - - // settleTimeoutMs > 0 so drainCycles spawns the settle-wait listeners. - const watchPromise = watchUpdate('podman', runtimeDir, 0, 5000); - - fakeWatcher.emit('all', 'change', 'src/a.ts'); - await waitForExportCalls(1); // cycle 1 started; still in-flight (export pending) - - fakeWatcher.emit('all', 'change', 'src/b.ts'); - // Let the second change's own (0ms) debounce timer fire and observe - // running === true, setting pendingChange rather than starting a - // concurrent drainCycles(). - await new Promise(resolve => setTimeout(resolve, 10)); - - releaseCycle1(); // let cycle 1 finish; keepGoing becomes true - - // Wait for the settle-wait's two events subscriptions (rhdh + - // install-dynamic-plugins) to spawn. - const deadline = Date.now() + 5000; - while (settleChildren.length < 2) { - if (Date.now() > deadline) { - throw new Error('Timed out waiting for settle-wait children to spawn'); - } - await new Promise(resolve => setImmediate(resolve)); - } + const { watchPromise, settleChildren, spawnMock } = + await enterSettleWait(5000); const exitSpy = jest .spyOn(process, 'exit') @@ -1138,11 +1124,7 @@ describe('watchUpdate', () => { exitSpy.mockRestore(); void watchPromise; - // Restore the default mock implementation for subsequent tests. - spawnMock.mockImplementation(() => { - fakeChild = new FakeChildProcess(); - return fakeChild; - }); + restoreDefaultSpawnMock(spawnMock); }); it('update --watch deploys the current tree immediately, before entering watch mode', async () => { diff --git a/src/commands/dev/command.ts b/src/commands/dev/command.ts index b5e42a3..88dd951 100644 --- a/src/commands/dev/command.ts +++ b/src/commands/dev/command.ts @@ -605,7 +605,7 @@ export async function watchUpdate( Task.log( `[watch] Change received during cycle — waiting for runtime to settle...`, ); - if (settlePromise) await settlePromise; + if (settlePromise !== undefined) await settlePromise; } // Cleared only now, after any settle-wait completes — not in a // `finally` right after the cycle itself. Clearing it earlier let a From 5a5fb6f6f0eafba5dfd57cb58961565f838accbb Mon Sep 17 00:00:00 2001 From: Stan Lewis Date: Mon, 28 Sep 2026 05:58:58 -0400 Subject: [PATCH 10/11] fix(plugin-dev): address review nits on watch paths and event status fallback - Support lowercase status field on container events as an action fallback in waitForContainerEvent and add unit test coverage. - Update comment in scheduleUpdate to reflect the while-loop cycle draining mechanism instead of referring to a stale finally block. - Include tsconfig.json in watched files lists across AGENTS.md, CHANGELOG.md, and README.md. Assisted-By: opencode Signed-off-by: Stan Lewis rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED --- AGENTS.md | 2 +- CHANGELOG.md | 2 +- README.md | 2 +- src/commands/dev/command.test.ts | 17 +++++++++++++++++ src/commands/dev/command.ts | 8 +++++--- 5 files changed, 25 insertions(+), 6 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index c0440fe..0bf4a52 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -128,7 +128,7 @@ plugin code. `update` re-exports, re-stages, and then restarts the RHDH service. `start` and `update` both block on `waitForRhdhReady` (poll-based, 120s default timeout) before returning, printing the RHDH URL once it responds. Both also accept `--watch`, which hands off into `watchUpdate`: a chokidar watcher on -`src/` and `package.json` (500ms debounce, serialized cycles) that repeats the +`src/`, `package.json`, and `tsconfig.json` (500ms debounce, serialized cycles) that repeats the same export/stage/restart cycle as a one-shot `update` on every source change, so a source edit doesn't require re-running the command by hand. diff --git a/CHANGELOG.md b/CHANGELOG.md index a25d0ff..1106f3d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Added -- **`plugin dev`:** Add `--watch` to `rhdh-cli plugin dev start`, and a standalone `rhdh-cli plugin dev update --watch`, for continuous re-export/re-stage/restart on source changes ([RHIDP-16673](https://redhat.atlassian.net/browse/RHIDP-16673), [#222](https://github.com/redhat-developer/rhdh-cli/pull/222)). Watches `src/` and `package.json` with a 500ms debounce and serializes cycles so a change arriving mid-cycle queues exactly one follow-up; prints a refresh URL once RHDH responds. `plugin dev start` now also shows phased `[1/4]`–`[4/4]` progress through build/export, runtime start, plugin install, and readiness polling. `plugin dev update` and `plugin dev restart` fail fast with an actionable message when RHDH Local isn't running yet, instead of surfacing a raw compose/container error. +- **`plugin dev`:** Add `--watch` to `rhdh-cli plugin dev start`, and a standalone `rhdh-cli plugin dev update --watch`, for continuous re-export/re-stage/restart on source changes ([RHIDP-16673](https://redhat.atlassian.net/browse/RHIDP-16673), [#222](https://github.com/redhat-developer/rhdh-cli/pull/222)). Watches `src/`, `package.json`, `tsconfig.json` with a 500ms debounce and serializes cycles so a change arriving mid-cycle queues exactly one follow-up; prints a refresh URL once RHDH responds. `plugin dev start` now also shows phased `[1/4]`–`[4/4]` progress through build/export, runtime start, plugin install, and readiness polling. `plugin dev update` and `plugin dev restart` fail fast with an actionable message when RHDH Local isn't running yet, instead of surfacing a raw compose/container error. ## 2.1.1 - 2026-09-21 diff --git a/README.md b/README.md index 5f3c7b3..91fe1b1 100644 --- a/README.md +++ b/README.md @@ -98,7 +98,7 @@ rhdh-cli plugin dev update `update` and `restart` require the runtime to already be running — start it first with `plugin dev start`, or they fail fast with an actionable message instead of a raw compose/container error. Like `start`, `update` blocks until RHDH is reachable again (up to two minutes) and prints the URL on success. -Pass `--watch` to `start` or `update` to keep the CLI running: it watches `src/` and `package.json` and automatically re-exports, re-stages, and restarts the runtime on every change, so you don't have to re-run `update` by hand. +Pass `--watch` to `start` or `update` to keep the CLI running: it watches `src/`, `package.json`, and `tsconfig.json` and automatically re-exports, re-stages, and restarts the runtime on every change, so you don't have to re-run `update` by hand. ```bash rhdh-cli plugin dev start --watch diff --git a/src/commands/dev/command.test.ts b/src/commands/dev/command.test.ts index 12577e2..888ab5b 100644 --- a/src/commands/dev/command.test.ts +++ b/src/commands/dev/command.test.ts @@ -1477,6 +1477,23 @@ describe('waitForContainerEvent', () => { await expect(p).resolves.toBeUndefined(); }); + it('matches events using the lowercase status field as an action fallback', async () => { + const p = waitForContainerEvent('docker', 'rhdh', 'die', 5000); + await new Promise(resolve => setImmediate(resolve)); + fakeChild.stdout.emit( + 'data', + Buffer.from( + `${JSON.stringify({ + status: 'die', + Actor: { Attributes: { 'com.docker.compose.service': 'rhdh' } }, + })}\n`, + ), + ); + await new Promise(resolve => setImmediate(resolve)); + await expect(p).resolves.toBeUndefined(); + expect(fakeChild.kill).toHaveBeenCalled(); + }); + it('logs a warning and still resolves when the events command fails to spawn', async () => { const mockTask = Task as jest.Mocked; mockTask.log.mockClear(); diff --git a/src/commands/dev/command.ts b/src/commands/dev/command.ts index 88dd951..eb24973 100644 --- a/src/commands/dev/command.ts +++ b/src/commands/dev/command.ts @@ -385,6 +385,7 @@ export async function waitForContainerEvent( const event = JSON.parse(trimmed) as { Action?: string; Status?: string; + status?: string; Attributes?: Record; // Docker nests the compose-service label here instead of at the // top level (verified against Docker's documented events JSON @@ -392,7 +393,7 @@ export async function waitForContainerEvent( // (verified against real `podman events --format json` output). Actor?: { Attributes?: Record }; }; - const action = event.Action ?? event.Status ?? ''; + const action = event.Action ?? event.Status ?? event.status ?? ''; const svc = event.Attributes?.['com.docker.compose.service'] ?? event.Actor?.Attributes?.['com.docker.compose.service'] ?? @@ -621,8 +622,9 @@ export async function watchUpdate( debounceTimer = setTimeout(async () => { debounceTimer = undefined; if (running) { - // A cycle is active — record the intent and let the cycle's finally - // block start another one when it finishes. + // A cycle is active — record the intent so the while loop in + // drainCycles picks up another cycle when the current one + // (including any settle wait) finishes. pendingChange = true; return; } From ce9d1d6b84488911b3d470d4e688ca3b2065bb0f Mon Sep 17 00:00:00 2001 From: Stan Lewis Date: Mon, 28 Sep 2026 06:24:38 -0400 Subject: [PATCH 11/11] fix(plugin-new): pin @backstage/cli-defaults in resolutions for RHDH 2.1 profile Transitive resolution of @backstage/cli-defaults 0.1.6+ by @backstage/cli pulls in @backstage/cli-module-package-manager-yarn, which contains an unresolved Yarn patch locator (got@patch:got@npm%3A11.8.2#~/.yarn/patches/got-npm-11.8.2-c1eb105458.patch) causing yarn install to fail with ENOENT in scaffolded standalone plugin projects. Pinning @backstage/cli-defaults to 0.1.5 in resolutions prevents this transitive resolution and restores clean yarn install in generated projects. Assisted-By: opencode Signed-off-by: Stan Lewis rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED --- src/commands/new/__snapshots__/command.test.ts.snap | 9 ++++++--- src/commands/new/command.test.ts | 2 ++ src/commands/new/rhdhProfiles.ts | 3 +++ 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/src/commands/new/__snapshots__/command.test.ts.snap b/src/commands/new/__snapshots__/command.test.ts.snap index 869d678..5295e25 100644 --- a/src/commands/new/__snapshots__/command.test.ts.snap +++ b/src/commands/new/__snapshots__/command.test.ts.snap @@ -99,7 +99,8 @@ npx @red-hat-developer-hub/cli plugin export "packageManager": "yarn@4.17.1", "version": "0.1.0", "resolutions": { - "@types/express": "4.17.21" + "@types/express": "4.17.21", + "@backstage/cli-defaults": "0.1.5" } } ", @@ -297,7 +298,8 @@ npx @red-hat-developer-hub/cli plugin export "packageManager": "yarn@4.17.1", "version": "0.1.0", "resolutions": { - "@types/express": "4.17.21" + "@types/express": "4.17.21", + "@backstage/cli-defaults": "0.1.5" } } ", @@ -462,7 +464,8 @@ npx @red-hat-developer-hub/cli plugin export "packageManager": "yarn@4.17.1", "version": "0.1.0", "resolutions": { - "@types/express": "4.17.21" + "@types/express": "4.17.21", + "@backstage/cli-defaults": "0.1.5" } } ", diff --git a/src/commands/new/command.test.ts b/src/commands/new/command.test.ts index 2900857..841f829 100644 --- a/src/commands/new/command.test.ts +++ b/src/commands/new/command.test.ts @@ -124,7 +124,9 @@ describe('createPluginProject', () => { ); expect(packageJson.devDependencies.typescript).toBe('5.4.5'); expect(packageJson.resolutions['@types/express']).toBe('4.17.21'); + expect(packageJson.resolutions['@backstage/cli-defaults']).toBe('0.1.5'); expect(Object.keys(packageJson.resolutions).sort()).toEqual([ + '@backstage/cli-defaults', '@types/express', ]); expect( diff --git a/src/commands/new/rhdhProfiles.ts b/src/commands/new/rhdhProfiles.ts index 806e734..54f74bf 100644 --- a/src/commands/new/rhdhProfiles.ts +++ b/src/commands/new/rhdhProfiles.ts @@ -34,6 +34,9 @@ export const rhdhProfiles: Record = { // transitives follow the ranges published by upstream packages. // Newer express type packages are incompatible with the RHDH 2.1 toolchain. '@types/express': '4.17.21', + // Pin cli-defaults to prevent transitive resolution of 0.1.6+ which pulls in + // cli-module-package-manager-yarn with broken internal yarn patches. + '@backstage/cli-defaults': '0.1.5', }, // Keep frontend development dependencies out of backend and module projects. templateRoleOverlays: {