Reuse PTY startup handshake during shell startup#128
Merged
Conversation
Contributor
|
Thanks for the pull request. A maintainer will review it when available. Please keep the PR focused, explain the why in the description, and make sure local checks pass before requesting review. Contribution guide: https://github.com/AI-Shell-Team/aish/blob/main/CONTRIBUTING.md |
Contributor
|
This pull request description looks incomplete. Please update the missing sections below before review. Missing items:
|
📝 WalkthroughWalkthroughAfter PTY startup drain, PTYManager records startup readiness and initial CWD via new properties; PTYAIShell._setup_pty now consults these flags to optionally sync the backend CWD, mark the backend session ready, and transition the shell from "booting" to "editing" (removing the prior unconditional sleep). Changes
Sequence DiagramsequenceDiagram
participant Shell as PTYAIShell
participant Manager as PTYManager
participant Backend as Backend/PTY
rect rgba(100,150,255,0.5)
note over Backend,Manager: PTY emits control events during boot
Backend->>Manager: control events (session_ready, prompt_ready, ...)
activate Manager
Manager->>Manager: decode control events
Manager->>Manager: record startup_cwd, continuation prompt
Manager->>Manager: set startup_session_ready / startup_prompt_ready
Manager->>Manager: set startup_ready
deactivate Manager
end
rect rgba(100,200,100,0.5)
note over Shell,Manager: Shell setup uses startup handshake
Shell->>Manager: call _setup_pty()
Shell->>Manager: check startup_ready, read startup_cwd
activate Manager
Manager-->>Shell: startup_ready=true, startup_cwd
deactivate Manager
Shell->>Backend: sync working directory to startup_cwd
Shell->>Shell: mark backend session ready
Shell->>Shell: set _backend_session_ready and _shell_phase="editing"
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/aish/terminal/pty/manager.py`:
- Around line 238-239: The startup handshake flags are left from previous runs;
before starting a new process in PTYManager (the method where saw_session_ready
= self._startup_session_ready and saw_startup_prompt_ready =
self._startup_prompt_ready are set) explicitly reset self._startup_session_ready
= False and self._startup_prompt_ready = False so each start begins with a fresh
handshake state, then read those flags into saw_session_ready and
saw_startup_prompt_ready; make the same change for the other occurrence around
lines 276-277 so both start paths clear the prior-state flags before use.
In `@tests/shell/runtime/test_shell_pty_core.py`:
- Around line 267-297: The test uses hardcoded "/tmp/..." paths which trip Ruff
S108; change the test to accept a tmp_path (or tmpdir) fixture and replace all
"/tmp/frontend" and "/tmp/backend" literals with str(tmp_path / "frontend") and
str(tmp_path / "backend") respectively (update the _StartedPTYManager.__init__
assertions, its startup_cwd value, the monkeypatched os.chdir lambda,
shell._current_cwd initial value, and the final asserts) so paths are generated
from the temporary fixture rather than hardcoded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 08d27af6-763f-4113-96fb-0603163c2192
📒 Files selected for processing (4)
src/aish/shell/runtime/app.pysrc/aish/terminal/pty/manager.pytests/shell/runtime/test_shell_pty_core.pytests/terminal/pty/test_pty_control_protocol.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/shell/runtime/test_shell_pty_core.py`:
- Around line 288-303: The test should ensure the patched time.sleep is not
invoked during PTYAIShell._setup_pty; replace the no-op patch with a spy or a
stub that records calls (or raises if called) and add an assertion after
PTYAIShell._setup_pty that verifies the spy was never called (i.e., startup
fallback wait was not used). Reference the patched symbol
"aish.shell.runtime.app.time.sleep" and the function under test
"PTYAIShell._setup_pty" so the check directly ties the sleep call absence to the
startup handshake path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 16c88dff-4d42-4a11-b6a5-8a50962c543f
📒 Files selected for processing (2)
src/aish/terminal/pty/manager.pytests/shell/runtime/test_shell_pty_core.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/aish/terminal/pty/manager.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/shell/runtime/test_shell_pty_core.py (1)
291-305: Consider assertingos.chdiris called to harden cwd-sync coverage.Right now Line 291 patches
os.chdirto a no-op lambda. Switching to a mock and asserting the call would better catch regressions where cwd assignment happens without full sync behavior.Proposed test hardening
- monkeypatch.setattr("aish.shell.runtime.app.os.chdir", lambda _cwd: None) + chdir_mock = Mock() + monkeypatch.setattr("aish.shell.runtime.app.os.chdir", chdir_mock) @@ assert shell._backend_session_ready is True assert shell._shell_phase == "editing" sleep_mock.assert_not_called() + chdir_mock.assert_called_once_with(backend_cwd)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/shell/runtime/test_shell_pty_core.py` around lines 291 - 305, Replace the no-op patch of os.chdir with a mock so the test asserts the call: instead of monkeypatch.setattr("aish.shell.runtime.app.os.chdir", lambda _cwd: None) use a mock (e.g., MagicMock) and after calling PTYAIShell._setup_pty(shell) assert that the mock was called with backend_cwd; keep existing assertions for shell._current_cwd, current_env_info, _backend_session_ready and _shell_phase and ensure sleep_mock.assert_not_called() remains. This targets PTYAIShell._setup_pty and the shell._current_cwd synchronization behavior so regressions where os.chdir is not invoked will be caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/shell/runtime/test_shell_pty_core.py`:
- Around line 291-305: Replace the no-op patch of os.chdir with a mock so the
test asserts the call: instead of
monkeypatch.setattr("aish.shell.runtime.app.os.chdir", lambda _cwd: None) use a
mock (e.g., MagicMock) and after calling PTYAIShell._setup_pty(shell) assert
that the mock was called with backend_cwd; keep existing assertions for
shell._current_cwd, current_env_info, _backend_session_ready and _shell_phase
and ensure sleep_mock.assert_not_called() remains. This targets
PTYAIShell._setup_pty and the shell._current_cwd synchronization behavior so
regressions where os.chdir is not invoked will be caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 80ac08e4-328b-40a6-80dc-b23a5fb2e8d7
📒 Files selected for processing (1)
tests/shell/runtime/test_shell_pty_core.py
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
1 participant
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.
Summary
PTYManagerPTYAIShell._setup_pty()instead of waiting for a second backend-ready timeoutValidation
pytest tests/terminal/pty/test_pty_control_protocol.py tests/shell/runtime/test_shell_pty_core.py3.1sto about0.89sin local measurementSummary by CodeRabbit
New Features
Bug Fixes
Tests