fix: restore "empty version means latest" behavior broken by PR #44 hardening

[j4ys0n]

Problem

After merging #44 and #45, installs and upgrades through the API started failing. Reproduced by booting the API, installing @missionsquad/mcp-helper-tools, and exercising the package endpoints. The root cause is in #44's injection hardening (#45 checked out clean — its SSRF validation only affects oauth2 servers with a registrationEndpoint and behaves as intended):

1. Empty version now rejected. The API documents version as optional, defaults to latest, and pre-NPM Package Version Command Injection Vulnerability in MissionSquad/mcp-api #44 code used falsy checks (version ? \${name}@${version}` : name), so version: ""/version: nullinstalled latest. #44's guard isversion !== undefined, so those requests now fail with Invalid package version: . Version must be a valid npm semver, range, or tag.onPOST /packages/installandPUT /packages/:name/upgrade` — breaking any client that sends an empty version field.

2. npm name allowlist applied to Python packages. In upgradePackage and checkForUpdates, the persisted-name re-validation ran before the python-runtime branch. PyPI names may legally contain uppercase letters (the python install path accepts /^[a-zA-Z0-9][a-zA-Z0-9_.-]*$/), but the npm allowlist is lowercase-only — so such Python packages could never be upgraded or update-checked again.

Fix

• installPackage/upgradePackage normalize the version input before validation: null, "", and whitespace-only mean "latest", and surrounding whitespace is trimmed. Genuinely invalid specs are still rejected by the same allowlist.
• The persisted-name re-validation in upgradePackage/checkForUpdates now uses the allowlist matching the package's runtime (python regex for python records, npm regex otherwise).
• Bumped version to 1.11.11.

The security posture of #44 is unchanged: all npm invocations still go through the execFile-based runNpm (no shell), and malicious names/versions ("1.0.0; touch /tmp/pwn", --registry=..., etc.) are still rejected — re-verified live and covered by the existing tests.

Verification

• Booted the API and confirmed: install with ""/null version succeeds and installs latest; upgrade with "" succeeds (helper-tools → 1.1.1); mcp/tool/call against the installed server works; malicious version specs still return the validation error.
• Added 10 regression tests (empty/null/whitespace versions on install + upgrade, version trimming, uppercase python names in upgrade/check-updates). Full suite: 144 tests passing across 6 suites.

🤖 Generated with Claude Code

https://claude.ai/code/session_015MvwXBH8tFAebZWZExXVm3

---

Generated by Claude Code

[j4ys0n]

Automated review 🤖

Summary of Changes

This PR fixes two regressions introduced by PR #44's injection-hardening in src/services/packages.ts: (1) empty/null/whitespace version inputs are now normalized to undefined (meaning "latest") before allowlist validation, restoring the documented optional-version behavior; (2) persisted Python package names are now re-validated against a Python-specific regex (PYTHON_NAME_RE) rather than the npm allowlist, fixing upgrade and update-check failures for PyPI packages with uppercase letters. Ten regression tests are added to test/packages-injection.spec.ts. Version bumped to 1.11.11.

Key Changes & Positives

• 🟢 normalizeVersionInput (packages.ts, new static method) cleanly centralizes the falsy-version normalization in one place, called at both installPackage and upgradePackage entry points, preventing the same regression from recurring if new entry points are added.
• 🟢 PYTHON_NAME_RE and isValidPythonPackageName are extracted as named, documented constants/methods, making the runtime-dispatch logic in checkForUpdates and upgradePackage readable and auditable.
• 🟢 The packageRuntime variable in upgradePackage is hoisted and reused (eliminating the redundant const runtime = packageInfo.runtime ?? 'node' that existed just before the python branch), a minor but clean DRY improvement.
• 🟢 Test coverage directly exercises the regression scenarios: test.each over ['', ' ', null, undefined] for install, an explicit empty-string upgrade test, and two Python uppercase-name tests (MarkupSafe) for both checkForUpdates and upgradePackage.

Potential Issues & Recommendations

1. Issue / Risk: normalizeVersionInput accepts unknown and falls back to String(version) for non-string, non-null, non-undefined inputs (e.g., a number 123 becomes "123"). The TypeScript call sites pass string | undefined today, but the broad signature could silently coerce unexpected types at runtime if the request shape ever widens.

   • Impact: A numeric version like 123 would pass through as "123" and then be validated by NPM_VERSION_SPEC_RE; likely harmless today but the unknown input type is broader than necessary.
   • Recommendation: Narrow the parameter type to string | null | undefined to match actual API contract and remove the String(version) fallback, or add an explicit type guard and return an error for non-string non-null inputs.
   • Status: 🟡 Needs review

2. Issue / Risk: In upgradePackage (packages.ts, hunk at line ~950), version is reassigned via version = PackageService.normalizeVersionInput(version) on a parameter that was declared as string | undefined in the function signature. This mutates the parameter in place rather than using a new const.

   • Impact: Minor style/readability issue; TypeScript will accept it since string | undefined is assignable, but it obscures the fact that the original caller value is being transformed.
   • Recommendation: Declare const normalizedVersion = PackageService.normalizeVersionInput(version) and use normalizedVersion throughout the function body for clarity.
   • Status: 🟡 Needs review

3. Issue / Risk: The checkForUpdates Python test (packages-injection.spec.ts, "checkForUpdates still checks python packages with uppercase names") mocks execFilePromisifiedMock globally for the test, matching on cmdArgs[0] === 'index' && cmdArgs[1] === 'versions'. This is a pip-index-versions invocation, but the mock returns a custom stdout format ('Available versions: 3.0.0, 2.0.0\n') — if the actual pip output parsing logic changes, this test will silently pass while the real behavior breaks.

   • Impact: The test validates the name-allowlist fix but does not guard against pip output format regressions.
   • Recommendation: Confirm the mock stdout matches the exact format the production parser expects, or add a comment noting the expected pip output format for future maintainers.
   • Status: 🟡 Needs review

Approval Recommendation

Approve with caveats

• Consider narrowing normalizeVersionInput's parameter type from unknown to string | null | undefined to match the actual API contract.
• Consider using a new const normalizedVersion in upgradePackage instead of reassigning the version parameter for clarity.
• Verify the pip stdout mock format in the checkForUpdates uppercase-Python test matches what the production parser actually consumes.

[Copilot]

Pull request overview

Restores the pre-#44 API behavior where an empty/null/whitespace version means “latest”, and fixes runtime-specific persisted-name re-validation so Python packages (including those with uppercase names) can be upgraded and update-checked again without weakening the command-injection hardening.

Changes:

• Normalize version inputs (null/empty/whitespace → undefined, trim surrounding whitespace) before npm version allowlist validation.
• Re-validate persisted package names using the correct allowlist per runtime (npm vs PyPI) in upgradePackage and checkForUpdates.
• Add regression tests and bump package version to 1.11.11.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
src/services/packages.ts	Normalizes version before validation and applies runtime-appropriate name allowlists for upgrade/update-check paths.
test/packages-injection.spec.ts	Adds regression tests for empty/null/whitespace versions and uppercase Python package names through upgrade/update-check paths.
package.json	Bumps the published version to 1.11.11.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[j4ys0n]

Automated review 🤖

Summary of Changes

This PR fixes two regressions introduced by PR #44's injection-hardening in src/services/packages.ts: (1) empty/null/whitespace version inputs being incorrectly rejected instead of treated as "install latest", and (2) Python packages with uppercase names being silently skipped during upgrade and update-check because their persisted names failed the npm-only name allowlist. The fix adds a normalizeVersionInput helper, a isVersionInputTypeValid type guard, a PYTHON_NAME_RE regex, and runtime-aware name validation branching. Ten regression tests are added to test/packages-injection.spec.ts.

Key Changes & Positives

• normalizeVersionInput / isVersionInputTypeValid helpers (packages.ts, new static methods ~L128–L148): Clean separation between type rejection (non-string types like true, 123) and semantic normalization ("", null, whitespace → undefined). Prevents both coercion attacks and false rejections without conflating the two concerns. 🟢
• Runtime-aware name re-validation (packages.ts, checkForUpdates ~L866 and upgradePackage ~L991): The pkg.runtime === 'python' branch now routes to PYTHON_NAME_RE instead of isValidNpmName, correctly preserving defense-in-depth for both runtimes without over-blocking legitimate PyPI names. 🟢
• normalizedVersion threaded through upgradePackage (packages.ts ~L1028, L1069): Both the pip spec (name==version) and the npm spec (name@latest) now use the normalized value, so the version ?? 'latest' fallback is consistent with the install path. 🟢
• Regression test coverage (packages-injection.spec.ts): test.each over ['', ' ', null, undefined] for install, dedicated upgrade empty-string test, non-string type rejection tests, whitespace-trim test, and two Python uppercase-name tests covering both checkForUpdates and upgradePackage. Directly mirrors the described failure modes.

Potential Issues & Recommendations

1. Issue / Risk: isVersionInputTypeValid is typed version is string | null | undefined but the TypeScript signature of upgradePackage already declares version?: string, meaning the runtime guard is only meaningful if callers bypass TypeScript (e.g., raw req.body.version from the controller). The comment in the hunk at ~L964 acknowledges this ("the controller passes req.body.version through without validating its type"), but the controller itself is not patched in this PR.

   • Impact: If the controller is later refactored or a new route is added, the type annotation on upgradePackage may give false confidence that the guard is redundant and it could be removed, re-opening the regression.
   • Recommendation: Either add a brief // intentional runtime guard — controller does not validate body types comment at the call site in the controller, or add a controller-level type check so the service guard becomes a true defense-in-depth layer rather than the primary line of defense.
   • Status: 🟡 Needs review

2. Issue / Risk: The PYTHON_NAME_RE (/^[a-zA-Z0-9][a-zA-Z0-9_.-]*$/) does not enforce PEP 508 / PyPI's normalization rules, which also permit consecutive separators (e.g., my--package) and treat -, _, . as equivalent. This is the same regex used at install time (per the PR description), so it is consistent, but it is slightly more permissive than the canonical PyPI spec.

   • Impact: Low practical risk since names originate from install-time validation and are stored, not user-supplied at upgrade time; however, a name like my..pkg would pass the regex but may not resolve on PyPI.
   • Recommendation: Document the known delta from PEP 508 in the regex comment, or tighten to /^[a-zA-Z0-9]([a-zA-Z0-9_.-]*[a-zA-Z0-9])?$/ to at least require alphanumeric end characters. Not a blocker.
   • Status: 🟡 Needs review

3. Issue / Risk: The upgradePackage Python path at ~L1028 uses normalizedVersion for the pip spec (name==normalizedVersion), but the pip show result parsing that sets result.package?.version (tested in the new upgradePackage allows python packages with uppercase names test) is not shown in the diff — it relies on pre-existing logic. If pip show returns the canonical normalized name (e.g., markupsafe lowercase) and the stored name is MarkupSafe, the version extraction may work but the stored name field could diverge from what pip reports.

   • Impact: Cosmetic/minor — the version update path appears to work (test passes), but name normalization on the stored record is not addressed.
   • Recommendation: Verify that the packageInfo.name stored after upgrade reflects the canonical PyPI name or document that case-preservation is intentional.
   • Status: 🟡 Needs review

Tests

• test/packages-injection.spec.ts: The test.each for empty versions correctly asserts both result.success === true and that the final npm arg is the bare package name (no trailing @). The non-string rejection tests assert zero install calls, which is the right signal. The Python uppercase tests mock pip index versions and pip show output in the format the existing parsers expect — good fidelity to real behavior.
• The upgradePackage treats empty-string version as "upgrade to latest" test at ~L450 asserts cmdArgs equals ['install', 'left-pad@latest'] — this directly covers the normalizedVersion ?? 'latest' fix at L1069.

Approval Recommendation

Approve with caveats

• Consider adding a comment or controller-level type guard so the isVersionInputTypeValid runtime check in upgradePackage is clearly understood as intentional defense-in-depth rather than something future refactors might remove as "redundant" given the TypeScript signature.
• Document (or tighten) the PYTHON_NAME_RE delta from PEP 508 in the regex comment — not a blocker, but worth a one-liner.

[j4ys0n]

Automated review 🤖

Summary of Changes

This PR fixes two regressions introduced by PR #44's injection-hardening: (1) empty/null/whitespace version inputs being incorrectly rejected instead of treated as "install latest", and (2) Python packages with uppercase names being silently skipped during upgrade and update-check because their names failed the npm-only lowercase allowlist. The fix adds a normalizeVersionInput helper, a PYTHON_NAME_RE regex, and runtime-aware name validation in upgradePackage and checkForUpdates, with 10 new regression tests.

Key Changes & Positives

• 🟢 normalizeVersionInput (src/services/packages.ts, new static method) cleanly separates the "empty means latest" normalization from the security allowlist, making the intent explicit and the logic easy to audit.
• 🟢 isVersionInputTypeValid guards against non-string types (booleans, numbers, objects) being coerced through the allowlist — closes a subtle injection vector where true could have resolved to the npm tag "true".
• 🟢 Runtime-aware name re-validation in upgradePackage (~line 996) and checkForUpdates (~line 871) correctly dispatches to PYTHON_NAME_RE vs. isValidNpmName, fixing the silent-skip regression for uppercase PyPI names like MarkupSafe.
• 🟢 The runtime variable in upgradePackage is now derived once as packageRuntime from packageInfo.runtime ?? 'node' and reused, eliminating the earlier duplicate derivation at the try block.
• 🟢 Test coverage is concrete and regression-focused: emptyVersions and nonStringVersions parameterized tables, whitespace-trim test, upgradePackage empty-string test, and two Python uppercase-name tests (checkForUpdates + upgradePackage).

Potential Issues & Recommendations

1. Issue / Risk: PYTHON_NAME_RE (/^[a-zA-Z0-9][a-zA-Z0-9_.-]*$/) is intentionally more permissive than PEP 508 (noted in the comment), but it also permits names ending in . or - (e.g., my-pkg-), which are not valid PyPI names and could cause confusing pip errors rather than a clean validation rejection.

   • Impact: Low — pip will reject such names at resolution time, so there is no security risk, but the error surfaced to the caller will be a raw pip stderr rather than the API's structured error message.
   • Recommendation: Consider anchoring the tail: /^[a-zA-Z0-9][a-zA-Z0-9_.-]*[a-zA-Z0-9]$|^[a-zA-Z0-9]$/ to match PEP 508's requirement that names end with an alphanumeric. The existing comment already acknowledges this; a follow-up issue or TODO would suffice if a stricter regex is deferred.
   • Status: 🟡 Needs review

2. Issue / Risk: The upgradePackage test for uppercase Python names (test/packages-injection.spec.ts, ~line 630) mocks pip show output (cmdArgs[0] === 'show') to return Version: 3.0.0, then asserts result.package?.version === '3.0.0'. If the version-extraction logic reads from the installed package.json (as the node path does via readFileMock) rather than from pip output, the mock may be exercising a different code path than intended, and the assertion could pass vacuously.

   • Impact: The test may not actually verify that the pip upgrade ran and the version was read from pip's output — a future refactor could break the behavior without breaking the test.
   • Recommendation: Add an assertion that execFilePromisifiedMock was called with args matching ['show', 'MarkupSafe'] (or the pip equivalent) to confirm the pip path was taken, not just that the result shape is correct.
   • Status: 🟡 Needs review

3. Issue / Risk: The installPackage hunk (~line 449) destructures request without version, then reads request.version separately for the type guard. If InstallPackageRequest is a plain object interface (not a class), this is fine, but if it is ever a class instance where property access has side effects, the double-read is inconsistent.

   • Impact: Negligible in practice given the codebase, but the pattern is slightly surprising.
   • Recommendation: Destructure version after the type guard: const version = PackageService.normalizeVersionInput(request.version) immediately after the guard, keeping the guard on request.version — which is already what the code does. No change needed; just confirming the pattern is intentional.
   • Status: 🟢 Correct

Tests

The checkForUpdates uppercase-Python test (test/packages-injection.spec.ts, ~line 584) mocks execFilePromisifiedMock globally for the duration of that test. Confirm the mock is properly reset between tests (via beforeEach/afterEach or mockReset) so the custom pip index versions implementation does not leak into subsequent tests in the suite.

Approval Recommendation

Approve with caveats

• Consider tightening PYTHON_NAME_RE to require an alphanumeric final character (or file a follow-up issue), to surface invalid PyPI names as structured API errors rather than raw pip stderr.
• Add an execFilePromisifiedMock call-assertion to the upgradePackage uppercase-Python test to confirm the pip code path was actually exercised.

[j4ys0n]

Automated review 🤖

Summary of Changes

This PR fixes two regressions introduced by PR #44's injection hardening in src/services/packages.ts: (1) empty/null/whitespace version inputs were incorrectly rejected instead of being treated as "install latest", and (2) the npm name allowlist was applied to Python package names during upgrade and update-check, silently skipping legitimate PyPI packages with uppercase letters. The fix adds normalizeVersionInput/isVersionInputTypeValid helpers, a PYTHON_NAME_RE constant, and runtime-aware name validation, with 10 new regression tests covering all affected paths.

Key Changes & Positives

• normalizeVersionInput + isVersionInputTypeValid in src/services/packages.ts cleanly separate the "is this a valid type?" check from the "is this a meaningful value?" normalization, preventing both coercion of true/123 into npm tags and rejection of legitimate empty-version semantics. 🟢
• PYTHON_NAME_RE with boundary enforcement (/^[a-zA-Z0-9]([a-zA-Z0-9_.-]*[a-zA-Z0-9])?$/) is strictly tighter than the previous install-time regex (/^[a-zA-Z0-9][a-zA-Z0-9_.-]*$/) — it now also rejects trailing separators (trailing-dash-, trailing.dot.), and the new tests in test/packages-python.spec.ts cover exactly those boundary cases. 🟢
• Runtime-aware name re-validation in both upgradePackage and checkForUpdates (pkg.runtime === 'python' ? isValidPythonPackageName : isValidNpmName) correctly scopes each allowlist to its subprocess, eliminating the silent-skip bug for uppercase PyPI names. 🟢
• The runtime local variable in upgradePackage is hoisted to packageRuntime before the try/catch and reused in the pip branch, eliminating the previous redundant const runtime = packageInfo.runtime ?? 'node' inside the inner try. 🟢

Potential Issues & Recommendations

1. Issue / Risk: PYTHON_NAME_RE is more restrictive than the previous install-time regex at src/services/packages.ts (the old installPackage python path used /^[a-zA-Z0-9][a-zA-Z0-9_.-]*$/, which allowed trailing separators). The new regex is now used for both install-time validation and the re-validation in upgrade/checkForUpdates. Any package installed under the old, more-permissive regex with a trailing separator in its name will now be silently skipped during upgrade/checkForUpdates — the same class of problem this PR fixes for npm names.

   • Impact: Low probability in practice (PyPI itself rejects such names), but a package record written through a non-standard path could be permanently un-upgradeable without a clear error.
   • Recommendation: In the checkForUpdates and upgradePackage skip/error paths, log the specific runtime alongside the name (already done for checkForUpdates warn log) and consider emitting a warn in upgradePackage's nameIsValidForRuntime branch rather than returning a generic error, to aid diagnosis.
   • Status: 🟡 Needs review

2. Issue / Risk: isVersionInputTypeValid is declared private static but the type guard version is string | null | undefined accepts null, yet the upgradePackage controller cast in src/controllers/packages.ts is req.body.version as string | undefined — null from a JSON body ({"version": null}) will be typed as string | undefined at the call site, so the guard's null branch is exercised only when the service is called programmatically, not from the HTTP path.

   • Impact: Functionally harmless — null from JSON body will be cast to string | undefined (TypeScript erases the cast at runtime, so the actual value is still null), and normalizeVersionInput handles null correctly. But the controller cast is misleading and could cause confusion in future maintenance.
   • Recommendation: Change the controller cast to req.body.version as string | null | undefined to accurately reflect what JSON deserialization can produce.
   • Status: 🟡 Needs review

3. Issue / Risk: The upgradePackage test for empty-string version (test/packages-injection.spec.ts, "upgradePackage treats empty-string version as 'upgrade to latest'") asserts cmdArgs equals ['install', 'left-pad@latest'] but only checks installCalls[0] — if the mock setup triggers an earlier npm call (e.g., a version-check), the assertion could pass against the wrong call.

   • Impact: Minor test reliability concern; does not affect production behavior.
   • Recommendation: Use .find() to locate the specific install left-pad@latest call rather than indexing [0], consistent with the pattern used in other tests in the same file.
   • Status: 🟡 Needs review

Tests

• test/packages-injection.spec.ts: 10 new cases cover empty/null/whitespace/non-string versions on install and upgrade, version trimming, and uppercase Python names in upgrade and checkForUpdates — directly targeting all three regression scenarios.
• test/packages-python.spec.ts: 4 new parametrized cases cover the tightened PYTHON_NAME_RE boundary rules (trailing dash/dot/underscore, leading dash), confirming the stricter regex rejects what it should.
• The checkForUpdates uppercase-python test mocks execFilePromisifiedMock globally for that test, which could interfere with other tests in the suite if mock cleanup is not handled — worth confirming beforeEach/afterEach reset behavior in the test harness.

Approval Recommendation

Approve with caveats

• Consider updating the controller cast in src/controllers/packages.ts from as string | undefined to as string | null | undefined to accurately reflect JSON body semantics.
• Confirm the checkForUpdates uppercase-python test's execFilePromisifiedMock override is properly scoped/reset to avoid cross-test interference.
• (Optional) Improve the upgradePackage invalid-name error path to log the runtime for easier diagnosis of any legacy records with boundary-violating names.

[j4ys0n]

Automated review 🤖

Summary of Changes

This PR fixes two regressions introduced by PR #44's injection hardening in src/services/packages.ts: (1) empty/null/whitespace version inputs were incorrectly rejected instead of being treated as "install latest", and (2) the npm name allowlist was applied to Python package names during upgrade and update-check, blocking legitimate mixed-case PyPI packages. The fix adds normalizeVersionInput, isVersionInputTypeValid, and isValidPythonPackageName static helpers, threads them through installPackage, upgradePackage, and checkForUpdates, and adds 10 regression tests plus boundary-case tests for the new Python name regex.

Key Changes & Positives

• normalizeVersionInput + isVersionInputTypeValid separation (src/services/packages.ts, new static methods): cleanly separates "is this a type we accept?" from "is this a meaningful value?" — prevents non-string coercion (true → "true") while preserving the documented "" / null → latest behavior. 🟢
• Runtime-aware name re-validation (src/services/packages.ts, checkForUpdates ~L871 and upgradePackage ~L996): both subprocess-guard sites now branch on pkg.runtime === 'python' before choosing the allowlist, correctly unblocking mixed-case PyPI names without weakening the npm guard. 🟢
• PYTHON_NAME_RE anchored with alphanumeric boundary (src/services/packages.ts, L118): the regex ^[a-zA-Z0-9]([a-zA-Z0-9_.-]*[a-zA-Z0-9])?$ enforces leading/trailing alphanumeric, closing the gap where the old install-time regex ^[a-zA-Z0-9][a-zA-Z0-9_.-]*$ allowed trailing separators. The new boundary tests in packages-python.spec.ts confirm this. 🟢
• beforeEach mock reset comment (test/packages-injection.spec.ts, L111): clarifies that mockResolvedValue replaces per-test mockImplementation, preventing future confusion about mock-leak risk.

Potential Issues & Recommendations

1. Issue / Risk: PYTHON_NAME_RE comment acknowledges consecutive separators (e.g. my..pkg) are accepted, deferring rejection to pip. This is a known, documented gap, but it means the subprocess-argument safety claim is slightly weaker than stated — a crafted name like my--pkg passes the allowlist and reaches pip.

   • Impact: Low practical risk since pip will reject the name at resolution time and execFile prevents shell interpretation, but the comment sets an expectation of "subprocess-argument safety" that isn't fully met for all separator combinations.
   • Recommendation: Either tighten the regex to disallow consecutive separators ((?!.*[_.\-]{2})) or update the comment to explicitly say "consecutive separators are intentionally deferred to pip for rejection" rather than framing it as a safety gap.
   • Status: 🟡 Needs review

2. Issue / Risk: upgradePackage in packages.ts controller (~L419) logs version ? ' to version ' + version : '' using the raw (pre-normalized) version value, so a whitespace-only version like " " logs " to version " even though it will be treated as latest.

   • Impact: Minor — misleading log output only; no security or behavioral impact.
   • Recommendation: Move the log line after normalizeVersionInput is called in the service, or pass normalizedVersion to the log.
   • Status: 🟡 Needs review

3. Issue / Risk: The upgradePackage test for empty-string version (test/packages-injection.spec.ts, ~L453) mocks readFileMock.mockResolvedValue(JSON.stringify({ version: '2.0.0', main: 'index.js' })) to simulate the post-upgrade package.json read, but the existingPackage fixture's runtime field is not shown in the diff — if it defaults to undefined/node, the test is correct, but if a future change to the fixture adds runtime: 'python', the npm code path assertion would silently break.

   • Impact: Test fragility; no production risk.
   • Recommendation: Explicitly assert runtime: 'node' in the existingPackage fixture used by this test, or inline the fixture.
   • Status: 🟡 Needs review

Tests

• test/packages-injection.spec.ts: 10 new tests cover empty/null/whitespace/undefined install versions, non-string type rejection on install and upgrade, whitespace trimming, empty-string upgrade-to-latest, uppercase Python names in checkForUpdates and upgradePackage. Coverage is thorough for the stated regression scenarios.
• test/packages-python.spec.ts: 4 new boundary tests (trailing-dash-, trailing-dot., trailing_underscore_, -leading-dash) validate the new PYTHON_NAME_RE end-anchor behavior. Note there is no test for the acknowledged gap of consecutive separators (e.g. my..pkg) — acceptable given the documented deferral to pip, but worth a comment in the test file.
• The checkForUpdates uppercase-Python test uses a mockImplementation that correctly distinguishes pip index versions from other calls, consistent with the beforeEach reset comment added above it.

Approval Recommendation

Approve with caveats

• Address or explicitly accept the PYTHON_NAME_RE consecutive-separator gap (issue 1) — update the comment to be unambiguous about the intentional deferral.
• Fix the misleading pre-normalization log in the controller (issue 2) — low priority but easy.
• Consider hardening the existingPackage fixture reference in the empty-string upgrade test (issue 3).

[j4ys0n]

Automated review 🤖

Summary of Changes

This PR fixes two regressions introduced by PR #44's injection hardening in src/services/packages.ts: (1) empty/null/whitespace version inputs were incorrectly rejected instead of being treated as "install latest", and (2) the npm name allowlist was applied to Python package names during upgrade and update-check, blocking legitimate uppercase PyPI names. The fix adds normalizeVersionInput, isVersionInputTypeValid, and isValidPythonPackageName static helpers, threads them through installPackage, upgradePackage, and checkForUpdates, and adds 10 targeted regression tests across two spec files.

Key Changes & Positives

• normalizeVersionInput + isVersionInputTypeValid separation (src/services/packages.ts, new static methods): cleanly separates "is this a type we accept?" from "is this a meaningful value?" — prevents boolean/number coercion while preserving the documented "" / null → latest semantics. 🟢
• Runtime-aware name re-validation (src/services/packages.ts, checkForUpdates ~L872 and upgradePackage ~L997): the pkg.runtime === 'python' branch now routes to PYTHON_NAME_RE instead of isValidNpmName, correctly unblocking uppercase PyPI names like MarkupSafe. 🟢
• PYTHON_NAME_RE extracted as a named constant with a clear comment explaining why consecutive separators are intentionally deferred to pip — good defense-in-depth rationale documented inline.
• beforeEach comment in test/packages-injection.spec.ts (~L111) explaining that mockResolvedValue resets per-test mockImplementation is a useful guard against future test-isolation confusion.
• Regression tests are well-scoped: test.each over emptyVersions and nonStringVersions in packages-injection.spec.ts, plus boundary tests for Python name rules in packages-python.spec.ts, directly mirror the failure modes described in the PR.

Potential Issues & Recommendations

1. Issue / Risk: isVersionInputTypeValid is only called in installPackage and upgradePackage, but checkForUpdates constructs upgrade specs from persisted pkg.version without a type guard (src/services/packages.ts, checkForUpdates loop). If a record has a non-string version field (e.g., written by a migration or external tool), the pip/npm spec construction could silently coerce it.

   • Impact: Low probability in practice since DB writes go through validated paths, but the defense-in-depth story is inconsistent with the rest of the hardening.
   • Recommendation: Apply normalizeVersionInput (or at minimum a typeof pkg.version === 'string' guard) to pkg.version before building the pip/npm spec in the checkForUpdates loop.
   • Status: 🟡 Needs review

2. Issue / Risk: PYTHON_NAME_RE (/^[a-zA-Z0-9]([a-zA-Z0-9_.-]*[a-zA-Z0-9])?$/) accepts single-character names (the ? makes the inner group optional), but the comment says "starting AND ending with an alphanumeric (single-character names allowed)" — this is intentional. However, the regex also accepts names like a (length 1) which is valid per PEP 508, so this is fine. Worth confirming the regex does NOT accept the empty string: "".match(...) returns null because [a-zA-Z0-9] requires at least one char — correct. No action needed, but the isValidPythonPackageName wrapper already guards typeof name === 'string' redundantly since PYTHON_NAME_RE.test on a non-string would throw — minor.

   • Impact: Negligible; the typeof name === 'string' check in isValidPythonPackageName is harmless redundancy.
   • Recommendation: No change required; optionally remove the redundant typeof check since callers always pass typed strings, but it's a safe guard.
   • Status: 🟢 Correct

3. Issue / Risk: The upgradePackage test for empty-string version (test/packages-injection.spec.ts, ~L453) mocks readFileMock.mockResolvedValue(JSON.stringify({ version: '2.0.0', main: 'index.js' })) to simulate reading package.json post-upgrade, but the existingPackage fixture's runtime is not explicitly set in the spread — if existingPackage defaults to runtime: undefined, the packageRuntime === 'python' branch is skipped correctly (falls through to npm path), but the test doesn't assert the npm path was taken vs. the python path.

   • Impact: The test still validates the correct outcome (left-pad@latest install call), so the regression is caught. The ambiguity is cosmetic.
   • Recommendation: Explicitly set runtime: 'node' in the existingPackage fixture used by this test to make the intent unambiguous.
   • Status: 🟡 Needs review

Tests

• test/packages-injection.spec.ts: 10 new cases cover emptyVersions × install, nonStringVersions × install, upgradePackage non-string rejection, whitespace trimming, empty-string upgrade → latest, uppercase Python name in checkForUpdates, and uppercase Python name in upgradePackage. Coverage is directly tied to the two regression scenarios.
• test/packages-python.spec.ts: 4 new boundary cases for PYTHON_NAME_RE (trailing dash/dot/underscore, leading dash) confirm the regex end-anchor behavior.
• Gap: No test covers checkForUpdates with a persisted non-string version field (see Issue add packages service for automatically installing mcp servers from npm #1 above).

Approval Recommendation

Approve with caveats

• Consider adding a normalizeVersionInput / type guard for pkg.version in the checkForUpdates loop to maintain consistent defense-in-depth (Issue add packages service for automatically installing mcp servers from npm #1).
• Optionally set runtime: 'node' explicitly in the existingPackage fixture for the empty-string upgrade test to remove ambiguity (Issue minor updates and openapi specs #3, cosmetic).

[j4ys0n]

Re the latest automated review — no changes made this round, because both checkbox items rest on incorrect premises:

Issue 1 (type-guard pkg.version in checkForUpdates): pkg.version never reaches a subprocess in that loop. The python path invokes pipIndexLatestVersion(venvAbsolutePath, pkg.name, ...) and the npm path invokes npm view <pkg.name> version — the version token there is the literal npm field name, not the persisted value. pkg.version is only used in pure-JS comparisons (compareVersions, !==) and in the response payload, so there is no spec construction to coerce and nothing for a type guard to protect.

Issue 3 (pin runtime: 'node' in the existingPackage fixture): already the case — the fixture declares runtime: 'node' explicitly (test/packages-injection.spec.ts, existingPackage definition). It just isn't visible in the diff hunks.

Issue 2 was marked "no change required" in the review itself; the redundant typeof guard in isValidPythonPackageName is intentional defense-in-depth for persisted records.

---

Generated by Claude Code