fix(collaboration): scope catalog-state guidance to what we can attribute - #1407
Conversation
|
✅ Deterministic PR hygiene checks passed. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughV2 collaboration guidance now suppresses catalog-derived guidance for ChangesCatalog Guidance
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CatalogStateCollector
participant CollaborationGuidance
participant Caller
CatalogStateCollector->>CollaborationGuidance: Provide catalog state
alt stale or unknown
CollaborationGuidance-->>Caller: Return no catalog-derived guidance
else fresh or not_running
CollaborationGuidance-->>Caller: Return preferred-model guidance
end
🚥 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 |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
6b496df to
96132bc
Compare
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
The wording split is directionally useful, but the current head still preserves the underlying request-scoping bug confirmed in #1395, so I am requesting changes.
collectCodexAppServerCatalogState() folds every current-user app-server into one global state, and the inbound request carries no sender PID or catalog fingerprint. A known-stale process A can therefore make a fresh process B receive this PR's do not set model or reasoning_effort instruction. Rewording only unknown does not fix that case, and the same identity limitation applies to both stale and unknown.
For either state, suppress OpenCodex's disk-derived preferred-model, roster, fallback, and custom guidance, but do not prohibit options advertised by the active spawn_agent tool. The minimal safe behavior is to return no OCX-authored v2 guidance for stale or unknown; keep fresh and not_running unchanged.
Please update the regression so a mixed stale/fresh process set can still produce the global stale observation without producing a blanket no-override instruction for an unidentified request. Also update the catalog/subagent behavior documentation, because this changes the user-visible guidance contract. Once that is done on current dev and exact-head CI is green, this can cover both #1354 and #1395 without a duplicate PR.
96132bc to
2e5f4f4
Compare
|
|
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Requesting changes. The wording split is useful, but the current head is not merge-ready.
Merge blockers
1. Global stale/unknown state still blocks valid overrides for unrelated fresh sessions
collectCodexAppServerCatalogState() folds every detected current-user app-server into one global state. A stale process A therefore makes the global result stale, even if the current request came from a fresh process B. multiAgentGuidanceText() has no sender PID/catalog identity, yet this PR still injects do not set model or reasoning_effort overrides for that global state.
That preserves the confirmed #1395 request-scoping bug. For stale and unknown, suppress OpenCodex-authored disk/catalog guidance, but do not prohibit model/reasoning options exposed by the active spawn_agent tool. The minimal safe behavior is to emit no OCX-authored v2 model guidance for those states, or otherwise make the active tool schema authoritative.
2. The new regression test locks in that incorrect contract
The added test explicitly requires both stale and unknown to contain do not set, so it codifies the #1395 bug instead of protecting the intended boundary.
Please add a mixed stale/fresh regression: one process starts before the catalog, another after it, collector resolves globally to stale, and an unidentified current v2 request must not receive a blanket no-override instruction.
Additional correctness gap
The new unknown message says (app-server start time or catalog timestamp unreadable), but unknown can also result from process-enumeration failure. That still asserts a cause which is not established. If any unknown guidance remains, keep it cause-neutral, e.g. Could not verify whether Codex's model catalog is current.
Docs / readiness
Because #1407 is now the selected overlapping fix path for #1354 and #1395, update the catalog/subagent guidance documentation to describe the actual stale/unknown contract.
The reviewed head is also behind current dev, and exact-head React Doctor / Cross-platform CI have not executed jobs yet (action_required, zero jobs). Rebase or merge current dev and get exact-head CI green before merge.
Security
I found no new exploitable security vulnerability in this diff. The blocking issues are correctness, request scoping, and regression coverage.
|
|
2e5f4f4 to
0ec70c5
Compare
…bute
`collectCodexAppServerCatalogState()` folds every app-server owned by the
current user into ONE global observation, and an inbound request carries no
sender PID or catalog fingerprint. So a stale process A makes the global state
`stale` even when the request came from a fresh process B, and `unknown` can be
reached by a process-enumeration failure that says nothing about any particular
server.
v2 guidance nonetheless injected
do not set model or reasoning_effort overrides until Codex restarts
off that global observation. Two problems follow:
* it prohibits options the active `spawn_agent` tool legitimately advertises,
for a request that cannot be attributed to the stale process (lidge-jun#1395); and
* the same text was emitted for `unknown`, asserting a cause that was never
established and a restart that cannot clear a failed measurement (lidge-jun#1354).
For `stale` and `unknown`, withhold opencodex's own disk-derived claims —
preferred model, roster, fallback, custom guidance — and say nothing about
overrides, leaving the active tool schema authoritative. `fresh` and
`not_running` are unchanged.
The previous regression asserted that both states contain `do not set`, which
codified the scoping bug; it is replaced by coverage that a mixed stale/fresh
process set resolving globally to `stale` produces no blanket no-override
instruction for an unidentified request.
Documents the four-state guidance contract in the sub-agent surface guide,
including why `unknown` is not a synonym for `stale`.
Verified:
bun test tests/multi-agent-compat.test.ts -> 43 pass, 0 fail
bun test tests/codex-app-server-processes.test.ts -> 30 pass, 1 skip, 0 fail
bun x tsc --noEmit -> clean
Fixes lidge-jun#1354
Fixes lidge-jun#1395
0ec70c5 to
550091d
Compare
|
✅ Action performedReview finished.
|
|
Thanks both — you're right, and the second point is the one that stings: my regression asserted What changed
Regression replaced. The new one builds a genuinely mixed process set through the collector — pid 42 starting before the catalog, pid 43 after — asserts the global observation really does resolve to On the cause-neutrality point — you were right and I'd made the same mistake I was fixing. My Docs — added a four-state table to the sub-agent surface guide covering what each state does to guidance, why a global observation can't be attributed to a request, and why restarting clears Rebased onto current Happy to keep iterating if the null-guidance shape isn't quite what you had in mind for |
abhisheksharma2411
changed the title
![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`:
- Line 231: Update the stale-state recovery statement in the sub-agent surface
guide to say that stale clears only when every detected current-user app-server
has started after the final catalog write; do not claim that restarting Codex
alone necessarily clears stale, while preserving the existing distinction for
unknown.
🪄 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: 157ed10a-e253-4a05-8b8e-1906a1f87a8c
📒 Files selected for processing (3)
docs-site/src/content/docs/guides/sub-agent-surface.mdsrc/server/responses/collaboration.tstests/multi-agent-compat.test.ts
|
|
Clarify the distinction between 'stale' and 'unknown' states in the documentation.
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== PR diff summary ==='
git diff --stat HEAD^ HEAD
echo
echo '=== Changed files ==='
git diff --name-only HEAD^ HEAD
echo
echo '=== Changed hunks ==='
git diff --unified=80 HEAD^ HEAD -- \
src/server/responses/collaboration.ts \
tests/multi-agent-compat.test.ts \
docs-site/src/content/docs/guides/sub-agent-surface.md
echo
echo '=== Implementation outline ==='
ast-grep outline src/server/responses/collaboration.ts --items all --view expanded
echo
echo '=== Focused catalog-state references ==='
rg -n -C 5 '\b(stale|unknown|catalog|reasoning_effort|spawn_agent)\b' \
src/server/responses/collaboration.ts \
tests/multi-agent-compat.test.ts \
docs-site/src/content/docs/guides/sub-agent-surface.mdLength of output: 96661 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Current commit and candidate bases ==='
git status --short --branch
git log --oneline --decorate -12
echo
git branch --all --contains HEAD || true
echo
echo '=== Available dev refs ==='
git for-each-ref --format='%(refname:short) %(objectname)' 'refs/remotes/*/dev' 'refs/heads/dev' || true
base_ref=""
for ref in origin/dev refs/remotes/origin/dev dev; do
if git rev-parse --verify --quiet "$ref" >/dev/null; then
base_ref="$ref"
break
fi
done
if [ -z "$base_ref" ]; then
echo 'ERROR: No dev ref is available for full PR comparison.'
exit 0
fi
merge_base="$(git merge-base HEAD "$base_ref")"
echo
echo "=== Full PR range: ${merge_base}..HEAD (base: ${base_ref}) ==="
git log --oneline "${merge_base}..HEAD"
echo
git diff --stat "${merge_base}..HEAD"
echo
git diff --unified=50 "${merge_base}..HEAD" -- \
src/server/responses/collaboration.ts \
tests/multi-agent-compat.test.ts \
docs-site/src/content/docs/guides/sub-agent-surface.md
echo
echo '=== App-server collector outline ==='
collector="$(fd -a '^app-server-processes\.ts$' src | head -n 1)"
printf 'collector=%s\n' "$collector"
if [ -n "$collector" ]; then
ast-grep outline "$collector" --items all --view expanded
echo
echo '=== Catalog-state collector implementation ==='
rg -n -A100 -B15 '\bcollectCodexAppServerCatalogState\s*\(' "$collector"
fiLength of output: 2546 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Commit ancestry ==='
git show -s --format='commit=%H%nparents=%P%nsubject=%s' 7779c057 550091d0 04222e36
echo
echo '=== Available PR diff: 7779c057..HEAD ==='
git diff --stat 7779c057..HEAD
git diff --unified=60 7779c057..HEAD -- \
src/server/responses/collaboration.ts \
tests/multi-agent-compat.test.ts \
docs-site/src/content/docs/guides/sub-agent-surface.md
echo
echo '=== Guidance control flow, Lines 226-365 ==='
nl -ba src/server/responses/collaboration.ts | sed -n '226,365p'
echo
echo '=== Catalog-state collector source ==='
collector="$(fd -a '^app-server-processes\.ts$' src | head -n 1)"
printf 'collector=%s\n' "$collector"
if [ -z "$collector" ]; then
echo 'ERROR: app-server-processes.ts was not found under src.'
exit 0
fi
ast-grep outline "$collector" --items all --view expanded
rg -n -A130 -B20 '\bcollectCodexAppServerCatalogState\s*\(' "$collector"
echo
echo '=== Documentation state section with line numbers ==='
nl -ba docs-site/src/content/docs/guides/sub-agent-surface.md | sed -n '202,235p'
echo
echo '=== Tests for catalog-state guidance with line numbers ==='
nl -ba tests/multi-agent-compat.test.ts | sed -n '115,215p'Length of output: 27091 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Guidance control flow ==='
sed -n '226,365p' src/server/responses/collaboration.ts | cat -n
echo
echo '=== Catalog-state collector files ==='
fd -a '^app-server-processes\.ts$' src
collector="$(fd -a '^app-server-processes\.ts$' src | head -n 1)"
if [ -z "$collector" ]; then
echo 'ERROR: app-server-processes.ts was not found under src.'
exit 0
fi
echo
echo "=== Collector outline: ${collector} ==="
ast-grep outline "$collector" --items all --view expanded
echo
echo '=== Collector implementation and related state tests ==='
rg -n -A150 -B30 '\bcollectCodexAppServerCatalogState\s*\(' "$collector"
test_collector="$(fd -a '^codex-app-server-processes\.test\.ts$' tests | head -n 1)"
if [ -n "$test_collector" ]; then
echo
echo "=== Collector test references: ${test_collector} ==="
rg -n -A45 -B12 '\b(stale|fresh|unknown|not_running|collectCodexAppServerCatalogState)\b' "$test_collector"
fi
echo
echo '=== multiAgentGuidanceText call sites ==='
rg -n -A8 -B8 '\bmultiAgentGuidanceText\s*\(' src tests \
-g '*.ts' -g '!tests/multi-agent-compat.test.ts'
echo
echo '=== Documentation state section ==='
sed -n '202,235p' docs-site/src/content/docs/guides/sub-agent-surface.md | cat -nLength of output: 48343
The repository sandbox did not permit test execution. The inspected test coverage and implementation are consistent with the PR objective.
|
|
Tip For best results, initiate chat on the files or code changes.
You are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes.
You are interacting with an AI system. |
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Approving. The head scopes catalog-state guidance to what we can attribute: stale/unknown withhold disk-derived claims without a false restart/no-override prescription for unidentified requests, with regressions for mixed stale/fresh and unknown.
|
Thanks @abhisheksharma2411 — this was useful because it stopped treating a failed catalog measurement ( Merging now. |
3 participants
Fixes #1354.
Summary
v2 collaboration guidance mapped
unknownto the same text asstale:unknowndoes not mean the catalog changed. It means the comparison could not be made at all.collectCodexAppServerCatalogStatereturnsunknownwhen the catalog mtime is unreadable, when process enumeration fails, or when any app-server's start time is unreadable:A transient
code-mode-hostprocess that exits between enumeration and the start-time read lands exactly there, which is why the message appears intermittently and alternates with normal guidance from turn to turn, as the reporter describes.So the text asserted a cause that was never established, and prescribed a remedy that cannot work — restarting Codex does not clear
unknown, because the state is a failed measurement rather than a stale process.Suppressing overrides while the state is unknown is still correct and is unchanged. Only the claim changes.
unknownnow gets its own message, mirroring the honest phrasingocx doctoralready uses for the same state (src/cli/doctor.ts):An operator reading a turn can now tell "the catalog changed" apart from "we could not check", and is no longer told to perform a restart that would not help.
Verification
Reproduced with a failing test before the fix —
unknownreturned the stale text verbatim:Commands run against the
devbase (branch is on3976d34):The new test pins four things:
stalekeeps its exact wording;unknownasserts neither the cause nor the restart remedy; both still suppress the override instruction and positive model claims (so the #857 behaviour cannot regress); and the two messages stay distinguishable from each other.The existing #857 test — which asserts both states suppress positive claims — passes unchanged.
Checklist
unknownwording is deliberately aligned with the existingocx doctortext so the two surfaces agree. Happy to add a note if you'd like one.Notes for review
not_runningandfreshare untouched.unknowntext so the instruction itself is unchanged; only the justification and the remedy differ.doctor.tson purpose — if you'd rather the two surfaces diverge, or prefer different phrasing, happy to adjust.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
Tests