fix: align supported-versions with the real /api/info contract; exception-aware fallback#3406
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📜 Recent review details⏰ Context from checks skipped due to timeout. (3)
WalkthroughAdds unauthenticated ChangesSupported versions handling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant powerMonitor
participant checkSupportedVersionServers
participant updateSupportedVersionsData
participant buildFallbackCandidates
participant validateFallbackAndDispatch
powerMonitor->>checkSupportedVersionServers: resume event
checkSupportedVersionServers->>updateSupportedVersionsData: refresh configured servers
updateSupportedVersionsData->>buildFallbackCandidates: cached and builtin payloads
buildFallbackCandidates-->>updateSupportedVersionsData: ordered candidates
updateSupportedVersionsData->>validateFallbackAndDispatch: validate candidates
validateFallbackAndDispatch-->>updateSupportedVersionsData: dispatch selected verdict and source
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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. For unrecoverable errors, disable the tool in CodeRabbit configuration. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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.
Actionable comments posted: 2
🤖 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/servers/supportedVersions/apiContract.main.spec.ts`:
- Around line 53-55: In the retry logic surrounding the contract request, update
the catch block that assigns `lastError` so it immediately rethrows errors that
are not network failures; only retain and retry errors classified as
network-related, preventing reachable 4xx contract failures from being masked by
later responses.
In `@src/servers/supportedVersions/main.ts`:
- Around line 617-636: Candidate validation in the supported-version selection
logic currently aborts on the first malformed payload, preventing fallback
evaluation. Update the candidate evaluation around isServerVersionSupported so
each candidate’s validation failure is caught independently, allowing the next
candidate to be tried; dispatch the error only when all candidates fail to
produce a verdict. Add a regression test covering a malformed cache candidate
followed by a valid builtin candidate.
🪄 Autofix (Beta)
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
Run ID: 9147c4d8-0684-4bef-aa6f-5fd0df3ced02
📒 Files selected for processing (4)
src/servers/supportedVersions/apiContract.main.spec.tssrc/servers/supportedVersions/main.main.spec.tssrc/servers/supportedVersions/main.tssrc/servers/supportedVersions/types.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript for all new code unless explicitly told otherwise.
Use Fuselage components for all UI work; only create custom components when Fuselage does not provide what is needed.
Import UI components from@rocket.chat/fuselage.
CheckTheme.d.tsfor valid color tokens before using Fuselage theme colors.
Use optional chaining with fallbacks for platform-specific APIs, especially Linux-only process APIs likeprocess.getuid(),process.getgid(),process.geteuid(), andprocess.getegid().
Only mock platform-specific APIs when defensive coding is not possible.
Use TypeScript strict mode.
Use React functional components with hooks.
Redux actions must follow the Flux Standard Action (FSA) pattern.
Use camelCase for file names and PascalCase for components.
**/*.{ts,tsx}: Use TypeScript for new code unless explicitly told otherwise.
Use Fuselage components from@rocket.chat/fuselagefor UI work unless the design requires something Fuselage does not provide.
CheckTheme.d.tsfor valid color tokens before using Fuselage colors.
Verify library props, APIs, and tokens against official docs or local.d.tsfiles instead of assuming.
Use React functional components with hooks.
Redux actions follow FSA shape.
Use camelCase for file names and PascalCase for components.
Prefer clear names over unnecessary comments.
Prefer editing existing files over creating new abstractions unless the new abstraction removes real complexity or matches an existing pattern.
Files:
src/servers/supportedVersions/apiContract.main.spec.tssrc/servers/supportedVersions/types.tssrc/servers/supportedVersions/main.tssrc/servers/supportedVersions/main.main.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
*.spec.tsfor renderer process tests.
Files:
src/servers/supportedVersions/apiContract.main.spec.tssrc/servers/supportedVersions/main.main.spec.ts
**/*.main.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
*.main.spec.tsfor main process tests.Main-process specs use
*.main.spec.ts.
Files:
src/servers/supportedVersions/apiContract.main.spec.tssrc/servers/supportedVersions/main.main.spec.ts
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Renderer specs use
*.spec.ts/*.spec.tsx.
Files:
src/servers/supportedVersions/apiContract.main.spec.tssrc/servers/supportedVersions/main.main.spec.ts
src/*/*/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Renderer specs must live in a Jest-matched nested path, such as
src/<module>/<subdir>/*.spec.ts(x); flatsrc/<module>/*.spec.tsfiles are not discovered by the currenttestMatch.
Files:
src/servers/supportedVersions/apiContract.main.spec.tssrc/servers/supportedVersions/main.main.spec.ts
🔇 Additional comments (4)
src/servers/supportedVersions/types.ts (1)
57-93: LGTM!src/servers/supportedVersions/apiContract.main.spec.ts (1)
1-52: LGTM!Also applies to: 56-127
src/servers/supportedVersions/main.main.spec.ts (1)
1-2: LGTM!Also applies to: 46-62, 101-104, 164-194, 294-297, 617-620, 834-839, 1821-2093, 2521-2522
src/servers/supportedVersions/main.ts (1)
100-129: LGTM!Also applies to: 589-616, 637-648, 694-704, 820-840
| } catch (error) { | ||
| lastError = error; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not retry contract failures as network failures.
Because only the final error is classified, a reachable 4xx can be followed by a 5xx and incorrectly skip the suite. Immediately rethrow non-network errors.
Proposed fix
} catch (error) {
lastError = error;
+ if (!isNetworkFailure(error)) {
+ throw error;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (error) { | |
| lastError = error; | |
| } | |
| } catch (error) { | |
| lastError = error; | |
| if (!isNetworkFailure(error)) { | |
| throw error; | |
| } | |
| } |
🤖 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/servers/supportedVersions/apiContract.main.spec.ts` around lines 53 - 55,
In the retry logic surrounding the contract request, update the catch block that
assigns `lastError` so it immediately rethrows errors that are not network
failures; only retain and retry errors classified as network-related, preventing
reachable 4xx contract failures from being masked by later responses.
| let chosen = candidates[0]; | ||
| let verdict = await isServerVersionSupported( | ||
| server, | ||
| fallbackVersions, | ||
| chosen.versions, | ||
| freshCommitHash, | ||
| fallbackSource | ||
| chosen.source | ||
| ); | ||
| const other = candidates[1]; | ||
| if (!verdict.supported && other) { | ||
| const otherVerdict = await isServerVersionSupported( | ||
| server, | ||
| other.versions, | ||
| freshCommitHash, | ||
| other.source | ||
| ); | ||
| if (otherVerdict.supported) { | ||
| chosen = other; | ||
| verdict = otherVerdict; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Continue when an individual fallback candidate is malformed.
A malformed first candidate throws out of the shared try, so the second candidate is never evaluated. A corrupted fresher cache can therefore prevent a valid builtin payload from rescuing the server. Catch validation failures per candidate and dispatch an error only if no candidate yields a verdict; add a malformed-cache/builtin-rescue regression test.
🤖 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/servers/supportedVersions/main.ts` around lines 617 - 636, Candidate
validation in the supported-version selection logic currently aborts on the
first malformed payload, preventing fallback evaluation. Update the candidate
evaluation around isServerVersionSupported so each candidate’s validation
failure is caught independently, allowing the next candidate to be tried;
dispatch the error only when all candidates fail to produce a verdict. Add a
regression test covering a malformed cache candidate followed by a valid builtin
candidate.
jeanfbrito
changed the base branch from
fix/supported-versions-hardening
to
master
jeanfbrito
force-pushed
the
fix/supported-versions-fixture-realism
branch
from
47f9d10 to
db94a64
Compare
…nfo contract; try both fallback sources The desktop's ServerInfo type declared fields the server never returns to unauthenticated callers (verified against the server source, apps/meteor/server/api/lib/getServerInfo.ts): - Remove the fictional uniqueId field. No server version includes it in /api/info; the workspace uniqueID comes from settings.public and is resolved on demand by withExceptionScopeUniqueId. Remove the dead serverInfoResult.uniqueId fallback in updateSupportedVersionsData. - Mark build, marketplaceApiVersion, and commit optional and document that they only appear for authenticated view-statistics callers; the desktop always calls /api/info unauthenticated. The sha-exception path uses the persisted server.gitCommitHash pushed by the injected script instead. - Rebuild the default test fixture to mirror the real unauthenticated wire shape (trimmed major.minor version, no uniqueId/commit/info) so the suite can no longer validate assumptions against fields real servers do not send — the gap that let the exception-scope defect pass CI. Switch beforeEach to jest.resetAllMocks(), fixing latent cross-test mock leakage the honest fixtures surfaced. Also rework the offline fallback to try BOTH remaining sources instead of picking one by timestamp: the bundled builtin payload never carries tenant exceptions, so preferring a fresher builtin over a cache that holds the tenant's exception blocked a workspace that should stay usable. Candidates are checked in freshness order and the first source that supports the server wins; blocking requires every available source to fail. This keeps the stale-cache rescue from #3388 while never losing a cached exception to it.
Asserts the unauthenticated /api/info response shape against a real server (open.rocket.chat): trimmed major.minor version, no uniqueId, no commit/info blocks, string minimumClientVersions, JWT-shaped supportedVersions.signed. Skips only on genuine network failure or SKIP_CONTRACT_TESTS=1, so contract drift between the desktop and the server cannot hide behind mocks that agree with themselves.
jeanfbrito
force-pushed
the
fix/supported-versions-fixture-realism
branch
from
db94a64 to
2518825
Compare
1 participant
Jira: CORE-2409
Stacked on #3405 (which stacks on #3404) — retarget to
masteras the stack merges.What
Closes the contract-drift gap that let the exception-scope defect (#3404) pass CI for 2.5 years, and fixes an exception-loss window introduced by the fallback source selection in #3388.
Contract alignment (verified against the server source,
apps/meteor/server/api/lib/getServerInfo.ts)The desktop's
ServerInfotype declared fields the unauthenticated/api/inforesponse never contains:uniqueIdremoved — no server version returns it (the field does not exist in the server'sIWorkspaceInfo). The workspace uniqueID comes fromsettings.publicand is resolved on demand. The deadserverInfoResult.uniqueIdfallback is removed.commit/build/marketplaceApiVersionmarked optional and documented as authenticated-only (view-statistics); the desktop always calls/api/infounauthenticated. sha-exception matching uses the persistedserver.gitCommitHashpushed by the injected script (Meteor.gitCommitHash), which is its only real data source.major.minorversion, nouniqueId/commit/info). The previous fixtures mirrored the fictional type, so CI validated the desktop against itself. SwitchingbeforeEachtojest.resetAllMocks()fixed latent cross-test mock leakage the honest fixtures surfaced.Live wire-contract test
New
apiContract.main.spec.tsasserts the unauthenticated/api/infoshape against open.rocket.chat (6 assertions). Skips only on genuine network failure orSKIP_CONTRACT_TESTS=1; a reachable server with a drifted contract fails the suite.Exception-aware offline fallback
#3388 made the offline fallback prefer the bundled builtin payload when fresher than the cache. The builtin never carries tenant exceptions, so a workspace kept usable by a cached exception could lose it to a fresher builtin and be blocked. The fallback now tries BOTH sources in freshness order and the first one that supports the server wins — blocking requires every available source to fail. This preserves #3388's stale-cache rescue while never losing a cached exception to it. Covered by three new tests (cache-exception rescue, builtin rescue, certain block).
Tests
tsc --noEmitandyarn lintclean.Summary by CodeRabbit