Conversation
- add subagent run contracts, coordinator, and skill catalog wiring across server orchestration - persist/project subagent runs and hide subagent threads from normal snapshot thread lists - add web report card UX for subagent runs and composer integration for report reuse - harden git/worktree lifecycle with safe missing-worktree cleanup and local branch delete support
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure the |
github-actions
bot
added
size:XXL
![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: aa07df8452
ℹ️ 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".
| projectId: parentThread.projectId, | ||
| title: buildSubagentThreadTitle(skill, event.payload.task), | ||
| model: parentThread.model, | ||
| runtimeMode: parentThread.runtimeMode, |
There was a problem hiding this comment.
Creating the sub-agent thread with runtimeMode: parentThread.runtimeMode breaks /skill when the parent thread is in approval-required mode: hidden sub-agent turns can emit approval/user-input requests, but those threads are intentionally omitted from client snapshots (ProjectionSnapshotQuery filters out threadKind === "subagent"), so users have no UI path to answer them and the run can stall in running indefinitely. Use a non-interactive mode for hidden threads or surface those requests on the parent thread.
Useful? React with 👍 / 👎.
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| pr: null, | ||
| }; | ||
|
|
||
| const makeSubagentCoordinator = Effect.gen(function* () { |
There was a problem hiding this comment.
🟠 High Layers/SubagentCoordinator.ts:178
The try/catch in processStartRequested only catches synchronous exceptions, not Effect failures from yield*. When git.createWorktree, orchestrationEngine.dispatch, or upsertRun fail as Effects, the error propagates through the Effect error channel and bypasses the catch block entirely—so cleanup never runs and the run stays stuck without error state. Consider using Effect.catchAll or a similar Effect error handler instead of try/catch to ensure all failure paths execute cleanup and mark the run as failed.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/subagents/Layers/SubagentCoordinator.ts around line 178:
The `try/catch` in `processStartRequested` only catches synchronous exceptions, not Effect failures from `yield*`. When `git.createWorktree`, `orchestrationEngine.dispatch`, or `upsertRun` fail as Effects, the error propagates through the Effect error channel and bypasses the catch block entirely—so cleanup never runs and the run stays stuck without error state. Consider using Effect.catchAll or a similar Effect error handler instead of try/catch to ensure all failure paths execute cleanup and mark the run as failed.
Evidence trail:
apps/server/src/subagents/Layers/SubagentCoordinator.ts lines 255-378 (REVIEWED_COMMIT) - shows processStartRequested function with try/catch inside Effect.gen and yield* statements inside the try block. Effect-TS/language-service README.md on GitHub (https://github.com/Effect-TS/language-service/blob/main/README.md) - documents `tryCatchInEffectGen` diagnostic that 'Discourages try/catch in Effect generators in favor of Effect error handling'. The same file also uses Effect.catch() at lines 215, 228, 252, 364, 423, 457, 537 showing proper Effect error handling patterns in the codebase.
| } | ||
|
|
||
| function extractSection(markdown: string, heading: string): string | null { | ||
| const pattern = new RegExp(`^##\\s+${heading}\\s*$([\\s\\S]*?)(?=^##\\s+|$)`, "im"); |
There was a problem hiding this comment.
🟠 High Layers/SubagentCoordinator.ts:96
The $ inside the lookahead (?=^##\s+|$) matches end-of-line (not end-of-string) because the m flag is set. Since [\s\S]*? is non-greedy, it matches zero characters and then the lookahead immediately succeeds via $ at the end of the heading line. This causes the capture group to always be empty, so extractSection returns null for every heading. Replace $ in the lookahead with (?![\s\S]) to anchor to end-of-string instead of end-of-line.
| const pattern = new RegExp(`^##\\s+${heading}\\s*$([\\s\\S]*?)(?=^##\\s+|$)`, "im"); | |
| const pattern = new RegExp(`^##\\s+${heading}\\s*$([\\s\\S]*?)(?=^##\\s+|(?![\\s\\S]))`, "im"); |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/subagents/Layers/SubagentCoordinator.ts around line 96:
The `$` inside the lookahead `(?=^##\s+|$)` matches end-of-line (not end-of-string) because the `m` flag is set. Since `[\s\S]*?` is non-greedy, it matches zero characters and then the lookahead immediately succeeds via `$` at the end of the heading line. This causes the capture group to always be empty, so `extractSection` returns `null` for every heading. Replace `$` in the lookahead with `(?![\s\S])` to anchor to end-of-string instead of end-of-line.
Evidence trail:
apps/server/src/subagents/Layers/SubagentCoordinator.ts line 96 (commit REVIEWED_COMMIT) shows the regex pattern: `new RegExp(`^##\\s+${heading}\\s*$([\\s\\S]*?)(?=^##\\s+|$)`, "im")`. The `m` flag enables multiline mode where `$` matches before newlines, not just at end-of-string. JavaScript MDN documentation confirms multiline flag behavior: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/multiline
There was a problem hiding this comment.
🟡 Medium
t3code/apps/web/src/composer-logic.ts
Lines 175 to 176 in aa07df8
When the user types exactly /skill (no trailing space), commandQuery is "skill", which matches the generic SLASH_COMMANDS check on line 176 and returns kind: "slash-command" instead of kind: "slash-skill". This is inconsistent with /model, which has a dedicated check on line 168 that returns kind: "slash-model" immediately. Add an equivalent special case for "skill" before the generic SLASH_COMMANDS check.
}
+ if (commandQuery.toLowerCase() === "skill") {
+ return {
+ kind: "slash-skill",
+ query: "",
+ rangeStart: lineStart,
+ rangeEnd: cursor,
+ };
+ }
if (SLASH_COMMANDS.some((command) => command.startsWith(commandQuery.toLowerCase()))) {🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/src/composer-logic.ts around lines 175-176:
When the user types exactly `/skill` (no trailing space), `commandQuery` is `"skill"`, which matches the generic `SLASH_COMMANDS` check on line 176 and returns `kind: "slash-command"` instead of `kind: "slash-skill"`. This is inconsistent with `/model`, which has a dedicated check on line 168 that returns `kind: "slash-model"` immediately. Add an equivalent special case for `"skill"` before the generic `SLASH_COMMANDS` check.
Evidence trail:
apps/web/src/composer-logic.ts lines 18, 165-182, 197-205 at REVIEWED_COMMIT. Line 18 shows SLASH_COMMANDS includes 'skill'. Lines 168-174 show special case for 'model' returning 'slash-model'. Line 176 shows generic SLASH_COMMANDS check returning 'slash-command'. Lines 197-205 show skillMatch regex (returning 'slash-skill') is only reached after the generic check, making it unreachable for `/skill` without trailing space.
1 participant
test
Note
Add sub-agent skill orchestration with
/skillcomposer command and worktree thread managementthread.subagent.startvia/skill <id> <task>in the composer, theSubagentCoordinatorservice starts runs, and users can accept reports, open worktree threads, or discard runs from the newSubagentReportCardUI.thread.subagent-upserted,thread.subagent-start-requested,thread.subagent-cleanup-requested,thread.subagent-report-accepted) with corresponding decider handlers and projector logic in orchestration.ts.SkillCatalogservice that readsSKILL.mdfiles from~/.codex/skills, exposes skills vialistSkills/getSkillById, and surfaces them in theserverGetConfigWebSocket response.projection_thread_subagent_runstable (migration 014) and attaches sortedsubagentRunsarrays to thread snapshots.GitCorewithdeleteLocalBranchand makesremoveWorktreea no-op for missing paths; forwardsdeveloperInstructionsthrough the provider turn pipeline to the Codex manager.removeWorktreeno longer errors on missing paths or when git reports 'is not a working tree'.📊 Macroscope summarized aa07df8. 44 files reviewed, 9 issues evaluated, 3 issues filtered, 3 comments posted
🗂️ Filtered Issues
apps/server/src/git/Layers/GitCore.ts — 0 comments posted, 1 evaluated, 1 filtered
result.failureon aResult.Resulttype returned byEffect.resultwill fail at runtime. TheResult.Resultfailure case uses the propertyerror, notfailure. This causes a crash whenremoveWorktreeencounters a git error that is not "is not a working tree" - the code at lines 1285, 1293, and 1294 will throwCannot read properties of undefined (reading 'message')becauseresult.failureisundefined. [ Failed validation ]apps/server/src/wsServer.test.ts — 0 comments posted, 1 evaluated, 1 filtered
removeWorktreefunction inapps/server/src/git/Layers/GitCore.ts(changed in this commit), after callingEffect.result(...), the code accessesresult.failure.messageon the failure branch. However,Effect.resultreturns aResult.Result<A, E>whose failure variant has anerrorproperty, notfailure. Accessingresult.failureyieldsundefined, causingTypeError: Cannot read properties of undefined (reading 'message')at runtime whenevergit worktree removefails. The same incorrect property access (result.failure) appears three times in the error-handling path. The fix is to replaceresult.failurewithresult.error. While this bug is in a reference file, it is introduced by this commit and would surface at runtime when theSubagentCoordinator'scleanupResourcesinvokesgit.removeWorktreeon a failed worktree removal. [ Cross-file consolidated ]apps/web/src/composer-logic.ts — 1 comment posted, 2 evaluated, 1 filtered
ComposerTriggerKindincludes"slash-skill"andSLASH_COMMANDSincludes"skill", butdetectComposerTriggerlacks early-return handling for the completed/skillcommand. When a user types exactly/skill(no trailing space), the function returns{ kind: "slash-command", query: "skill" }instead of{ kind: "slash-skill", ... }. This is inconsistent with the/modelcase (lines 168-175) which returns"slash-model"immediately whencommandQuery === "model". Consumers expecting"slash-skill"kind for skill autocomplete triggering won't receive it until a space is typed. [ Out of scope ]