Validate skill listing prices strictly

[ayskobtw-lil]

Fixes #457.

Summary

• replace Number.parseInt coercion for skills new --price with strict digit-only safe-integer validation
• reject negative, fractional, signed, hex-like, exponent-like, suffixed, and unsafe-integer values before any manifest is written
• reuse parsed price and tags for both the manifest and marketplace command generation

Root Cause

Number.parseInt accepts partially numeric strings such as 5abc, 1e2, 0x10, and +5, so the previous guard still persisted invalid listing prices.

Verification

• npx --yes pnpm@9.12.0 exec vitest run packages/cli/src/commands/skills.test.ts --testNamePattern "skills new command"
• npx --yes pnpm@9.12.0 exec vitest run packages/cli/src/commands/skills.test.ts
• npx --yes pnpm@9.12.0 --filter @profullstack/sh1pt typecheck

[greptile-apps]

Greptile Summary

Replaces the permissive Number.parseInt price guard in skills new with a strict digit-only validator (parsePriceSats) that rejects signed, fractional, hex-like, exponent-like, suffixed, and unsafe-integer strings. The PR also deduplicates validateSnapName, validateAppId, and validatePackageId across the three packaging targets, tightens their regex rules, and adds comprehensive test coverage for all rejection paths.

• parsePriceSats applies /^\d+$/ then Number.isSafeInteger and also reuses the parsed price and tags variables so the marketplace command generation no longer re-evaluates those expressions inline.
• pkg-snap extends validateSnapName with a consecutive-hyphen check and moves the guard into renderSnapcraftYaml; the old single-regex duplicate at the bottom of the file is removed.
• pkg-flatpak / pkg-winget both remove their stale duplicate validator definitions while keeping the improved versions that were already added near the top of each file.

Confidence Score: 5/5

Safe to merge; the changes tighten input validation without removing any existing behaviour, and all rejection paths are covered by new tests.

All changed paths are additive validation constraints. The logic in parsePriceSats is correct — the regex excludes every invalid format documented in the PR description, and Number.isSafeInteger catches overflow. The packaging-target validators correctly remove duplicate function definitions that would have been TypeScript compile errors. No runtime regressions were found.

packages/targets/pkg-snap/src/index.ts has a minor redundancy: validateSnapName is invoked twice during build() (once directly, once through renderSnapcraftYaml), but this does not affect correctness.

Important Files Changed

Filename	Overview
packages/cli/src/commands/skills.ts	Replaces parseInt coercion with strict digit-only parsePriceSats helper; reuses parsed price and tags to avoid expression duplication in the manifest builder.
packages/cli/src/commands/skills.test.ts	Adds a new 'skills new command' describe block covering valid price, all invalid price variants (negative, fractional, hex, exponent, suffixed, unsafe-integer), and confirms no manifest is written on rejection.
packages/targets/pkg-snap/src/index.ts	Adds consecutive-hyphen check to validateSnapName, moves the call into renderSnapcraftYaml, and removes the old duplicate definition at the bottom; build() now calls validateSnapName twice (explicitly then again via renderSnapcraftYaml).
packages/targets/pkg-flatpak/src/index.ts	Tightens segment regex to require a leading letter and structured hyphen groups; removes the stale duplicate validateAppId from below renderFlatpakManifest.
packages/targets/pkg-winget/src/index.ts	Removes the old duplicate validatePackageId from the bottom; the new canonical version now lives at the top with a leading-dot guard replacing the explicit path-traversal check (still covered by the per-segment regex).

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["--price raw"] --> B["parsePriceSats(raw)"]
    B --> C["value = raw.trim()"]
    C --> D{"/^\\d+$/ test"}
    D -- fail --> E["throw: not a non-negative integer"]
    D -- pass --> F["price = Number(value)"]
    F --> G{"Number.isSafeInteger(price)?"}
    G -- false --> H["throw: exceeds safe integer range"]
    G -- true --> I["return price"]
    I --> J["write manifest + marketplace command"]
    E --> K["console.error + process.exit(1)"]
    H --> K

Loading

_{Reviews (2): Last reviewed commit: "Address snap validation review" | Re-trigger Greptile}

[greptile-apps]

trim() silently accepts whitespace-padded input (e.g., --price " 5 " normalises to 5 without any warning). More subtly, it also means leading-zero strings like "007" pass the regex unchanged and resolve to 7. If the intent is strict validation of what the user typed, both cases should be rejected rather than coerced. Removing trim() and testing raw directly would make the function's rejection contract completely unambiguous.

Suggested change

	const value = raw.trim();
	if (!/^\d+$/.test(value)) {
	if (!/^\d+$/.test(raw)) {

[greptile-apps]

If trim() is removed (so raw is tested directly), the Number(value) conversion should also use raw to stay consistent — keeping value alive solely to pass into Number() while the regex already ran on raw would be confusing. The suggestion below aligns both checks on the same variable.

Suggested change

	const price = Number(value);
	const price = Number(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!

[ayskobtw-lil]

Update: I folded in the current package-validator cleanup that was already fixing the full-suite CI break on master, so this PR can be tested independently while the package-target PR is still open.\n\nThe original skills fix is unchanged: listing prices now reject non-integer, negative, partially parsed, and unsafe integer values before writing a manifest.\n\nAdditional CI-unblock scope:\n- remove duplicate pkg-flatpak/pkg-winget validators that caused import/build failures\n- keep the snap validator review fixes, including no filesystem side effects on invalid snap names\n\nVerification:\n- npx --yes pnpm@9.12.0 exec vitest run packages/cli/src/commands/skills.test.ts packages/targets/pkg-flatpak/src/index.test.ts packages/targets/pkg-winget/src/index.test.ts packages/targets/pkg-snap/src/index.test.ts -> 39 passed\n- npx --yes pnpm@9.12.0 --filter @profullstack/sh1pt --filter @profullstack/sh1pt-target-pkg-flatpak --filter @profullstack/sh1pt-target-pkg-winget --filter @profullstack/sh1pt-target-pkg-snap typecheck -> passed

[github-actions]

🤖 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: git fetch upstream master && git rebase upstream/master.

[github-actions]

🤖 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: git fetch upstream master && git rebase upstream/master.

[github-actions]

🤖 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: git fetch upstream master && git rebase upstream/master.

[github-actions]

🤖 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: git fetch upstream master && git rebase upstream/master.