Repository navigation
test(github): re-probe tracked upstreams after SSH reconnect - #12979
dcieslak19973 wants to merge 2 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between f405969781216a257157935a519a82c961f04689 and 3fb41b4. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughAdded a test for SSH tracked-upstream probing. The test confirms that a disconnected repository does not invoke local Git. After reconnection, the repository probes tracked upstreams through the SSH provider. Merge Risk: ⚪ Minimal · up to This test-only change adds regression coverage without changing production behavior, and the supplied validation passes. No actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Description checkExplanation The description explains the change, motivation, test-only scope, linked issue, verification results, compatibility impact, and visual proof. It omits several template headings and checklist items, but the required context is substantially present. Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/github/client.test.ts (2)
1955-1971: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winTest the reconnect retry path.
This test covers only the first disconnected lookup. It does not prove that
probeFailed: truekeeps the empty upstream map out of the snapshot cache. After the provider becomes available, repeat the lookup and assert that the provider is queried again.
1973-1985: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert both local probe commands.
localProbes.length > 0can pass when only one helper invokes local Git. Assert that bothrev-parseandfor-each-refappear in the captured calls.Suggested assertion
- expect(localProbes.length).toBeGreaterThan(0) + const localProbeCommands = localProbes.map(([args]) => (args as string[])[0]) + expect(localProbeCommands.some(command => command === 'rev-parse')).toBe(true) + expect(localProbeCommands.some(command => command === 'for-each-ref')).toBe(true)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7f8bdc5b-897b-4724-aebe-0dfe1a8ed833
📒 Files selected for processing (2)
src/main/github/client.test.tssrc/main/github/client.ts
|
Upstream https://github.com/dcieslak19973/orca/actions/runs/31215871344 — all 43 jobs green: typecheck, static analysis, packaging (linux + windows), Git compatibility, shell contracts, cross-version wire compatibility, and all 16 test shards. Posting so the suite result is visible without needing the approval click first. |
|
Upstream https://github.com/dcieslak19973/orca/actions/runs/31291648841 — all 30 jobs green. The fork's |
4d95d4b to
3383cd9
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 893d0bb9-7a4c-4e87-9228-f26a42584a5d
📥 Commits
Reviewing files that changed from the base of the PR and between c0a7754 and 3383cd93c6d96c81ea0bb1c864b31908f4c9c2cb.
📒 Files selected for processing (2)
src/main/github/client-tracked-upstream-snapshot.test.tssrc/main/github/client.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/github/client.ts
3383cd9 to
d81e43f
Compare
nwparker
changed the title
f113eb5 to
f405969
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/github/client-tracked-upstream-snapshot.test.ts (1)
744-744: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a platform-safe repository path.
'/repo-root'assumes POSIX path syntax. DefinerepoPathwith the repository’s Node/Electron path utility and reuse it for both lookups and assertions.As per coding guidelines,
**/*.{ts,tsx,js,jsx}: Usepath.joinor Electron/Node path utilities for file paths; never assume/or\.Also applies to: 754-754
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f14bac2b-42cb-4647-bb96-9cf3102d4c04
📥 Commits
Reviewing files that changed from the base of the PR and between 6989c92 and f405969781216a257157935a519a82c961f04689.
📒 Files selected for processing (1)
src/main/github/client-tracked-upstream-snapshot.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review.
f405969 to
4a7f33b
Compare
Readiness verdict: READY TO MERGEStandalone readiness review of head Why this is a real guard, not a passing-by-default testThe production behavior under test lives in const provider = connectionId ? getSshGitProvider(connectionId) : null
if (connectionId && !provider) {
throw new Error(SSH_GIT_PROVIDER_UNAVAILABLE_MESSAGE)
}I mutation-tested that guard against the new test rather than trusting a green run:
Both assertions are load-bearing: A covers the local-fallthrough guard, C covers stale reuse across reconnect. Mutation B survives only because an empty snapshot misses the Disconnected-SSH / network-failure evidence (Linux openclaw)Real on-disk Git repository, real
This is the case that matters for the SSH execution boundary: loss of contact yields a hard failure, never a local-host answer misattributed to the remote. Gates run at final head
|
| Gate | macOS | Linux openclaw |
|---|---|---|
vitest run src/main/github |
675 passed (61 files) | 675 passed (61 files) |
Focused client-tracked-upstream-snapshot.test.ts |
19 passed | 19 passed |
| Real-repo disconnected/reconnect runtime probe | — | passed |
typecheck:node |
passed | — |
lint |
passed | — |
This closes the gap in the PR description, where the full src/main/github suite, typecheck, and lint had been run against the identical diff at an earlier commit (f113eb5338c) rather than the final head — all now re-confirmed at 4a7f33b348e.
Compatibility
Test-only. No production runtime, persisted data, RPC/wire contract, stream opcode, mobile surface, dependency, packaging, Git-provider, or UI change. Git usage is limited to for-each-ref --format (well below the Git 2.25 baseline; exercised here on 2.43.0). Nothing a paired client and remote host exchange is altered, so mixed-version pairs are unaffected.
Visual proof
N/A — justified. Main-process test coverage only; no renderer code, component, token, or rendered surface is touched, so there is no UI state to screenshot. Confirmed by the diff touching exactly one src/main/** test file.
CI and mergeability
MERGEABLE / mergeStateStatus=CLEAN, not a draft. 46 checks SUCCESS, 1 SKIPPED (e2e, correctly skipped — "detect changed e2e specs" found no e2e spec changes). Both workflows green.
Residual issues
None found. No code changes were required; local HEAD is identical to the PR head, so nothing was pushed.
Greptile SummaryAdds regression coverage ensuring tracked-upstream discovery does not invoke local Git while an SSH provider is disconnected and re-probes through the restored provider after reconnection.
Confidence Score: 5/5The test-only change appears safe to merge. The added test directly exercises the disconnect/reconnect sequence and verifies both the absence of local Git fallback and the restored SSH-provider probe, with no production behavior changed.
|
| Filename | Overview |
|---|---|
| src/main/github/client-tracked-upstream-snapshot.test.ts | Adds focused regression coverage for SSH reconnect behavior without changing production code. |
Reviews (1): Last reviewed commit: "test(github): assert SSH upstream reprob..." | Re-trigger Greptile
|
Closing: this is test-only and the behavior it covers (re-probing tracked upstreams via the SSH git provider after reconnect in |
What
Adds the missing regression coverage for tracked-upstream discovery across an SSH disconnect/reconnect. The test proves that a disconnected lookup does not invoke local Git and that the next lookup reaches the restored SSH provider instead of reusing a disconnected empty result.
The original production fix from this PR was independently merged in #14945 and is already on
main. During rebase,client.tshad been split into focused modules, so the obsolete source hunk was resolved in favor of currentmain; the one unique reconnect regression test remains.Refs #12977.
Verification
4a7f33b348e:f113eb5338c):pnpm exec vitest run src/main/github— 675 passedpnpm run typecheck:node— passedpnpm run lint— passedpnpm run build:electron-vite— passed (two existing CSS optimizer warnings)4a7f33b348e:rev-parseandfor-each-refboth routed through the provider and returned the exact headCompatibility
Test-only change. No production runtime, persisted data, mobile surface, RPC/wire contract, dependency, packaging, Git-provider, or UI behavior changes.
Visual proof
N/A — test-only main-process coverage with no rendered UI change.