fix(installer): drop RETURN-trap leak and guard ctl deploy on template config#33
Conversation
…e config
Two follow-up fixes uncovered by the v0.1.0 fresh-install dry run:
- install.sh: remove the RETURN trap that referenced a function-local
`staging` variable. Bash's RETURN trap is process-wide, so it kept
firing on later function returns where `$staging` no longer existed
and printed "line 516: staging: unbound variable" after install
completed. Replace with explicit cleanup at end of install_release().
- bin/maestro-relay-ctl.sh: cmd_deploy now mirrors install.sh's
config_complete check, refusing with a friendly error when the .env
is empty or still has `your_*` template values. Previously it
invoked the deploy script directly and surfaced a Node stack trace
("Missing required env var: DISCORD_BOT_TOKEN") to the user.
Bump to 0.1.1.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughConfiguration validation is added to enforce complete Discord settings before deployment. The ChangesDeployment Validation & Installation Refinement
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
bin/maestro-relay-ctl.sh (2)
158-174: 💤 Low value
cmd_deployguard is correct and matches the PR's stated behavior.The validation correctly short-circuits with a user-friendly
diebefore attempting Node deployment. One small point: the error message at line 163 names$INSTALL_DIR/.env(the symlink), whileinstall.sh'sdeploy_commandsnames$CONFIG_DIR/.env(the canonical file). Both paths resolve to the same file, so this is transparent to the user, but pointing directly at$CONFIG_DIR(which is available in scope) would be slightly more consistent and avoid any confusion for users who inspect the path and find a symlink:- if ! config_complete "$env_file"; then - die "Config at $env_file is incomplete or contains template values. Edit it before running deploy." - fi + if ! config_complete "$env_file"; then + die "Config at $CONFIG_DIR/.env is incomplete or contains template values. Edit it before running deploy." + fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/maestro-relay-ctl.sh` around lines 158 - 174, The error message in cmd_deploy uses $INSTALL_DIR/.env which may be a symlink and differs from install.sh's messaging; update the die call in function cmd_deploy to reference the canonical $CONFIG_DIR/.env instead (use the existing env_file logic or replace $INSTALL_DIR with $CONFIG_DIR in the message) so that cmd_deploy, the require_install check, and the deployment validation consistently report the same canonical config path; ensure you still check the same file existence and keep the surrounding logic in cmd_deploy (enabled_providers parsing and dispatch to dist/providers/discord/deploy.js) unchanged.
145-156: 💤 Low value
config_completeimplementation is correct.The sed pattern correctly handles inline comments, empty values, and template placeholders. The
head -n1guard for duplicate keys is a nice touch.Minor observation: this function is byte-for-byte identical to
config_completeininstall.sh. Since both scripts are standalone executables that cannot reliablysourceeach other at runtime, the duplication is acceptable — but worth a comment noting that any future change to the validation logic (e.g., adding a new required key) must be applied to both files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/maestro-relay-ctl.sh` around lines 145 - 156, The function config_complete is duplicated in another script (install.sh); add a short inline comment above the config_complete function noting that this duplication is intentional because the scripts are standalone and reminding maintainers to apply any future validation changes (e.g., adding required keys) to both copies; reference the function name config_complete and the other script install.sh in the comment so reviewers know where the twin implementation lives.install.sh (1)
146-184: 💤 Low valueLGTM — correct fix for the process-wide RETURN-trap bug.
The explicit
rm -rf "$staging"at line 183 cleanly replaces the misfiring trap. One minor tradeoff worth acknowledging: if an earlier statement ininstall_release(e.g., thetarat line 150 or thecpat line 163) fails underset -Eeuo pipefail, control jumps to the ERR trap inmain(rollback_install) without passing through line 183, leaving themktemp -ddirectory behind. In practice this is harmless —/tmpis volatile — but if you want airtight cleanup a local EXIT/ERR handler scoped to the function body is the idiomatic approach:install_release() { local tag="$1" tarball="$2" local staging staging="$(mktemp -d)" + trap 'rm -rf "$staging"; trap - RETURN' RETURN tar -xzf "$tarball" -C "$staging" ... - rm -rf "$staging" ok "Extracted release to $INSTALL_DIR" }Note: Bash's RETURN trap fires once per function return (including error exits), so clearing it (
trap - RETURN) immediately inside the trap body prevents the re-firing issue that motivated this PR in the first place. This is entirely optional — the explicitrmis already a net improvement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@install.sh` around lines 146 - 184, install_release currently creates a staging dir with staging="$(mktemp -d)" but may leak it if the function exits early under set -Eeuo pipefail; add a local EXIT (or ERR) trap at the top of install_release that removes "$staging" (e.g., trap 'rm -rf "$staging"' EXIT) and clears itself before normal return to avoid RETURN-trap re-firing; ensure the trap references the staging variable and that the final explicit rm -rf "$staging" still runs or is made idempotent so cleanup always occurs even on error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@bin/maestro-relay-ctl.sh`:
- Around line 158-174: The error message in cmd_deploy uses $INSTALL_DIR/.env
which may be a symlink and differs from install.sh's messaging; update the die
call in function cmd_deploy to reference the canonical $CONFIG_DIR/.env instead
(use the existing env_file logic or replace $INSTALL_DIR with $CONFIG_DIR in the
message) so that cmd_deploy, the require_install check, and the deployment
validation consistently report the same canonical config path; ensure you still
check the same file existence and keep the surrounding logic in cmd_deploy
(enabled_providers parsing and dispatch to dist/providers/discord/deploy.js)
unchanged.
- Around line 145-156: The function config_complete is duplicated in another
script (install.sh); add a short inline comment above the config_complete
function noting that this duplication is intentional because the scripts are
standalone and reminding maintainers to apply any future validation changes
(e.g., adding required keys) to both copies; reference the function name
config_complete and the other script install.sh in the comment so reviewers know
where the twin implementation lives.
In `@install.sh`:
- Around line 146-184: install_release currently creates a staging dir with
staging="$(mktemp -d)" but may leak it if the function exits early under set
-Eeuo pipefail; add a local EXIT (or ERR) trap at the top of install_release
that removes "$staging" (e.g., trap 'rm -rf "$staging"' EXIT) and clears itself
before normal return to avoid RETURN-trap re-firing; ensure the trap references
the staging variable and that the final explicit rm -rf "$staging" still runs or
is made idempotent so cleanup always occurs even on error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 53dcedd8-15a8-46bd-9c2a-9e4a119a9b29
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
bin/maestro-relay-ctl.shinstall.shpackage.json
Brings in installer bugfixes (PRs #33-#35), .maestro/ untrack (#36), and the docs split (#37) that landed on main after rc was branched. Conflict resolutions: - Kept rc's queue error-hiding (PR #39) and per-message logger.error. - Kept rc's WAL-checkpoint shutdown alongside main's split DB import. - Combined main's docs split with rc's Slack provider additions in README.md, AGENTS.md, .env.example, install.sh, providers.ts, migrations.ts. - Extended config_complete in maestro-relay-ctl.sh to cover the Slack key set when ENABLED_PROVIDERS=slack.
1 participant
Summary
Two follow-ups discovered during a fresh-install dry run of v0.1.0 in a sandboxed
\$HOME.install.sh— drop the RETURN trap on\$stagingcleanup, replace with explicitrm -rfat the end ofinstall_release(). Bash's RETURN trap is process-wide, not function-local, so it kept firing on later function returns where\$staginghad gone out of scope and printedline 516: staging: unbound variableafter install completed.bin/maestro-relay-ctl.sh— `cmd_deploy` now mirrors `install.sh`'s `config_complete` check, refusing with a friendly error when the `.env` is empty or still has `your_*` template values. Previously it surfaced a Node stack trace (Missing required env var: DISCORD_BOT_TOKEN) to the user.Bumps to `0.1.1` (patch).
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Chores