fix(workflow): stop e2e-slot mangling the stack session name - #4836
Merged
Conversation
basename emits a trailing newline, and tr -c 'A-Za-z0-9_-' '_' rewrote it to an underscore, so stack_session always returned a name with a spurious trailing '_' that $(...) could no longer strip. Consequences, all silent: - stop_stack looked up the wrong tmux session, found nothing and returned early, so 'release' never actually stopped the worktree's dev stack -- the guarantee the slot/stack unification was added to provide. Stacks outlived their slots. - status always printed 'stack=none' even with the stack up. - stack_is_covered never matched on the recorded worktree, leaving only the owner-name-prefix fallback. A slot whose owner name does not start with the worktree basename made a live stack look uncovered, so 'stacks --reap' would pnpm dev:stop a running E2E. Keeping \n in the preserved set fixes all three; other unsafe characters are still replaced.
Contributor
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryThe one-line fix correctly keeps Files Reviewed (1 files)
Reviewed by claude-sonnet-5 · Input: 12 · Output: 2.4K · Cached: 200.8K Review guidance: REVIEW.md from base branch |
iscekic
enabled auto-merge (squash)
July 28, 2026 16:42
jeanduplessis
approved these changes
Jul 28, 2026
iscekic
added a commit
that referenced
this pull request
Jul 28, 2026
…stack (#4842) stop_stack set a global $wt, then called stack_is_covered, whose loop reassigns the same global once per slot directory. On return $wt held the LAST slot's worktree, so '(cd $wt && pnpm dev:stop)' stopped whichever section sorted last. $sess stayed correct, so the has-session guard confirmed our own stack was up and then stopped someone else's. Observed live: releasing file-preview-2975's slot killed kilo-dev-hermes-mem-c716 while leaving kilo-dev-file-preview-2975 up, with that section's verifier still mid-round. dev:stop is not at fault - it resolves its target from 'git rev-parse --show-toplevel' in the process cwd. It was handed the wrong cwd. Latent until #4836: stack_session previously returned a mangled name, so has-session always failed and stop_stack returned before the cd. Fixing the name made the guard pass and exposed the shadowing. Declares local in stack_is_covered, stop_stack and reap, and records the incident under learnings/.
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
stack_session()sanitises a worktree basename withtr -c 'A-Za-z0-9_-' '_'.basenameemits a trailing newline, the newline is outside the preserved set, sotrrewrites it to_— and$(...)can no longer strip it. Every session name came back with a spurious trailing underscore:Why it matters
The slot/stack unification promises that
releasefrees the slot and stops the worktree's stack, so a stack can never outlive the slot that entitled it. That guarantee was not holding. Three silent failures:releasenever stopped the dev stack.stop_stacklooked upkilo-dev-<name>_,tmux has-sessionfailed, and it returned early treating the stack as already gone. Stacks outlived their slots — the exact over-subscription the cap exists to prevent.statusalways reportedstack=none, even with the stack plainly up.stack_is_coverednever matched on the recorded worktree, so coverage worked only via the owner-name-prefix fallback. A slot whose owner name does not begin with the worktree basename would make a live stack look uncovered — andstacks --reapwould thenpnpm dev:stopa running E2E.Observed on a live host:
slot-1 … [/Users/igor/Projects/.worktrees/mobile-ui-ddc7 stack=none]whiletmux has-session -t kilo-dev-mobile-ui-ddc7exited 0.Fix
Keep
\nin the preserved set sotrleaves the newline alone for$(...)to strip. Other unsafe characters are still replaced (weird.name/x→weird_name_x).Verification
Against live slot state, before and after:
bash -nclean.stacksstill reports every running stack as covered, now on the worktree path rather than by accident of the fallback.Found while enforcing the new slot rules across running dev stacks after #4826.