Validate Windows distribution mode#599
Conversation
Greptile SummaryThis PR adds runtime validation of the
Confidence Score: 4/5Safe to merge; changes are small and well-scoped with no impact on valid-distribution code paths. The implementation is correct — validation fires before any side effects in both Both changed files are straightforward; the test file is worth a second look to confirm the ship coverage intent. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[build or ship called] --> B[requireDistribution]
B --> C{Valid distribution?}
C -- No --> D[throw Error: must be msstore, msi, or both]
C -- Yes --> E[return validated distribution]
E --> F{dryRun?}
F -- Yes in build --> G[write package-plan JSON and return artifact]
F -- Yes in ship --> H[return id dry-run]
F -- No in build --> I[select ext from plan.distribution and return artifact]
F -- No in ship --> J[build Store URL from validated distribution and return]
Reviews (1): Last reviewed commit: "Validate Windows distribution mode" | Re-trigger Greptile |
| }, | ||
| async ship(ctx, config) { | ||
| const distribution = requireDistribution(config); | ||
| ctx.log(`publish ${config.appId}@${ctx.version} · distribution=${config.distribution}`); |
There was a problem hiding this comment.
The log line still reads from
config.distribution (the raw, pre-narrowed value) rather than the already-validated distribution local. While the two values are always equal after requireDistribution returns, using the unvalidated field here is inconsistent with how the rest of ship (and the entire build path) uses the narrowed local. If the log is ever moved above the requireDistribution call during a future refactor, an invalid distribution would be silently printed before the error is thrown.
| ctx.log(`publish ${config.appId}@${ctx.version} · distribution=${config.distribution}`); | |
| ctx.log(`publish ${config.appId}@${ctx.version} · distribution=${distribution}`); |
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!
| it('rejects unsupported distributions while shipping', async () => { | ||
| await expect(adapter.ship(fakeShipContext({ | ||
| version: '1.0.0', | ||
| dryRun: true, | ||
| }) as any, { | ||
| appId: 'Acme.Tool', | ||
| publisherId: 'CN=publisher', | ||
| distribution: 'preview', | ||
| } as any)).rejects.toThrow('desktop-win distribution must be one of: msstore, msi, both'); | ||
| }); |
There was a problem hiding this comment.
Ship-rejection test only exercises the dry-run path
The new ship rejection test passes dryRun: true, which causes the production code to hit requireDistribution → throw before reaching the dryRun early return. That's correct and the test does verify the right thing. However, because the test never reaches the non-dry-run branch (the URL-construction logic), a regression that accidentally placed requireDistribution after the dryRun guard would not be caught — the test would still pass (dryRun short-circuits and never reaches the moved call). Consider adding a non-dryRun assertion or at least a comment explaining that validation intentionally fires before the dry-run check.
Fixes #598.
Changes:
Validation: