fix: update grok session after new - #2683
Conversation
📝 WalkthroughWalkthroughGrok integration version 2 reports the session-start source. Terminal state handling updates same-owner Grok sessions after ChangesGrok session identity
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GrokCLI
participant GrokHook
participant Herdr
participant TerminalState
GrokCLI->>GrokHook: Emit session payload with source "new"
GrokHook->>Herdr: Report session ID and session_start_source
Herdr->>TerminalState: Process session identity
TerminalState->>TerminalState: Replace same-owner Grok session
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
kangal-bot
added
the
ai-review
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/integration/assets/grok/herdr-agent-state.ps1 (1)
44-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the argument array variable.
Line 44 overwrites PowerShell's automatic
$argsvariable. Use a dedicated variable such as$herdrArgs. This avoids changing automatic parameter state and removes the PSScriptAnalyzer warnings on Lines 44 and 52.Proposed fix
-$args = @( +$herdrArgs = @( "pane", "report-agent-session", $env:HERDR_PANE_ID, "--source", "herdr:grok", "--agent", "grok", "--seq", "$seq", "--agent-session-id", "$sessionId" ) if (-not [string]::IsNullOrWhiteSpace($sessionStartSource)) { - $args += @("--session-start-source", "$sessionStartSource") + $herdrArgs += @("--session-start-source", "$sessionStartSource") } try { - & $herdr `@args` 2>$null | Out-Null + & $herdr `@herdrArgs` 2>$null | Out-NullSource: Linters/SAST tools
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4400930c-fd6e-4f4d-9647-f2010500722a
📒 Files selected for processing (11)
docs/next/CHANGELOG.mddocs/next/website/src/content/docs/integrations.mdxdocs/next/website/src/content/docs/ja/integrations.mdxdocs/next/website/src/content/docs/session-state.mdxdocs/next/website/src/content/docs/zh-cn/integrations.mdxsrc/integration/assets/grok/herdr-agent-state.ps1src/integration/assets/grok/herdr-agent-state.shsrc/integration/mod.rssrc/integration/tests.rssrc/terminal/state.rstests/cli/hooks.rs
|
Agreed, renamed the PowerShell argument array to avoid shadowing the automatic |
Greptile SummaryThe PR updates Grok’s managed integration so
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/terminal/state.rs | Adds narrowly scoped same-owner Grok new replacement while explicitly preserving the different-owner guard. |
| src/integration/assets/grok/herdr-agent-state.sh | Forwards the Grok session-start source in the Unix socket request using the established schema field. |
| src/integration/assets/grok/herdr-agent-state.ps1 | Forwards the session-start source through a PowerShell argument array without shadowing PowerShell’s automatic argument variable. |
| src/integration/mod.rs | Advances the expected Grok integration version to match the changed bundled assets. |
| src/integration/tests.rs | Covers bundled asset metadata, source reporting support, and current versus outdated Grok integration versions. |
| tests/cli/hooks.rs | Verifies that the Unix Grok hook emits both the new session ID and new session-start source. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
G[Grok SessionStart event] --> H[Managed Grok hook]
H -->|session ID and source=new| R[pane.report_agent_session]
R --> O{Existing session owner}
O -->|Same Grok owner| U[Replace persisted Grok session ID]
O -->|Different owner| X[Reject replacement]
U --> P[Use current conversation for API results and restore]
Reviews (2): Last reviewed commit: "fix: avoid powershell args shadowing" | Re-trigger Greptile
|
@akbash-bot the implementation review is clean. the |
|
Rebase is resolved locally onto current I cannot update this PR's canonical head branch: |
akbash-bot
added
the
maintainer-needed
3 participants
Summary
SessionStart.sourcethrough the native session hook on Unix and Windowsnewsession replacements while preserving different-owner guardsChecks
cargo test --test cli grok_hook_reports_new_session_source -- --nocapturecargo test grok_new_session -- --nocapturecargo test grok_v1_integration_status_is_outdated -- --nocapturecargo test grok_v2_integration_status_is_current -- --nocapturecargo test bundled_integration_asset_versions_match_expected_versions -- --nocapturecargo test bundled_integration_assets_report_session_refs -- --nocapturecargo clippy --all-targets --locked -- -D warningslive_server_holds_one_pty_master_fd_per_panetest failed identically on unchanged master because its replacement server did not appearrefs #2681