Conversation
Implements `rhdh-cli plugin dev [start|stop|update|logs|status]` to manage a plugin's lifecycle against an existing RHDH Local checkout. - start: exports and stages the plugin into local-plugins/, writes rhdh-cli.generated.yaml, and brings up the Compose runtime - update: re-exports, re-stages, restarts the installer and RHDH service without tearing down the full runtime - stop: stops Compose services; --clean also removes containers and networks (volumes and config are preserved) - logs: streams Compose logs; --installer shows installer output, --rhdh shows RHDH output, --all includes all services - status: interprets installer exit code and RHDH service state into a human-readable status message - --configure: non-invasively adds the CLI-managed config include to dynamic-plugins.override.yaml on first use Staging uses fs.remove + dereference:false copy to handle relative symlinks in node_modules/.bin without errors on repeated runs. Also fixes two issues discovered during live testing: - plugin new: inject version '0.1.0' in adaptStandaloneProject so upstream templates (which omit version) produce packages that npm pack can handle in the RHDH Local installer - plugin export: add preflight version checks in both backend.ts and frontend.ts that fail fast with a clear message when package.json is missing a version field, rather than letting npm pack fail inside the installer container with a cryptic error - plugin export: ensureDir before writing config schema in command.ts Assisted-By: OpenCode Signed-off-by: Stan Lewis <[email protected]> rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 7:05 PM UTC · Completed 7:31 PM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-4-6 · Effort: high · Cost: $6.42 |
|
Risk Assessment: moderate (2/5) DetailsTier 1 and Tier 2 signals are unchanged from the prior review — the same large blast radius (12 files, 1008 lines) is offset by zero security-sensitive paths, no CI or dependency changes, and a non-first-time human author; the only delta since the prior review is bug-fix commits on the new dev/ files which still carry no git history, leaving the composite at 2/moderate. Previous runRisk Assessment: moderate (2/5) DetailsTier 1 sub-score ~2.1 (weighted 62%): large blast radius and 989 lines pull upward, but no security-sensitive paths, no CI changes, no dependency files, and a normal human non-first-time author profile keep the average low. Tier 2 sub-score ~2 (weighted 38%): three new dev/ files have no prior history; export-dynamic-plugin files show modest fix activity but no regressions against the PRs changed lines. No Tier 3 (Jira issue not accessible). Composite rounds to 2, unchanged from prior review. Previous run (2)Risk Assessment: moderate (2/5) DetailsScore drops from prior 3 to 2 because Tier 2 history is clean — the three new dev/ files have no regression history, zero reverts across all changed files, and low average churn — partially offset by large blast radius and below-threshold test-file ratio. Previous run (3)Risk Assessment: moderate (2/5) DetailsModerate risk: a well-structured new feature addition by an experienced contributor with unit tests and no dependency or CI changes, offset by a large blast radius classification and modifications to export-dynamic-plugin files that carry significant fix/revert history. |
ReviewFindingsMedium
Low
Next steps:
Previous runReviewFindingsMedium
Low
Next steps:
Previous run (2)ReviewFindingsMedium
Low
Next steps:
Previous run (3)ReviewFindingsMedium
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Add Apache 2.0 license headers to command.ts and index.ts
- Move YAML import to third-party import group
- Use replaceAll('/', '-') in stagePlugin for complete scope segment replacement
- Filter non-JSON lines and add try/catch in parseComposeStatus
- Log a warning in updateGeneratedConfig when overwriting a different plugin entry
- Add --follow flag to plugin dev logs for streaming output
- Add CHANGELOG entries for plugin dev command and breaking plugin export changes
Assisted-By: claude-sonnet-4-6@default
rh-pre-commit.version: 2.4.0
rh-pre-commit.check-secrets: ENABLED
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
gashcrumb
left a comment
gashcrumb
left a comment
There was a problem hiding this comment.
Addressed all fullsend findings in the follow-up commit b9e8f9b. Summary below; inline comments cover each thread.
Fixed: license headers, YAML import order, replaceAll, parseComposeStatus hardening, overwrite warning, --follow flag for logs, CHANGELOG entries.
Dismissed: the actionsToRun edge-case note (functional by design — composeArgs throws a clear error for unknown actions).
…tory SonarCloud flagged fs.chmod(destination, 0o755) as setting world-accessible permissions. The chmod was applied only to the top-level directory and was speculative — fs.copy preserves source permissions from dist-dynamic, which the export process already creates with appropriate modes. Remove it. Assisted-By: claude-sonnet-4-6@default rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
Assisted-By: claude-sonnet-4-6@default rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
- Use node:path import prefix (typescript:S7772) - Extract nested template literal expressions into local variables in formatRuntimeStatus (typescript:S4624, lines 350/356/359) Assisted-By: claude-sonnet-4-6@default rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
Run lint, prettier, and tsc before pushing to a PR branch. Assisted-By: claude-sonnet-4-6@default rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
…bcommands Each plugin dev action (start, update, stop, logs, status) is now a proper Commander subcommand with only the flags that apply to it: - start: --rhdh-local-dir, --container-tool, --configure - update: --rhdh-local-dir, --container-tool - stop: --rhdh-local-dir, --container-tool, --clean - logs: --rhdh-local-dir, --container-tool, --rhdh, --installer, --all, --follow - status: --rhdh-local-dir, --container-tool This makes plugin dev -h list all available subcommands and prevents nonsensical option combinations like plugin dev status --configure or plugin dev start --clean. command.ts exports one handler per subcommand plus all shared helpers. actionsToRun() is removed; each handler directly invokes the compose actions it needs. AGENTS.md documents the subcommand convention so future command groups follow the same pattern. Assisted-By: claude-sonnet-4-6@default rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 1:46 PM UTC · Completed 2:05 PM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-4-6 · Effort: high · Cost: $4.22 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
In a Backstage monorepo workspace the tsconfig.json sits at the workspace root (e.g. workspaces/todo/) with outDir: dist-types and rootDir: '.', so tsc writes types to <workspaceRoot>/dist-types/<relative-plugin-path>/ rather than inside the plugin directory itself. validateProjectFiles() was always checking <pluginDir>/dist-types, which never exists in a monorepo. Introduce resolveDistTypes() which discriminates on paths.targetDir vs paths.targetRoot: - targetDir === targetRoot (standalone): dist-types is inside the plugin dir - targetDir !== targetRoot (monorepo workspace): dist-types is at <targetRoot>/dist-types/<relative-path-from-root-to-plugin> Three new tests cover: standalone resolution, monorepo path calculation, and the full validateProjectFiles() flow against a simulated monorepo layout. Assisted-By: claude-sonnet-4-6@default rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
- composeArgs: replace four positional boolean params with a LogsOptions object for call-site clarity; update all callers and tests - composeStatusArgs: pass --all unconditionally (both Docker Compose and Podman Compose >=1.3 accept it); fixes potential misreporting on Podman when the installer container exits before status is queried - parseComposeStatus: wrap the array-format JSON.parse branch in try/catch, consistent with the JSON Lines branch - stagePlugin: guard against a crafted package name (e.g. '..') resolving the destination outside local-plugins/ before calling fs.remove - import order: move node:path below third-party imports, matching sibling files that give Node built-ins their own group - Remove box-drawing section separator comments; use plain blank lines between logical groups to stay consistent with the rest of src/commands/ - README: document that the default container tool is podman and how to switch to docker Assisted-By: claude-sonnet-4-6@default rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
…setup SonarCloud reported 3.4% duplicated lines on the PR (threshold: 3%) due to near-identical setup/teardown in the two stagePlugin tests. Extract a shared withStagingDirs() helper that creates srcDir/runtimeDir, writes package.json, sets the test global, and handles cleanup in a finally block. Assisted-By: claude-sonnet-4-6@default rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 2:50 PM UTC · Completed 3:08 PM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-4-6 · Effort: high · Cost: $5.14 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- stagePlugin: extend path-traversal guard to also reject empty or
identity-resolving relative paths (e.g. package name '@' strips to '',
which path.relative returns as '' — the same issue as '.'). Guard now
checks !relative || relative === '.' in addition to the existing
startsWith('..') and isAbsolute checks. Test covers both '..' and '@'.
- formatRuntimeStatus: add undefined check before comparing exitCode to
'0'. Previously an exited installer container with no ExitCode field
(exitCode returns undefined) would satisfy undefined !== '0' and be
misreported as a plugin installation failure. Guard is now:
exitCode !== undefined && exitCode !== '0'. Test covers this case.
Assisted-By: claude-sonnet-4-6@default
rh-pre-commit.version: 2.4.0
rh-pre-commit.check-secrets: ENABLED
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 4:49 PM UTC · Completed 5:07 PM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-4-6 · Effort: high · Cost: $4.72 |
There was a problem hiding this comment.
Note: The following review comments could not be posted on the diff (GitHub returned 422) and are included here instead:
README.md(file-level): Line 128 · [medium] stale-doc
The ## Commands → ### Plugin Development Commands bullet list still omits plugin dev. The PR added a prose section documenting usage but the command index was not updated. A reader consulting the Commands section will find no entry for the new subcommand group (start, update, stop, logs, status).
Suggested fix: Add a bullet to the Plugin Development Commands list: - \plugin dev`: Export a dynamic plugin and manage its lifecycle against an existing RHDH Local runtime (`start`, `update`, `stop`, `logs`, `status`)`
…uacy restart: - New 'plugin dev restart' subcommand stops and restarts the RHDH service without re-exporting or re-staging the plugin. Useful when changing RHDH Local configuration (app-config, override YAML, etc.) without touching plugin code. 'plugin dev update' remains the command for plugin code changes. - Registered in src/commands/index.ts alongside the other dev subcommands. - Documented in README, CHANGELOG, and AGENTS.md. stale-doc (fullsend): - Add 'plugin dev' bullet (including 'restart') to the Commands index in README. - Update CHANGELOG and README prose to include 'restart' in the subcommand list. test-adequacy (fullsend): - Add test for parseComposeStatus JSON array branch (Podman Compose format). - Add test for malformed JSON array input triggering the try/catch error path. Assisted-By: claude-sonnet-4-6@default rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
|
Addressed in [stale-doc] Added [test-adequacy] Added two [protected-path] The AGENTS.md changes are intentional — they document the [feat] Also added |
podman-compose ps does not support --all and exits with an error when it is passed. The earlier change to pass --all unconditionally broke plugin dev status and plugin dev restart for podman users. Restore the original discriminator: --all is added only for docker, where compose ps requires it to include exited containers. podman-compose includes exited containers by default (verified with 1.6.0) so the flag is omitted. composeStatusArgs() takes containerTool again. Update the call site in getRuntimeStatus and add a test for the podman variant alongside the existing docker test. Assisted-By: claude-sonnet-4-6@default rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
|
| updateGeneratedConfig, | ||
| } from './command'; | ||
|
|
||
| describe('plugin dev', () => { |
There was a problem hiding this comment.
The argv/validation tests are in good shape, but none of the lifecycle handlers actually hit run/execFile, so CI wouldn't catch a regression that swallows ExitCodeError. Can we mock those and assert stop/status still reject when the child process fails?
| async function resolveAndValidate(opts: OptionValues) { | ||
| const runtimeDir = await validateRuntime( | ||
| resolveRuntimeDir(opts.rhdhLocalDir), | ||
| ); | ||
| const containerTool = validateContainerTool(opts.containerTool); | ||
| return { runtimeDir, containerTool }; | ||
| } |
There was a problem hiding this comment.
validateContainerTool catches a bad --container-tool value, but we never check that podman/docker is actually on PATH. A missing engine currently shows up as spawn podman ENOENT. Are we able to do the same --version preflight as plugin package and tell people to pass --container-tool docker if that is what they use?
| 'prepare-and-install-dynamic-plugins.sh', | ||
| 'wait-for-plugins-and-start.sh', | ||
| ]; | ||
| const generatedConfig = 'configs/dynamic-plugins/rhdh-cli.generated.yaml'; |
There was a problem hiding this comment.
rhdh-local gitignores *.local.yaml and local-plugins/*, but not *.generated.yaml, so this file shows up in git status after a successful start. Can we name it rhdh-cli.generated.local.yaml so we stay off the checkout's working tree without needing an rhdh-local change?
| export async function start(opts: OptionValues) { | ||
| const { runtimeDir, containerTool } = await resolveAndValidate(opts); | ||
| await validateProjectFiles(); | ||
| await ensureGeneratedConfigIncluded(runtimeDir, opts.configure); | ||
| await exportCommand({ build: true, install: true }); | ||
| await stagePlugin(runtimeDir); | ||
| await compose(containerTool, runtimeDir, composeArgs('start')); | ||
| } |
There was a problem hiding this comment.
Nit: update and restart finish with getRuntimeStatus, but start just returns after up -d. Can we log the same interpreted status here so a failed installer is obvious without a follow-up plugin dev status?
| - `command.ts` — per-subcommand handlers (`start`, `update`, `stop`, `logs`, | ||
| `status`) plus all shared helpers: runtime validation, config management, | ||
| plugin staging, Compose argument builders, and status formatting. | ||
| - `index.ts` — re-exports the five handlers for lazy-loading via | ||
| `src/commands/index.ts`. | ||
|
|
||
| **Runtime contract:** `plugin dev` requires an explicit RHDH Local checkout via | ||
| `--rhdh-local-dir <path>` or the `RHDH_LOCAL_DIR` environment variable. It | ||
| validates the presence of `compose.yaml`, `compose-dynamic-plugins-root.yaml`, | ||
| `prepare-and-install-dynamic-plugins.sh`, and `wait-for-plugins-and-start.sh`. | ||
| It never mutates the checkout's Git state or user-owned configuration files. |
There was a problem hiding this comment.
Nit: this is six handlers now (restart is missing from the command.ts list too). Same section, “never mutates … user-owned configuration files” is a bit strong given --configure appends to the override YAML, maybe “never touches Git-tracked files” instead?
| 'package.json', | ||
| )} before exporting as a dynamic plugin.`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Nit: Can we include a small test that a versionless package.json now fails with this message to prevent it from going silent later. Same with the frontend update as well.
| export async function update(opts: OptionValues) { | ||
| const { runtimeDir, containerTool } = await resolveAndValidate(opts); | ||
| await validateProjectFiles(); | ||
| await exportCommand({ build: true, install: true }); | ||
| await stagePlugin(runtimeDir); | ||
| for (const action of ['install-dynamic-plugins', 'stop-rhdh', 'start-rhdh']) { | ||
| await compose(containerTool, runtimeDir, composeArgs(action)); | ||
| } | ||
| Task.log(await getRuntimeStatus(containerTool, runtimeDir)); | ||
| } |
There was a problem hiding this comment.
Nit: start checks that the generated include is in the override file, but update will export, stage, and bounce the installer even when it is missing. Can we call ensureGeneratedConfigIncluded(runtimeDir, false) here so that case fails with the existing --configure hint instead of a silent no-op?
| await fs.writeFile( | ||
| file, | ||
| YAML.stringify({ | ||
| plugins: [ | ||
| { | ||
| package: pluginPackage, | ||
| disabled: false, | ||
| pullPolicy: 'Always', | ||
| }, | ||
| ], | ||
| }), |
There was a problem hiding this comment.
Nit: this always rewrites the generated file to just package / disabled / pullPolicy, so any pluginConfig someone adds here is gone on the next update. Worth a README note that extra config belongs in dynamic-plugins.override.yaml under plugins: for the same package?
4 participants
Summary
Implements
rhdh-cli plugin dev(RHIDP-16672) — a command that exports a dynamic plugin and manages its lifecycle against an existing RHDH Local checkout.Commands
plugin dev startplugin dev updateplugin dev stop--cleanalso removes containers and networksplugin dev logs--rhdhfor RHDH output,--installerfor installer outputplugin dev status--rhdh-local-dir <path>orRHDH_LOCAL_DIRpoints at an existing RHDH Local checkout.--configurenon-invasively adds the CLI-managed config include todynamic-plugins.override.yamlon first use.Runtime contract
local-plugins/<name>/for the RHDH Local installer'snpm packpathconfigs/dynamic-plugins/rhdh-cli.generated.yamlstop --cleanusescompose down— volumes and config are always preservedAlso fixes
plugin new: inject"version": "0.1.0"inadaptStandaloneProject— upstream standalone templates omitversion, butnpm pack(used by the RHDH Local installer) requires itplugin export: preflight check in bothbackend.tsandfrontend.tsthat fails fast with a clear message whenpackage.jsonis missing aversionfield, rather than lettingnpm packfail inside the installer containerplugin export:ensureDirbefore writing the config schema incommand.tsstagePluginusesfs.remove+fs.copy({ dereference: false })to preserve relative symlinks innode_modules/.bin/and avoid self-copy errors on repeatedupdaterunsTesting
plugin dev start→plugin dev update→plugin dev stopagainst a real RHDH Local instance runningquay.io/rhdh-community/rhdh:nextensureGeneratedConfigIncluded,updateGeneratedConfig,stagePluginsymlink handling,validateProjectFiles(backend requiresdist-types, frontend does not), andformatRuntimeStatusCloses #RHIDP-16672