Repository navigation
feat(hooks): cross-platform terminal identity [OMN-7295] - #1069
Merged
Merged
Conversation
…t binding [OMN-7295]
PPID-based terminal ID generation that persists across /clear and survives
session restarts. Each terminal tab gets a unique ID stored in
.onex_state/terminal/tid_{ppid}. Heuristic bootstrap for Phase 2.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughA new module adds best-effort "stable per terminal tab" ID generation and on-disk persistence, plus validation. Unit tests exercise generation, persistence, uniqueness across state dirs, and format checks. Changes
Sequence Diagram(s)sequenceDiagram
participant Shell as ShellProcess
participant Module as terminal_identity.py
participant FS as FileSystem
participant Host as HostnameService
Shell->>Module: call get_or_create_terminal_id(state_dir)
Module->>Module: compute shell key (PPID/PID fallback)
Module->>FS: ensure state_dir/terminal exists
alt id file exists
Module->>FS: read tid_<shell_key>
FS-->>Module: stored_id
Module->>Module: validate stored_id (is_terminal_id_valid)
alt valid
Module-->>Shell: return stored_id
else invalid
Module->>Host: get hostname
Host-->>Module: hostname
Module->>Module: generate new ID ("terminal-"+hostname+uuid)
Module->>FS: write tid_<shell_key> with new ID
Module-->>Shell: return new ID
end
else id file missing
Module->>Host: get hostname
Host-->>Module: hostname
Module->>Module: generate new ID ("terminal-"+hostname+uuid)
Module->>FS: write tid_<shell_key> with new ID
Module-->>Shell: return new ID
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
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 (2)
tests/unit/hooks/lib/test_terminal_identity.py (1)
10-19: Consider using proper package imports instead of sys.path manipulation.Manual
sys.pathinsertion is fragile and can cause import conflicts. If the hooks lib is a proper package, configurepyproject.tomlorconftest.pyto add it to the Python path during test discovery.♻️ Alternative: conftest.py approach
In
tests/conftest.pyor a localconftest.py:import sys from pathlib import Path # Add once at conftest level instead of per-test-file sys.path.insert(0, str(Path(__file__).resolve().parents[3] / "plugins" / "onex" / "hooks" / "lib"))Then the test file simplifies to:
-import sys -from pathlib import Path +from pathlib import Path import pytest -# 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 +from terminal_identity import ( 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 - 19, Replace the per-test sys.path manipulation in tests/unit/hooks/lib/test_terminal_identity.py by configuring imports at the test-suite level: remove the sys.path.insert block and instead add the hooks/lib path in tests/conftest.py (or via pyproject/pytest.ini import-path settings) so the module terminal_identity can be imported normally; update the test to import get_or_create_terminal_id and is_terminal_id_valid directly (no sys.path changes) and ensure conftest computes the same Path used previously to locate the plugins/onex/hooks/lib directory.plugins/onex/hooks/lib/terminal_identity.py (1)
59-68: Consider adding error handling for file I/O.File read/write operations (lines 60, 68) can fail due to permission issues, disk full, or concurrent access. Currently, exceptions would propagate unhandled.
For a "best-effort" utility, wrapping I/O in try/except and generating a fresh ID on failure would improve resilience.
♻️ Proposed resilient read/write
if id_file.exists(): - existing = id_file.read_text().strip() - if existing and is_terminal_id_valid(existing): - return existing + try: + existing = id_file.read_text().strip() + if existing and is_terminal_id_valid(existing): + return existing + except OSError: + pass # Fall through to generate new ID hostname = platform.node().split(".")[0][:16] short_uuid = uuid.uuid4().hex[:8] terminal_id = f"terminal-{hostname}-{short_uuid}" - id_file.write_text(terminal_id) + try: + id_file.write_text(terminal_id) + except OSError: + pass # ID still usable for this session even if not persisted return terminal_id🤖 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 59 - 68, The file I/O for id_file.read_text() and id_file.write_text() can raise exceptions; wrap the read and write operations in try/except (catch OSError/IOError or Exception) around the existing id_file.exists() block and the write_text() call so that read failures fall through to generate a new terminal_id and write failures do not crash the process; ensure you still validate using is_terminal_id_valid(existing) when read succeeds, return the generated terminal_id even if write fails, and optionally log a warning (if a logger exists) inside the except blocks to record the error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/onex/hooks/lib/terminal_identity.py`:
- Around line 32-40: The function _get_shell_key currently always returns
os.getppid(), but the docstring incorrectly claims Windows-specific "session
leader PID" behavior; update the docstring to state that _get_shell_key returns
the parent process ID (PPID) on all platforms (falling back to PID if PPID not
available) or, if you prefer to implement platform-specific behavior, replace
the body of _get_shell_key with Windows logic (e.g., using psutil to get the
session leader or Win32 APIs to derive the console/session leader PID) and keep
the docstring describing the implemented platform differences; reference the
function name _get_shell_key when making the change.
---
Nitpick comments:
In `@plugins/onex/hooks/lib/terminal_identity.py`:
- Around line 59-68: The file I/O for id_file.read_text() and
id_file.write_text() can raise exceptions; wrap the read and write operations in
try/except (catch OSError/IOError or Exception) around the existing
id_file.exists() block and the write_text() call so that read failures fall
through to generate a new terminal_id and write failures do not crash the
process; ensure you still validate using is_terminal_id_valid(existing) when
read succeeds, return the generated terminal_id even if write fails, and
optionally log a warning (if a logger exists) inside the except blocks to record
the error.
In `@tests/unit/hooks/lib/test_terminal_identity.py`:
- Around line 10-19: Replace the per-test sys.path manipulation in
tests/unit/hooks/lib/test_terminal_identity.py by configuring imports at the
test-suite level: remove the sys.path.insert block and instead add the hooks/lib
path in tests/conftest.py (or via pyproject/pytest.ini import-path settings) so
the module terminal_identity can be imported normally; update the test to import
get_or_create_terminal_id and is_terminal_id_valid directly (no sys.path
changes) and ensure conftest computes the same Path used previously to locate
the plugins/onex/hooks/lib directory.
🪄 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: f3842f07-95a3-4d4a-8ad5-3cff9d20e0a8
📒 Files selected for processing (2)
plugins/onex/hooks/lib/terminal_identity.pytests/unit/hooks/lib/test_terminal_identity.py
…implementation [OMN-7295] Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
jonahgabriel
enabled auto-merge
April 2, 2026 15:36
jonahgabriel
added this pull request to the merge queue
Apr 2, 2026
1 of 2 tasks
Merged
2 of 3 tasks
jonahgabriel
added a commit
that referenced
this pull request
May 15, 2026
…e_core shim (SEAM-2) Replaces the 450-line inline implementation with a thin shim that delegates all logic to omnibase_core.validation.validator_skill_backing_node. Public API re-exported for backward compat: load_allowlist, extract_backing_node, check_node_liveness, scan, _omnimarket_available, _resolve_omnimarket_nodes_root. NodeViolation alias preserved for validate_skill_aspiration.py. BLOCKED ON: omnibase_core PR #1069 (feat(OMN-10171)) must merge and version bump before this PR can merge. omniclaude pins omnibase-core==0.40.1; the new module omnibase_core.validation.validator_skill_backing_node is not in that version. After omnibase_core publishes, update the pin and merge this PR.
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
.onex_state/terminal/tid_{ppid}Test plan
Part of Epic OMN-7291 (Agent Identity & Session Continuity Phase 2)
Summary by CodeRabbit
New Features
Tests