Implement factory mid-task Slack questions#333
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR adds GitHub issue ingestion into Linear mirrors, a new ChangesGitHub Issue Ingestion, Agent Messaging, and Slack Bridging
Stray node_modules Symlink
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
Note over GitHubMount,FactoryLoop: GitHub Issue Ingestion
GitHubMount->>FactoryLoop: relayfile /github/repos/<owner>/<repo>/issues/.../<n>.json
FactoryLoop->>FactoryLoop: `#ensureGithubIngestionReady` → sub-root probe
FactoryLoop->>FactoryLoop: parseGithubIssue → GithubIssueSource
FactoryLoop->>FactoryLoop: findExistingMirror → match Linear source metadata
FactoryLoop->>LinearWriteback: createIssue(labels, source, team)
end
rect rgba(144, 238, 144, 0.5)
Note over Agent,HumanInSlack: Agent Question Slack Bridging
Agent->>InternalFleetClient: relay_inbound [factory-needs-input] body
InternalFleetClient->>FactoryLoop: onAgentMessage(AgentMessage)
FactoryLoop->>FactoryLoop: parseAgentQuestion → dedupe → `#slackDispatchThreadFor`
FactoryLoop->>SlackWatcher: post question to dispatch thread
SlackWatcher->>HumanInSlack: message in Slack thread
HumanInSlack->>SlackWatcher: reply
SlackWatcher->>FactoryLoop: answer
FactoryLoop->>Agent: sendInput(answer)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed due to a network error. 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for mirroring factory-labeled GitHub issues into Linear create drafts, closing Linear mirrors when GitHub issues are closed, and routing mid-task agent questions to Slack dispatch threads. The code review feedback highlights a performance bottleneck in #findGithubIssueMirror due to O(N * M) disk reads, suggests making repository label matching more robust, recommends tracking whether GitHub ingestion is enabled via a private field to avoid unnecessary mount operations, and advises using consistent fallback instructions for LLM agents.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| for (const path of await this.#mount.listTree(ISSUE_ROOT)) { | ||
| if (!isLinearIssueMirrorCandidatePath(path)) { | ||
| continue | ||
| } | ||
| const issue = await this.#readIssue(path) | ||
| if (issue && linearIssueMirrorsGithubIssue(issue, ghIssue)) { | ||
| return issue | ||
| } | ||
| } |
There was a problem hiding this comment.
Performance Bottleneck: O(N * M) Disk Reads
In #findGithubIssueMirror, the code lists all files in ISSUE_ROOT and reads each one sequentially using this.#readIssue(path) to check if it mirrors the GitHub issue. Since #findGithubIssueMirror is called for every GitHub issue during ingestion, this results in an O(N * M) complexity of disk/mount reads (where N is the number of GitHub issues and M is the number of Linear issues).
With hundreds or thousands of issues, this will cause severe performance degradation and potential rate-limiting or timeouts on the mount.
Recommendation:
Consider pre-loading or caching the candidate Linear issues once per iteration of runOnce or #ingestGithubIssues, and passing the cached list/map to #findGithubIssueMirror to reduce the complexity to O(N + M) reads.
| #repoLabelForGithubIssue(ghIssue: GithubIssueSource): string | undefined { | ||
| const entry = Object.entries(this.#config.repos.byLabel) | ||
| .find(([, repo]) => repo.toLowerCase() === ghIssue.repo.toLowerCase()) | ||
| return entry?.[0] | ||
| } |
There was a problem hiding this comment.
Robust Repository Label Matching
If repos.byLabel is configured with only the repository name (e.g., "pear" instead of "AgentWorkforce/pear"), the direct comparison repo.toLowerCase() === ghIssue.repo.toLowerCase() will fail because ghIssue.repo always contains the owner prefix. Comparing with ghIssue.repoName as a fallback makes the routing much more robust.
#repoLabelForGithubIssue(ghIssue: GithubIssueSource): string | undefined {
const entry = Object.entries(this.#config.repos.byLabel)
.find(([, repo]) => {
const r = repo.toLowerCase()
return r === ghIssue.repo.toLowerCase() || r === ghIssue.repoName.toLowerCase()
})
return entry?.[0]
}| #offAgentExit?: () => void | ||
| #offDeliveryFailed?: () => void | ||
| #offAgentMessage?: () => void | ||
| #starting?: Promise<void> |
There was a problem hiding this comment.
Declare #githubIngestionEnabled Field
Declare the private field #githubIngestionEnabled to track whether the GitHub issue mount is available and ready for ingestion.
| #offAgentExit?: () => void | |
| #offDeliveryFailed?: () => void | |
| #offAgentMessage?: () => void | |
| #starting?: Promise<void> | |
| #offAgentExit?: () => void | |
| #offDeliveryFailed?: () => void | |
| #offAgentMessage?: () => void | |
| #githubIngestionEnabled = false | |
| #starting?: Promise<void> |
| const githubReady = await this.#mount.ensureSubRoot(GITHUB_ISSUE_ROOT, { timeoutMs: 90_000 }) | ||
| if (githubReady !== 'ready') { | ||
| this.#logger.warn?.(`[factory] ${GITHUB_ISSUE_ROOT} sub-root is not mounted; GitHub issue ingestion disabled`) | ||
| } |
There was a problem hiding this comment.
Avoid Unnecessary Directory Listing When GitHub Mount is Missing
If ensureSubRoot for GITHUB_ISSUE_ROOT fails, we log a warning but still attempt to list and ingest GitHub issues on every iteration. This causes unnecessary mount operations and warnings.
We should track whether GitHub ingestion is enabled using a private field #githubIngestionEnabled and skip #ingestGithubIssues if it is false.
| const githubReady = await this.#mount.ensureSubRoot(GITHUB_ISSUE_ROOT, { timeoutMs: 90_000 }) | |
| if (githubReady !== 'ready') { | |
| this.#logger.warn?.(`[factory] ${GITHUB_ISSUE_ROOT} sub-root is not mounted; GitHub issue ingestion disabled`) | |
| } | |
| const githubReady = await this.#mount.ensureSubRoot(GITHUB_ISSUE_ROOT, { timeoutMs: 90_000 }) | |
| this.#githubIngestionEnabled = githubReady === 'ready' | |
| if (!this.#githubIngestionEnabled) { | |
| this.#logger.warn?.(`[factory] ${GITHUB_ISSUE_ROOT} sub-root is not mounted; GitHub issue ingestion disabled`) | |
| } |
| async #ingestGithubIssues(opts: { dryRun?: boolean } = {}): Promise<void> { | ||
| for (const path of await this.#githubIssuePaths()) { | ||
| await this.#handleGithubIssueChange(path, opts) | ||
| } | ||
| } |
There was a problem hiding this comment.
Skip Ingestion If GitHub Mount is Disabled
Check #githubIngestionEnabled before listing and ingesting GitHub issues to avoid unnecessary mount operations.
async #ingestGithubIssues(opts: { dryRun?: boolean } = {}): Promise<void> {
if (!this.#githubIngestionEnabled) {
return
}
for (const path of await this.#githubIssuePaths()) {
await this.#handleGithubIssueChange(path, opts)
}
}| 'Open a PR targeting `main` when done.', | ||
| 'Use `gh pr create --base main` and report the PR URL.', | ||
| `DM the reviewer \`${input.reviewerName}\` when the PR is ready.`, | ||
| 'If blocked and you need human input, DM `broker` with `FACTORY_NEEDS_INPUT`, `Issue: <key>`, and `Question: <your question>` so the factory can relay it to the issue Slack thread.', |
There was a problem hiding this comment.
Consistent Fallback Instructions for LLM Agents
To avoid confusing the LLM agent with conflicting instructions, use the new preferred [factory-needs-input] format and factory target consistently, matching the fallback instruction on line 61.
| 'If blocked and you need human input, DM `broker` with `FACTORY_NEEDS_INPUT`, `Issue: <key>`, and `Question: <your question>` so the factory can relay it to the issue Slack thread.', | |
| 'If blocked and you need human input, DM `factory` with `[factory-needs-input] <your question>` so the factory can relay it to the issue Slack thread.', |
![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: 6fe5532cba
ℹ️ 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".
| } | ||
|
|
||
| const githubIssuePathParts = (path: string): { owner: string; repo: string; number: number } | undefined => { | ||
| const match = path.match(/^\/github\/repos\/([^/]+)\/([^/]+)\/issues\/by-id\/(\d+)\.json$/u) |
There was a problem hiding this comment.
The repository’s GitHub issue records are exposed under /github/repos/<owner>/<repo>/issues/<number>.json (for example, src/renderer/src/stores/issues-store.ts and the mock records use that shape), but this parser only accepts issues/by-id/*.json and the live glob uses the same suffix. In installations with the existing mount shape, runOnce, startup backfill, and live subscriptions never recognize factory-labeled GitHub issues, so no Linear mirrors are created.
Useful? React with 👍 / 👎.
| description: githubIssueMirrorDescription(issue), | ||
| stateId: config.stateIds.readyForAgent, | ||
| labels: [{ name: repoLabel }], | ||
| team: { key: config.safety.requireTeamKey }, |
There was a problem hiding this comment.
GitHub mirror payloads provide only team.key, but MountLinearWriteback.createIssue writes only teamId or team.id to the draft, so the actual /linear/issues/factory-create-* file has no team information. When a factory-labeled GitHub issue is mirrored through the real Linear writeback path, Linear cannot reliably create the issue in the AR team, and the guard still passes because it checked the pre-stripped payload.
Useful? React with 👍 / 👎.
| const titleHasAcceptedFactoryMarker = (title: string, configuredMarker: string): boolean => | ||
| titleHasFactoryMarker(title, configuredMarker) || titleHasFactoryMarker(title, '[factory]') |
There was a problem hiding this comment.
Accepting [factory] here applies to every Linear issue, not just factory-created GitHub mirrors. With the default [factory-e2e] gate, or any stricter custom requireTitlePrefix, a human-created AR issue titled [factory] ... now passes isInFactoryScope and can be dispatched, weakening the safety guard beyond the mirror use case described in the README.
Useful? React with 👍 / 👎.
Findings
Addressed Comments
Verification
No files were edited. |
Code-review fixes for the factory mid-task Slack questions / GitHub-to-Linear mirroring PR: - Match the live GitHub issue mount shape: accept both /github/repos/<owner>/<repo>/issues/<n>.json (the shape the renderer issues-store reads from real mounts) and the nested .../issues/by-id/<n>.json variant in the parser and live glob (codex P1). - Preserve team routing on GitHub mirror create drafts: when no teamId is resolvable, carry team.key through createIssue so the Linear writeback can route by key instead of writing a teamless draft. Scoped to creates only; setState/postComment unchanged (codex P1). - Gate the bare [factory] scope marker to GitHub mirrors only (source.provider === github), so a stricter custom requireTitlePrefix still rejects human-authored [factory] issues (codex P2). - Cache existing Linear mirror candidates once per ingestion pass so dedupe is O(N + M) reads instead of re-scanning ISSUE_ROOT per GitHub issue (gemini perf). - Match repos.byLabel entries against both owner/name and the bare repo name (gemini robustness). - Probe the GitHub issue sub-root once and skip ingestion entirely when it is absent; lazy so the standalone runOnce() path still ingests when present (gemini). - Align the always-present blocked-input instruction with the preferred `factory` / [factory-needs-input] format (gemini consistency). Adds unit tests for each fix and updates affected expectations. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Findings
Edited
Addressed comments
Verification
Not printing |
1 participant
Summary
onAgentMessagehook forrelay_inboundevents so dispatched agents can send explicit needs-input questions to the factory.Verification
npx tsc -p packages/factory-sdk/tsconfig.json --noEmitnpx vitest run packages/factory-sdk/src/dispatch/templates.test.ts packages/factory-sdk/src/fleet/internal-fleet-client.test.ts packages/factory-sdk/src/orchestrator/factory.test.tsNotes
fleet.sendInputpath.