Skip to content

fix(cli): validate skill listing prices#458

Open
sevencat2004 wants to merge 1 commit into
profullstack:masterfrom
sevencat2004:fix/skills-price-validation
Open

fix(cli): validate skill listing prices#458
sevencat2004 wants to merge 1 commit into
profullstack:masterfrom
sevencat2004:fix/skills-price-validation

Conversation

@sevencat2004
Copy link
Copy Markdown

Fixes #457

Summary

  • validate sh1pt skills new --price before writing manifests
  • reject negative, fractional, and non-numeric values by falling back to 0
  • reuse the sanitized price for generated marketplace publish commands
  • add CLI tests for negative and fractional prices

Validation

  • corepack pnpm test -- packages/cli/src/commands/skills.test.ts
  • node_modules.bin\tsc.CMD -p packages/cli/tsconfig.json --noEmit

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 28, 2026

Greptile Summary

This PR adds a parsePrice helper to the skills new CLI command, replacing the old parseInt(opts.price, 10) || 0 expression (which silently accepted negative values, since -5 || 0 evaluates to -5). The new validator rejects any non-digit-only string via /^\d+$/ and additionally guards against unsafe integer overflow via Number.isSafeInteger.

  • Core fix (skills.ts): parsePrice is called once and its result is reused in both the top-level price field and the marketplace command strings, eliminating the previous duplication where each site re-ran parseInt independently.
  • Tests (skills.test.ts): Three new integration tests in skills new command exercise the rejection of negative prices (-5→0), acceptance of a valid integer (100→100), and rejection of fractional prices (1.9→0), asserting on both the serialized manifest and the generated ugig command string.

Confidence Score: 5/5

The price-validation logic is correct and the tests cover the key edge cases; no new defects are introduced by this change.

The parsePrice implementation is sound: the /^\d+$/ regex correctly excludes the sign character, ruling out negatives, and also excludes the decimal point, ruling out fractions. Number.isSafeInteger handles the pathological large-integer case. The marketplace command generation now consistently uses the sanitized value. The new tests confirm the advertised behaviour end-to-end by reading the written manifest rather than mocking internals.

No files require special attention for the price-validation change itself. Previously flagged Unicode regressions in skills.ts remain unresolved in this revision.

Important Files Changed

Filename Overview
packages/cli/src/commands/skills.ts Adds parsePrice helper that gates on /^\d+$/ and Number.isSafeInteger; replaces old `parseInt
packages/cli/src/commands/skills.test.ts Adds a skills new command describe block with tests for negative (-5 to 0), valid positive (100 to 100), and fractional (1.9 to 0) prices. Tests verify both manifest.price and the generated ugig command string.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["parsePrice(value: string)"] --> B{"/^\\d+$/.test(value)?"}
    B -- No --> C["return 0\n(rejects: negatives, decimals,\n non-numeric strings)"]
    B -- Yes --> D["Number.parseInt(value, 10)"]
    D --> E{"Number.isSafeInteger(parsed)?"}
    E -- No --> F["return 0\n(rejects: very large integers)"]
    E -- Yes --> G["return parsed"]
    G --> H["price used in manifest\n& marketplace commands"]
Loading

Reviews (3): Last reviewed commit: "fix(cli): validate skill listing prices" | Re-trigger Greptile

Comment thread packages/cli/src/commands/skills.ts
Comment thread packages/cli/src/commands/skills.test.ts Outdated
Comment thread packages/cli/src/commands/skills.ts
@sevencat2004 sevencat2004 force-pushed the fix/skills-price-validation branch from eb33100 to 894a1e7 Compare May 28, 2026 16:50
@sevencat2004 sevencat2004 force-pushed the fix/skills-price-validation branch from 894a1e7 to f8dfc23 Compare May 28, 2026 16:59
@sevencat2004
Copy link
Copy Markdown
Author

Submitted this PR for the ugig bounty/gig: "Need someone to test submit bugs and PRs to fix those bugs".

The PR is ready for review:

Solana wallet for bounty payout:
Dy4yMkjCfupxaURt6iTMUrxqSDEmAJPPkKF66QahxJZD

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

skills new accepts invalid listing prices

1 participant