Repository navigation
fix(attach): launch goose with GOOSE_MODE=auto under --dangerous instead of the nonexistent --non-interactive flag - #363
Conversation
…ead of the nonexistent --non-interactive flag Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com>
kubestellar-prow
Bot
added
the
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
kubestellar-prow
Bot
added
the
size/M
|
hive/awaiting-ci: CI pending — sweep will merge when green. 🐝 Hive Agent: — hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88 |
hivecommons-hive
Bot
added
author/hive
![hivecommons-hive[bot]](https://avatars.githubusercontent.com/in/3568013?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed (security) — no exploitable findings. GOOSE_MODE=auto is a hardcoded constant (src/attach.ts:177), not user-derived, and --dangerous is already an explicit opt-in, so no trust-boundary change.
Advisory (low, non-blocking): the existing-session branch at src/attach.ts:364-369 still gates on resolveDangerousFlag(cli) only, so for goose with --dangerous against a live session no warning or restart hint is printed (and the hint template buildCliCommand(cliCmd, dangerFlag) can't carry an env prefix). Worth a follow-up so goose users get the same "session already exists, approvals may not be enabled" notice as other CLIs.
— hive: agent=ci-maintainer backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Reviewed (intent-alignment) — diff implements exactly what #362 proposed (DANGEROUS_ENV prefix, README row, matrix + orchestration tests, changelog fragment); no scope creep.
Advisory (non-blocking): removing goose from DANGEROUS_FLAGS also silences the existing-session path. src/attach.ts:371-376 gates the "--dangerous was set but session already exists" warning on resolveDangerousFlag(cli) only, so pluk attach --cli=goose --dangerous against a live session now emits no warning at all (it did before, albeit naming the bogus flag). Consider if (dangerFlag || dangerEnv) there, with the restart hint prefixed by dangerEnv.
— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Style review (head 5bb5a72)
- Existing-session branch not updated to the new env-prefix mapping —
src/attach.ts:363-369still resolves onlyresolveDangerousFlag(cli). Since goose was removed fromDANGEROUS_FLAGS(src/attach.ts:164-172),pluk attach --cli=goose --dangerousagainst an already-running session now emits no "--dangerous was set but session already exists" warning at all (before this PR it warned, albeit naming the wrong flag), and the restart hint on line 368 has no way to includeGOOSE_MODE=auto. The twodangerouslySkipPermissionsbranches now encode the mapping differently, which is the kind of drift the issue's "parallelDANGEROUS_ENV" design was meant to avoid. Suggested fix: computedangerEnvin the else-branch too, warn whendangerFlag || dangerEnv, and prefix the hint command withdangerEnv(e.g.${dangerEnv ? dangerEnv + ' ' : ''}${buildCliCommand(cliCmd, dangerFlag)}), plus an orchestration test mirroringtest/attach-orchestration.test.js:196-203for goose.
Nit (non-blocking): test/cli-support-matrix.test.js:136 failure message still says "DANGEROUS_FLAGS has no flag for …" even though the test now also accepts DANGEROUS_ENV.
— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
Hive filed review findings that do not block this PR as backlog items:
Cap: 10 backlog item(s) per PR per day. |
There was a problem hiding this comment.
Reviewed (correctness) — fix is correct; one advisory gap.
src/attach.ts:345-358:GOOSE_MODE=auto goose sessionis prepended on the tmuxsend-keysshell line, so it lands as an env assignment for the pane shell — matches the #362 expected command. Verified at 5bb5a72:test/attach-orchestration.test.js+test/cli-support-matrix.test.jspass (46/46) and both parsers (recordEntriesL96,readmeDangerousFlagsL107) pick up the newDANGEROUS_ENVrecord and README row.- Advisory (low): the existing-session branch at
src/attach.ts:362-369still only checksresolveDangerousFlag(cli). For goose that is now'', sopluk attach --cli=goose --dangerousagainst an already-running session prints no "session already exists, CLI may not have … enabled" warning at all (before this PR it warned, citing the wrong flag). Considerconst dangerHint = dangerFlag || dangerEnvthere so the warning (and restart hint) still fire for env-driven CLIs.
— hive: agent=adjudicator backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
docs-currency — one in-scope gap between README promise and behavior (read at head 5bb5a72).
- Existing-session
--dangerouswarning/restart hint silently dropped for goose —src/attach.ts:364-369still gates the "session already exists" warning and the restart command onresolveDangerousFlag(cli)alone. With goose removed fromDANGEROUS_FLAGS(src/attach.ts:164-172),dangerFlagis''for goose, sopluk attach <existing> --cli=goose --dangerousnow prints nothing — whileREADME.md:55still promises "pluk will print a warning with the command to restart the CLI with the right flag." Before this PR goose at least got a (wrong) hint; now it gets none, and the README text is no longer true for goose. Fix: gate ondangerFlag || dangerEnvand build the hint as${dangerEnv} ${buildCliCommand(cliCmd, dangerFlag)}(shell-quoted) so the restart command isGOOSE_MODE=auto goose session; add an existing-session goose case next totest/attach-orchestration.test.js:198.
README --dangerous row (README.md:133), changelog fragment, and the support-matrix test union otherwise track the new behavior.
— hive: agent=adjudicator backend=copilot model=claude-fable-5.1 copilot=1.0.88
hivecommons-hive
Bot
added
stage/070-fixing
…EROUS_ENV The existing-session branch only consulted resolveDangerousFlag(), so once goose moved to DANGEROUS_ENV it silently lost its "session already exists" warning and restart hint. Gate on dangerFlag || dangerEnv and prefix the restart hint with the env assignment (GOOSE_MODE=auto goose session). Add an existing-session goose orchestration test and update the stale support-matrix assertion message to mention DANGEROUS_ENV. Hive-Fix-Attempt: 1/6 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com>
hivecommons-hive
Bot
added
stage/050-review-pending
There was a problem hiding this comment.
Reviewed (correctness) — approve; one advisory.
Verified at head 1223fd5: src/attach.ts:345-349 prepends GOOSE_MODE=auto to the shell-quoted launch command and :353 only warns when neither flag nor env exists; the existing-session restart hint (:364-372) carries the same prefix. Built and ran test/attach-orchestration.test.js + test/cli-support-matrix.test.js at head: 47/47 pass.
- Advisory (low):
test/cli-support-matrix.test.js:67-72("every CLI with a --dangerous flag is also launchable via CLI_COMMANDS") still iterates onlyDANGEROUS_FLAGS; the two sibling guards at:116-117and:134-135were widened to includeDANGEROUS_ENVbut this one was not, so a futureDANGEROUS_ENVentry for a non-launchable CLI would slip past. One-line fix: iterate[...recordKeys(attachSource,'DANGEROUS_FLAGS'), ...recordKeys(attachSource,'DANGEROUS_ENV')].
— hive: agent=adjudicator backend=copilot model=claude-fable-5.1 copilot=1.0.88
Widens the CLI_COMMANDS launchability guard to cover DANGEROUS_ENV entries, matching the two sibling guards that already include them. Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com>
|
hive/awaiting-ci: widened the CLI_COMMANDS launchability guard to cover DANGEROUS_ENV entries (review advisory). CI pending — sweep will merge when green. 🐝 Hive Agent: — hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88 |
There was a problem hiding this comment.
Read src/attach.ts at f6a11c2 plus the matrix/orchestration tests; this looks correct to me.
- The env-prefix form matches the existing idiom in
buildPipePaneCommand(src/attach.ts:124,PLUK_RUN_DIR=…), so it adds no new shell assumption. - Both launch paths are covered: new-session (
src/attach.ts:348-355) prependsGOOSE_MODE=autoand only warns when neither a flag nor an env exists; the existing-session hint (src/attach.ts:366-372) now carries the env in both the label and the quoted restart command, and the new orchestration test pins that output. - Upstream check: goose's
Config::get_paramreadsenv::var(key.to_uppercase())before the config file (block/goose crates/goose/src/config/base.rs:773-777), andgoose configureitself prints that a setGOOSE_MODEenv "will override the configuration" (crates/goose-cli/src/commands/configure.rs:1542);GooseMode::Autois the first option inselect_goose_mode. SoGOOSE_MODE=autois a real auto-approve switch, not a guess. cli-support-matrix.test.jsrecordKeys/recordEntriesregex matches the newconst DANGEROUS_ENV: Record<string, string> = {…};literal, so the README-sync and no-silent-no-op gates now cover the env map too.
No findings. CI (test 22/24/26) is green.
— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
Thank you for your contribution! Your PR has been merged. We'd love to hear how your experience was: share feedback |
hivecommons-hive
Bot
removed
role/trusted
Closes #362
Summary
Tests
Not run locally; CI is the verdict.
— hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88
Closes #364 (stale support-matrix assertion message refreshed in 1223fd5)