fix(api/dashboard): omit the package manager when the source has none - #409
Closed
Rish-it wants to merge 4 commits into
Closed
fix(api/dashboard): omit the package manager when the source has none#409Rish-it wants to merge 4 commits into
Rish-it wants to merge 4 commits into
Conversation
detectPackageManager returns the "unknown" sentinel when nothing in the
source identifies a package manager — a stock Compose project carries
neither a manifest nor a lockfile. That sentinel is not a member of
ALL_PACKAGE_MANAGERS, and the wizard echoes a scan straight back into
POST /projects/ensure, so the deploy fails validation on /packageManager
with "Expected union value".
Omit the field instead. It is already optional on every write body, so
absence is the shape they were built for, and the sentinel stays internal
where project-root-detector reads it. Coercing at detection would instead
hand "npm i --force" to every lockfile-less repo, since
getInstallCommand("unknown") deliberately returns an empty string.
One choke point covers all four scan surfaces: /deployments/prepare,
scanLocal, detectStack and the folder-upload scan all resolve through
resolveProjectInfo.
Fixes oblien#389
…age manager pickComposePrimary copied the scan's packageManager onto the single-mode snapshot verbatim. Every other read of that value in the wizard already guards with `|| "npm"`; this one did not, and it is on the compose path, which is exactly where the scan now omits the field. The snapshot is persisted through POST /projects/ensure, so an absent value has to resolve to a real package manager here rather than travel as undefined.
The API omits packageManager when the source has none, so the required string on PrepareAppConfig no longer describes what arrives. Typing it honestly is what surfaced the unguarded read in pickComposePrimary.
There was a problem hiding this comment.
Pull request overview
Fixes a dashboard/API deploy flow where sources without a manifest/lockfile were detected as packageManager: "unknown", which then got echoed into POST /projects/ensure and rejected by the backend schema. The change makes the API omit packageManager when the detector returns the "unknown" sentinel, and updates the dashboard to fall back to "npm" when the field is absent.
Changes:
- API: Make
ProjectInfo.packageManageroptional and omit it when stack detection returns the"unknown"sentinel. - API: Add a regression test ensuring the scan result remains valid against
EnsureProjectBodywhen the source has no package manager. - Dashboard: Default compose primary
packageManagerto"npm"when absent; update types and add a test covering the fallback.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| apps/dashboard/src/lib/api/deploy.ts | Makes PrepareAppConfig.packageManager optional to reflect the now-omitted field from API scans. |
| apps/dashboard/src/context/deployment/mode-config.ts | Falls back to "npm" for compose primary when singleAppCandidate.packageManager is absent. |
| apps/dashboard/src/context/deployment/mode-config.test.ts | Adds a test ensuring compose primary snapshot resolves an absent package manager to "npm". |
| apps/api/test/modules/deployments/prepare.service.test.ts | Adds a regression test ensuring a lockfile-less/manifest-less compose source produces a scan that validates against EnsureProjectBody. |
| apps/api/src/modules/deployments/prepare.service.ts | Makes ProjectInfo.packageManager optional and omits it when detection returns "unknown". |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+132
to
+134
| expect( | ||
| Value.Check(EnsureProjectBody, { name: "immich", packageManager: result.packageManager }), | ||
| ).toBe(true); |
…sentinel Filtering the "unknown" sentinel by name is only correct while that is the only value detection can report outside ALL_PACKAGE_MANAGERS. Check membership in that list instead — it is the same list PackageManagerEnum is generated from, so a scan echoed back into POST /projects/ensure is valid by construction rather than because the one sentinel we knew about was filtered out. applyMetadataOverrides never touches packageManager, so detection is the only source and the two checks are equivalent today. This one stays correct if a second sentinel is ever added.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A stock Compose project fails to deploy with a 400 on
/packageManager. Detection reports the"unknown"sentinel for a source with no manifest and no lockfile, the wizard echoes the scan back intoPOST /projects/ensure, and"unknown"is not a member of the list that route validates against. Omit the field instead of emitting a value our own write schema rejects.Motivation
detectPackageManagerends with a sentinel rather than a package manager (stack-detector.ts:144):That string is truthy, which is what makes it slip through. The wizard carries seven
|| "npm"fallbacks between a scan and the ensure body — every one of them was written to catch a missing package manager, and every one of them passes"unknown"straight through. It reachesPOST /projects/ensure, whosepackageManageris drawn fromALL_PACKAGE_MANAGERS(project.schema.ts:287), and the deploy dies there.Reproduced against
b22c3701with the reporter's case — the official immich Compose file, nopackage.json, no lockfile:mainpackageManager"unknown"falsetrue[{"path":"/packageManager","message":"Expected union value","value":"unknown"}][]The error object on the left is the payload from the issue, field for field.
This is not Compose-specific. Any source without a lockfile or manifest lands on the same sentinel — a plain static site fails identically. No project ever persisted
"unknown", since the schema always rejected it, so there is no stored value to migrate.Related issue
Closes #389
Changes
apps/api
modules/deployments/prepare.service.ts—toProjectInfosurfacespackageManageronly when it is a member ofALL_PACKAGE_MANAGERS, the same listPackageManagerEnumis generated from, and omits it otherwise;ProjectInfo.packageManagerbecomes optional. Gating on the list rather than on the"unknown"string keeps the scan valid by construction instead of correct only while that is the sole sentinel.applyMetadataOverridesnever touches the field, so detection is the only source and the two are equivalent today. One choke point covers all four scan surfaces:/deployments/prepare,scanLocal,detectStackand the folder-upload scan all resolve throughresolveProjectInfo, and the two scan routes already shareprojectInfoToScanResponse.test/modules/deployments/prepare.service.test.ts— regression test on the existing tmpdir harness, asserting the scan result validates againstEnsureProjectBody.apps/dashboard
context/deployment/mode-config.ts—pickComposePrimarygets the|| "npm"fallback its seven siblings already had. This is the single unguarded read in the wizard, and it sits on the compose path, which is exactly where the field now goes missing.context/deployment/mode-config.test.ts— covers that fallback throughbuildSingleModeSnapshot.lib/api/deploy.ts—PrepareAppConfig.packageManageroptional, so the type describes what actually arrives.Deliberately not changed:
detectPackageManageritself. Coercing at detection looks tempting, butgetInstallCommand("unknown")returns""on purpose (stack-detector.ts:668) — mapping the sentinel to"npm"there would handnpm i --forceto every lockfile-less repo. The sentinel stays internal, whereproject-root-detector.ts:593reads it.Omitting rather than substituting is what the write bodies were built for:
packageManagerisType.Optionalon all of them, and absence makes the wizard's existing|| "npm"guards fire as designed instead of being dead code.Verification
RED first, on the branch with only the source fix reverted:
Same for the dashboard half, with only the
mode-config.tsfallback reverted:Both fail without the change and pass with it.
End-to-end against committed
HEAD— real resolver, real temp filesystem, real write schema. Immich's compose file, plus a pnpm repo to confirm a genuine value is still passed through untouched:Against
mainthe compose row prints"unknown"andfalse, with the reporter's error object.Full suites, two separate clean runs each:
Four commits, each green on its own: the API fix with its test, the dashboard fallback with its test, the type change (which only typechecks once the fallback exists — that ordering is deliberate), then the switch from the sentinel check to list membership.
One note rather than a silent tick on the checklist:
bun formatwas not run across the repo. Three of the files this PR touches already failprettier --checkon cleanmain, so formatting them would reformat lines this PR has no business touching. Both files I authored are prettier-clean, and I fixed the one formatting issue that was genuinely mine.Questions for a maintainer
@Hydralerne — one thing worth your call, not a blocker for this PR:
The same field is validated two different ways.
CreateProjectBody/EnsureProjectBodydrawpackageManagerfromPackageManagerEnum(project.schema.ts:287), butSetOptionsBodyonPOST /projects/:id/optionstakes a bareType.Optional(Type.String())(:543) — so the Runtime tab's save path accepts any string the create path would reject. Nothing in this report goes through that route, and tightening it is a behavior change rather than a bug fix, so I left it alone. Worth a follow-up if you want the two to agree.Checklist
bun run test,bun run --cwd <workspace> lint, andbun formatall pass locally — tests and typecheck pass; see the note under Verification for whybun formatwas deliberately not run