fix: validate skills new --price as non-negative integer (#457)#468
fix: validate skills new --price as non-negative integer (#457)#468nikos1005 wants to merge 1 commit into
Conversation
…k#457) Reject negative values and silently-truncated floats instead of persisting them to sh1pt.skill.json.
Greptile SummaryThis PR fixes
Confidence Score: 4/5Safe to merge — the fix correctly prevents negative and fractional prices from being written to the manifest, and the default value of '0' passes validation without issue. The validation logic is sound: the /^\d+$/ regex reliably rejects negatives, decimals, and non-numeric input, and parseInt on a digits-only string is always safe. The only remaining rough edges are a redundant phrase in the error message and calling the validator twice rather than capturing the result in a variable — neither affects correctness. Only packages/cli/src/commands/skills.ts changed; the double invocation of validatePrice on lines 367 and 370 is worth a second look. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["skills new --price value"] --> B["opts.price as string"]
B --> C["validatePrice(raw)"]
C --> D{"trimmed matches digit-only regex?"}
D -- No --> E["throw Error: Invalid --price"]
D -- Yes --> F["parseInt(trimmed, 10)"]
F --> G["manifest.price = number"]
F --> H["marketplaces builder price = number"]
G --> I["saveManifest to sh1pt.skill.json"]
H --> I
Reviews (1): Last reviewed commit: "fix: validate skills new --price as non-..." | Re-trigger Greptile |
| function validatePrice(raw: string): number { | ||
| const trimmed = raw.trim(); | ||
| if (!/^\d+$/.test(trimmed)) { | ||
| throw new Error(`Invalid --price "${raw}": must be a non-negative integer (sats). Got "${raw}".`); |
There was a problem hiding this comment.
Redundant value echo in error message
raw appears twice in the same message — once as "${raw}" after Invalid --price and again as Got "${raw}". The second occurrence adds no new information. If the intent is to distinguish the original input from the trimmed value, the second slot should reference trimmed; otherwise, drop the trailing Got "${raw}"..
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!
| price: validatePrice(opts.price), | ||
| skillFile, | ||
| sourceUrl: opts.sourceUrl, | ||
| marketplaces: Object.fromEntries(MARKETPLACES.map(mp => [mp.id, { enabled: true, status: 'pending', command: 'command' in mp && mp.command ? mp.command({ name, title, description, tagline: opts.tagline, category: opts.category, tags: opts.tags.split(',').map(t => t.trim()).filter(Boolean).slice(0, 10), price: Number.parseInt(opts.price, 10) || 0, skillFile, sourceUrl: opts.sourceUrl, marketplaces: {} }) : undefined, note: 'note' in mp ? mp.note : undefined }])) as SkillManifest['marketplaces'], | ||
| marketplaces: Object.fromEntries(MARKETPLACES.map(mp => [mp.id, { enabled: true, status: 'pending', command: 'command' in mp && mp.command ? mp.command({ name, title, description, tagline: opts.tagline, category: opts.category, tags: opts.tags.split(',').map(t => t.trim()).filter(Boolean).slice(0, 10), price: validatePrice(opts.price), skillFile, sourceUrl: opts.sourceUrl, marketplaces: {} }) : undefined, note: 'note' in mp ? mp.note : undefined }])) as SkillManifest['marketplaces'], |
There was a problem hiding this comment.
validatePrice called twice for the same input
validatePrice(opts.price) is invoked once for manifest.price (line 367) and again inside the marketplaces builder (line 370). While both calls return the same value and neither has side effects, calling a throwing validator twice is unexpected. Extracting the result into a const price = validatePrice(opts.price) before the manifest literal would make the intent clearer and avoid the double call.
Summary
Fixes
sh1pt skills new --priceaccepting invalid values.Problem
--price -5) were silently persisted tosh1pt.skill.json--price 1.9) were silently truncated byNumber.parseInt()Fix
Added
validatePrice()function that uses/^\\d+$/regex to ensure the price is a non-negative integer string before parsing.Fixes #457