fix(cli): require SPRITES_TOKEN for Fly's implicit sandbox default - #528
Open
SomSamantray wants to merge 6 commits into
Open
fix(cli): require SPRITES_TOKEN for Fly's implicit sandbox default#528SomSamantray wants to merge 6 commits into
SomSamantray wants to merge 6 commits into
Conversation
…oot time qm check resolved SANDBOX_BACKEND for a Fly target from a table that never derived the implicit "sprites" default sandboxCoreEnv actually applies at boot when sandbox.app is set and sandbox.backend is left unset. A Fly deployment relying on that implicit default could pass qm check and qm secrets push without SPRITES_TOKEN ever being required, then fail to boot (yc-software#423). Mirror sandboxCoreEnv's default in TARGET_ENV_DEFAULTS.fly and cross-reference both sites so they don't drift apart again. Several test fixtures across the CLI suite had the same implicit-default shape and were relying on the bug to avoid provisioning SPRITES_TOKEN; updated them to supply it now that it's correctly required.
Replace the mirrored SANDBOX_BACKEND default expression (previously kept in sync only by a pair of cross-reference comments) with one exported function both sandboxCoreEnv and TARGET_ENV_DEFAULTS.fly call, so the boot-time and check-time defaults can no longer drift apart the way yc-software#423 required.
Remove explanatory comment above flyImplicitSandboxBackend to comply with this repo's zero-comments standard (AGENTS.md).
Required by CI: this PR changes cli/src, which ships in @yc-software/qm.
Required by CI's formatting check.
The e2e "plan (fly) reports secrets: ok when every required secret is staged" test builds its config via flyConfig(), which (through writeConfig()'s default template) always sets sandbox.app with no explicit sandbox.backend -- the same implicit-sprites-default shape this PR's fix now correctly requires SPRITES_TOKEN for. ALL_SECRETS never included it, so the fake fly backend never reported it staged and qm plan correctly flagged it missing, breaking the test. This fixture was missed earlier because `npm test` doesn't run the separate `test:e2e` suite. Confirmed via CI logs and by re-running the affected test locally.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A Fly deployment that left
sandbox.backendunset (relying on the implicit"sprites"default) could passqm checkandqm secrets pushwithout ever being toldSPRITES_TOKENwas required, then fail to boot because the sandbox substrate had no token. The two computations had drifted apart:sandboxCoreEnv(the actual boot-time env builder) already applied the implicit"sprites"default for Fly, butqm check's secret-requirement resolver (TARGET_ENV_DEFAULTS.fly) never derived that same default.This extracts a single shared helper,
flyImplicitSandboxBackend, that both the boot-time and check-time paths call, soqm checknow requiresSPRITES_TOKENexactly when a deployment will actually need it, and the two can't silently diverge again the way they did here.Added a regression test covering the four relevant scenarios (implicit default, explicit backend, no sandbox configured, and AWS's own unrelated default staying unaffected), and found three existing test fixtures across the CLI suite that were unknowingly relying on the old bug to avoid provisioning
SPRITES_TOKEN— updated those to supply it. Full CLI test suite passes on this branch (the small number of remaining failures are pre-existing environment gaps unrelated to this change, present onmainas well).Fixes #423.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.