Fix lane launch ownership, privacy, and attach regressions#288
Conversation
|
Implementation status for exact head
GitHub CI is currently in progress. This PR must not be merged until CI is green and exact-head review remains valid. No release is included. The live Mac-mini/PTY/Discord canary is deferred to the post-review/merge release gate and has not been performed or claimed. — |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 833620003d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ( | ||
| LaneLaunchState::NoWorkerEffect, | ||
| "session-create-failed", | ||
| "lane tmux session creation failed", |
There was a problem hiding this comment.
When create_detached_lane_session returns PossiblyExecuted because wait_with_output() failed after spawning tmux, the detached session may already have been created, but this branch now records command lanes as NoWorkerEffect/session-create-failed and returns before marker verification or cleanup. In that response-loss case the registry says there was no worker effect while an unowned tmux session is left running, which blocks accurate retry/retirement decisions; split definite nonzero failures from true wait/result-loss ambiguity or keep the ambiguous handling for the latter.
Useful? React with 👍 / 👎.
| launch_lane_session(args, config).await?; | ||
| if attach { | ||
| attach_session(&requested_session).await?; |
There was a problem hiding this comment.
For clawhip tmux new --thread ... --attach from a real terminal, this new lane path calls attach_session, but that helper runs tmux attach-session with Command::output(), so stdout/stderr are captured instead of attaching the tmux client to the current terminal; tmux(1) describes attach-session as attaching “in the current terminal.” The fake tmux test does not catch this, and the command can now report a failed attach after a successful launch rather than actually attaching; spawn the attach command with inherited stdio/TTY for this path.
Useful? React with 👍 / 👎.
|
Exact-head gate update for
This is not the terminal release status. The live Mac-mini/PTY/Discord canary is deferred to the post-review/merge release gate and was not performed or claimed. No release has been performed. — |
|
MERGE_READY Reviewed replacement head: Adversarial dispositions:
Verification:
CI evidence: https://github.com/Yeachan-Heo/clawhip/actions/runs/29142384082 Squash merge is authorized for this exact clean head. Merged-dev dogfood and issue #287 closure remain post-merge gates. The live Mac-mini/PTY/Discord canary was not performed; release remains held. No release or publish action is authorized. — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a79ac9ac8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) -> DetachedSessionOutcome { | ||
| match result { | ||
| Ok(status) if status.success() => DetachedSessionOutcome::Created, | ||
| Ok(_) => DetachedSessionOutcome::DefinitiveFailure, |
There was a problem hiding this comment.
Fresh evidence: this helper now maps every non-success Ok(ExitStatus) to DefinitiveFailure, but an ExitStatus produced by a signal still arrives as Ok(_). If the tmux client is killed after it sends new-session to the server, the session may already exist; for command lanes this is then recorded as NoWorkerEffect/session-create-failed and returns before marker verification, leaving an unowned session with incorrect retry evidence. Only ordinary nonzero exit codes should be definitive, while signaled statuses should remain PossiblyExecuted.
Useful? React with 👍 / 👎.
|
MERGE_READY Post-merge verification completed from a clean detached worktree at exact Merged-dev evidence:
Release hold: this Linux verifier is not the authorized live Mac-mini environment and no live Discord credentials/target were used. The Mac-mini + real PTY + live Discord canary is the only remaining release gate. No release, tag, crate publish, artifact publish, or live Discord action was performed. — |
1 participant
Closes #287.
Summary
--attachthrough the existing attach helper after successful owned/finalized launches, with failure propagationRegression coverage
Validation
cargo fmt --checkcargo checkcargo clippy -- -D warningscargo test(679 unit tests plus all integration targets; lane suite 21/21)git diff --checkGates
Do not merge until exact-head GJC review and CI are green. The live Mac-mini/PTY/Discord canary remains a post-review/merge release gate and was not performed or claimed here. No release is included.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]