refactor(api,ssh,gateway,ui): replace edition booleans with SHELLHUB_EDITION enum#6666
Conversation
|
Claude finished @luizhf42's task in 10m 41s —— View job Code Review CompleteReviewed 84 files across code quality, security, testing, language patterns, and architecture. 1 inline issue posted (1 medium). This is a clean, well-executed refactor. The five reviewers independently confirmed the core risk — the
Additional notes
If you've addressed the feedback and want a new review, tag |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
luizhf42
added
area/ui
area/api
status/needs-review
go
35c7cfe to
b4ad36a
Compare
|
/review |
|
Claude finished @otavio's task in 9m 9s —— View job Code Review CompleteReviewed 84 files across code quality, security, testing, language patterns, and architecture. 1 inline issue posted (1 medium), plus 1 cross-repo note below. Second pass on this refactor. The Inline finding
Additional notes
If you've addressed the feedback and want a new review, tag |
b4ad36a to
78e0a06
Compare
|
/review |
Code Review
|
|
Re: the cross-repo note on |
|
/review |
|
Claude finished @otavio's task in 9m 16s —— View job Code Review CompleteReviewed 84 files across code quality, security, testing, language patterns, and architecture — no new issues found. The code looks good as-is. The two items from the prior rounds — gateway edition normalization (
A couple of non-blocking notes already raised in prior rounds still stand (not re-reported inline): If you push additional changes and want a new review, tag |
154bca1 to
b4780fc
Compare
b4780fc to
04ae9c0
Compare
|
/review |
|
Claude finished @otavio's task in 6m 37s —— View job Code Review CompleteReviewed 86 files across code quality, security, testing, language patterns, and architecture. No inline issues — the branch was reworked since the last round and the two previously-flagged normalization gaps (gateway The core risk was re-verified and is sound: the Additional notes
If you've addressed the feedback and want a new review, tag |
Replace `enterprise: boolean` + `cloud: boolean` in `ClientConfig`
with a single `edition: Edition` field. Add `isCommunity()`,
`isEnterprise()`, `isCloud()`, and `isEnterpriseOrCloud()` predicates
to `env.ts`, matching the Go vocabulary.
Delete `utils/features.ts` (`isPremiumFeature`/`hasMfaSupport`);
callers use `isEnterpriseOrCloud()` directly. Fix `Login.tsx` SSO
gate: was `isEnterprise()` (exact), now `isEnterpriseOrCloud()` so
SAML works on cloud too.
Add `src/__mocks__/env.ts` manual mock and a global `vi.mock("@/env")`
in `setup.ts` so test files no longer repeat a 10-line factory.
CurrentEdition() panics on an unrecognized SHELLHUB_EDITION, but it is only reached lazily by the edition predicates. gateway validates at boot and api trips the panic during route registration, but ssh only reaches a predicate mid-session and cli mid-command, so a misconfigured value would surface as a runtime panic rather than a clean startup failure. Add envs.ResolveEdition() returning an error, route CurrentEdition() through it (preserving the panic invariant), and validate the edition at the start of the ssh and cli entrypoints, log.Fatal-ing with a clear message on an invalid value.
…json gen-config.sh normalized SHELLHUB_EDITION but accepted any value. An invalid edition would be written verbatim into config.json, and the frontend predicates (isEnterpriseOrCloud() === edition !== "community") would then silently unlock the Pro UI on garbage input. Validate the normalized value against community|enterprise|cloud and exit 1 before writing, mirroring the guard in bin/docker-compose so the ui container fails loudly at startup instead of serving a wrong edition.
.env.enterprise declared SHELLHUB_EDITION=enterprise, but the wrapper only loads it once the edition is already non-community (peeked from ./.env or ./.env.override). Because it sits after ./.env in COMPOSE_ENV_FILES, setting SHELLHUB_EDITION=cloud in ./.env was clobbered back to enterprise in the containers while the wrapper still selected the cloud overlays from its own peek — a split-brain state. The file is shared by the enterprise and cloud editions, so it cannot know which one applies; the edition always comes from ./.env or ./.env.override. Drop the assignment so peek and container value agree in every path.
The gateway parsed SHELLHUB_EDITION itself: an envconfig struct field, a manual trim/lowercase in applyDefaults, and a separate validation switch with a near-duplicate error message. Now that pkg/envs exposes ResolveEdition, collapse all three into one call so edition normalization, validation, and the error string live in a single place. The Edition field is dropped (the nginx template only consumes EnableEnterprise/EnableCloud), and behavior is unchanged: the same env var is read, normalized, defaulted to community, and validated identically.
d22cdfe to
4691a7d
Compare
|
/review |
|
Claude finished @otavio's task in 6m 52s —— View job Code Review CompleteReviewed 86 files across code quality, security, testing, language patterns, and architecture — no new issues found. The code looks good as-is. This is a re-review round. The branch was reworked since the last round (now 6 commits) and both previously-flagged normalization gaps are resolved: The two known non-blocking notes from prior rounds still stand and are intentionally not re-reported inline: If you push additional changes and want a new review, tag |
2 participants
What
Replaces the
SHELLHUB_ENTERPRISEandSHELLHUB_CLOUDboolean env vars with a singleSHELLHUB_EDITION=community|enterprise|cloudenum, making the previously illegal fourth state (cloud=true, enterprise=false) unrepresentable by design.Why
Two booleans encoded three editions but permitted a fourth, invalid combination guarded only by a runtime check in
bin/docker-compose.IsEnterprise()meant "at least enterprise" (true on cloud too), and the samecloud || enterpriseexpression appeared under five different local names in the frontend. The build layer already used an enum (ARG EDITION=community|enterprise), so build-time and runtime representations were inconsistent.Closes shellhub-io/team#156
Changes
pkg/envs: addedEditiontype (Community,Enterprise,Cloud) andCurrentEdition()that readsSHELLHUB_EDITION, normalizes (trim + lowercase), defaults to community, and hard-fails on invalid values. Reimplemented predicates:IsCommunity(),IsEnterprise()(now exact),IsCloud()(now exact),IsEnterpriseOrCloud()(new). Addedenvstest.SetEdition()andenvstest.SetRawEdition()helpers that replace all call-order-coupled mock stubs across 35+ test blocks.gateway/config.go: resolvesSHELLHUB_EDITIONonce and derivesEnableEnterprise/EnableCloudbools for the nginx template, which stays unchanged.ssh/session: license+firewall check uses exactIsEnterprise(), asciinema saving usesIsEnterpriseOrCloud().bin/docker-compose: resolvesSHELLHUB_EDITIONwith validation (invalid value = hard exit), selects compose overlays and env files by edition. Deletes thecloud-requires-enterpriseguard..env,.env.enterprise,docker-compose*.yml: two booleans collapsed toSHELLHUB_EDITION.ui/apps/console/src/env.ts:ClientConfig.{enterprise, cloud}replaced withedition: Edition. Same four predicates as Go:isCommunity(),isEnterprise(),isCloud(),isEnterpriseOrCloud().ui/apps/console/src/__mocks__/env.ts+test/setup.ts: manual mock withvi.fn()-backedgetConfig, globalvi.mock("@/env")in setup. Eliminates the 10-linevi.mockfactory repeated in 29 test files.ui/apps/console/src/utils/features.ts: deleted.isPremiumFeature()andhasMfaSupport()were thin wrappers; callers now importisEnterpriseOrCloud()directly.isEnterprise()(exact match, excluded cloud) gated SAML SSO — changed toisEnterpriseOrCloud()so SAML works on cloud too.configuring.mdxandupgrading.mdxto referenceSHELLHUB_EDITION.Testing
pkg/envstests cover normalization (trim, lowercase, invalid → error, empty → community) and all four predicates across all three editions.tests/compose/wrapper.batsandtests/compose/with_cloud.batsupdated and passing.