fix(factory): fall back to direct GitHub API when the Relayfile issue projection is stale - #225
fix(factory): fall back to direct GitHub API when the Relayfile issue projection is stale#225khaliqgant wants to merge 19 commits into
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
📝 WalkthroughWalkthroughThe PR adds projection-first GitHub issue resolution with an unauthenticated REST fallback. It records resolution metadata, supports qualified selectors, revalidates fallback-resolved issues during dispatch, and expands CLI, adapter, and recovery test coverage. ChangesGitHub issue resolution
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant FleetCLI
participant RelayfileProjection
participant GithubApiIssueRead
participant FactoryLoop
FleetCLI->>RelayfileProjection: Resolve issue selector
RelayfileProjection-->>FleetCLI: Return issue or projection health
FleetCLI->>GithubApiIssueRead: Read eligible fallback issue
GithubApiIssueRead-->>FleetCLI: Return found, not-found, or indeterminate
FleetCLI->>FactoryLoop: Dispatch resolved issue
FactoryLoop->>GithubApiIssueRead: Revalidate missing projection
GithubApiIssueRead-->>FactoryLoop: Return issue or provider result
FactoryLoop-->>FleetCLI: Return dispatch result with resolution metadata
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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: cff7303c6a
ℹ️ 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".
| headers: { | ||
| accept: 'application/vnd.github+json', | ||
| 'user-agent': '@agent-relay/factory', | ||
| }, |
There was a problem hiding this comment.
When the configured repository is private, this request carries no GitHub authentication, so GitHub returns the same 404 used for nonexistent resources; line 44 then converts that response to undefined, and triage/dispatch incorrectly reports zero matches. Because every RelayfileCloudMountClient receives this reader even when its connected GitHub App can access private repositories, the new stale-projection recovery path cannot recover private issues.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3eb01ca. GithubConnectionRead.getIssue now returns a three-outcome GithubIssueLookup (found / not-found / indeterminate) instead of GithubConnectionIssue | undefined. The reader confirms repo visibility with a cached, unauthenticated repo-level GET before trusting an issue-level 404 as not-found; a 404 against a repo it can't confirm public now returns indeterminate instead. Both consumers (orchestrator/factory.ts#readGithubIssue, cli/fleet.ts findGithubIssueThroughConnection) were updated to stop collapsing indeterminate into absence — the CLI path now also refuses to dispatch to a single found match when another probed repo in the same org-wide lookup is indeterminate, since a same-numbered issue could exist there too and silently returning the one repo that answered would misroute dispatch.
| function configuredGithubIssueRepos(config: FactoryConfig): string[] { | ||
| const candidates = config.repos.default | ||
| ? [config.repos.default] |
There was a problem hiding this comment.
When a multi-repository config has repos.default, this reduces the configured repository set to only that default. parseGithubIssueSelector reuses this set to validate explicit repo#number selectors, so a configured non-default route such as cloud#222 is rejected as unconfigured even if it appears in byLabel, byProject, or keywordRules. Keep the default-only behavior for bare numbers, but validate qualified selectors against all configured routes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1cef8c3. Split configuredGithubIssueRepos (kept as-is, still collapses to repos.default for bare-number resolution — that behavior is intentional) from a new allConfiguredGithubIssueRepos (default ∪ every route from byLabel/byProject/keywordRules), and pointed parseGithubIssueSelector's qualified-selector validation at the new one. A route like cloud#222 reachable only through byLabel now validates correctly even when repos.default is set to something else.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 12 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…ed route configuredGithubIssueRepos collapsed to repos.default whenever it was set, so parseGithubIssueSelector rejected a valid repo-qualified selector (e.g. cloud#222) reachable only through byLabel, byProject, or keywordRules. Bare-number resolution intentionally stays default-only; qualified selectors now validate against the full configured route set via a new allConfiguredGithubIssueRepos.
… misses
GithubApiIssueRead made every lookup unauthenticated, then treated a 404
as a confirmed absence. GitHub returns the same 404 for "issue does not
exist" and "repository is private, existence hidden from an
unauthenticated caller" — so a real issue in a private repo was
misreported as gone, and the reader's own doc comment ("provider-
authoritative", "returns undefined only when GitHub authoritatively
reports no issue") was already false for anything it couldn't prove
public.
GithubConnectionRead.getIssue now returns a three-outcome
GithubIssueLookup (found / not-found / indeterminate) instead of
GithubConnectionIssue | undefined. The reader confirms repository
visibility with one unauthenticated, cached repo-level GET before
trusting an issue-level 404 as not-found; a 404 against a repo it
cannot confirm public, or a rate-limited/ambiguous probe response,
degrades to indeterminate rather than throwing or manufacturing
absence.
Both real consumers of the port are updated to stop collapsing
indeterminate into absence:
- orchestrator/factory.ts #readGithubIssue no longer folds an
indeterminate result into the phantomSkipped ("confirmed gone")
counter; it now counts and warn-logs it separately.
- cli/fleet.ts findGithubIssueThroughConnection throws a distinct
"could not determine" error instead of a false "found 0 matches",
and — the sharper bug — refuses to dispatch to a single found match
when another configured repo in the same org-wide probe came back
indeterminate, since a same-numbered issue could exist there too and
silently picking the one repo that answered would misroute dispatch.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/orchestrator/factory.ts (1)
2389-2399: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not mutate the resolved
DispatchResultin place.Line 2393 assigns
issueResolutiononto the object returned by#dispatchUnlocked. That object is frequently a retained record, not a fresh value:
- Line 2407 returns
existingRecord.result(held by the batch).- Line 2413 returns
durable.result(loaded from the durable lifecycle).- Line 2548 returns
recoveredRecord.result.- Line 2619 returns
record.result, which#saveDispatchLifecyclelater persists throughlifecycleFromInFlightRecord.The mutation therefore writes CLI-scoped resolution provenance into batch and durable lifecycle state, and a later
#saveDispatchLifecyclecan persist it. The duplicate-suppression path at line 2386 also returns the same shared promise, so a second caller observes the first caller'sissueResolution.Return a copy instead of mutating the shared object.
🐛 Proposed fix to return a copy
try { const result = await dispatched - if (decision.issueResolution) result.issueResolution = structuredClone(decision.issueResolution) - return result + return decision.issueResolution + ? { ...result, issueResolution: structuredClone(decision.issueResolution) } + : result } finally {🤖 Prompt for 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. In `@src/orchestrator/factory.ts` around lines 2389 - 2399, In the dispatch flow around `#dispatchUnlocked`, stop assigning issueResolution directly onto the resolved DispatchResult because it may be shared by batch or durable lifecycle records. Return a shallow copy of the result with a cloned issueResolution when decision.issueResolution exists, while preserving the original result for callers without issue resolution and leaving the in-flight cleanup unchanged.
🧹 Nitpick comments (2)
src/mount/github-api-issue-read.ts (1)
132-162: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider coalescing concurrent visibility probes.
#isRepoPubliccaches only the resolved verdict. Two concurrentgetIssuecalls for the same repository send two probe requests. Unauthenticated GitHub REST allows roughly 60 requests per hour per IP, and each lookup already costs up to two requests. Caching the in-flight promise keeps the probe to one request per repository.♻️ Proposed change to cache the in-flight probe
- readonly `#repoIsPublic` = new Map<string, boolean>() + readonly `#repoIsPublic` = new Map<string, boolean>() + readonly `#repoIsPublicInFlight` = new Map<string, Promise<boolean | undefined>>()async `#isRepoPublic`(owner: string, name: string): Promise<boolean | undefined> { const key = `${owner}/${name}`.toLowerCase() const cached = this.#repoIsPublic.get(key) if (cached !== undefined) return cached + const inFlight = this.#repoIsPublicInFlight.get(key) + if (inFlight) return inFlight + const probe = this.#probeRepoIsPublic(key, owner, name) + .finally(() => this.#repoIsPublicInFlight.delete(key)) + this.#repoIsPublicInFlight.set(key, probe) + return probe + } + async `#probeRepoIsPublic`(key: string, owner: string, name: string): Promise<boolean | undefined> { let response: Response🤖 Prompt for 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. In `@src/mount/github-api-issue-read.ts` around lines 132 - 162, Update `#isRepoPublic` to cache and reuse the in-flight visibility probe promise per normalized repository key, so concurrent callers share one request. Ensure the entry is cleared after the probe settles, while preserving the existing resolved true/false cache behavior and undefined result handling.src/orchestrator/factory.ts (1)
4679-4692: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winA fallback-resolved issue is re-fetched on every read.
The provider result is parsed and returned but never cached.
#dispatchUnlockedcalls#readIssuefor the same path at line 2434, again at line 2629, and again at line 2655. A live dispatch of a fallback-resolved issue therefore performs three GitHub lookups, and each lookup costs up to two unauthenticated requests throughGithubApiIssueRead. The unauthenticated GitHub REST budget is roughly 60 requests per hour per IP, so a single dispatch can consume a noticeable share of it and later reads can degrade toindeterminate.Cache the parsed issue per identity for the duration of the dispatch.
🤖 Prompt for 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. In `@src/orchestrator/factory.ts` around lines 4679 - 4692, Cache the parsed fallback issue by its GitHub identity within the dispatch lifecycle so repeated `#readIssue` calls reuse it instead of invoking `#mount.githubRead` again. Update the fallback branch around githubIssueIdentity and `#indexDependencyIssue` to consult and populate a per-dispatch cache, while preserving the existing lookup, parsing, warning, and return behavior for uncached issues.
🤖 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 `@src/cli/fleet.ts`:
- Around line 1902-1916: Update the qualified repository resolution around
requested, configured, and expanded so label-based selectors can resolve routes
outside config.repos.org. When requested lacks “/”, try the org-qualified
candidate and the matching config.repos.byLabel value, then retain the first
candidate that matches configured; preserve direct owner/repository selectors
and the existing error when neither candidate is configured.
In `@src/orchestrator/factory.ts`:
- Around line 13473-13475: Update the issue comment formatting around
decision.issueResolution so dispatchComment/#postIssueComment outputs only
issueResolution.source, omitting issueResolution.detail. Preserve detail in the
JSON result and logging paths, and leave the existing undefined behavior when no
resolution exists unchanged.
---
Outside diff comments:
In `@src/orchestrator/factory.ts`:
- Around line 2389-2399: In the dispatch flow around `#dispatchUnlocked`, stop
assigning issueResolution directly onto the resolved DispatchResult because it
may be shared by batch or durable lifecycle records. Return a shallow copy of
the result with a cloned issueResolution when decision.issueResolution exists,
while preserving the original result for callers without issue resolution and
leaving the in-flight cleanup unchanged.
---
Nitpick comments:
In `@src/mount/github-api-issue-read.ts`:
- Around line 132-162: Update `#isRepoPublic` to cache and reuse the in-flight
visibility probe promise per normalized repository key, so concurrent callers
share one request. Ensure the entry is cleared after the probe settles, while
preserving the existing resolved true/false cache behavior and undefined result
handling.
In `@src/orchestrator/factory.ts`:
- Around line 4679-4692: Cache the parsed fallback issue by its GitHub identity
within the dispatch lifecycle so repeated `#readIssue` calls reuse it instead of
invoking `#mount.githubRead` again. Update the fallback branch around
githubIssueIdentity and `#indexDependencyIssue` to consult and populate a
per-dispatch cache, while preserving the existing lookup, parsing, warning, and
return behavior for uncached issues.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2b35f072-178d-4f5d-9ca7-f6e5f13521c3
📒 Files selected for processing (12)
.agent-notes/factory-dispatch-unblock.mdsrc/cli/fleet.test.tssrc/cli/fleet.tssrc/index.tssrc/mount/github-api-issue-read.test.tssrc/mount/github-api-issue-read.tssrc/mount/relayfile-cloud-mount-client.tssrc/orchestrator/factory.tssrc/ports/index.tssrc/ports/mount.tssrc/triage/schema.tssrc/types.ts
If the repo-visibility probe cleanly confirms a repo is not visible unauthenticated (a 404, not an ambiguous/rate-limited response), the issue-level GET was still being made and only then discarded as indeterminate at its own 404. That second call can never inform the result — an invisible repo's issues are invisible too — so it only burned rate-limit budget (60 req/hr, unauthenticated) for nothing, and in this org private is the common case, not the edge. isPublic === false now returns indeterminate immediately, before the issue fetch. Added a test asserting the issue endpoint is never called when the repo probe cleanly 404s, checked on fetch call count rather than outcome alone.
allConfiguredGithubIssueRepos (added in 1cef8c3) includes every byLabel/byProject/keywordRules route regardless of owner, but parseGithubIssueSelector's expansion only ever tried `${repos.org}/${requested}` when repos.org was set, and fell back to the label's own route only when repos.org was unset. A label routed to a different owner than repos.org was still rejected as unconfigured even though it is in the validated set. Try the org expansion first, then the label's own route, and keep the first that matches a configured repo.
issueResolution.detail (and the localMountDegradedReason it can embed) is free text describing local operator state — e.g. "mount state is missing at /Users/<name>/.relayfile/<workspace>/.relay/state.json". dispatchComment interpolated it directly into the comment body posted to the issue via #postIssueComment, and the same string was mirrored into DispatchResult.comments. factory is a public repo; this PR introduced issueResolution, so this was a regression the PR added. Audited every consumer of issueResolution.detail and localMountDegradedReason: dispatchComment (this PR's only construction site of a comment string containing either) is the sole render path that reaches a posted comment. Other #postIssueComment callers in this file (label-dispatch-failure and dependency-park notices) don't touch issueResolution at all. Emit only issueResolution.source in the comment — a closed two-value enum, so an allowlist rather than trying to scrub paths out of free text. detail remains untouched in the JSON issueResolution field and in logs, per the existing dispatch/triage JSON output and #logger calls, which read the field directly rather than through this function. Exported dispatchComment for a direct unit test (matching this file's existing pattern for testing pure helpers, e.g. githubIssuePathParts) asserting the rendered comment string never contains an absolute path for a degraded-mount fallback resolution.
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…ity set Unit 2 — restart recovery. #githubApiFallbackIssues is process-local, so after a restart it starts empty even though a durably-persisted decision still records issueResolution.source === 'github-api-fallback'. Durable dispatch resume (#resumeDurableDispatch) re-reads the issue before continuing and, without eligibility restored, throws "issue is not currently readable" on every retry — the lifecycle never leaves 'retryable'. Extracted the existing #dispatchUnlocked registration logic into #registerGithubApiFallbackEligibility(decision) and call it from both #dispatchUnlocked and #resumeDurableDispatch, so eligibility is restored from the decision itself rather than assumed lost. Unit 4 — bound the set. #githubApiFallbackIssues was append-only for the life of the process, growing without bound in a long-running `factory start --mode live` daemon. Bounded it at 2,000 entries, evicting the least-recently-registered identity at capacity. Bounding a set unit 2 depends on being complete risks quietly re-breaking unit 2: eviction and "never was eligible" look identical to a reader unless kept distinguishable. Evicted identities move into a second, separately-bounded record (200 entries) purely for that distinction; #readGithubIssue now checks it on a miss and counts/logs "eligibility evicted" (githubIssueApiFallbackEligibilityEvicted) separately from a confirmed-gone phantom skip (githubIssuePhantomSkipped), so an operator investigating a false phantom-skip has a way to tell the two apart instead of both collapsing into the same silent signal. The bounding/eviction/distinguishability mechanics are exported as a pure function (rememberBoundedFallbackEligibility, no class state) and tested directly against small max sizes — bounding, eviction, recency refresh on re-registration, eviction-record restoration, and bounding of the eviction record itself. Testing the full wiring at the production-sized cap (2,000 entries) through real dispatch cycles would need thousands of spawns per test run; the pure function is the same code path #rememberGithubApiFallbackEligible calls, so this is exhaustive on the mechanics without that cost. Flagging that tradeoff rather than presenting it as full end-to-end coverage.
Titled as if the GitHub API fallback resolves the issue, but the assertions verify the opposite: the projection stays preferred (source: relayfile-projection) and githubRead.getIssue is never called. Applied cubic's exact suggested title.
…ization path Three consecutive review rounds found a different normalization gap in parseGithubIssueSelector's qualified-selector resolution: round 1 collapsed to repos.default; round 2's org-expansion never consulted byLabel for a cross-owner route; round 3 compared a bare byLabel route against a normalized configured entry and never matched. Each round patched the specific case found rather than the shared cause: an ad-hoc, hand-rolled candidate/comparison list that only handled whichever combination of org/label/qualification it happened to be built against. Replaced it with the invariant every configured entry already satisfies: resolve the requested selector through the same resolveGithubIssueRepoCandidates canonicalization (label mapping, org prefixing, canonical-route lookup) used to build the configured route set, then compare only normalized against normalized. Deleted the separate ad-hoc expansion entirely. Tested as a 3-dimensional matrix (bare vs qualified selector x repos.org set vs unset x label route written bare vs owner/repo, plus case-insensitivity and an already-qualified selector) via a direct unit test of the exported parseGithubIssueSelector, rather than one regression case per bug a reviewer happened to name. Confirmed the two rows matching round 3's exact bug shape fail against the prior (round 2) resolution logic and pass with this one; the other seven rows already passed under round 2's logic, isolating exactly the case that was actually broken. Also fixed an internally inconsistent fixture in the round-2 cross- owner test: its mock issue content hardcoded owner "AgentWorkforce" while the test's own scenario is a repo owned by "OtherOrg" — an inconsistent fixture can pass for the wrong reason. githubIssueFile now takes an optional owner parameter (default unchanged) instead of hardcoding one.
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
… per call site Registering eligibility at exactly two sites (#dispatchUnlocked, #resumeDurableDispatch) against ~21 #readIssue/#readGithubIssue call sites is the same shape that produced three rounds of selector bugs: a fallback-backed record that reached 'publishing' (PR publication) after a restart could not reread its issue, because nothing on that path ever registered it — #publishImplementerPullRequest reads the issue directly and had no idea the eligibility cache existed. Adding a third registration call at that one site would only repeat the mistake for whichever site is found next. Eligibility is now derived, not remembered. #readGithubIssue resolves it itself on a cache miss via #deriveGithubApiFallbackEligibility, which checks (a) an optional decisionHint, for the one read that happens before its own record exists in the batch (the initial live dispatch, which validates scope before it is tracked at all), or (b) the currently in-flight batch, scanned for a record whose decision was resolved through the fallback. BatchTracker.restore/start insert a durably-resumed or freshly-dispatched record into the batch before any phase handler runs, so every other call site — publishing, parking, question-handling, completion, and any future one — resolves correctly with no changes and nothing to remember. A successful derived lookup warms the existing bounded cache so repeated reads of the same issue stay cheap. Derivation intentionally scans the in-memory batch rather than looking up the durable record directly by its composite key (uuid/key/path): the real uuid is built from GitHub's node_id/id when content is available, which a path alone cannot reconstruct, so a direct durable-store lookup keyed that way would not reliably match. The in-memory batch instead matches by GitHub identity (owner/repo/number) against a repository's own issue path, sidestepping that problem entirely, at the cost of only ever finding an issue that is currently tracked in this process (a durably-restored record always is by the time any phase handler reads it). Exported the derivation as a pure function (isGithubApiFallbackEligible) and tested it directly: eligible from a tracked record alone with no hint and nothing registered (the guard against a future call site reintroducing this by omission), not eligible with no match, not eligible for a projection-sourced (non-fallback) record, not eligible for a different identity, eligible from decisionHint alone before any record is tracked, and decisionHint does not leak to a different identity. Also added a full FactoryLoop-level red check: a fallback-backed GitHub issue whose PR publication is deliberately parked at 'publishing' pre-restart (publisher fails until reactivated), then resumed by a fresh createFactory instance with an empty in-memory cache. Confirmed failing before this fix — the restarted process's publish retries never reach the publisher a second time, stuck throwing "issue is no longer readable" on every attempt — and passing after it.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/orchestrator/factory.ts (2)
2434-2436: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAvoid mutating a shared
DispatchResultobject.
#dispatchUnlockedcan return objects that are stored elsewhere:existingRecord.result,record.result, anddurable.result. Line 2435 mutates that same object, soissueResolutionis written into the batch record and into any other holder of the reference. A later#saveDispatchLifecyclethen persists it. Return a copy instead.Also note the duplicate-suppression path at Line 2426 returns
inFlightdirectly, so a coalesced caller can observe the result before this enrichment runs. Enriching a copy inside a shared wrapper promise removes both problems.♻️ Proposed change
- const dispatched = this.#dispatchUnlocked(decision, opts) + const dispatched = this.#dispatchUnlocked(decision, opts).then((result) => + decision.issueResolution + ? { ...result, issueResolution: structuredClone(decision.issueResolution) } + : result) this.#dispatchInFlight.set(key, dispatched) try { - const result = await dispatched - if (decision.issueResolution) result.issueResolution = structuredClone(decision.issueResolution) - return result + return await dispatched } finally {🤖 Prompt for 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. In `@src/orchestrator/factory.ts` around lines 2434 - 2436, Update the dispatch flow around `#dispatchUnlocked` so issueResolution is added to a new DispatchResult copy rather than mutating the shared result. Enrich the result inside the shared wrapper promise used for duplicate suppression, ensuring coalesced callers receive the enriched copy instead of returning inFlight directly.
4758-4806: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDistinguish "no GitHub reader mounted" from a confirmed miss.
If
identityis eligible butthis.#mount.githubReadis undefined, control falls to Line 4804. The code then incrementsgithubIssuePhantomSkippedand logs "after projection and provider lookup", although no provider lookup ran. That mislabels an unavailable provider as a confirmed absence, which is the exact distinction the rest of this block preserves.🔍 Proposed change
if (eligible && identity) this.#rememberGithubApiFallbackEligible(identity) + if (parts && identity && eligible && !this.#mount.githubRead) { + this.#increment('githubIssueUnverifiable') + this.#logger.warn?.('[factory] GitHub API fallback is eligible but no GitHub reader is mounted', { + path, + repo: `${parts.owner}/${parts.repo}`, + number: parts.number, + }) + return undefined + } if (parts && identity && eligible && this.#mount.githubRead) {🤖 Prompt for 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. In `@src/orchestrator/factory.ts` around lines 4758 - 4806, Handle the eligible-identity case where this.#mount.githubRead is unavailable before the confirmed-miss path in the GitHub issue fallback flow. In that branch, avoid incrementing githubIssuePhantomSkipped or logging that provider lookup occurred; instead preserve the unavailable-provider distinction using the existing fallback outcome signaling used by this block.
🧹 Nitpick comments (1)
src/orchestrator/factory.test.ts (1)
6736-6738: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSet explicit timeouts on these
vi.waitForcalls.Both waits use the default Vitest timeout, while the later waits in the same test use
4_000. The PR notes timing-sensitive suite failures. Aligning the timeouts reduces that risk at no cost.♻️ Proposed change
- await vi.waitFor(() => expect(attempts).toBe(1)) + await vi.waitFor(() => expect(attempts).toBe(1), { timeout: 4_000 }) await vi.waitFor(async () => expect(await state().getDispatchLifecycle('factory-test', issueKey(decision.issue))) - .toMatchObject({ phase: 'publishing' })) + .toMatchObject({ phase: 'publishing' }), { timeout: 4_000 })🤖 Prompt for 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. In `@src/orchestrator/factory.test.ts` around lines 6736 - 6738, Set an explicit 4,000 ms timeout on both vi.waitFor calls in the test, including the attempts assertion and the dispatch lifecycle publishing assertion, matching the later waits in the same test.
🤖 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 `@src/orchestrator/factory.test.ts`:
- Around line 6732-6764: Update the retry assertion in the test around
first.stop(), providerReady, and restarted.start() to capture the attempts count
immediately after the first process stops, then require the restarted process to
increase that baseline rather than asserting an absolute value of 2. Keep the
subsequent lifecycle assertion unchanged so the test still verifies publishing
completed after restart.
---
Outside diff comments:
In `@src/orchestrator/factory.ts`:
- Around line 2434-2436: Update the dispatch flow around `#dispatchUnlocked` so
issueResolution is added to a new DispatchResult copy rather than mutating the
shared result. Enrich the result inside the shared wrapper promise used for
duplicate suppression, ensuring coalesced callers receive the enriched copy
instead of returning inFlight directly.
- Around line 4758-4806: Handle the eligible-identity case where
this.#mount.githubRead is unavailable before the confirmed-miss path in the
GitHub issue fallback flow. In that branch, avoid incrementing
githubIssuePhantomSkipped or logging that provider lookup occurred; instead
preserve the unavailable-provider distinction using the existing fallback
outcome signaling used by this block.
---
Nitpick comments:
In `@src/orchestrator/factory.test.ts`:
- Around line 6736-6738: Set an explicit 4,000 ms timeout on both vi.waitFor
calls in the test, including the attempts assertion and the dispatch lifecycle
publishing assertion, matching the later waits in the same test.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 253c0c1c-bb5a-40f0-9726-aefcb16408af
📒 Files selected for processing (6)
src/cli/fleet.test.tssrc/cli/fleet.tssrc/mount/github-api-issue-read.test.tssrc/mount/github-api-issue-read.tssrc/orchestrator/factory.test.tssrc/orchestrator/factory.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/mount/github-api-issue-read.ts
- src/mount/github-api-issue-read.test.ts
- src/cli/fleet.test.ts
- src/cli/fleet.ts
attempts incremented before the providerReady gate in both the round-4
API-fallback-publishing red check and its pre-existing Linear sibling
("takes over a persisted publishing phase after the owner stops"), so
a failed publish attempt counted regardless of why it failed. The
still-running first process's 1s dispatch-lifecycle retry timer
(DISPATCH_LIFECYCLE_RETRY_MS) can fire another failing attempt in the
window between the 'publishing' phase check and first.stop() resolving,
which could push the API-fallback test's attempts to 2 before the
restarted process ever read the issue — satisfying the exact absolute
count the test asserted without exercising the behavior it exists to
prove. The sibling's primary gate (restarted.status().counters.done)
is race-immune, but its trailing exact-count assertion was vulnerable
to the same race in the opposite direction (a spurious failure if an
extra pre-restart retry occurred).
Both now baseline attempts immediately after first.stop() and assert
the restarted process strictly increased it, rather than asserting an
absolute count — correct regardless of how many retries occurred
before the restart.
Confirmed the corrected API-fallback test still red-checks: reverted
#readGithubIssue to the pre-derivation (registration-only) design and
reran — fails on attempts never exceeding the pre-restart baseline,
timing out as intended. Restored the fix; passes, stable across
repeated runs.
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…ifications and lifecycles A restarted fallback-backed clarification cancelled a valid human reply instead of resuming: #clarificationIssueStillActive (which gates whether a wake resumes or is cancelled) reads the issue via #readIssue with no hint, and #deriveGithubApiFallbackEligibility only scanned batch.inFlight. The clarification restore path (#restoreClarifications-adjacent code near #resumeWaitingClarification) rebuilds an InFlightIssue locally from the persisted decision purely to re-arm the Slack watcher — it never inserts it into the batch, so the in-flight scan alone missed it, the read failed, the issue was treated as having left factory scope, and the wake was cancelled (clarificationWakesCancelledStaleIssue) — silently discarding the human's answer. Confirmed the stated limitation from 793f252 ("a durably-restored record is always in the batch") is false specifically for this path: a decision can durably outlive the original dispatch call in (at least) three distinct shapes, not one. Widened #deriveGithubApiFallbackEligibility to gather candidates from all three: batch.inFlight (existing), #state.listWaitingClarifications (new — the clarification-parked case), and #state.listDispatchLifecycles filtered to non-terminal (new — a durable dispatch not currently reflected in the in-memory batch at all, e.g. a non-durable-fleet dispatch never reaches the batch either). decisionHint remains a pure optimization ahead of the scan, now also passed at the one clarification call site that already holds its decision (#clarificationIssueStillActive, from #resumeWaitingClarification) — cheap, but not required for correctness, since 20-odd other read sites still pass none and rely entirely on the scan. isGithubApiFallbackEligible's candidate parameter was already source-agnostic (generalized from round 4's "inFlight" naming); added two small pure mapping functions (githubApiFallbackCandidatesFromWaitingClarifications, githubApiFallbackCandidatesFromDispatchLifecycles, the latter excluding terminal lifecycles) so the gathering logic for the two new sources is directly unit-tested in isolation, not only through the full-process red check. Red check: parks a fallback-backed GitHub clarification, restarts with an empty in-memory cache, delivers a human reply directly through the durable state store, and asserts the team resumes (clarificationTeamsWoken reaches 1) rather than the wake being cancelled. Confirmed failing before this fix (the counter never increments, hand-reverted to the round-5 in-flight-only derivation) and passing after, stable across 5 repeated runs. Guard tests: a fallback-backed decision reachable from ONLY the waiting-clarifications source, and from ONLY a non-terminal dispatch lifecycle, both resolve eligible; a terminal (complete/abandoned) lifecycle is excluded from candidates entirely, individually and alongside a non-terminal one for a different issue.
2 participants
Why
Factory currently dispatches nothing org-wide, and the cause is not in Factory's routing.
Targeted issue resolution is projection-only:
runFactoryCommand→readIssueArg→findIssuePathtries canonical repo-scoped paths, lists the configured Relayfile GitHub issue roots, and throws on zero matches before triage can run. When the Relayfile projection is stale, every lookup misses andfactory triagereportsfound 0 matches— which reads identically to "there is no such issue".The projection has been frozen since 2026-08-03T07:26Z. So the failure mode is silent: Factory is healthy, its routing is correct, and it dispatches nothing.
What changed
A read-only direct GitHub REST fallback, used only after the projection has demonstrably failed.
subscribed/polling.repo#numberandowner/repo#numberselectors make a targeted fallback exactly one authoritative lookup instead of an ambiguous org-wide probe.issueResolution; fallback records also carrylocalMountDegraded,localMountDegradedReason, andeventListener, so a decision states which source answered it.Verification
Live CLI through
runFleetCli:factory#222→ exit 0, sourcegithub-api-fallback, projectionno-match, routed only toAgentWorkforce/factory.factory#999999→ exit 1, no decision emitted. Dry-run dispatch of an issue missing both Factory markers → exit 1, no dispatch; dry-run dispatch offactory#222→ exit 0, carryinggithub-api-fallbackin both the result and its dispatch comment.Red-checked in both directions rather than assumed:
Focused build and suites: exit 0, 158 tests passed.
Known caveats, stated rather than buried
600 < 500) fails in isolation and is pre-existing — not introduced here.Forbidden— that route additionally requires a deployed sponsor persona, which a local Factory workspace join is not. Replaced with the read-only direct REST client. GitHub writes remain on Relayfile app-authored writeback; Factory never handles a GitHub token.Scope
No queue, Cloudflare, mount, daemon or launchd mutation. No default-branch push. Read-only on the GitHub side.
Authored by the
factory-dispatch-api-fallbacklane, which completed the work and red-checks but could not open a PR: sanctioned Relayfile app-authored write returned HTTP 403, and it correctly declined to shell out toghinstead. Opened on its behalf; that 403 is a real gap worth fixing separately — a lane that cannot publish its own work is blocked no matter how finished the work is.🤖 Generated with Claude Code
Summary by cubic
Restores Factory triage/dispatch by adding a read-only GitHub REST fallback when the Relayfile issue projection is stale. Keeps projection-first behavior, hardens selector validation and privacy, and derives fallback eligibility across dispatch, publishing, clarifications, and durable lifecycles so restarts keep working.
Bug Fixes
repo#numberandowner/repo#number, validate against all configured routes (including cross‑org label routes andrepos.orgexpansions), and match labels case‑insensitively; bare numbers remain default‑only; refuse dispatch if uniqueness across configured repos can’t be confirmed.issueResolution. Dispatch re‑reads fallback issues via the provider and applies the same scope/readiness/label gates. The orchestrator only uses the provider read for issues already resolved via the fallback.issueResolution.source(no local paths or reasons).Migration
issueResolutionfield.Written for commit 91add5f. Summary will update on new commits.