Validate Lambda deploy config - #645
Conversation
Greptile SummaryThis PR adds input validation for optional Lambda deploy config fields (
Confidence Score: 3/5The core validation logic is correct and well-structured, but a regex bug in the environment variable validator will reject valid single-character key names, which is a functional defect that could surface in real deployments. The env var key regex ( Pay close attention to the Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[build / ship called] --> B[normalizedConfig]
B --> C{Validation}
C -->|functionName blank| ERR1[throw: functionName is required]
C -->|text field blank| ERR2[throw: deploy-lambda requires field]
C -->|memorySize out of range| ERR3[throw: must be integer 128-10240]
C -->|timeout out of range| ERR4[throw: must be integer 1-900]
C -->|env key fails regex| ERR5[throw: must start with a letter]
C -->|layer ARN blank| ERR6[throw: deploy-lambda requires layers idx]
C -->|all valid| D[normalized Config]
D --> E[renderPlan / updateArgs / createArgs]
E --> F[region helper]
F -->|config.region set| G[use config.region]
F -->|config.region unset| H{ctx.secret AWS_REGION}
H -->|defined and non-blank| I[use secret value]
H -->|blank string| ERR7[throw: deploy-lambda requires AWS_REGION]
H -->|undefined| J[fallback us-east-1]
Reviews (1): Last reviewed commit: "Validate Lambda deploy config" | Re-trigger Greptile |
| if (value === undefined) return undefined; | ||
| const entries = Object.entries(value); | ||
| for (const [key, entryValue] of entries) { | ||
| if (!/^[A-Za-z][A-Za-z0-9_]+$/.test(key)) { |
There was a problem hiding this comment.
The environment variable key regex uses
+ (one-or-more) for the character class after the first letter, which means single-character names like X, A, or Z are always rejected even though they are fully valid POSIX and AWS Lambda environment variable names. Any caller passing { X: '1' } or a similar single-letter key gets an error that doesn't match the stated rule.
| if (!/^[A-Za-z][A-Za-z0-9_]+$/.test(key)) { | |
| if (!/^[A-Za-z][A-Za-z0-9_]*$/.test(key)) { |
| function region(ctx: { secret(key: string): string | undefined }, config: Config): string { | ||
| return config.region ?? ctx.secret('AWS_REGION') ?? 'us-east-1'; | ||
| return optionalText(config.region, 'region') ?? optionalText(ctx.secret('AWS_REGION'), 'AWS_REGION') ?? 'us-east-1'; | ||
| } |
There was a problem hiding this comment.
optionalText wrapping ctx.secret('AWS_REGION') silently changes error behaviour: if the vault returns a blank string (""), the call throws "deploy-lambda requires AWS_REGION" instead of falling back to 'us-east-1'. An environment where the secret was stored as an empty string would now hard-error at every build / ship call rather than using the default region.
| function applyOptionalCreateArgs(args: string[], config: Config): string[] { | ||
| config = normalizedConfig(config); |
There was a problem hiding this comment.
normalizedConfig is now called multiple times on the same execution path. All validators are idempotent so this causes no incorrect behaviour today, but each call redundantly re-validates every field. If a heavier side-effect is ever added to a validator, this chain would multiply its impact unexpectedly.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
12 similar comments
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
Fixes #644.
Changes:
Validation: