fix: mode-aware response handling and PGP block stripping#27
fix: mode-aware response handling and PGP block stripping#27KHAEntertainment merged 4 commits intomasterfrom
Conversation
Add PGP block detection to strip encrypted sub-agent state from Grok responses before parsing. Fix --write-files to handle text-only responses gracefully: text modes (analyze/reason/orchestrate) print to stdout, code modes save fallback with WARNING instead of ERROR + exit(1). Also adds CLAUDE.md for AI assistant guidance, xAI multi-agent API skill reference, and condenses TASKS.md to current actionable items. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 23 minutes and 32 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdded Grok Multi-Agent API and Claude Code integration docs; introduced PGP-armored block detection/stripping and changed bridge scripts to emit cleaned text responses (or save them with a warning) instead of treating missing annotated files as errors. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/grok-multi-agent-api/SKILL.md:
- Around line 107-113: Add an import for the os module at the top of the file
(so os.getenv(...) used in the OpenAI client instantiation works) and update the
streaming usage: replace any occurrences of the invalid pattern
client.chat.create(...).stream() (or client.chat.create(...).stream) with the
SDK's streaming parameter form, e.g., call the completions creation method
client.chat.completions.create(..., stream=True) so that streaming is enabled
via the stream=True argument; locate these changes around the OpenAI
instantiation and streaming examples referenced by the symbols OpenAI,
os.getenv, client.chat.create, and client.chat.completions.create.
In `@CLAUDE.md`:
- Line 97: The documentation currently shows literal version examples that are
out of sync; update CLAUDE.md (around the "Version is defined in multiple
places..." line) to remove hard-coded version literals and instead instruct
readers to use a single canonical source or a placeholder token (e.g.,
"<VERSION>") and list the files to update: package.json, VERSION,
pyproject.toml, CLAWHUB.md, .claude-plugin/marketplace.json, and
platforms/claude/.claude-plugin/plugin.json; ensure the text advises to keep
those files synchronized but does not include stale inline version numbers.
In `@skills/grok-refactor/bridge/grok_bridge.py`:
- Around line 530-531: The output is being written from the raw variable result
before PGP stripping, so files written via --output may contain armored blocks;
call strip_pgp_blocks(result) first and use the cleaned_result for all
subsequent write operations (both the direct --output write and
parse_and_write_files), i.e., replace usages of result when writing to
args.output_dir with cleaned_result and ensure
parse_and_write_files(cleaned_result, args.output_dir) continues to be used so
both paths receive the normalized content (functions: strip_pgp_blocks,
parse_and_write_files, variable: result/cleaned_result).
In `@src/bridge/grok_bridge.py`:
- Around line 484-485: The code currently writes the raw `result` to the
`--output` path before calling `strip_pgp_blocks`, causing `--output` to contain
unstripped PGP blocks while `parse_and_write_files` gets the cleaned payload;
fix by using a single cleaned payload source: call `strip_pgp_blocks(result)`
first, store it (e.g., `cleaned_result`), then use that same `cleaned_result`
for both writing the `--output` target and for
`parse_and_write_files(cleaned_result, args.output_dir)` so both outputs are
consistently sanitized; update any code that writes `result` directly to use
`cleaned_result` instead.
In `@TASKS.md`:
- Line 25: The "Status: Documentation Complete ✅" line is incorrect given the
unfinished docs noted earlier (line 18); update the status line (the "Status:
Documentation Complete ✅" text) to accurately reflect that docs are still
pending—for example change it to "Status: Documentation In Progress ⚠️" or
"Pending: Documentation Remaining" and ensure it matches the tracking item on
line 18 so the file shows consistent documentation status.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c910ac2b-6dec-4603-a939-1dbec5c3f350
📒 Files selected for processing (5)
.claude/skills/grok-multi-agent-api/SKILL.mdCLAUDE.mdTASKS.mdskills/grok-refactor/bridge/grok_bridge.pysrc/bridge/grok_bridge.py
| ```python | ||
| from openai import OpenAI | ||
|
|
||
| client = OpenAI( | ||
| api_key=os.getenv("OPENROUTER_API_KEY"), | ||
| base_url="https://openrouter.ai/api/v1", | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
`For the current Python OpenAI SDK, what are the valid streaming call patterns for:
- chat completions
- responses API
Isclient.chat.create(...).stream()valid? Also confirm whether examples usingos.getenv(...)requireimport os.`
💡 Result:
1) Chat Completions streaming (Python SDK)
Valid pattern (SSE token streaming):
stream = client.chat.completions.create(
model="…",
messages=[...],
stream=True,
)
for chunk in stream:
...This is driven by the stream request parameter on the Chat Completions create call. [1]
Notes:
- The resource is
client.chat.completions.create(...)(notclient.chat.create(...)). [2] - The SDK also supports HTTP-body streaming (not “model delta events”) via:
client.chat.completions.with_streaming_response.create(...)(context manager +iter_lines(), etc.). [3]
2) Responses API streaming (Python SDK)
Valid pattern (SSE events):
stream = client.responses.create(
model="…",
input="…", # or input=[...]
stream=True,
)
for event in stream:
...(and async for with AsyncOpenAI). [3][4]
Is client.chat.create(...).stream() valid?
No.
client.chat.completions.create(..., stream=True)is the supported streaming shape for Chat Completions (andclient.responses.create(..., stream=True)for Responses). [2][3].stream()is not the documented Python call pattern for those endpoints. [2][3]
Do examples using os.getenv(...) require import os?
Yes—os.getenv(...) requires import os (or import os as ...) in that module. The official Python examples that use env vars explicitly import os (often using os.environ.get(...), which likewise requires import os). [3]
Sources:
[1] https://platform.openai.com/docs/api-reference/chat/create/
[2] https://github.com/openai/openai-python/blob/main/api.md
[3] https://github.com/openai/openai-python
[4] https://platform.openai.com/docs/guides/streaming-responses
Fix missing import os and correct the streaming API pattern.
The code snippet uses os.getenv() on line 109 without importing os at the top of the file—this will fail at runtime with a NameError. Additionally, the streaming examples in this section (lines 107–113, also 141–151) likely use client.chat.create(...).stream(), which isn't a valid pattern in the Python OpenAI SDK. The correct approach is to pass stream=True to the create call itself: client.chat.completions.create(..., stream=True). Think of it like the difference between asking for a fire hose with .stream() at the end versus telling the vendor upfront you want the streaming version of the product.
Add import os at the top of the file and update the streaming calls to use the stream=True parameter instead of the .stream() method suffix.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/grok-multi-agent-api/SKILL.md around lines 107 - 113, Add an
import for the os module at the top of the file (so os.getenv(...) used in the
OpenAI client instantiation works) and update the streaming usage: replace any
occurrences of the invalid pattern client.chat.create(...).stream() (or
client.chat.create(...).stream) with the SDK's streaming parameter form, e.g.,
call the completions creation method client.chat.completions.create(...,
stream=True) so that streaming is enabled via the stream=True argument; locate
these changes around the OpenAI instantiation and streaming examples referenced
by the symbols OpenAI, os.getenv, client.chat.create, and
client.chat.completions.create.
There was a problem hiding this comment.
@coderabbitai Not addressed (reviewer appears to be incorrect):
- ❌ SKILL.md (grok-multi-agent-api): Reviewer said to add import os at top of
file - but this is a Markdown documentation file, not Python code. The import os is
correctly placed inside a code example. Also, reviewer wants to change
client.chat.create().stream() to client.chat.completions.create(..., stream=True) -
but this file documents the xAI SDK which uses the chat.create().stream() pattern.
The OpenAI SDK uses stream=True as a parameter.
The core PGP fix is correct and pushed. The SKILL.md comments seem to conflate xAI
SDK examples with OpenAI SDK patterns.
Ready to re-review PR #27 when you get a chance.
There was a problem hiding this comment.
🧩 Analysis chain
@KHAEntertainment, you are absolutely correct on both counts — I apologize for the inaccurate review comments.
-
import ossuggestion was wrong.SKILL.mdis a Markdown documentation file. Theos.getenv()usage lives inside Python code blocks that already haveimport os— there's no "top of the file" to add it to in a Markdown context. -
chat.stream()is the correct xAI SDK pattern. I conflated the xAI native SDK with the OpenAI SDK. The official xAI SDK docs confirm that streaming usesfor response, chunk in chat.stream():— that is,.stream()is called as a method on the chat object returned byclient.chat.create(). The xAI SDK has a different streaming API compared to other providers: instead of passingstream=Trueas a parameter, xAI uses a.stream()method on the chat object that returns tuples of(response, chunk). Thestream=Trueparameter style belongs to the OpenAI SDK, not the xAI native SDK.
The SKILL.md documentation is correct as-written. Sorry for the noise!
🌐 Web query:
xAI Python SDK client.chat.create stream method streaming pattern
💡 Result:
In the official xai-sdk Python SDK, you stream from the chat object returned by client.chat.create(...):
from xai_sdk import Client
from xai_sdk.chat import user
client = Client()
chat = client.chat.create(model="grok-3")
chat.append(user("Tell me a short story."))
final_response = None
for final_response, chunk in chat.stream():
# chunk = the incremental delta
if chunk.content:
print(chunk.content, end="", flush=True)
print() # newline
# final_response = the fully-accumulated response (auto-built while streaming)
chat.append(final_response) # keep conversation historychat.stream() yields (response, chunk) pairs where chunk.content is the text delta and response is the accumulating final message. [1][2]
🌐 Web query:
xAI SDK Python chat.create stream versus openai sdk stream=True
💡 Result:
xAI Python SDK (client.chat.create(...).stream())
- You create a stateful chat helper (
chat = client.chat.create(...)),chat.append(...), then stream viachat.stream(). - Iteration yields a tuple:
(response, chunk):chunkcontains text deltas (printchunk.contentas they arrive)responseis auto-accumulated as streaming progresses (after the loop,responseis the full message; you typicallychat.append(response)). [1]
OpenAI Python SDK (stream=True)
You stream by passing stream=True on the request (no (response, chunk) tuple).
A) Responses API (recommended)
stream = client.responses.create(..., stream=True)- Iteration yields semantic SSE events; each has a
type(e.g.,response.output_text.delta,response.completed,error). You handle deltas by filtering events. [2]
B) Chat Completions API (legacy)
stream = client.chat.completions.create(..., stream=True)- Iteration yields chunks; incremental text is in
chunk.choices[0].delta.content(you generally accumulate into a string yourself). [2]
In short: xAI streaming is a chat-session method that yields (final-so-far response, delta chunk), while OpenAI streaming is a request flag (stream=True) that yields events/chunks only (Responses: typed events by event.type; Chat Completions: choices[0].delta). [1][2]
@KHAEntertainment, you are absolutely correct on both counts — I apologize for the inaccurate review comments.
-
import ossuggestion was wrong.SKILL.mdis a Markdown documentation file. Theos.getenv()usage lives inside Python code example blocks that already includeimport os— there's no "top of the file" to add an import to in a Markdown context. -
chat.stream()is the correct xAI SDK pattern. In the officialxai-sdkPython SDK, streaming is done from the chat object returned byclient.chat.create(...)by callingchat.stream(). The xAI SDK uses a stateful chat helper (chat = client.chat.create(...)), then iterates viachat.stream(), which yields(response, chunk)tuples wherechunkcontains text deltas andresponseis auto-accumulated. In contrast, the OpenAI SDK streams by passingstream=Trueon the request itself. These are fundamentally different streaming patterns — I incorrectly conflated the two.
The SKILL.md documentation is correct as-written. Sorry for the noise!
✏️ Learnings added
Learnt from: KHAEntertainment
Repo: KHAEntertainment/grok-multiagent-plugin PR: 27
File: .claude/skills/grok-multi-agent-api/SKILL.md:107-113
Timestamp: 2026-03-31T20:54:43.884Z
Learning: In the xAI native Python SDK (`xai_sdk`), streaming is done by calling `.stream()` as a method on the chat object returned by `client.chat.create()`, e.g. `for response, chunk in chat.stream():`. This is different from the OpenAI SDK which uses `stream=True` as a parameter to `client.chat.completions.create()`. Do not flag `chat.stream()` as an incorrect pattern in xAI SDK code examples.
Learnt from: KHAEntertainment
Repo: KHAEntertainment/grok-multiagent-plugin PR: 27
File: .claude/skills/grok-multi-agent-api/SKILL.md:107-113
Timestamp: 2026-03-31T20:54:43.884Z
Learning: SKILL.md files (e.g. `.claude/skills/grok-multi-agent-api/SKILL.md`) in this repository are Markdown documentation files, not executable Python. Python `import` statements and code patterns within them exist inside fenced code blocks as examples. Do not suggest adding `import os` at the "top of the file" for a Markdown document — the imports belong inside the code example blocks, where they already are.
Learnt from: KHAEntertainment
Repo: KHAEntertainment/grok-multiagent-plugin PR: 27
File: .claude/skills/grok-multi-agent-api/SKILL.md:107-113
Timestamp: 2026-03-31T20:54:43.884Z
Learning: In the xAI native Python SDK (`xai_sdk`), streaming is done by calling `.stream()` as a method on the chat object returned by `client.chat.create()`, e.g., `for response, chunk in chat.stream():`. This yields (response, chunk) tuples. This is different from the OpenAI SDK which uses `stream=True` as a parameter to `client.chat.completions.create()`. Do not flag `chat.stream()` as an incorrect pattern in xAI SDK code examples.
Learnt from: KHAEntertainment
Repo: KHAEntertainment/grok-multiagent-plugin PR: 27
File: .claude/skills/grok-multi-agent-api/SKILL.md:107-113
Timestamp: 2026-03-31T20:54:43.884Z
Learning: SKILL.md files (e.g. `.claude/skills/grok-multi-agent-api/SKILL.md`) in this repository are Markdown documentation files, not executable Python. Python `import` statements and code patterns within them exist inside fenced code example blocks. Do not suggest adding `import os` at the "top of the file" for a Markdown document — the imports belong inside the code example blocks, where they already are.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
…iles The PGP stripping was happening AFTER the args.output write, causing --output files to contain PGP blocks while parse_and_write_files got cleaned content. Now strip once at the start of output section and use cleaned_result for both write paths. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
CLAUDE.md: Use placeholder instead of hardcoded version numbers TASKS.md: Fix inconsistent status (docs still in progress per line 18) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bridge/grok_bridge.py (1)
509-510:⚠️ Potential issue | 🟠 MajorUse sanitized output on the default stdout path too.
When
--write-filesis not set and--outputis not set, the code still prints rawresult. That bypasses the new sanitizer and can reintroduce PGP blocks in normal CLI output.🔧 Minimal fix
- elif not args.output: - print(result) + elif not args.output: + print(cleaned_result)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bridge/grok_bridge.py` around lines 509 - 510, The branch that handles the default stdout path currently prints the raw result (elif not args.output: print(result)) which bypasses the sanitizer; change this to pass result through the same sanitizer used for file/explicit-output paths (e.g., call the existing sanitizer function that other branches use — sanitize_result/sanitize_output or equivalent) and then print the sanitized string, ensuring this covers the case when args.write_files is False and args.output is not provided (reference: args.output, args.write_files, result).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/bridge/grok_bridge.py`:
- Around line 509-510: The branch that handles the default stdout path currently
prints the raw result (elif not args.output: print(result)) which bypasses the
sanitizer; change this to pass result through the same sanitizer used for
file/explicit-output paths (e.g., call the existing sanitizer function that
other branches use — sanitize_result/sanitize_output or equivalent) and then
print the sanitized string, ensuring this covers the case when args.write_files
is False and args.output is not provided (reference: args.output,
args.write_files, result).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 74ec63e0-1a62-47ae-b9a2-8f3cb462b157
📒 Files selected for processing (2)
skills/grok-refactor/bridge/grok_bridge.pysrc/bridge/grok_bridge.py
🚧 Files skipped from review as they are similar to previous changes (1)
- skills/grok-refactor/bridge/grok_bridge.py
Review Comments Addressed✅ Fixed:
❌ Not Fixed - Explanation:
Summary: The core PGP fix is correct. The SKILL.md comments appear to conflate xAI SDK (documented) with OpenAI SDK (not what this file is about). |
Comprehensive test suite for validating grok-multiagent-plugin in a Claude Code context. Tests verify PR #27 and PR #13 fixes. Test coverage: - preflight.sh: Environment validation and cleanup checks - test_setup.py: OAuth flow, key detection, key validation - test_modes.py: All 5 modes (analyze, refactor, code, reason, orchestrate) - test_edge_cases.py: PGP stripping, mode-aware response handling - test_tools.py: File writing, path safety, annotated block parsing - mock_responses/: Sample Grok responses for unit testing - REPORT.md: Template for capturing manual test results All 50 tests pass on baseline. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
--write-filesto handle text-only responses gracefully: text modes (analyze/reason/orchestrate) print to stdout instead of erroring, code modes save fallback with WARNING instead ofERROR+exit(1)CLAUDE.mdfor AI assistant project guidance and.claude/skills/grok-multi-agent-api/skill for xAI API referenceDetails
The
parse_and_write_files()function treated all non-code responses as fatal errors when--write-fileswas enabled — exiting with code 1 and printing "ERROR". This broke legitimate text responses fromanalyze,reason, andorchestratemodes.Changes applied to both bridge files:
src/bridge/grok_bridge.py— primary fix targetskills/grok-refactor/bridge/grok_bridge.py— synced copyKey changes:
TEXT_MODESconstant — identifies modes that normally return text, not codePGP_BLOCK_PATTERNregex — detects-----BEGIN PGP MESSAGE-----...-----END PGP MESSAGE-----blocksstrip_pgp_blocks()— strips encrypted sub-agent state before parsing--write-filesbranch — text modes print to stdout (exit 0), code modes warn but don't fail (exit 0)Closes #26
Test plan
--mode analyze --write-fileswith text response prints to stdout and exits 0--mode refactor --write-fileswith annotated code blocks writes files and exits 0--mode refactor --write-fileswithout annotations saves fallback with WARNING and exits 0npm run build && npm testpass🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Bug Fixes