Skip to content

docs(adapters): pin new_window command contract as shlex-joined argv#60

Merged
pbean merged 1 commit into
bmad-code-org:mainfrom
dracic:docs/new-window-command-contract
Jul 4, 2026
Merged

docs(adapters): pin new_window command contract as shlex-joined argv#60
pbean merged 1 commit into
bmad-code-org:mainfrom
dracic:docs/new-window-command-contract

Conversation

@dracic

@dracic dracic commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Summary

Multiplexer.new_window's command: str parameter had unspecified semantics: the POSIX/tmux backend hands it to a shell (operators like && would be interpreted) while a backend that re-splits the string treats operators as literal argv. Both existing callers build the command with shlex.quote-joins, so nothing is broken today — but a future caller passing a real shell line would break platform-specifically.

Docs-only contract pin, no behavior change:

  • multiplexer.py — the new_window docstring now states the contract: command is a POSIX shlex-joined argv string, not a shell line; shell-operator behavior is backend-defined and callers must not rely on it.
  • test_multiplexer.py — one new test pins the POSIX leaf's side of the contract: an operator-bearing shlex-joined command reaches tmux verbatim as the single trailing argument (full argv asserted, symmetric with the existing byte-identity tests).

Testing

  • ruff check + ruff format --check clean
  • pytest tests/test_multiplexer.py — 12 passed

Closes #59

The seam's `command` parameter had unspecified semantics: the POSIX
backend hands it to a shell while a re-splitting backend treats
operators as literal argv. State the contract in the ABC docstring
(shlex-joined argv, operator behavior backend-defined) and pin the
POSIX leaf passing the string to tmux verbatim.

Closes bmad-code-org#59
@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@dracic, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 37 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: afda1b25-9641-4b33-b687-523f5a235248

📥 Commits

Reviewing files that changed from the base of the PR and between bec1724 and 6c16ab5.

📒 Files selected for processing (2)
  • src/bmad_loop/adapters/multiplexer.py
  • tests/test_multiplexer.py
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@augmentcode

augmentcode Bot commented Jul 3, 2026

Copy link
Copy Markdown
🤖 Augment PR Summary

Summary: Pins TerminalMultiplexer.new_window(command) semantics to a shlex-joined argv string so callers don’t accidentally rely on backend-specific shell-line parsing.

What changed:

  • Clarified the command: str contract in src/bmad_loop/adapters/multiplexer.py, including a warning about &&/| operator behavior being backend-defined.
  • Added a unit test in tests/test_multiplexer.py asserting that an operator-bearing, shlex-joined command is passed through to tmux new-window verbatim as the single trailing argument.

🤖 Was this summary useful? React with 👍 or 👎

@augmentcode augmentcode Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed. No suggestions at this time.

Comment augment review to trigger a new review at any time.

@pbean pbean left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validated — approving. This is a clean, minimal, docs-only contract pin (+ one pinning test) with no runtime behavior change, and it does not risk the core or the other-OS backend path.

Verification performed:

  • Docstring is accurate. tmux_base.py new_window passes command verbatim as the single trailing arg to tmux new-window (run via a shell → operators would be interpreted); the eventual re-splitting backend treats them as literal argv. The docstring describes both correctly.
  • Existing callers already comply with the documented contract — both build command as a shlex-quoted join, so no raw shell line ever reaches the seam:
    • generic.py build_command" ".join(shlex.quote(a) …)
    • probe.py _ProbeLauncher.start" ".join(shlex.quote(a) …)
      The PR pins the de-facto contract; nothing is retrofitted or broken.
  • New test is correct — empirically confirmed. Reproduced its logic against the real tmux_base.py: the asserted argv is byte-identical to new_window's actual output, and the operator-bearing shlex-joined command reaches tmux verbatim as one trailing argument. Symmetric with the existing test_new_window_posix_argv_byte_identical.
  • No core / other-OS risk. Additive documentation on an abstract method plus a passing test. It reduces future risk by pinning the contract before the Windows/psmux backend lands — exactly the cross-backend divergence #59 flags.

Nice defensive close on #59. LGTM. 👍

@pbean pbean merged commit ea01d9e into bmad-code-org:main Jul 4, 2026
9 checks passed
@dracic dracic deleted the docs/new-window-command-contract branch July 4, 2026 08:22
@pbean pbean mentioned this pull request Jul 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Multiplexer seam: new_window's command parameter semantics differ between backends and are unspecified

2 participants