Skip to content

Fix degraded integration recovery guidance - #204

Open
willwashburn wants to merge 2 commits into
mainfrom
fix/203-degraded-connection-remediation
Open

Fix degraded integration recovery guidance#204
willwashburn wants to merge 2 commits into
mainfrom
fix/203-degraded-connection-remediation

Conversation

@willwashburn

@willwashburn willwashburn commented Jul 29, 2026

Copy link
Copy Markdown
Member

Fixes #203.

When Relayfile reports an integration as degraded after initial sync is already complete, Factory now directs the operator to have a workspace owner repair or reconnect the provider. It no longer tells them to wait for an initial sync that has finished.

Verification:

  • npx vitest run src/mount/relayfile-integration-preflight.test.ts (10 passed)
  • npm run build
  • git diff --check

Summary by cubic

Clarifies operator guidance when an integration is degraded after initial sync completes. Previously we told operators to wait; now we direct them to have a workspace owner repair or reconnect the provider.

  • When state: degraded and initialSyncState: complete, notReadyMessage shows a repair/reconnect message and avoids “wait for initial sync.”
  • Adds a unit test to assert the new behavior; no API or config changes required.

Written for commit c558869. Summary will update on new commits.

Review in cubic

@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Changes

Relayfile preflight

Layer / File(s) Summary
Completed degraded-state handling
src/mount/relayfile-integration-preflight.ts, src/mount/relayfile-integration-preflight.test.ts
Completed degraded integrations now produce repair or reconnect guidance, with coverage confirming no connection or wait calls occur during dry-run preflight.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested reviewers: khaliqgant

Poem

A rabbit found a sync gone gray,
“Don’t wait,” it thumped, “repair today!”
The preflight spoke with clearer cheer,
No needless OAuth hops appeared.
Fresh guidance now bounds through the burrow.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes implement #203 by detecting degraded, complete syncs and directing owners to repair or reconnect.
Out of Scope Changes check ✅ Passed The diff stays focused on the degraded-integration guidance and its test coverage.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check ✅ Passed The title clearly and concisely describes the main change: improved recovery guidance for degraded integrations.
Description check ✅ Passed The description directly explains the degraded integration behavior, the new recovery guidance, and the verification performed.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/203-degraded-connection-remediation

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.

@coderabbitai coderabbitai 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.

🧹 Nitpick comments (1)
src/mount/relayfile-integration-preflight.test.ts (1)

135-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover the new state-normalization path.

This fixture only uses canonical lowercase values, so it would still pass if .trim().toLowerCase() were removed. Add a mixed-case/whitespace case and assert the intended diagnostic formatting.

🤖 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/relayfile-integration-preflight.test.ts` around lines 135 - 155,
Extend the test for ensureFactoryIntegrations with mixed-case and
surrounding-whitespace state values, such as state and initialSyncState, to
exercise the normalization path. Update the expected rejection assertion to
verify the diagnostic displays the normalized lowercase values, while preserving
the existing assertions that no connection or wait calls occur.
🤖 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.

Nitpick comments:
In `@src/mount/relayfile-integration-preflight.test.ts`:
- Around line 135-155: Extend the test for ensureFactoryIntegrations with
mixed-case and surrounding-whitespace state values, such as state and
initialSyncState, to exercise the normalization path. Update the expected
rejection assertion to verify the diagnostic displays the normalized lowercase
values, while preserving the existing assertions that no connection or wait
calls occur.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: dead6eaa-169f-4cd9-bb0a-cc38f9cbe03c

📥 Commits

Reviewing files that changed from the base of the PR and between 565690e and 5f1c2a4.

📒 Files selected for processing (2)
  • src/mount/relayfile-integration-preflight.test.ts
  • src/mount/relayfile-integration-preflight.ts

@khaliqgant

Copy link
Copy Markdown
Member

Stale-PR triage: REVIVE

Triage sweep of the 6 open PRs (all untouched since 2026-07-21..07-30), evaluated against origin/main @ f13600f.

Still wanted. Closes #203, which is still OPEN.

Not superseded. src/mount/relayfile-integration-preflight.ts on main still returns the unconditional message:

$ git show origin/main:src/mount/relayfile-integration-preflight.ts | grep -n 'wait for its initial sync'
128:  return `[factory] ${provider} is connected but not ready${details ? ` (${details})` : ''}; wait for its initial sync, then retry.`

There is no degraded + complete branch anywhere on main, so the impossible-wait advice #203 describes is still live.

Rebases cleanly. Tested in an isolated detached worktree, not the shared checkout:

$ git rebase f13600f32107e499bfccc6f971f7863fe5822927
(exit 0 — zero conflicts)

CI. gh run list --branch fix/203-degraded-connection-remediation returns exactly one workflow, CI, success @ 5f1c2a4 (the current head). No other workflow is configured for this branch.

Cost to revive: lowest of the six. 29 additions, 0 deletions, 2 files, MERGEABLE, 0 review threads, clean rebase. This is the cheapest PR in the set to land.

Recommendation only — I do not close or merge. Posted by an automated triage lane.

@khaliqgant

Copy link
Copy Markdown
Member

Refreshed against current main — one real test failure. Not merging.

This PR was a merge candidate on the principal's rule (green CI, zero unresolved review threads). It was not safe: its green was from 2026-07-29 and the branch was 57 commits behind main, so that result said nothing about today's main.

I updated the branch (5f1c2a41c5588697) so CI would actually run. Result on the refreshed head, run 32011189325:

  • packageFAIL
  • CodeRabbit, cubic · AI code reviewer, kubernetes-provider-e2e, load-e2e, verification-gate-e2e, verification-stack-e2e — pass

The failure, src/orchestrator/factory.test.ts:12327 (1 failed / 1636 passed):

AssertionError: expected { …(11) } to match object { phase: 'running' }
-   "phase": "running",
+   "phase": "dispatching",

This looks like a genuine integration conflict rather than flake — a deterministic state-machine phase mismatch, not a timeout — most likely against f6050e0 (#274, dispatch exit codes) or 7122524 (#275, released invocation spawn) which both landed after this branch was last touched. A clean textual merge can still combine two individually correct sides into a defect; the seam is what needs checking, not the hunks.

That should be confirmed rather than assumed: re-run the single test in isolation and check its exit code before treating it as real, because a failing set that varies run-to-run while each member passes alone is CPU contention on a shared machine, not a defect.

Owner: factory-stale-prs-0817. Deliverable is the disposition question first — whether this PR's premise still holds against current main — then the fix if it does. No merge; the principal owns that gate.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Integration preflight gives impossible wait advice for degraded completed syncs

2 participants