Repository navigation
Remove IPC version to indicate unstable nature at this moment - #192
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe IPC protocol version fields, protocol mismatch validation, and ChangesIPC protocol version removal
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant IPCClient
participant IPCApplicationBridge
participant IPCQueryRouter
IPCClient->>IPCApplicationBridge: Send request without protocol version
IPCApplicationBridge->>IPCQueryRouter: Request version or capabilities
IPCQueryRouter-->>IPCApplicationBridge: Return appVersion
IPCApplicationBridge-->>IPCClient: Return response without protocol version
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/ARCHITECTURE.md (1)
882-883: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRemove the stale
"version": 6field from this flow example.The request example immediately above still shows the removed top-level version field at Line 876, while this updated flow says authorization is the only check. This contradicts the new contract and
docs/IPC-CLI.md.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/ARCHITECTURE.md` around lines 882 - 883, Update the flow example surrounding “verifies authorization token” in docs/ARCHITECTURE.md by removing the stale top-level "version": 6 field from the request example above it, leaving authorization as the only check and keeping the remaining flow unchanged.Tests/NehirTests/IPCModelsTests.swift (1)
81-82: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAdd negative assertions for the removed fields.
These deletions remove checks for obsolete symbols but do not verify the new contract. Assert that encoded request, response, and event envelopes omit
version(and that version results omitprotocolVersion) so a future reintroduction of those fields fails tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/NehirTests/IPCModelsTests.swift` around lines 81 - 82, Add negative JSON assertions to publicDTOsRoundTripThroughJSON for request, response, and event envelopes, verifying encoded output omits version and version results omit protocolVersion. Ensure the assertions fail if any removed field is reintroduced while preserving the existing round-trip checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/IPC-CLI.md`:
- Around line 182-184: Update the fenced code block containing the `nehirctl
version --json` example to specify an appropriate language identifier,
preferably `sh` (or `text`), while preserving the command unchanged.
---
Outside diff comments:
In `@docs/ARCHITECTURE.md`:
- Around line 882-883: Update the flow example surrounding “verifies
authorization token” in docs/ARCHITECTURE.md by removing the stale top-level
"version": 6 field from the request example above it, leaving authorization as
the only check and keeping the remaining flow unchanged.
In `@Tests/NehirTests/IPCModelsTests.swift`:
- Around line 81-82: Add negative JSON assertions to
publicDTOsRoundTripThroughJSON for request, response, and event envelopes,
verifying encoded output omits version and version results omit protocolVersion.
Ensure the assertions fail if any removed field is reintroduced while preserving
the existing round-trip checks.
🪄 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 Plus
Run ID: c8db6af9-9548-4cab-9a48-4c17260646cb
📒 Files selected for processing (10)
.changeset/20260728221243-remove-the-ipc-protocol-version-field-and-docume.mdSources/Nehir/IPC/IPCApplicationBridge.swiftSources/Nehir/IPC/IPCQueryRouter.swiftSources/NehirCtl/CLIRenderer.swiftSources/NehirIPC/IPCModels.swiftTests/NehirTests/CLIRendererTests.swiftTests/NehirTests/IPCModelsTests.swiftTests/NehirTests/IPCServerTests.swiftdocs/ARCHITECTURE.mddocs/IPC-CLI.md
💤 Files with no reviewable changes (2)
- Tests/NehirTests/CLIRendererTests.swift
- Tests/NehirTests/IPCServerTests.swift
| ``` | ||
| nehirctl version --json | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Specify the fenced-block language.
Use ```sh (or ```text) for the nehirctl version --json example to satisfy Markdown linting.
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 182-182: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/IPC-CLI.md` around lines 182 - 184, Update the fenced code block
containing the `nehirctl version --json` example to specify an appropriate
language identifier, preferably `sh` (or `text`), while preserving the command
unchanged.
Source: Linters/SAST tools
|
@coderabbitai stop |
|
✅ Action performedReviews paused. |
299d031 to
1fb1df3
Compare
Summary
Remove the IPC protocol version field and document the IPC surface as unstable
Breaking change for IPC clients. The
versionfield is gone from IPC requests, responses, and event envelopes, and theprotocolVersionfield is gone from theversioncommand and thecapabilitiesquery. The server no longer rejects requests over a version mismatch, so theprotocol_mismatcherror code is removed too. The number never changed as the protocol grew, so it could not tell a client anything useful.The IPC protocol and
nehirctloutput format are documented as unstable: they can change in any release. Integrations should key on the Nehir app version, still reported asappVersionbynehirctl versionand thecapabilitiesquery.Release notes
Choose one:
.changeset/*.mdfor user-visible changes (mise run changeset -- patch "...").no release notelabel to make CI skip intentional.Validation
Summary by CodeRabbit
Breaking Changes
protocol_mismatcherror.nehirctl versionand capabilities now report only the Nehir app version.Documentation