feat(infra): make deployment configuration immutable - #1202
Conversation
📝 WalkthroughWalkthroughThe change adds trusted deploy-role attestation, immutable deployment configuration releases, stage-scoped IAM policies, runtime-secret lifecycle management, guarded deployment previews, and stricter Runner and SST validation. CI no longer materializes ChangesDeployment and configuration lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
📦 BoxLite review — couldn't completepowered by BoxLite |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (33)
apps/infra/scripts/runtime-secret-generation-guard.mjs (1)
63-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidate
stageoutside the swallowingtry.
readRuntimeSecretGenerationsvalidatesawsCliPathandregionbefore thetry, but notstage.runtimeSecretName(stage, definition.id)runs at Line 95, inside thetry. An invalid or missingstagetherefore surfaces ascould not verify runtime secret generations from AWS metadata, which points the operator at AWS instead of at the argument.The same
tryalso converts a missing AWS CLI binary, a timeout, and anAccessDeniedresponse into that one message. The broad catch is correct for non-disclosure, andruntime-secret-generation-guard.test.mjsLines 143-149 depend on it. Hoisting only the stage check keeps that property and returns a precise error for caller input.♻️ Proposed change
if (typeof region !== 'string' || !region) { throw new Error('runtime secret generation guard requires region') } + // Validate caller input before the catch that redacts AWS metadata errors, + // so an invalid stage does not masquerade as an AWS failure. + validateRuntimeSecretStage(stage) const execute =Add the import:
import { RUNTIME_SECRET_DEFINITIONS, RUNTIME_SECRET_INITIALIZATION_TAG, RUNTIME_SECRET_INITIAL_VALUE_TAG, runtimeSecretName, runtimeSecretNeedsGeneratedInitialVersion, + validateRuntimeSecretStage, } from './runtime-secrets.mjs'🤖 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 `@apps/infra/scripts/runtime-secret-generation-guard.mjs` around lines 63 - 111, Validate the stage argument before entering the swallowing try block in readRuntimeSecretGenerations, alongside the existing awsCliPath and region checks. Reject missing or non-string stage values with a precise caller-input error, while leaving the broad catch around AWS execution and generation parsing unchanged.apps/infra/scripts/runtime-secrets-cli.mjs (2)
40-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the shared AWSCURRENT helpers.
hasCurrentVersionreimplements theVersionIdsToStagescontract that this PR centralizes inapps/infra/scripts/runtime-secret-version-stages.mjs. That module exportsnormalizeRuntimeSecretVersionStagesandcurrentRuntimeSecretVersionIdfor the same AWS response shape, andruntime-secret-generation-guard.mjsLines 17-20 already import them.The two implementations also disagree on malformed input. The shared helper rejects a malformed version map.
hasCurrentVersionreturnsfalse, soprintStatusprintsUNSETfor a secret whose metadata could not be parsed.printStatusalready models a third state for the SST side at Line 80, so the runtime-secret side can report the same uncertainty instead of a definiteUNSET.🤖 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 `@apps/infra/scripts/runtime-secrets-cli.mjs` around lines 40 - 46, Update hasCurrentVersion and its printStatus caller to reuse normalizeRuntimeSecretVersionStages and currentRuntimeSecretVersionId from the shared version-stages module instead of inspecting VersionIdsToStages locally. Preserve distinct handling for a valid map with no AWSCURRENT and malformed metadata, so runtime-secret status reports uncertainty rather than incorrectly printing UNSET.
352-356: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winValidate
ghcrUsernameinsidebuildDefaultRunnerSecretMigration.
buildDefaultRunnerSecretMigrationinterpolatesghcrUsernameraw at Line 388. Line 497 opens the drop-in withcat > "$DROPIN" << DROPIN, which is an unquoted heredoc, so the shell expands$and backticks in that body.The two sibling builders in this file validate the same value themselves.
buildExtraRunnerGhcrMigrationchecks the charset at Line 133.buildDefaultRunnerLegacyRollbackchecks it at Lines 519-521. This builder does not.The single caller
reconcileDefaultRunnervalidates at Lines 720-722, so there is no exploitable path today. The function is exported and is called directly byruntime-secrets-contract.test.mjsat Lines 1018, 1150, and 1395, so a future caller can bypass the check.🔒️ Proposed fix
export function buildDefaultRunnerSecretMigration({ region, runnerTokenSecretArn, ghcrSecretArn, ghcrUsername }) { + if (ghcrUsername && !/^[A-Za-z0-9_.-]+$/.test(ghcrUsername)) { + throw new Error('ghcrUsername contains unsupported characters') + } const runnerTokenArnBase64 = Buffer.from(runnerTokenSecretArn).toString('base64')The three builders also disagree on how they express the GHCR toggle.
buildExtraRunnerGhcrMigrationtakes an explicitghcrEnabledboolean. This builder andbuildDefaultRunnerLegacyRollbackinfer the toggle from a truthyghcrUsername. Aligning the three signatures would remove that ambiguity.Also applies to: 385-389
🤖 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 `@apps/infra/scripts/runtime-secrets-cli.mjs` around lines 352 - 356, Add the same allowed-character validation used by buildExtraRunnerGhcrMigration and buildDefaultRunnerLegacyRollback at the start of buildDefaultRunnerSecretMigration, before encoding or interpolating ghcrUsername. Also align this builder’s GHCR toggle with the sibling builders by accepting an explicit ghcrEnabled boolean and using it instead of inferring enablement from a truthy ghcrUsername, updating its callers accordingly.apps/infra/scripts/runner-update-binary.test.mjs (1)
644-645: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the stale assertion message.
Line 644 reports "fail on the missing CLI", but Line 645 now asserts the deployment operation-lock failure. Align the message with the checked behaviour so a failing run points at the right cause.
♻️ Proposed wording
- assert.equal(result.status, 1, `expected the roll to run and fail on the missing CLI:\n${result.stdout}`) + assert.equal( + result.status, + 1, + `expected the roll to reach the deployment operation lock and fail there:\n${result.stdout}`, + )🤖 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 `@apps/infra/scripts/runner-update-binary.test.mjs` around lines 644 - 645, Update the assertion message in the test around result.status to describe the expected deployment operation-lock failure instead of a missing CLI, while preserving the existing status and stderr assertions.apps/infra/scripts/environment-bootstrap.test.mjs (1)
50-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the non-boolean gate rejection.
cloudFormationParameterOverridesthrows'CloudFormation safety gates must be boolean'for non-boolean input. No test exercises that branch. A regression that coerces'true'into the override list would pass the current suite.♻️ Proposed additional assertion
assert.throws(() => cloudFormationParameterOverrides({ repo: 'not-a-repo', stage: 'dev' }), /must look like/) + assert.throws( + () => + cloudFormationParameterOverrides({ + repo: 'boxlite-ai/boxlite', + stage: 'dev', + runnerCommandTagGateEnabled: 'true', + }), + /must be boolean/, + )🤖 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 `@apps/infra/scripts/environment-bootstrap.test.mjs` around lines 50 - 67, Add an assertion to the cloudFormationParameterOverrides test that passes a non-boolean value such as a string for a safety-gate option and verifies it throws the “CloudFormation safety gates must be boolean” error, covering both gate inputs as appropriate.apps/infra/scripts/runner-artifact-build.test.mjs (1)
127-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe
loadEnvironmentassertion cannot fail.
RunnerArtifactBuilderno longer destructuresloadEnvironment, so the injected hook is discarded by the constructor.loadedEnvironmenttherefore staysfalsefor any implementation, including one that reintroduces dotenv loading through an internal import. The assertion on Line 151 gives no protection.Assert the property at the source boundary instead, for example that
runner-artifact-build.mjsimports no dotenv loader. The remaining assertions in this test (release pinning, config store calls, region propagation) are meaningful and should stay.🤖 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 `@apps/infra/scripts/runner-artifact-build.test.mjs` around lines 127 - 158, Replace the ineffective loadedEnvironment hook assertion in the test around RunnerArtifactBuilder with a source-boundary assertion that verifies runner-artifact-build.mjs does not import or expose a dotenv loader. Remove the discarded loadEnvironment injection and related loadedEnvironment state, while preserving the release pinning, configStore call, and region propagation assertions.apps/infra/scripts/runner-update-binary.mjs (1)
563-586: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
withRunnerUpdateOperationLockreleases the lock before an async callback settles.The
finallyblock runs as soon asupdate()returns. If a caller passes an async function, the lock is released while the update is still in progress, and a concurrent operation can start. The currentmaincallback is synchronous, so no defect exists today. Make the constraint explicit so a later async refactor cannot silently break exclusivity.♻️ Proposed guard
if (typeof update !== 'function') throw new Error('runner update operation callback is required') const store = createStore({ awsCliPath: environment.AWS_CLI_PATH || 'aws', region }) const inheritedOwnerId = environment[DEPLOYMENT_OPERATION_LOCK_OWNER_ENV] if (inheritedOwnerId) { store.assertDeploymentOperationLockOwner({ stage, ownerId: inheritedOwnerId }) return update() } const lock = store.acquireDeploymentOperationLock({ stage }) + let result try { - return update() + result = update() + if (typeof result?.then === 'function') { + throw new Error('the Runner update callback must be synchronous; the lock is released on return') + } + return result } finally { store.releaseDeploymentOperationLock(lock) }🤖 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 `@apps/infra/scripts/runner-update-binary.mjs` around lines 563 - 586, Update withRunnerUpdateOperationLock to explicitly reject callbacks that return a promise before the acquired lock can be released, preserving synchronous callback behavior and ensuring asynchronous updates cannot silently run without lock coverage.apps/infra/scripts/sst-config-contract.test.mjs (1)
444-444: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the source-pinning regex non-greedy.
[\s\S]*is greedy and unanchored. The assertion passes ifartifactPolicy: extraRunnerArtifactPolicyappears anywhere after theextraRunners.map((r) => ({opening, including outside that object literal. Use a lazy quantifier to keep the assertion tight.♻️ Proposed change
- assert.match(upgrades, /\.\.\.extraRunners\.map\(\(r\) => \(\{[\s\S]*artifactPolicy: extraRunnerArtifactPolicy/) + assert.match(upgrades, /\.\.\.extraRunners\.map\(\(r\) => \(\{[\s\S]*?artifactPolicy: extraRunnerArtifactPolicy/)🤖 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 `@apps/infra/scripts/sst-config-contract.test.mjs` at line 444, Update the regex in the source-pinning assertion around extraRunners.map to use a non-greedy cross-line match instead of the greedy [\s\S]* quantifier, so artifactPolicy: extraRunnerArtifactPolicy must occur within the matched object literal.apps/infra/scripts/sst-event-log-security.test.mjs (1)
48-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a default branch to the fake AWS CLI.
An unmatched command writes nothing and exits 0. A caller then receives empty stdout and fails with a JSON parse error instead of a clear signal. The fake AWS CLI in
apps/infra/scripts/sst-native-environment.test.mjs(Lines 351-353) exits 90 for unhandled commands. Use the same behavior here.♻️ Proposed change
} else if (args[0] === 'secretsmanager' && args[1] === 'describe-secret') { process.stdout.write(JSON.stringify({ Tags: [ { Key: 'boxlite:initial-value', Value: 'generated' }, { Key: 'boxlite:initialization', Value: 'pending' }, ], VersionIdsToStages: {}, })) +} else { + process.stderr.write('unhandled synthetic aws command: ' + args.join(' ')) + process.exit(90) }🤖 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 `@apps/infra/scripts/sst-event-log-security.test.mjs` around lines 48 - 85, Update the FAKE_CONFIG_AWS script in sst-event-log-security.test.mjs to add a final fallback branch for unmatched AWS commands that exits with status 90, matching the fake CLI behavior in sst-native-environment.test.mjs. Keep all existing command handlers unchanged.apps/infra/scripts/sst-native-environment.test.mjs (1)
21-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated synthetic deployment-config fixture in two test files. Both files build the same canonical deployment-config document and derive a release id from it. The shared root cause is the absence of one exported test fixture. A future schema change must be applied twice, and a partial update lets the two suites diverge silently.
apps/infra/scripts/sst-native-environment.test.mjs#L21-L45: replace the inline document andCONFIG_RELEASEwith an import from a shared fixture module.apps/infra/scripts/sst-event-log-security.test.mjs#L22-L46: replaceSYNTHETIC_CONFIG_SOURCEandSYNTHETIC_CONFIG_RELEASEwith the same shared fixture import.🤖 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 `@apps/infra/scripts/sst-native-environment.test.mjs` around lines 21 - 45, Extract the duplicated canonical deployment-config document and release identifier into one exported shared test fixture module. In apps/infra/scripts/sst-native-environment.test.mjs:21-45, replace CONFIG_SOURCE and CONFIG_RELEASE with imports from that fixture; make the same replacement for SYNTHETIC_CONFIG_SOURCE and SYNTHETIC_CONFIG_RELEASE in apps/infra/scripts/sst-event-log-security.test.mjs:22-46, preserving each suite’s existing references.apps/infra/scripts/sst-command-contract.test.mjs (1)
151-166: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCapture wrapper output for failure diagnosis.
The wrapper runs with
stdio: 'ignore'. If the exit code is not 0, the assertion at Line 166 reports only the code. Capturestderrand include it in the assertion message to make CI failures actionable.♻️ Proposed change
- stdio: 'ignore', + stdio: ['ignore', 'ignore', 'pipe'], }) + let stderr = '' + wrapper.stderr.setEncoding('utf8') + wrapper.stderr.on('data', (chunk) => { + stderr += chunk + }) try { - assert.deepEqual(await waitForExit(wrapper), { code: 0, signal: null }) + assert.deepEqual(await waitForExit(wrapper), { code: 0, signal: null }, stderr)🤖 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 `@apps/infra/scripts/sst-command-contract.test.mjs` around lines 151 - 166, Update the wrapper process setup and waitForExit assertion to capture stderr instead of discarding it via stdio: 'ignore'. Include the captured stderr in the assertion failure message while preserving the expected { code: 0, signal: null } result.apps/infra/scripts/sst-stage.mjs (1)
4-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo modules define the same stage grammar independently.
SST_STAGE_PATTERN = /^[a-z0-9]{1,20}$/is declared twice. Both copies gate the same stage value on the same execution path, so a change to one copy makes the guards disagree and the weaker copy decides.
apps/infra/scripts/sst-stage.mjs#L4-L8: exportSST_STAGE_PATTERN(or avalidateSstStagehelper) as the single source of truth for the stage grammar.apps/infra/scripts/sst-native-environment.mjs#L45-L48: remove the local declaration at line 7 and import the exported constant fromsst-stage.mjs.🤖 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 `@apps/infra/scripts/sst-stage.mjs` around lines 4 - 8, Export SST_STAGE_PATTERN from sst-stage.mjs as the single source of truth, then remove the duplicate local pattern in apps/infra/scripts/sst-native-environment.mjs and import the shared constant there. Preserve both modules’ existing validation behavior.apps/infra/scripts/sst-event-log-security.mjs (2)
114-121: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueThe
finallyre-preparation can replace the command result with its own error.If
runSstCommand()throws or returns an exit code, andprepareSstLogSecuritythen fails in thefinallyblock, the log-security error propagates and the original error or exit code is lost. The caller inapps/infra/scripts/sst-with-cloudflare.mjs(lines 477-483) then reportssecure log cleanup failedand setsexitCode = 1, hiding the real SST failure. The fail-closed exit status is correct. Only the diagnostic is degraded.Consider logging the cleanup failure and preserving the original outcome.
♻️ Proposed change to preserve the original outcome
export async function withSstLogSecurity(infraRoot, runSstCommand) { await prepareSstLogSecurity(infraRoot) + let commandFailed = false try { return await runSstCommand() + } catch (error) { + commandFailed = true + throw error } finally { - await prepareSstLogSecurity(infraRoot) + try { + await prepareSstLogSecurity(infraRoot) + } catch (cleanupError) { + if (!commandFailed) throw cleanupError + console.error(`secure log cleanup failed after a command failure: ${cleanupError.message}`) + } } }🤖 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 `@apps/infra/scripts/sst-event-log-security.mjs` around lines 114 - 121, Update withSstLogSecurity so failures from the finally-block prepareSstLogSecurity cleanup are logged without replacing the original runSstCommand result or error. Preserve the command’s exit code or thrown exception, while retaining fail-closed behavior when cleanup is the only failure.
92-100: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace message-substring control flow with an explicit error marker.
Lines 97-99 decide whether to rethrow by matching the substring
refusing to run SSTinerror.message. This couples control flow to prose. If any message text changes, the branch silently changes behavior. Two throw sites here produce the same final error anyway, so you can drop the string test.♻️ Proposed simplification
- try { - const finalMetadata = await lstat(path) - if (!finalMetadata.isSymbolicLink() || (await readlink(path)) !== POSIX_NULL_DEVICE) { - throw new Error('fixed Pulumi log null sink verification failed; refusing to run SST') - } - } catch (error) { - if (error.message?.includes('refusing to run SST')) throw error - throw new Error('fixed Pulumi log null sink verification failed; refusing to run SST') - } + let verified = false + try { + const finalMetadata = await lstat(path) + verified = finalMetadata.isSymbolicLink() && (await readlink(path)) === POSIX_NULL_DEVICE + } catch { + verified = false + } + if (!verified) throw new Error('fixed Pulumi log null sink verification failed; refusing to run SST')🤖 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 `@apps/infra/scripts/sst-event-log-security.mjs` around lines 92 - 100, Update the error handling around the finalMetadata verification to remove the error.message substring check and its conditional rethrow. Let verification failures propagate through a single explicit throw path, preserving the existing final error message without coupling control flow to prose.apps/infra/scripts/sst-with-cloudflare.mjs (3)
93-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueA failed lock release is reported twice and retried at exit.
Line 97 clears
deploymentOperationLockonly afterreleaseDeploymentOperationLockreturns. If the release throws at line 546, the variable stays set. The handler at lines 104-110 then repeats the same synchronous AWS call duringprocess.exitand logs the same failure again. The exit status is already correct. Only the operator output is duplicated.Clear the variable before the call if a single attempt is intended.
♻️ Proposed change
function releaseDeploymentOperationLock() { if (!deploymentOperationLock) return const ownedLock = deploymentOperationLock + deploymentOperationLock = undefined deploymentOperationLockStore.releaseDeploymentOperationLock(ownedLock) - deploymentOperationLock = undefined }Also applies to: 545-550
🤖 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 `@apps/infra/scripts/sst-with-cloudflare.mjs` around lines 93 - 98, Update releaseDeploymentOperationLock to set deploymentOperationLock to undefined before calling deploymentOperationLockStore.releaseDeploymentOperationLock, so a failed release cannot be retried by the process-exit handler. Preserve the existing ownedLock value for the single release attempt and keep the current exit status behavior.
365-383: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
resolveAwsCliPathis called three times in the deploy preflight.Line 368 calls
resolveAwsCliPath(sstEnvironment)and discards the result. Line 401 calls it again. Line 419 and line 424 use the module-levelawsCliPath. Reuse the single module-level value so every call in this run uses the same executable path.♻️ Proposed change
- resolveAwsCliPath(sstEnvironment) + awsCliPath ??= resolveAwsCliPath(sstEnvironment)- { awsCliPath: resolveAwsCliPath(sstEnvironment) }, + { awsCliPath },Also applies to: 397-401, 414-427
🤖 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 `@apps/infra/scripts/sst-with-cloudflare.mjs` around lines 365 - 383, Update the deploy preflight around resolveAwsCliPath, including the calls near the initial setup and later artifact checks, to resolve the AWS CLI path once and reuse the module-level awsCliPath value throughout the run. Remove the discarded and repeated resolveAwsCliPath calls while preserving the existing command execution behavior.
238-256: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe deployment config is read from the ambient region, then the region is replaced.
Line 238 resolves
regionfromsstEnvironment. Line 244 reads the release with that region. Line 250 then replacesregionwithdeploymentConfigRelease.document.region. Every later AWS call, including the operation lock at line 272, the generation guard at line 274, and the provider credential reads at line 309, uses the replaced value.If the two regions differ, the release lookup and the lock live in different regions. Mutual exclusion still holds, because all runs derive the lock region from the same document. Add a short comment that records this intent, so a later reader does not treat the reassignment as a bug.
🤖 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 `@apps/infra/scripts/sst-with-cloudflare.mjs` around lines 238 - 256, In the deployment-config handling around resolveAndInjectDeploymentConfig and the subsequent region reassignment, add a concise comment documenting that the initial region is used to read the deployment config, while later AWS operations intentionally use deploymentConfigRelease.document.region, including the lock. Do not change the existing region resolution or reassignment behavior.apps/infra/scripts/deploy-role-boundary.mjs (1)
103-112: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the stage validation at Line 109 explicit.
githubDeployRoleStackName(stage)is called only to reuse itsrequireStageLikevalidation. The return value is discarded, so the call looks removable. Add a short comment or call the stage validator directly.♻️ Proposed clarification
- githubDeployRoleStackName(stage) + // Validation only: rejects a stage that cannot form a legal stack/role name. + githubDeployRoleStackName(stage)🤖 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 `@apps/infra/scripts/deploy-role-boundary.mjs` around lines 103 - 112, Make the stage validation in assertDeployPolicyContract explicit by documenting that githubDeployRoleStackName(stage) is intentionally invoked for its requireStageLike validation, or by calling the stage validator directly if it is accessible. Preserve the existing validation behavior and ensure the intent is clear despite the discarded return value.apps/infra/scripts/deploy-role-boundary.test.mjs (1)
59-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the
CREATE_COMPLETEand duplicate-stack cases.The accept path only exercises
UPDATE_COMPLETE.assertDeployRoleStackCompletealso acceptsCREATE_COMPLETE, and it rejects a list that contains the stack more than once. Neither branch is covered.💚 Proposed additional cases
for (const stacks of [ [], [{ StackName: 'boxlite-prod-github-deploy', StackStatus: 'UPDATE_COMPLETE' }], [{ StackName: 'boxlite-dev-github-deploy', StackStatus: 'UPDATE_IN_PROGRESS' }], [{ StackName: 'boxlite-dev-github-deploy', StackStatus: 'UPDATE_ROLLBACK_COMPLETE' }], + [ + { StackName: 'boxlite-dev-github-deploy', StackStatus: 'CREATE_COMPLETE' }, + { StackName: 'boxlite-dev-github-deploy', StackStatus: 'CREATE_COMPLETE' }, + ], ]) { assert.throws(() => assertDeployRoleStackComplete({ stage: 'dev', stacks }), /CloudFormation stack|complete/i) } + assert.equal( + assertDeployRoleStackComplete({ + stage: 'dev', + stacks: [{ StackName: 'boxlite-dev-github-deploy', StackStatus: 'CREATE_COMPLETE' }], + }).stackStatus, + 'CREATE_COMPLETE', + )🤖 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 `@apps/infra/scripts/deploy-role-boundary.test.mjs` around lines 59 - 75, Add coverage in the test for assertDeployRoleStackComplete by accepting the exact dev stack with CREATE_COMPLETE and rejecting a stacks list containing duplicate matching dev stack entries. Preserve the existing valid UPDATE_COMPLETE case and invalid-stage/status cases.apps/infra/scripts/deployment-config-capability.mjs (1)
132-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe
.resolve(guard is receiver-blind and will reject ordinary wrapper code.The regex
/\.(?:resolve|readRelease|prepare|prepareDocument|publish|activate|putCurrent|putRelease)\s*\(/matches any method call with those names, regardless of the receiver.path.resolve(...),Promise.resolve(...), andresolver.resolve(...)all match.liveTextstrips comments and strings, so only real code matches, which is exactly where these calls appear.If a future change adds
path.resolve()tosst-with-cloudflare.mjs, the probe fails withSST wrapper bypasses the composed deployment config loader. That message points a maintainer at the deployment-config loader, not at the harmless call that tripped the guard. Theselected-ref-contractjob in.github/workflows/deploy-infra.ymlthen blocks every deploy for that commit.Anchor the pattern to the deployment-config store receivers instead of bare method names.
♻️ Proposed tightening
requireCapability( !/\binjectDeploymentConfigEnvironment\b/.test(liveWrapper) && - !/\.(?:resolve|readRelease|prepare|prepareDocument|publish|activate|putCurrent|putRelease)\s*\(/.test(liveWrapper), + !/\b(?:new\s+DeploymentConfigStore|deploymentConfigStore|configStore)\b[\s\S]{0,200}?\.(?:resolve|readRelease|prepare|prepareDocument|publish|activate|putCurrent|putRelease)\s*\(/.test( + liveWrapper, + ), 'SST wrapper bypasses the composed deployment config loader', )Adjust the receiver names to match the identifiers the wrapper actually uses.
🤖 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 `@apps/infra/scripts/deployment-config-capability.mjs` around lines 132 - 136, Update the method-call regex in requireCapability to match only the deployment-config store receiver identifiers used by the wrapper, rather than any receiver with methods such as resolve or publish. Preserve detection of genuine deployment-config store calls while allowing unrelated calls like path.resolve(), Promise.resolve(), and resolver.resolve()..github/workflows/deploy-release.yml (1)
121-138: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThe deployment-config resolver runs on an unpinned Node in both workflows. Both workflows place the
Resolve deployment config releasestep, which invokesnode apps/infra/scripts/deployment-config-resolve.mjs, before theiractions/setup-node@v4step that pinsnode-version: '22'. The resolver therefore executes on the runner image's preinstalled Node while every other Node step uses the pinned version. A runner image bump can change that version and break the resolver, or hide a Node 22 dependency until an image rollout.
.github/workflows/deploy-release.yml#L121-L138: move theSet up Node.jsstep at lines 140-145 ahead of this resolver step..github/workflows/deploy-infra.yml#L428-L445: move theSet up Node.jsstep at lines 499-504 ahead of this resolver step, keeping it after theAttest assumed deploy role contractstep so the trusted verifier still runs first.🤖 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 @.github/workflows/deploy-release.yml around lines 121 - 138, Move the existing Set up Node.js step before the Resolve deployment config release step in .github/workflows/deploy-release.yml (lines 121-138), so deployment-config-resolve.mjs runs with Node 22. Apply the same ordering change in .github/workflows/deploy-infra.yml (lines 428-445), keeping Set up Node.js after Attest assumed deploy role contract so the trusted verifier remains first.apps/infra/scripts/deployment-config.mjs (1)
40-55: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe
^OTEL_denied pattern also blanks three release-classified keys.
SST_NATIVE_DENIED_AMBIENT_PATTERNSincludes/^OTEL_/at line 51. The second loop at lines 698-700 applies it to every key in the environment, with no registry exemption.OTEL_ENABLED,OTEL_TRACING_ENABLED, andOTEL_EXPORTER_OTLP_ENDPOINTarerelease-classified at lines 121-123, so the shield blanks them too.
injectDeploymentConfigEnvironmentrecovers from this only because it callsshieldSstEnvironmentat line 664 before it overlaysdocument.valuesat lines 665-667. The correctness of these three release values therefore depends on that statement order alone. Nothing asserts it. A future reordering, or any caller that shields without a following overlay, silently drops the release-supplied OTEL configuration.Add a regression test that a release value whose name matches a denied pattern survives injection.
♻️ Proposed test in apps/infra/scripts/deployment-config.test.mjs
+test('a release value whose name matches a denied ambient pattern survives injection', async () => { + const { injectDeploymentConfigEnvironment } = await configModule() + const environment = { OTEL_ENABLED: 'ambient-sentinel' } + + injectDeploymentConfigEnvironment( + { + releaseId: 'e'.repeat(64), + document: { + accountId: ACCOUNT_ID, + region: REGION, + schemaVersion: 1, + stage: STAGE, + values: { + BOXLITE_RUNTIME_SECRET_GENERATIONS: pendingRuntimeSecretGenerations, + OTEL_ENABLED: true, + OTEL_EXPORTER_OTLP_ENDPOINT: 'https://collector.example.test/v1/traces', + }, + }, + }, + environment, + ) + + assert.equal(environment.OTEL_ENABLED, 'true', 'shielding must not outlive the release overlay') + assert.equal(environment.OTEL_EXPORTER_OTLP_ENDPOINT, 'https://collector.example.test/v1/traces') +})Also applies to: 683-702
🤖 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 `@apps/infra/scripts/deployment-config.mjs` around lines 40 - 55, Add a regression test in the deployment configuration tests covering injectDeploymentConfigEnvironment with a release-classified OTEL key, such as OTEL_ENABLED or OTEL_EXPORTER_OTLP_ENDPOINT, whose name matches SST_NATIVE_DENIED_AMBIENT_PATTERNS. Assert that the release value survives environment shielding and injection, independent of ambient environment values or statement ordering.apps/infra/scripts/deployment-config-capability.test.mjs (1)
28-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd negative coverage for the remaining wrapper guards.
This test covers only the
.resolve(bypass branch.verifyDeploymentConfigCapabilityhas three more source guards inapps/infra/scripts/deployment-config-capability.mjs: the missing loader import (lines 122-127), the missing loader call (lines 128-131), and the reintroduced dotenv path (lines 137-142). None of them has a test. A regression that deletes one of those regexes would pass CI.♻️ Proposed additional cases
test('selected-ref capability permits lock-only store use but rejects direct config resolution', () => { const wrapperSource = readFileSync(new URL('sst-with-cloudflare.mjs', import.meta.url), 'utf8') assert.equal(verifyDeploymentConfigCapability({ wrapperSource }), true) assert.throws( () => verifyDeploymentConfigCapability({ wrapperSource: `${wrapperSource}\ndeploymentOperationLockStore.resolve({ stage })`, }), /bypasses the composed deployment config loader/, ) }) + +test('selected-ref capability rejects a wrapper that drops the loader or reintroduces dotenv', () => { + const wrapperSource = readFileSync(new URL('sst-with-cloudflare.mjs', import.meta.url), 'utf8') + + assert.throws( + () => + verifyDeploymentConfigCapability({ + wrapperSource: wrapperSource.replace(/deployment-config-loader\.mjs/g, 'deployment-config-legacy.mjs'), + }), + /does not import the composed deployment config loader/, + ) + assert.throws( + () => + verifyDeploymentConfigCapability({ + wrapperSource: `${wrapperSource}\nimport 'dotenv/config'\n`, + }), + /still has a routine dotenv path/, + ) + assert.throws( + () => + verifyDeploymentConfigCapability({ + wrapperSource: `${wrapperSource}\ninjectDeploymentConfigEnvironment(release, environment)\n`, + }), + /bypasses the composed deployment config loader/, + ) +})🤖 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 `@apps/infra/scripts/deployment-config-capability.test.mjs` around lines 28 - 38, Add negative test cases alongside the existing selected-ref test for each remaining guard in verifyDeploymentConfigCapability: omit the composed loader import, omit its invocation, and reintroduce the dotenv configuration path. Assert each modified wrapper source throws with the corresponding guard error, while retaining the existing valid-wrapper and deploymentOperationLockStore.resolve bypass coverage.apps/infra/scripts/deployment-config.test.mjs (1)
540-569: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a case that rejects a secret-classified key inside
values.
validateDocumentinapps/infra/scripts/deployment-config.mjsline 514 rejects anyvalueskey that is notrelease-classified and notreleaseMetadata. That guard is what keepsruntime-secretandprovider-secretnames out of a plaintext SSM release. No test exercises it.The existing
extraFieldSourcecase covers only an unexpected top-level field, not an unexpected key insidevalues.♻️ Proposed additional case
assert.throws( () => parseDeploymentConfigRelease(extraFieldSource, { ...expected, releaseId: deploymentConfigReleaseId(extraFieldSource), }), /unexpected|shape|field|canonical/i, ) + + const secretBearingDocument = { + ...JSON.parse(GOLDEN_SOURCE), + values: { ...JSON.parse(GOLDEN_SOURCE).values, ADMIN_API_KEY: 'secret-sentinel-in-plaintext-release' }, + } + const secretBearingSource = JSON.stringify(secretBearingDocument) + assert.throws( + () => + parseDeploymentConfigRelease(secretBearingSource, { + ...expected, + releaseId: deploymentConfigReleaseId(secretBearingSource), + }), + (error) => { + assert.match(error.message, /ADMIN_API_KEY/) + assert.equal(error.message.includes('secret-sentinel-in-plaintext-release'), false) + return true + }, + )🤖 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 `@apps/infra/scripts/deployment-config.test.mjs` around lines 540 - 569, Add a test case alongside the existing extraFieldSource assertion that constructs otherwise valid canonical source with a secret-classified key inside values, then calls parseDeploymentConfigRelease with the matching releaseId and asserts it throws a values-key validation error. Keep the existing top-level extra-field case unchanged and reuse the established fixture shape and expected bindings.apps/infra/scripts/bootstrap-runtime-secrets.test.mjs (1)
466-466: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the expected
describe-secretcount from the definition list.The literal
11duplicatesRUNTIME_SECRET_DEFINITIONS.length, which this file already imports on line 10. Adding a runtime secret definition breaks this assertion with a count mismatch that does not name the cause.♻️ Proposed refactor
- assert.equal(aws.calls.filter(({ operation }) => operation === 'describe-secret').length, 11) + assert.equal( + aws.calls.filter(({ operation }) => operation === 'describe-secret').length, + RUNTIME_SECRET_DEFINITIONS.length, + )🤖 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 `@apps/infra/scripts/bootstrap-runtime-secrets.test.mjs` at line 466, Update the describe-secret call-count assertion to derive its expected value from the imported RUNTIME_SECRET_DEFINITIONS.length instead of the duplicated literal 11, while preserving the existing aws.calls filtering and assertion behavior.apps/infra/scripts/bootstrap-config-preflight.test.mjs (1)
34-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the fake
awsandghscripts into one helper.The same shell fixtures are written three times. The third copy has already drifted: it omits the
secretsmanager describe-secretandiam list-open-id-connect-providersbranches, and it printsaws callinstead ofaws mutation.A new branch added to one copy will not reach the others. Extract one
writeFakeCli(fixture)helper and call it from all three tests.Also applies to: 163-197, 251-277
🤖 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 `@apps/infra/scripts/bootstrap-config-preflight.test.mjs` around lines 34 - 68, Extract the duplicated fake CLI setup into a shared writeFakeCli(fixture) helper containing the complete aws and gh script behavior, including the secretsmanager, IAM, and consistent mutation-marker branches. Replace the inline script-writing blocks in all three tests with calls to this helper so every fixture stays synchronized.apps/infra/scripts/bootstrap-environment.mjs (2)
611-614: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unreachable
updatingstate.
applyRuntimeSecretActionswrites onlygeneratedorexplicittoboxlite:initial-value. Removeupdatingfrom the accepted values and remove the recovery branch at lines 634-639.🤖 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 `@apps/infra/scripts/bootstrap-environment.mjs` around lines 611 - 614, Update applyRuntimeSecretActions to accept only generated and explicit for ownership.initialValue, removing updating from the validation list and deleting the corresponding recovery branch.
1320-1338: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBound the EC2 response without filtering to baseline IDs
evaluateRunnerCommandTagGatemust inspect instances outsidebaseline.resourcesbecause it rejects unexpected authorization-tagged instances. Filteringdescribe-instancesto baseline IDs would skip this check.Reduce the response to
InstanceId,State.Name, andTags, or use another bounded approach that preserves detection of extra and case-variant authorization tags.🤖 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 `@apps/infra/scripts/bootstrap-environment.mjs` around lines 1320 - 1338, The describeEc2Instances function currently requests an unbounded EC2 response; bound the returned data while preserving evaluation of instances outside baseline.resources. Retain each instance’s InstanceId, State.Name, and Tags, including case-variant authorization tags, so evaluateRunnerCommandTagGate can still detect unexpected tagged instances.apps/infra/scripts/bootstrap-environment-file.mjs (1)
76-89: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winParse the already-read source once.
loadDotenv({ path, ... })reads the file again after the scanner reads it. Useparse(source)andpopulate(environment, parsed, { override: false }), then deriveconfiguredKeysfrom the parsed object. Keep duplicate andexportchecks explicit. Do not rely onloaded.errorfor syntax errors;dotenvreports file-loading errors there and can silently skip unsupported lines.🤖 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 `@apps/infra/scripts/bootstrap-environment-file.mjs` around lines 76 - 89, Update the bootstrap dotenv parsing flow to parse the already-read source once with dotenv’s parse API, then populate environment using the parsed result with override disabled. Derive configuredKeys from the parsed object while retaining explicit duplicate-key and export validation in the scanner, and remove reliance on loaded.error for syntax validation; preserve file-loading error handling separately if needed.apps/infra/scripts/deployment-config-activate.test.mjs (1)
81-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for a non-generation guard error.
activateDeploymentConfigrethrows unchanged anyassertGenerationserror whose message does not match the generation-mismatch pattern (deployment-config-activate.mjsLine 88). No test covers that branch. A regression that widens the pattern would replace an unrelated failure, for example an AWS permission error, with the misleading--rebase-runtime-generationsadvice.♻️ Proposed test
+test('config activation propagates an unrelated guard failure unchanged', async () => { + const guardFailure = new Error('AccessDeniedException: secretsmanager:DescribeSecret') + const { dependencies, events } = fixture({ guardFailure }) + + await assert.rejects( + activateDeploymentConfig( + ['--stage', 'dev', '--release', RELEASE_ID], + { AWS_CLI_PATH: '/synthetic/aws', AWS_REGION: 'ap-southeast-1' }, + dependencies, + ), + /AccessDeniedException/, + ) + assert.equal(events.some(([event]) => event === 'activate'), false) +})🤖 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 `@apps/infra/scripts/deployment-config-activate.test.mjs` around lines 81 - 94, Add a test alongside the existing stale-generation guard test covering a non-generation error from assertGenerations, such as an AWS permission failure. Assert that activateDeploymentConfig rejects with the original error/message rather than --rebase-runtime-generations guidance, and verify the expected lifecycle events through lock-exit.apps/infra/scripts/deployment-config-resolve.test.mjs (1)
47-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for a store that returns a non-SHA release id.
runDeploymentConfigResolverejects areleaseIdthat fails the^[0-9a-f]{64}$check atdeployment-config-resolve.mjsLine 61-63, and it must not write stdout in that case. No test covers this branch. A workflow that captures stdout depends on it.♻️ Proposed test
+test('rejects a malformed store digest before writing stdout', () => { + assert.throws( + () => + runDeploymentConfigResolve(['--stage', 'dev'], { + environment: { AWS_CLI_PATH: process.execPath, AWS_REGION: 'ap-southeast-1' }, + createStore: () => ({ resolve: () => ({ releaseId: 'latest', document: {} }) }), + output: { write() { assert.fail('an invalid digest must not reach stdout') } }, + }), + /invalid SHA-256 release id/, + ) +})🤖 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 `@apps/infra/scripts/deployment-config-resolve.test.mjs` around lines 47 - 61, Add a test in the existing runDeploymentConfigResolve validation suite that configures createStore to return a non-SHA releaseId, then asserts the call throws the SHA-256 validation error and does not write stdout. Reuse the existing options guards and provide otherwise valid resolver arguments so the releaseId validation branch is exercised.apps/infra/scripts/deployment-config-store.test.mjs (1)
430-449: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for a missing inherited lock.
This test covers a matching owner and a different owner. It does not cover an absent lock parameter, which reaches the
the inherited deployment operation lock for stage ${stage} is unavailablebranch atdeployment-config-store.mjsLine 206-208. That branch protects the nestedrunner:updatereuse described inapps/infra/README.mdLine 125-127, where a child must refuse rather than proceed without the parent lock.♻️ Proposed test
+ assert.throws( + () => { + aws.parameters.delete(OPERATION_LOCK_PARAMETER) + store.assertDeploymentOperationLockOwner({ stage: STAGE, ownerId: FIRST_LOCK_OWNER }) + }, + /unavailable/, + ) + aws.parameters.set(OPERATION_LOCK_PARAMETER, FIRST_LOCK_OWNER)🤖 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 `@apps/infra/scripts/deployment-config-store.test.mjs` around lines 430 - 449, Add a test case alongside “validates a nested operation owner without acquiring or releasing the parent lock” that removes or leaves absent the operation lock parameter, then asserts assertDeploymentOperationLockOwner rejects with the unavailable-lock message. Verify no lock acquisition or release calls occur and keep the existing matching-owner and different-owner coverage unchanged.apps/infra/scripts/deployment-config-resolve.mjs (1)
17-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOne
--stage/--releaseparser is copied into two new CLIs. Both files implement the same^--(stage|release)$and^--(stage|release)=(.*)$matching, the same single-occurrence detection, and the samerequires a valueandunknown argumentmessages. The two copies will drift, and a validation fix applied to one CLI will silently miss the other.
apps/infra/scripts/deployment-config-resolve.mjs#L17-L47: replace the inlineparseOptionsbody with a shared helper, then keep only the resolver-specific step that callsdeploymentConfigCurrentParameteranddeploymentConfigReleaseParameter.apps/infra/scripts/deployment-config-activate.mjs#L14-L47: call the same shared helper and pass--rebase-runtime-generationsas a declared boolean flag, then keep only the--release is requiredcheck.🤖 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 `@apps/infra/scripts/deployment-config-resolve.mjs` around lines 17 - 47, Extract the duplicated stage/release parsing into a shared helper and update both CLI sites: in apps/infra/scripts/deployment-config-resolve.mjs lines 17-47, replace parseOptions with the helper while retaining only the deploymentConfigCurrentParameter and deploymentConfigReleaseParameter validation; in apps/infra/scripts/deployment-config-activate.mjs lines 14-47, call the same helper with rebase-runtime-generations declared as a boolean flag and retain only the --release required check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a6a4cffe-4583-4fdd-9b53-31b04f5295d6
⛔ Files ignored due to path filters (1)
apps/infra/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (86)
.github/workflows/README.md.github/workflows/build-apps-api-image.yml.github/workflows/deploy-infra.yml.github/workflows/deploy-release.yml.github/workflows/e2e-cloud.ymlapps/api/src/config/configuration.tsapps/infra/.env.exampleapps/infra/.gitignoreapps/infra/README.mdapps/infra/ci/github-deploy-role.yamlapps/infra/package.jsonapps/infra/policies/runner/validate.cjsapps/infra/scripts/bootstrap-config-preflight.test.mjsapps/infra/scripts/bootstrap-consumer-validation.mjsapps/infra/scripts/bootstrap-consumer-validation.test.mjsapps/infra/scripts/bootstrap-environment-file.mjsapps/infra/scripts/bootstrap-environment-file.test.mjsapps/infra/scripts/bootstrap-environment.mjsapps/infra/scripts/bootstrap-runtime-secrets.test.mjsapps/infra/scripts/bootstrap-sst-log-security.test.mjsapps/infra/scripts/cloudflare-provider-credentials.test.mjsapps/infra/scripts/cloudflare-provider-registration.mjsapps/infra/scripts/deploy-environment-validation.mjsapps/infra/scripts/deploy-environment-validation.test.mjsapps/infra/scripts/deploy-role-boundary.mjsapps/infra/scripts/deploy-role-boundary.test.mjsapps/infra/scripts/deployment-config-activate.mjsapps/infra/scripts/deployment-config-activate.test.mjsapps/infra/scripts/deployment-config-capability.mjsapps/infra/scripts/deployment-config-capability.test.mjsapps/infra/scripts/deployment-config-loader.mjsapps/infra/scripts/deployment-config-loader.test.mjsapps/infra/scripts/deployment-config-resolve.mjsapps/infra/scripts/deployment-config-resolve.test.mjsapps/infra/scripts/deployment-config-store.mjsapps/infra/scripts/deployment-config-store.test.mjsapps/infra/scripts/deployment-config.mjsapps/infra/scripts/deployment-config.test.mjsapps/infra/scripts/deployment-environment.mjsapps/infra/scripts/deployment-environment.test.mjsapps/infra/scripts/deployment-preview.mjsapps/infra/scripts/deployment-preview.test.mjsapps/infra/scripts/deployment-scope.mjsapps/infra/scripts/deployment-scope.test.mjsapps/infra/scripts/environment-bootstrap.mjsapps/infra/scripts/environment-bootstrap.test.mjsapps/infra/scripts/github-environment.mjsapps/infra/scripts/release-deployment-safety.test.mjsapps/infra/scripts/runner-artifact-build.mjsapps/infra/scripts/runner-artifact-build.test.mjsapps/infra/scripts/runner-command-tag-gate.mjsapps/infra/scripts/runner-command-tag-gate.test.mjsapps/infra/scripts/runner-instance-identity.cjsapps/infra/scripts/runner-inventory.cjsapps/infra/scripts/runner-inventory.test.mjsapps/infra/scripts/runner-policy-baseline.mjsapps/infra/scripts/runner-policy-baseline.test.mjsapps/infra/scripts/runner-policy.test.mjsapps/infra/scripts/runner-state-baseline.cjsapps/infra/scripts/runner-state-baseline.test.mjsapps/infra/scripts/runner-update-binary.mjsapps/infra/scripts/runner-update-binary.test.mjsapps/infra/scripts/runtime-secret-ecs-bindings.mjsapps/infra/scripts/runtime-secret-ecs-bindings.test.mjsapps/infra/scripts/runtime-secret-generation-guard.mjsapps/infra/scripts/runtime-secret-generation-guard.test.mjsapps/infra/scripts/runtime-secret-version-stages.mjsapps/infra/scripts/runtime-secret-version-stages.test.mjsapps/infra/scripts/runtime-secrets-cli.mjsapps/infra/scripts/runtime-secrets-contract.test.mjsapps/infra/scripts/runtime-secrets.mjsapps/infra/scripts/sst-command-contract.mjsapps/infra/scripts/sst-command-contract.test.mjsapps/infra/scripts/sst-config-contract.test.mjsapps/infra/scripts/sst-event-log-security.mjsapps/infra/scripts/sst-event-log-security.test.mjsapps/infra/scripts/sst-executable.mjsapps/infra/scripts/sst-executable.test.mjsapps/infra/scripts/sst-native-environment.mjsapps/infra/scripts/sst-native-environment.test.mjsapps/infra/scripts/sst-secret-status.mjsapps/infra/scripts/sst-stage.mjsapps/infra/scripts/sst-stage.test.mjsapps/infra/scripts/sst-with-cloudflare.mjsapps/infra/scripts/verify-deploy-role-boundary.mjsapps/infra/sst.config.ts
💤 Files with no reviewable changes (5)
- apps/infra/scripts/deploy-environment-validation.test.mjs
- apps/api/src/config/configuration.ts
- apps/infra/scripts/deployment-environment.test.mjs
- apps/infra/scripts/deployment-environment.mjs
- apps/infra/scripts/deploy-environment-validation.mjs
| await deploymentConfigStore.withDeploymentOperationLock({ stage }, async () => { | ||
| // Planning observes every secret while this stage's operation lock is held, | ||
| // so no cooperating bootstrap or stack evaluation can act on the old state. | ||
| const runtimeSecretPlan = planRuntimeSecrets({ | ||
| awsCliPath, | ||
| region, | ||
| stage, | ||
| seeds: runtimeSecretSeeds, | ||
| force, | ||
| }) | ||
| const runtimeSecretGenerations = runtimeSecretGenerationsFromPlan(runtimeSecretPlan) | ||
| const runtimeSecretInitializationEnabled = runtimeSecretInitializationRequired(runtimeSecretPlan) | ||
| const { sealBeforeEnable, remaining } = partitionRuntimeSecretPlanForInitializationGate(runtimeSecretPlan) | ||
| // Validate the exact observed/preallocated generations before any provider | ||
| // mutation. Publication itself stays last, after every referenced secret | ||
| // container and the deploy role are ready. | ||
| const deploymentConfigDocument = createDeploymentConfigDocument({ | ||
| environment: process.env, | ||
| configuredKeys, | ||
| stage, | ||
| region, | ||
| accountId: identity.Account, | ||
| runtimeSecretGenerations, | ||
| }) | ||
| canonicalizeDeploymentConfig(deploymentConfigDocument) | ||
| const cloudflareCredentialPlan = await planCloudflareCredentials({ | ||
| awsCliPath, | ||
| region, | ||
| stage, | ||
| force, | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect the deployment operation lock implementation for TTL, lease renewal, and steal behavior.
set -euo pipefail
ast-grep outline apps/infra/scripts/deployment-config-store.mjs --items all
rg -n -C 8 'withDeploymentOperationLock|deployment-operation-lock|ttl|TTL|expire|lease|stale' apps/infra/scripts/deployment-config-store.mjsRepository: boxlite-ai/boxlite
Length of output: 8213
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- lock implementation ---'
sed -n '160,242p' apps/infra/scripts/deployment-config-store.mjs
printf '%s\n' '--- prompt call sites ---'
sed -n '840,900p' apps/infra/scripts/bootstrap-environment.mjs
sed -n '1060,1110p' apps/infra/scripts/bootstrap-environment.mjs
printf '%s\n' '--- locked bootstrap sequence ---'
sed -n '1518,1600p' apps/infra/scripts/bootstrap-environment.mjs
printf '%s\n' '--- serialization documentation ---'
sed -n '380,400p' apps/infra/ci/github-deploy-role.yaml
printf '%s\n' '--- lock usage and tests ---'
rg -n -C 5 'withDeploymentOperationLock|acquireDeploymentOperationLock|deployment-operation-lock|planCloudflareCredentials|ensureOidcClientId' apps/infra --glob '!node_modules'Repository: boxlite-ai/boxlite
Length of output: 50376
Move interactive prompts outside the deployment operation lock. The SSM lock has no TTL or lease renewal and remains until callback completion. An unattended prompt in planCloudflareCredentials or ensureOidcClientId can block preview, apply, and bootstrap indefinitely.
🤖 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 `@apps/infra/scripts/bootstrap-environment.mjs` around lines 1525 - 1555, Move
interactive prompt work from planCloudflareCredentials and ensureOidcClientId
outside the withDeploymentOperationLock callback, completing all user input
before acquiring the lock. Keep the locked callback limited to non-interactive
planning, validation, provider mutations, and publication, while preserving the
existing credential and OIDC results for subsequent operations.
| if (commandContract.needsProviderCredentials) { | ||
| for (const { env, param } of CREDS) { | ||
| const name = `/boxlite/${stage}/${param}` | ||
| try { | ||
| sstEnvironment[env] = fetchRequiredProviderCredential(name, { awsCliPath, region }) | ||
| } catch (error) { | ||
| console.error(`sst-with-cloudflare: ${error.message}`) | ||
| process.exit(1) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
awsCliPath is initialized conditionally but consumed unconditionally. The module-level awsCliPath is assigned only at line 243 when commandContract.needsDeploymentConfig is true, and at line 294 when sstSecretMutation is set. Later blocks read it without a guard, so an unmatched command classification passes undefined into execFileSync and produces a misleading SSM error.
apps/infra/scripts/sst-with-cloudflare.mjs#L305-L315: addawsCliPath ??= resolveAwsCliPath(sstEnvironment)before theCREDSloop, and report a resolve failure with its own message.apps/infra/scripts/sst-with-cloudflare.mjs#L414-L427: reuse the same initializedawsCliPathforresolveAwsAccountIdandverifyRunnerArtifactinstead of relying on the deploy path having already set it.
📍 Affects 1 file
apps/infra/scripts/sst-with-cloudflare.mjs#L305-L315(this comment)apps/infra/scripts/sst-with-cloudflare.mjs#L414-L427
🤖 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 `@apps/infra/scripts/sst-with-cloudflare.mjs` around lines 305 - 315, Ensure
awsCliPath is initialized before credential loading by resolving it with
awsCliPath ??= resolveAwsCliPath(sstEnvironment) before the CREDS loop, and
catch/report resolution failures with a dedicated error message. In
apps/infra/scripts/sst-with-cloudflare.mjs lines 305-315, apply this
initialization before fetchRequiredProviderCredential; in lines 414-427, reuse
the same initialized awsCliPath for resolveAwsAccountId and verifyRunnerArtifact
rather than assuming the deployment path initialized it.
reconcileDefaultRunner and restoreDefaultRunnerLegacy validated only the shape of BOXLITE_RUNNER_TOKEN_SECRET_ARN and GHCR_SECRET_ARN, so a reconcile could install another stage's credential into a runner's systemd drop-in. One shared validator now binds all four runner transactions to the stage. Also: - reject non-aws partitions where the identity enters, since sst.config.ts only ever creates arn:aws boundary ARNs - require an explicit stage before asserting the Runner stage tag, which two undefineds previously satisfied - keep an operation's failure as the thrown error when releasing the SSM deployment lock also fails - fail activation when the release does not become current, so a lost pointer race cannot report success - resolve symlinks in the entry-point guards of the activate script and the preview gate, the last two carrying the unresolved form - point lock recovery at the README instead of a partial owner list - treat an omitted Secrets Manager Tags field as an untagged secret, and name the retag performed instead of reporting it as retained - chmod 0700 the two migration work directories that omitted it - share one region rule between the SST secret status writer and reader - reuse one resolved awsCliPath across the deploy preflight - drop an unused import, three unused test imports, and an argument the four runner transactions never read - give the rejected-argv loop its own infraRoot, so only argv validation order keeps it from deleting the working tree's .sst/log/sst.log - make five assertions load-bearing: guard the ordering terms that pass when absent, share one deploy-role placeholder resolver so the 10,240-byte check stops under-measuring the boundary ARN, emit the sentinel the leak check looks for, pin rotationPolicy in the registry contract, and pin the work-directory mode
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/infra/scripts/deployment-config-store.mjs (1)
238-259: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPreserve every operation failure outside
finally.Biome reports an error for the
throwon Line 253. This blocks linting.A callback can throw
undefined,null,0, or an empty string. In that case,!operationErroris true and the release error replaces the operation failure.Track failure with a separate boolean. Release the lock after the operation block. Then rethrow the original value after cleanup.
Proposed fix
const lock = this.acquireDeploymentOperationLock(options) + let result let operationError + let operationFailed = false try { - return await operation() + result = await operation() } catch (error) { + operationFailed = true operationError = error - throw error - } finally { - try { - this.releaseDeploymentOperationLock(lock) - } catch (releaseError) { - if (!operationError) throw releaseError - if (operationError instanceof Error && Object.isExtensible(operationError)) { - operationError.cause ??= releaseError - } + } + try { + this.releaseDeploymentOperationLock(lock) + } catch (releaseError) { + if (!operationFailed) throw releaseError + if (operationError instanceof Error && Object.isExtensible(operationError)) { + operationError.cause ??= releaseError } } + if (operationFailed) throw operationError + return resultAdd a regression test where
operation()throwsundefinedand lock release fails.🤖 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 `@apps/infra/scripts/deployment-config-store.mjs` around lines 238 - 259, Update the operation cleanup flow around the operationError handling to track whether operation() failed with a separate boolean, so falsy thrown values are preserved. Release the lock after the operation block, then rethrow the original value after cleanup rather than throwing from finally; retain release-error attachment only for an actual Error and add a regression test covering operation() throwing undefined while releaseDeploymentOperationLock fails.Source: Linters/SAST tools
🤖 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.
Duplicate comments:
In `@apps/infra/scripts/deployment-config-store.mjs`:
- Around line 238-259: Update the operation cleanup flow around the
operationError handling to track whether operation() failed with a separate
boolean, so falsy thrown values are preserved. Release the lock after the
operation block, then rethrow the original value after cleanup rather than
throwing from finally; retain release-error attachment only for an actual Error
and add a regression test covering operation() throwing undefined while
releaseDeploymentOperationLock fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0347148f-0294-43f2-ae8d-877adf092103
📒 Files selected for processing (19)
apps/infra/policies/runner/validate.cjsapps/infra/scripts/bootstrap-environment-file.test.mjsapps/infra/scripts/bootstrap-environment.mjsapps/infra/scripts/bootstrap-runtime-secrets.test.mjsapps/infra/scripts/bootstrap-sst-log-security.test.mjsapps/infra/scripts/deploy-role-boundary.mjsapps/infra/scripts/deploy-role-boundary.test.mjsapps/infra/scripts/deployment-config-activate.mjsapps/infra/scripts/deployment-config-activate.test.mjsapps/infra/scripts/deployment-config-store.mjsapps/infra/scripts/deployment-config-store.test.mjsapps/infra/scripts/deployment-preview.mjsapps/infra/scripts/release-deployment-safety.test.mjsapps/infra/scripts/runner-policy.test.mjsapps/infra/scripts/runtime-secret-generation-guard.test.mjsapps/infra/scripts/runtime-secrets-cli.mjsapps/infra/scripts/runtime-secrets-contract.test.mjsapps/infra/scripts/sst-with-cloudflare.mjsapps/infra/scripts/verify-deploy-role-boundary.mjs
💤 Files with no reviewable changes (1)
- apps/infra/scripts/verify-deploy-role-boundary.mjs
🚧 Files skipped from review as they are similar to previous changes (11)
- apps/infra/scripts/deployment-preview.mjs
- apps/infra/scripts/runtime-secret-generation-guard.test.mjs
- apps/infra/scripts/deploy-role-boundary.test.mjs
- apps/infra/policies/runner/validate.cjs
- apps/infra/scripts/runner-policy.test.mjs
- apps/infra/scripts/bootstrap-sst-log-security.test.mjs
- apps/infra/scripts/sst-with-cloudflare.mjs
- apps/infra/scripts/bootstrap-runtime-secrets.test.mjs
- apps/infra/scripts/runtime-secrets-cli.mjs
- apps/infra/scripts/bootstrap-environment.mjs
- apps/infra/scripts/release-deployment-safety.test.mjs
1 participant
What changed
apps/infra/.enva bootstrap-only operator input.DEPLOY_ENVmaterialization and routine dotenv loading.Why
DEPLOY_ENVcombined deployment settings and credentials in one mutable opaque GitHub secret. Every deployment recreated a credential-bearing.envfile, while preview and apply had no immutable configuration identity.The new model separates immutable non-secret configuration from mutable runtime secrets and gives every deployment an exact, auditable release digest.
Before
After
Impact
Routine deployments no longer require or create
apps/infra/.env. Intentional stage-configuration changes run bootstrap to publish a new immutable release. Blankconfig_releaseresolves/boxlite/<stage>/deploy-config/current; an explicit digest gives deterministic retry/rollback.The first migration is deliberately two-pass: bootstrap, expand apply, bootstrap again to seal generated secret versions and the Runner command tag gate, then preview/apply the finalized release.
Validation
make test:apps:infra— 503/503make test:apps:infra-configgit diff --check/boxlite/dev/deploy-config/currenttob34ae047f8df023b155bd6c5c3eb63b4eed486e6f9d27a3f822a13c4ee90ebfeCutover status
No stack apply has run. Before the first expand apply, rotate the audit-exposed Auth0 management client secret and Svix API token, update their SST secrets, and verify both integrations.
Keep the live
DEPLOY_ENVsecret until the compatible workflow is merged and the two-pass dev cutover is complete.Summary by CodeRabbit
New Features
Bug Fixes
Documentation