Skip to content

Opened against wrong repo — please ignore#3686

Closed
SergeSerb2 wants to merge 6 commits into
pingdotgg:mainfrom
SergeSerb2:feat/cross-provider-subagents
Closed

Opened against wrong repo — please ignore#3686
SergeSerb2 wants to merge 6 commits into
pingdotgg:mainfrom
SergeSerb2:feat/cross-provider-subagents

Conversation

@SergeSerb2

@SergeSerb2 SergeSerb2 commented Jul 4, 2026

Copy link
Copy Markdown

Opened here by mistake (intended for a fork). No content.

SergeSerb2 and others added 6 commits July 3, 2026 22:58
Adds a codex-review-required check that fails until the Codex bot has
submitted a review on the PR. Re-runs on pull_request_review so it
flips green as soon as Codex reviews. Enforced via branch ruleset.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Require the Codex review to target the current head SHA so stale
reviews no longer satisfy the gate after new pushes, aggregate
paginated review pages before counting, and ignore dismissed reviews.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codex posts 'no major issues' verdicts as issue comments carrying
'Reviewed commit: <sha>', not as PR reviews, so the gate now accepts
either form for the current head. The check also polls for up to 20
minutes and requests a review via '@codex review' (once per head)
rather than failing immediately and relying on event retriggers.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Per-head re-review requirements and automated '@codex review' requests
burn ChatGPT usage limits. The gate now passes once Codex has responded
to the PR at all (review or clean-verdict comment); re-reviews after
new pushes are handled manually by the maintainer.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ci: require Codex review before merge via status-check gate
Any provider session (Claude, Codex, Cursor, Grok, OpenCode) can now
spawn and drive sub-agent threads on any other configured provider
instance through four new MCP tools on the product MCP server:

- agent_list: connected provider instances, readiness, model slugs
- agent_spawn: create a thread on the target instance in the caller's
  project/worktree and send the initial prompt (returns immediately)
- agent_wait: block up to timeoutSeconds for turn completion and return
  the final assistant text; "running" on timeout so callers re-poll
  instead of tripping MCP client timeouts
- agent_send: follow-up prompts to a spawned thread

Spawned threads go through the orchestration engine, so they persist,
appear in the UI, and keep the normal approval/checkpoint flow. The
SubAgentCoordinator bounds nesting at depth 2, restricts send/wait to
the spawning session, and validates target readiness. Tools are gated
behind a new "agents" MCP capability issued alongside "preview".

Parent/child bookkeeping is in-memory only for now: after a server
restart spawned threads survive as ordinary threads but can no longer
be driven via agent_send/agent_wait.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 4, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 7268ae1a-6714-434b-b177-3c1afcf13ec5

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:XL 500-999 changed lines (additions + deletions). labels Jul 4, 2026
@SergeSerb2 SergeSerb2 closed this Jul 4, 2026
Comment on lines +9 to +10
permissions:
pull-requests: read

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium workflows/codex-review-gate.yml:9

The workflow grants only pull-requests: read, but line 35 calls GET repos/$REPO/issues/$PR/comments, which requires issues: read. The GITHUB_TOKEN lacks that scope, so gh api fails (or returns empty) and the clean-review fallback path never detects a Codex review. Add issues: read to the permissions block.

 permissions:
   pull-requests: read
+  issues: read
🤖 Copy this AI Prompt to have your agent fix this:
In file @.github/workflows/codex-review-gate.yml around lines 9-10:

The workflow grants only `pull-requests: read`, but line 35 calls `GET repos/$REPO/issues/$PR/comments`, which requires `issues: read`. The `GITHUB_TOKEN` lacks that scope, so `gh api` fails (or returns empty) and the clean-review fallback path never detects a Codex review. Add `issues: read` to the `permissions` block.

Comment on lines +52 to +57
interface SubAgentRecord {
readonly parentThreadId: ThreadId;
readonly depth: number;
/** `createdAt` of the most recent turn-start command sent to this child. */
readonly lastTurnRequestedAt: string;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High agents/SubAgentCoordinator.ts:52

requireChildOfCaller only checks record.parentThreadId === scope.threadId, so any MCP session on the same parent thread can call agent_send/agent_wait on a child it did not spawn. Since McpInvocationScope issues a fresh providerSessionId per session, the parent-child ownership check should also verify that the child's owning session matches scope.providerSessionId — otherwise the per-session ownership restriction is bypassed. Consider storing providerSessionId on the SubAgentRecord at spawn time and comparing it in requireChildOfCaller.

 interface SubAgentRecord {
   readonly parentThreadId: ThreadId;
+  readonly providerSessionId: string;
   readonly depth: number;
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/mcp/toolkits/agents/SubAgentCoordinator.ts around lines 52-57:

`requireChildOfCaller` only checks `record.parentThreadId === scope.threadId`, so any MCP session on the same parent thread can call `agent_send`/`agent_wait` on a child it did not spawn. Since `McpInvocationScope` issues a fresh `providerSessionId` per session, the parent-child ownership check should also verify that the child's owning session matches `scope.providerSessionId` — otherwise the per-session ownership restriction is bypassed. Consider storing `providerSessionId` on the `SubAgentRecord` at spawn time and comparing it in `requireChildOfCaller`.

REPO: ${{ github.repository }}
PR: ${{ github.event.pull_request.number }}
run: |
codex_reviewed() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High workflows/codex-review-gate.yml:24

codex_reviewed() counts any non-dismissed Codex review or issue comment on the PR without checking that it targets github.event.pull_request.head.sha. After a synchronize push, the previous Codex review/comment is still present on the PR, so the function returns success for a stale review and the gate lets unreviewed new commits merge. Consider filtering reviews/comments by the current head SHA (e.g. matching Reviewed commit: <sha> for issue comments and the review's commit_id for PR reviews) so only a review of the latest commit satisfies the check.

🤖 Copy this AI Prompt to have your agent fix this:
In file @.github/workflows/codex-review-gate.yml around line 24:

`codex_reviewed()` counts any non-dismissed Codex review or issue comment on the PR without checking that it targets `github.event.pull_request.head.sha`. After a `synchronize` push, the previous Codex review/comment is still present on the PR, so the function returns success for a stale review and the gate lets unreviewed new commits merge. Consider filtering reviews/comments by the current head SHA (e.g. matching `Reviewed commit: <sha>` for issue comments and the review's `commit_id` for PR reviews) so only a review of the latest commit satisfies the check.

})
.pipe(Effect.mapError(dispatchFailed("create sub-agent thread")));

const lastTurnRequestedAt = yield* startTurn(childThreadId, input.prompt, parent.runtimeMode);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium agents/SubAgentCoordinator.ts:276

If startTurn (dispatching thread.turn.start) fails after the thread.create dispatch succeeds, spawn returns a dispatch-failed error but the child thread is already persisted. Because the children map is only updated after both dispatches succeed, the orphaned thread is never recorded — subsequent agent_send/agent_wait calls for it will return thread-not-found and the caller cannot drive or clean it up. Consider registering the child in children immediately after thread.create succeeds (before startTurn), or rolling back the created thread when the turn dispatch fails.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/mcp/toolkits/agents/SubAgentCoordinator.ts around line 276:

If `startTurn` (dispatching `thread.turn.start`) fails after the `thread.create` dispatch succeeds, `spawn` returns a `dispatch-failed` error but the child thread is already persisted. Because the `children` map is only updated after both dispatches succeed, the orphaned thread is never recorded — subsequent `agent_send`/`agent_wait` calls for it will return `thread-not-found` and the caller cannot drive or clean it up. Consider registering the child in `children` immediately after `thread.create` succeeds (before `startTurn`), or rolling back the created thread when the turn dispatch fails.

@SergeSerb2 SergeSerb2 changed the title Add cross-provider sub-agent spawning via product MCP toolkit Opened against wrong repo — please ignore Jul 4, 2026

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 3 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit b16c18b. Configure here.

}
yield* Effect.sleep(Duration.millis(WAIT_POLL_INTERVAL_MILLIS));
}
},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale wait after agent_send

High Severity

The agent_wait function captures lastTurnRequestedAt only once when it starts. This value can become stale during polling, especially if a new turn is initiated concurrently. As a result, turnStatus might incorrectly report a child agent's turn as completed and return outdated finalText, even if a new turn is still running or not yet reflected.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit b16c18b. Configure here.

lastTurnRequestedAt,
});
return next;
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Orphan thread on spawn failure

Medium Severity

The agent_spawn command dispatches thread creation and the initial turn separately. If the initial turn fails, the thread is created in the system but isn't recorded in the in-memory sub-agent map, making it unreachable by agent_send or agent_wait.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit b16c18b. Configure here.

and (.user.login | ascii_downcase | contains("codex"))
and (.body | test("Reviewed commit:"))
)] | length')
[ "$n" -ge 1 ]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gate accepts stale Codex reviews

Medium Severity

The Codex review gate treats any historical bot comment containing Reviewed commit: as sufficient. After new commits are pushed, an review tied to an older SHA can still satisfy the check without re-reviewing the current head.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit b16c18b. Configure here.

@macroscopeapp

macroscopeapp Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs human review

4 blocking correctness issues found. This PR introduces a substantial new cross-provider sub-agent orchestration feature with ~1000 lines of new code. Multiple high-severity unresolved review comments identify security concerns (session ownership verification bypass) and correctness issues (stale review acceptance, orphan threads). The PR title itself indicates it may have been opened in error.

You can customize Macroscope's approvability policy. Learn more.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b16c18b2df

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[add[] | select(
(.user.type == "Bot")
and (.user.login | ascii_downcase | contains("codex"))
and (.body | test("Reviewed commit:"))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Require Codex review for the current head

This accepts any old clean Codex comment because it only tests for the marker, and the review path above similarly counts all non-dismissed Codex reviews without checking the review commit_id. On a synchronize run after new commits are pushed, an earlier Codex review/comment for the previous head still makes the required check pass, so the gate no longer proves the code being merged was reviewed; compare the review/comment SHA with github.event.pull_request.head.sha.

Useful? React with 👍 / 👎.

const latestTurn = thread.latestTurn;
// The projection lags the dispatched turn-start command; treat a missing
// or older latest turn as the requested turn still spinning up.
if (!latestTurn || latestTurn.requestedAt < sinceIso) return "running";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Return errors when a sub-agent turn never starts

When thread.turn.start is accepted but the provider startup/send path fails before any runtime turn id is projected, thread.latestTurn stays null while the failure is recorded on the session/activity stream. This branch keeps reporting running, so every agent_wait call just times out and tells the caller to poll again forever instead of surfacing the sub-agent failure; handle the session error/lastError (or failure activity) once the requested turn has not materialized.

Useful? React with 👍 / 👎.

pull_request:
types: [opened, synchronize, reopened, ready_for_review]
pull_request_review:
types: [submitted]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Recheck the gate when Codex reviews are dismissed

The workflow explicitly ignores dismissed reviews in the jq filter, but it never runs for the pull_request_review.dismissed action. If a Codex review is dismissed after this check has passed, the required status remains green until some unrelated PR event reruns it, so the gate can still report that a non-dismissed Codex review exists when it no longer does; include dismissed in this trigger.

Useful? React with 👍 / 👎.

);
yield* SynchronizedRef.update(children, (current) => {
const next = new Map(current);
next.set(input.threadId, { ...record, lastTurnRequestedAt });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep waits working after steering a running sub-agent

When agent_send is called while the child is still running, the Claude/OpenCode/Cursor adapters treat the follow-up as steering and reuse the active turn id rather than creating a new projected turn. Updating lastTurnRequestedAt to the follow-up command time then makes the existing turn look permanently older than the request, so agent_wait keeps returning running even after that steered turn completes; either reject/queue sends while running or track the active turn id instead of only the new request timestamp.

Useful? React with 👍 / 👎.

Comment on lines +270 to +271
branch: parent.branch,
worktreePath: parent.worktreePath,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid renaming the caller's temporary worktree branch

Copying the caller's branch into the child is unsafe when the caller is still on a generated temporary worktree branch: the child's first turn also runs the first-turn branch-renaming path for the same worktreePath, so it can rename the shared git branch using the child prompt and only update the child thread metadata. That leaves the parent thread pointing at a branch that no longer exists, or races the parent's own first-turn rename; skip the auto-rename for spawned children or avoid copying temporary branch names as if they were child-owned.

Useful? React with 👍 / 👎.

// or older latest turn as the requested turn still spinning up.
if (!latestTurn || latestTurn.requestedAt < sinceIso) return "running";
if (latestTurn.state === "running") return "running";
if ((thread.session?.activeTurnId ?? null) !== null) return "running";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Don't hide settled failures behind activeTurnId

When a provider reports runtime.error or a session.state.changed error/interruption while a turn is active, ingestion can write a non-running session status while preserving activeTurnId, and projection settles latestTurn.state to error/interrupted. This check then forces agent_wait back to running solely because that stale active turn id is still set, so callers keep polling instead of seeing the failed/interrupted sub-agent; only treat activeTurnId as running while the session status is actually running.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XL 500-999 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant