fix(codex): keep collaboration guidance before conversation - #1380
fix(codex): keep collaboration guidance before conversation#1380harryzhou2000 wants to merge 2 commits into
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe Responses parser now records continuation boundaries. Collaboration logic uses those boundaries to place generated guidance after leading protocol metadata and before conversation content. Stateful requests reuse or replace replayed guidance without duplication. Tests and documentation cover the updated ordering. ChangesDeveloper guidance injection
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ResponsesRequest
participant parseRequest
participant injectDeveloperMessage
participant InputItems
ResponsesRequest->>parseRequest: parse continuation input
parseRequest->>injectDeveloperMessage: provide continuation boundary
injectDeveloperMessage->>injectDeveloperMessage: detect replayed guidance
injectDeveloperMessage->>InputItems: insert or replace guidance before conversation
Possibly related PRs
Suggested reviewers: 🚥 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 |
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request has been marked Ready for Review. |
|
@harryzhou2000 Please finish your pr. |
![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
🤖 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-site/src/content/docs/guides/sub-agent-surface.md`:
- Around line 54-56: Update the documentation around the array-form stateless
Responses request ordering to explicitly state that generated guidance is
inserted after leading system and developer metadata, before conversational
input. Keep the existing stateful previous_response_id replay-prefix behavior
unchanged.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 405d5a07-ed5b-4f39-8780-45e28f90df7a
📒 Files selected for processing (4)
docs-site/src/content/docs/guides/sub-agent-surface.mdsrc/server/responses/collaboration.tstests/multi-agent-compat.test.tstests/responses-state.test.ts
8d5434a to
df7ad09
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
df7ad09 to
7108a85
Compare
|
|
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
This is a real bug fix and the current implementation is a strong merge candidate. I found no remaining code-level blocker on head 7108a85251701c17bed4ae0a0289af5359069906: stateless guidance stays after leading system/developer metadata (including additional_tools) and before conversation, while stateful continuations preserve tool-result-first deltas, final compaction ordering, and latest-tagged guidance semantics.
Verification on the submitted head:
- focused Responses/collaboration suite: 238 passed
- typecheck: passed
- privacy scan: passed
- diff check: passed
I also transplanted the commit locally onto current dev@87e3ff9f63aa9e210d37e73fb5b64cc939ee03d5; the same 238 tests, typecheck, privacy scan, and diff check passed without conflict.
The only remaining blocker is integration provenance: the PR branch is currently 3 commits behind dev, and GitHub does not show the full exact-head platform matrix for this head. Please rebase onto current dev and let the complete exact-head CI finish. If that matrix is green, this should be ready for independent maintainer merge.
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Requesting changes on the current head 7108a852.
There is still one merge-blocking ordering hole in the stateful path. The PR fixes the stateless root cause by keeping generated guidance before conversation input, but previous_response_id continuations unconditionally append changed guidance to both parsed.context.messages and the raw input delta. That is necessary for an unexpanded function_call_output delta, where the tool result must stay first, but it is wrong for an ordinary stateful user-message delta: the new user turn is now immediately before the changed <multi_agent_mode> block, so stable collaboration metadata again becomes recency-dominant input after conversation.
Please distinguish the two cases instead of treating every stateful delta as tool-result-first. Preserve leading tool-result/call protocol items when required, but insert changed generated guidance before the first conversational item in an ordinary stateful delta. Add a regression test for previous_response_id + changed guidance + a new user message, alongside the existing tool-output-first case.
The current A→B→A test does not cover this because it asserts only that the replacement is newest; it does not exercise a fresh user-message delta or verify the conversation/guidance boundary.
|
|
7108a85 to
ea42264
Compare
|
✅ Action performedReview finished.
|
|
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-site/src/content/docs/guides/sub-agent-surface.md`:
- Around line 56-58: Update the continuation guidance in the paragraph
describing stateful previous_response_id replay to document latest-tagged
deduplication: injectDeveloperMessage reuses guidance only when it matches the
latest tagged replay item; for an A-B-A sequence, retain the earlier A and
insert a new A after the replay boundary.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f554d92c-8c93-42fc-a890-17d128099da8
📒 Files selected for processing (5)
docs-site/src/content/docs/guides/sub-agent-surface.mdsrc/responses/parser.tssrc/server/responses/collaboration.tssrc/types.tstests/multi-agent-compat.test.ts
|
@Wibias The requested stateful-ordering fix is now on Changed guidance now follows leading tool/protocol items but precedes the first current conversational item. Regressions cover an ordinary new-user continuation, Local CI reproduction for this exact code:
macOS and Windows remain platform-only checks for Actions. Could you please approve/trigger fork CI for exact head |
ea42264 to
0651b86
Compare
|
|
|
|
|
@Wibias Final exact head is now |
3 participants
Summary
compaction_triggerordering, with focused regression coverage and documentation.The root cause was tail placement. Stateless HTTP requests have no trusted replay prefix, so OpenCodex generates guidance for each request; appending it after current conversation made stable metadata look like fresh, recency-dominant input.
Current
devseparately suppresses OpenCodex guidance when the host-global catalog observation isstaleorunknown(#1407). This PR applies when guidance is generated and does not reintroduce the removed catalog warning.Verification
bun run typecheckbun scripts/lint-gui-if-changed.tsbun run privacy:scanbun run doctor:gui:if-changed(React Doctor: no issues)bun test tests/multi-agent-compat.test.ts tests/responses-state.test.ts tests/openai-responses-passthrough.test.ts tests/responses-compaction-routing.test.ts(238 pass, 0 fail)ASTRO_TELEMETRY_DISABLED=1 bun run buildindocs-sitegit diff --checkChecklist
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit
Bug Fixes
Documentation