Repository navigation
feat(hooks): cross-platform terminal identity for agent binding [OMN-7295] - #1072
Closed
jonahgabriel wants to merge 0 commit into
Closed
jonahgabriel wants to merge 0 commit into
jonahgabriel wants to merge 0 commit into
Conversation
📝 WalkthroughWalkthroughA new Python module introducing terminal identity management is added. The module provides functions to generate, persist, and validate unique terminal identifiers per shell session using parent process IDs and UUID-based naming. Comprehensive unit tests validate creation, persistence, and format enforcement of these terminal IDs. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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: 1
🧹 Nitpick comments (1)
plugins/onex/hooks/lib/terminal_identity.py (1)
32-40: Docstring mentions Windows-specific behavior not implemented.The docstring states "session leader PID on Windows" but the implementation unconditionally uses
os.getppid()for all platforms. Either update the docstring to reflect the actual implementation or add the Windows-specific logic if intended.📝 Suggested docstring fix
def _get_shell_key() -> str: """Get a stable key for the current shell process. - Uses PPID (parent process ID) on Unix, or a combination of - session leader PID on Windows. Falls back to PID if PPID - is not available. + Uses PPID (parent process ID) which is available on all platforms + including Windows, macOS, and Linux. Behavior may vary by platform + and terminal emulator. """ ppid = os.getppid() return str(ppid)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/onex/hooks/lib/terminal_identity.py` around lines 32 - 40, The docstring for _get_shell_key mentions Windows-specific "session leader PID" behavior that isn't implemented; update the function so docstring matches reality: either implement the Windows-specific logic or simplify the docstring to state that the function currently returns the parent process ID (os.getppid()) on all platforms and falls back to PID if needed. Locate _get_shell_key and modify its docstring accordingly (or implement platform-specific behavior using Windows APIs if you prefer), ensuring the docstring and implementation are consistent.
🤖 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/unit/hooks/lib/test_terminal_identity.py`:
- Around line 10-14: Remove the redundant sys.path manipulation block that
inserts the hooks/lib path at test import time: delete the sys.path.insert(...)
call along with the preceding comment and the Path(...) expression (the block
that uses Path(__file__).resolve().parents[4] / "plugins" / "onex" / "hooks" /
"lib"). Rely on the existing tests/conftest.py setup that already configures
sys.path instead of duplicating it in test_terminal_identity.py.
---
Nitpick comments:
In `@plugins/onex/hooks/lib/terminal_identity.py`:
- Around line 32-40: The docstring for _get_shell_key mentions Windows-specific
"session leader PID" behavior that isn't implemented; update the function so
docstring matches reality: either implement the Windows-specific logic or
simplify the docstring to state that the function currently returns the parent
process ID (os.getppid()) on all platforms and falls back to PID if needed.
Locate _get_shell_key and modify its docstring accordingly (or implement
platform-specific behavior using Windows APIs if you prefer), ensuring the
docstring and implementation are consistent.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3a713e1e-a217-4d08-9bf2-92695b27224a
📒 Files selected for processing (2)
plugins/onex/hooks/lib/terminal_identity.pytests/unit/hooks/lib/test_terminal_identity.py
Comment on lines
+10
to
+14
| # Add hooks/lib to path so we can import the module directly | ||
| sys.path.insert( | ||
| 0, | ||
| str(Path(__file__).resolve().parents[4] / "plugins" / "onex" / "hooks" / "lib"), | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if other test files in this repo use the same sys.path pattern
rg -n "sys\.path\.insert" --type=py -g '*test*.py' | head -20Repository: OmniNode-ai/omniclaude
Length of output: 1656
🏁 Script executed:
head -60 tests/conftest.pyRepository: OmniNode-ai/omniclaude
Length of output: 2366
🏁 Script executed:
cat pyproject.toml | head -100Repository: OmniNode-ai/omniclaude
Length of output: 3217
🏁 Script executed:
cat -n tests/unit/hooks/lib/test_terminal_identity.pyRepository: OmniNode-ai/omniclaude
Length of output: 1830
Remove the redundant sys.path manipulation—conftest.py already configures this path.
The tests/conftest.py already adds plugins/onex/hooks/lib to sys.path (line 41), so lines 10–14 in this file are redundant. Remove them and rely on the central conftest configuration:
Suggested fix
# Add hooks/lib to path so we can import the module directly
-sys.path.insert(
- 0,
- str(Path(__file__).resolve().parents[4] / "plugins" / "onex" / "hooks" / "lib"),
-)
-
from terminal_identity import ( # noqa: E402
get_or_create_terminal_id,
is_terminal_id_valid,
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/hooks/lib/test_terminal_identity.py` around lines 10 - 14, Remove
the redundant sys.path manipulation block that inserts the hooks/lib path at
test import time: delete the sys.path.insert(...) call along with the preceding
comment and the Path(...) expression (the block that uses
Path(__file__).resolve().parents[4] / "plugins" / "onex" / "hooks" / "lib").
Rely on the existing tests/conftest.py setup that already configures sys.path
instead of duplicating it in test_terminal_identity.py.
jonahgabriel
force-pushed
the
jonah/omn-7295-terminal-identity
branch
from
April 2, 2026 17:08
8500867 to
035707d
Compare
This branch was previously deployed
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
terminal_identity.py— PPID-based heuristic that assigns a stable terminal ID per shell session.onex_state/terminal/tid_{ppid}, survives/clear, distinct per terminal tabContext
Part of Phase 2 Agent Identity (OMN-7291). Terminal identity is the bootstrap mechanism for binding agents to terminal sessions via
/login.Test plan
uv run pytest tests/unit/hooks/lib/test_terminal_identity.py -v— 4 tests passSummary by CodeRabbit
New Features
Tests