fix(subagents): preserve role routing and assign discovery early - #130
fix(subagents): preserve role routing and assign discovery early#130thisisjun786 wants to merge 12 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1587ae6bda
ℹ️ 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".
thisisjun786
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b811d3f4a
ℹ️ 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".
|
Follow-up inspection of This pass checked the The change addresses two separate problems:
I checked the automated findings with isolated synthetic fixtures against the current source. These checks invoke the local hook/resolver and fixture dispatch store; they do not create native agents or call a provider.
The current head has 13 successful hosted checks. Its existing focused-test receipt still matches this exact SHA (51 passed, 0 failed); the dispatch-guidance source and generated export also match. Those checks do not cover the five cases above. The published behavior evidence remains a small native-CLI case study: repaired Astra and Sol each used one configured explorer and completed, while narrow/no-delegation cases used none. The CLI-version change, earlier incomplete Sol probes, and recovered attempt-ID error remain disclosed. This establishes observed behavior, not universal dispatch behavior or net cost savings. blocking_issues: the five verified P2 cases above. This comment records their confirmation; it does not mark their review threads resolved. Verdict: repairs needed before merge. |
thisisjun786
changed the title
|
Follow-up to the reproduced five findings: all five are fixed and their review threads are resolved at
Evidence: red-green regressions, final independent review PASS, 266 current-head focused checks, 2,960 full-suite passes / 0 failures / 72 skips, and 13/13 hosted checks. The preserved local integration separately passed 2,961 / 0 / 72. The installed compiled code passed all 13 new tests; the actual installed hook CLI passed the payload smoke, with 13 matching source/artifact hashes and 29 trusted hook hashes. Repair record and reproducible checks. These checks establish routing/payload behavior; they do not claim universal delegation frequency or net cost savings. The earlier native behavior study retains its original scope and limitations. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: daa3cb6bb5
ℹ️ 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".
| if (/^\[CXC-DISPATCH:/m.test(dispatchScan)) { | ||
| let dispatchSource = ""; | ||
| let dispatchError: unknown; | ||
| const sources = dispatchSources(validItems ? textItems[0]?.text ?? "" : message, cwd); |
There was a problem hiding this comment.
When a managed V1 items request is produced by buildSpawnItems with cwd and a task containing an existing path, spawn-wrapper.ts:256-261 inserts a path-hint text item before the trailing task item where the dispatch marker is prepended. Inspecting only textItems[0] therefore misses the valid marker: the hook uses the primary configured model instead of the claimed candidate, adds the unmanaged warning, and leaves spawnIssued false even though the child is launched. Identify the producer-controlled task item rather than assuming the first text item is the dispatch header.
Useful? React with 👍 / 👎.
| // and before the allow/no-op below so a denial reaches the caller unchanged. | ||
| const gateCheck = checkFinalGatePrereqs( | ||
| evidenceExemptMessage, | ||
| updatedItems ? updatedItems.filter(item => item.type === "text").map(item => item.text).join("\n\n") : evidenceExemptMessage, |
There was a problem hiding this comment.
In a goalplan-bound session whose final-gate receipts are not yet present, any supplemental text item containing the literal [CXC-FINAL-GATE]—for example, a quoted log or this hook's source—now causes an unrelated items-form spawn to be denied because every text item is concatenated and checkFinalGatePrereqs uses a raw includes check. Non-text attachment metadata is deliberately excluded from marker authority, but quoted text needs the same protection; inspect only the producer-controlled task packet while still supporting the trailing task item emitted by buildSpawnItems.
Useful? React with 👍 / 👎.
| const mappedItems = validItems ? itemInput.map(item => { | ||
| if (item.type !== "text") return item; | ||
| const controlled = stripControlMarkers(item.text, true); | ||
| const normalized = skillsDir ? normalizeSkillMentions(controlled, skillsDir) : controlled; |
There was a problem hiding this comment.
When an items-form root spawn requests recursion and the host reapplies the hook to its rewritten input, this mapping strips the hook-minted [CXC-SUBSPAWN-GRANT:…] from the coordinator instruction. Because the public request token was already removed, the second pass also selects the ordinary leaf guard, leaving two contradictory guards and no usable capability, so the authorized child cannot perform its one permitted spawn. Recognize and preserve the exact hook-owned coordinator/grant prefix on root reapplication while still consuming the grant when a child actually presents it.
Useful? React with 👍 / 👎.
| ? (firstText < 0 ? "" : mappedItems[firstText].text as string) | ||
| : stripControlMarkers(message); | ||
| const normalizedMessage = !mappedItems && skillsDir ? normalizeSkillMentions(controlledMessage, skillsDir) : controlledMessage; | ||
| const role = managed?.role ?? inferRole(toolInput.agent_type, validItems ? dispatchScan : normalizedMessage); |
There was a problem hiding this comment.
For structured items where supporting text precedes the trailing task item, any quoted CXC-ROLE: line in that supporting text is included in dispatchScan and wins inferRole's first-marker match before the real task header. This can route an explicitly explorer-typed request to the reviewer configuration, including the wrong model, effort, and prompt, whenever a source excerpt contains such a marker before the task's TASK: line. Pass only the producer-controlled task header to role inference rather than concatenating every text item.
Useful? React with 👍 / 👎.
1 participant
Explorer requests could inherit the parent model when callers used native
items, while review-related words could override an explicitly selected explorer role. This change preserves explicit role selection, configured model/effort, attachments and both supported input forms. It also makes discovery ownership an early decision: delegate an independent question before reading that source, revisit the split as scope grows, and keep narrow lookups and explicit no-delegation local. SessionStart spells out the actual dispatch report schema.The reviewed payload paths now deduplicate skill bodies across all text items and enforce the complete projected-text limit, preserve item whitespace, and keep ignored-config warnings/guards stable on reapplication. Managed dispatch markers are accepted only in the first producer header or behind exact hook-owned prefixes; quoted logs cannot consume another spawn's claim. Both reviewer recovery producers preserve reviewer settings on native, legacy explorer, and fieldless hosts.
Validation:
daa3cb6b: focused source receipt 266 passed, 0 failed, clean source identity. Final implementation26f0bc25: full suite 3,032 tests, 2,960 passed, 0 failed, 72 skipped. The following commit only archives evidence and updates README counts.Evidence: payload repair and reproducible checks, earlier discovery behavior study.
Limits: delegation remains agent-followed guidance. The earlier CLI baseline/candidate versions differed (0.153.4/0.154.0), one Sol probe timed out, and a final attempt-ID error was rejected and recovered. That small sample establishes neither universal behavior nor net cost savings. Local account-catalog changes are preserved; private logs and account data remain outside Git. No provider defaults, retry policy, upstream merge or deployment is changed.
Current PR head
daa3cb6b: all 13 hosted checks passed, including Linux/macOS/Windows suites, the aggregate CI check and packed-install lifecycle. CI run, packed-install run. The preserved local integration at7cdeae00separately passed its 3,033-test suite (2,961 passed, 0 failed, 72 skipped), including its additional local catalog regression.