fix(cli): Windows npm install broken — openship.ps1 calls sh.exe instead of node.exe - #461
Open
kershrita wants to merge 40 commits into
Open
fix(cli): Windows npm install broken — openship.ps1 calls sh.exe instead of node.exe#461kershrita wants to merge 40 commits into
kershrita wants to merge 40 commits into
Conversation
producerOpts only forwarded sourceIds/command/exclude from policy.payloadConfig, silently dropping producer-specific keys like produceCommand/restoreCommand/artifactName. Every custom_command backup policy (including all mail-server backups) failed with "custom_command producer requires `produceCommand` in policy payload config".
destination.put() forwarded artifact.metadata straight into the S3 put call, where every key becomes an x-amz-meta-* HTTP header. Header values must be printable ASCII with no newlines, but custom_command artifacts store multiline produceCommand/ restoreCommand text in metadata, so the upload died with an invalid header error. Filter to header-safe string values before the put; the unfiltered metadata is still recorded on the run for restore.
Inbox rows show sender name on the top line and, below it, the SAME subject text repeated verbatim as the "preview" line - not a snippet, not empty, a literal duplicate. The IMAP driver already had a `snippet` field scaffolded on ThreadMessage (hardcoded to '') with no implementation behind it. Implemented: a bounded IMAP partial fetch (RFC 3501 TEXT section, capped ~320 bytes) rides along in the SAME batched FETCH command listThreads already issues for envelope/flags/bodyStructure - no extra IMAP round trip. Descends the MIME structure to find the first leaf part (matching MIME's own "primary alternative first" convention), decodes quoted-printable/base64 as needed, strips HTML tags/entities when the leaf is text/html (including a dangling tag at the very end, since the byte-bounded window commonly cuts markup mid-tag), and truncates to 140 chars. Best-effort throughout - any failure degrades to an empty snippet rather than breaking the row. Since the fetch window is a flat byte range across the whole multipart body (not scoped to one part), short leaf content commonly leaves budget that runs into the NEXT part's boundary line + headers (e.g. a one-sentence body followed by an attachment) - truncates at the next MIME boundary line before decoding to avoid leaking raw multipart markup into the snippet. List rows now show: sender, subject (unchanged), and the real 1-line body snippet beneath it - falls back to nothing (not the subject again) when extraction comes up empty for a given message.
org conflict issue solve
…al ports The port preflight for a containerized deployment enumerated the project's public endpoint ports and probed each one on the host. Those are the ports inside the container's network namespace, which cannot collide on the host at all: a published container binds exactly ONE host port, the loopback pin in DeployConfig.hostPort. Result: any project declaring container port 80 (a static site on nginx behind the Openship edge, for example) hits a false conflict against the edge itself. The prompt then offers to free the port to continue, which would stop the edge and take every routed site on the box down in order to publish one. Probe cfg.hostPort instead, which is also the backstop that host-port.ts already documents ensurePortAvailable as providing. The project's own outgoing deployment still holds that pin during preflight, since preflight runs before the old container is stopped and loopback-port cannot overlap, so skip the probe when the previous container's hostPort matches the incoming one; the pipeline releases it in the next step. Bare runtime is unchanged: there the app does own 127.0.0.1:<port> on the host.
fix(deployments): probe the published host port, not container-internal ports
fix(api): backup producers drop config and crash uploads with multiline metadata
fix(email): list rows show a subject-duplicate instead of a body snippet
`extractListSnippet` (532753a) decodes the bounded BODY[TEXT] partial fetch with a fixed utf8 decode, on whatever leaf `resolveLeafPart` lands on, and unescapes six hard-coded HTML entities. Three consequences, all visible in the inbox list rows: 1. The MIME charset is dropped. `resolveLeafPart` returned only `{type, encoding}` even though imapflow parses `parameters.charset`, so a `text/plain; charset=ISO-8859-1` quoted-printable part rendered as "la reuni<U+FFFD>n de ma<U+FFFD>ana" in the list while `getThread`, which goes through charset-aware mailparser, rendered the same message correctly as "la reunión de mañana". 2. Non-text leaves are decoded. `resolveLeafPart` descends `childNodes[0]` unconditionally, so an S/MIME `application/pkcs7-mime` message emitted raw DER, a scan-to-email `multipart/mixed` emitted "%PDF-1.7 ...", a PGP/MIME message emitted "Version: 1", and an inline-image-first `multipart/related` emitted raw JPEG bytes - despite the function's own contract of degrading to ''. 3. Only ` & < > " &oblien#39; '` were decoded, so the numeric and typographic references that dominate real marketing and transactional HTML survived literally ("We’re excited —"). `&` was also replaced before `<`/`>`, so `&lt;` double-unescaped to `<`. Fixes: - `LeafPartInfo` carries the lowercased `parameters.charset`, and `decodeCharset` routes the bytes through `TextDecoder`. Labels TextDecoder rejects throw and fall back to today's utf8 decode, so the change only widens what decodes correctly. The ASCII labels stay on the utf8 path deliberately: WHATWG resolves `us-ascii` to windows-1252, which would turn "Cafés" into "Cafés" for the common part that declares ASCII but carries UTF-8 bytes. - `extractListSnippet` returns '' when the resolved leaf is not `text/*`. A null leaf keeps the current behaviour so an unparseable structure is not newly suppressed. - `decodeHtmlEntities` resolves decimal, hexadecimal and a table of named references in a single left-to-right pass, which also removes the double-unescape. `TextDecoder` is imported from `node:util` because bun-types narrows the global constructor to `"utf-8" | "windows-1252" | "utf-16"`; the `node:util` export takes a `string`. Runtime behaviour is identical. One measured behaviour change beyond the three above: a `multipart/mixed` whose first part is `message/rfc822` previously leaked the forwarded message's headers into the row ("From: Ops Subject: Nightly backup report Content-Type: text/plain; charset=utf-8 All volumes backed up successfully.") and now yields ''. The two consumers of `snippet` render nothing for an empty value rather than substituting anything - `mail-list.tsx:506` is `snippet ? highlight(snippet) : null` and `command-palette-context.tsx:991` is `snippet || ''` - so the snippet line goes blank. The subject is rendered separately and unconditionally at `mail-list.tsx:488`, so the row keeps its subject either way. `extractListSnippet` is exported for the test, following `formatFromAddress` in `trpc/routes/mail.ts`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review follow-ups on the alternate-CA/EAB foundation: - renewCert: a lineage issued by a different directory than the one now configured cannot be renewed against the new CA (renew neither registers the account nor carries EAB). Compare the renewal conf's recorded server with the effective directory and force-reissue on mismatch — a one-time, self-healing switch. Drop the now-redundant --server from plain renew. - Write the ephemeral EAB config with mode 0600 from the start (_writeFile gained an optional mode applied to the temp file before the rename), so the HMAC key is never on disk world-readable, however briefly. - Redact the EAB HMAC inside _execCertbot's error paths (buffered throw and full accumulated stream output), so no caller can leak it by omitting a redacting catch; per-chunk redaction alone missed chunk-boundary splits. - Validate the OPENSHIP_ACME_* contract at API boot (EAB pair, base64url HMAC, printable kid, absolute CA bundle) so misconfiguration names the variable at startup instead of failing at the first deploy. - Docs: note that --config bypasses an operator cli.ini during EAB runs and document the reissue-on-CA-switch behavior. - Tests: renew mismatch/match paths, renew-error redaction, tightened 0600 ordering assertions, and complete the preserved-ACME-env assertions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… importer Apache's own mod_proxy documentation writes directives with quoted arguments — `ProxyPass "/" "http://127.0.0.1:8080/"` — and httpd treats those quotes as argument delimiters, not as part of the value. Verified on the `httpd:2.4` image (Apache/2.4.68): a vhost written that way is `Syntax OK` and live in `httpd -S`. Only `DocumentRoot` stripped quotes; every other read kept them, so a vhost written the documented way migrated as nothing. Measured end to end through `scanApache` -> `registerImportedSites` with a real `NginxProvider` over a fake executor: ProxyPass "/" "http://127.0.0.1:8080/" -> registered [], warning `Invalid proxy target (must be http/https URL): "http://127.0.0.1:8080/"`, no vhost conf written. The unquoted control registers app.example.com and writes the conf. ServerName "app.example.com" -> registered [], warning `skipped unsupported domain ""app.example.com""` SSLCertificateFile "/etc/letsencrypt/live/app.example.com/fullchain.pem" -> isSafeCertPath false (true for the unquoted control), warning `existing cert path looks unsafe - issuing a fresh certificate instead`: the operator's cert is dropped and a fresh ACME issuance is forced. The same regex also let `\s+` run across newlines, so the documented one-argument form inside `<Location>` swallowed the following line and parsed as `{ path: "\"http://127.0.0.1:8080/\"", url: "ProxyPassReverse" }`, failing closed at `assertValidUpstream`. This lands on the takeover path: by the time `registerImportedSites` runs the operator's Apache is stopped and Openship holds :80/:443, so an Apache box configured the way the docs show migrates zero sites. `unquote` is applied at the read points - `directive`, `hostOnly` and the ProxyPass captures - generalising the strip `DocumentRoot` already did. It keeps that unanchored form deliberately: httpd accepts an unterminated `DocumentRoot "/srv/x` (`AH00112 ... [/srv/x] does not exist`, `Syntax OK`) and resolves it to `/srv/x`, and requiring a matched pair would have turned that into a rejected static root. `proxyRoutes` reads ProxyPass line by line, so a mapping can no longer run past its own line, and accepts the one-argument form with its path taken from the enclosing `<Location>`. With no enclosing `<Location>` there is no path to give it, so the mapping is dropped rather than defaulted to `/`; httpd rejects that config outright (`AH00526: ProxyPass|ProxyPassMatch needs a path when not defined in a location`), so it cannot come from a running Apache. A `<LocationMatch>` path is a regex no single prefix can express, so its mappings are dropped too and the vhost is reported as regex proxying, alongside the existing ProxyPassMatch warning - previously it reached nginx as an upstream literally named `</LocationMatch>`. Quoted arguments containing whitespace are still unsupported for ProxyPass (its tokens stay `\S+`, as before) - this strips delimiters, it is not a full httpd argument lexer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adopt the repo's ephemeral-secret layout contract (runtime/git-ssh-material.ts) for the EAB ini: a fresh 0700 parent directory locked down before any secret bytes land — covering even the staged temp file — with the file at 0600 and cleanup removing the directory as a unit. Previously the ini sat directly in /etc/letsencrypt (typically 0755), where its name was enumerable by local users. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
fix(adapters): honour quoted and one-argument ProxyPass in the Apache importer
feat: support alternate ACME CAs and EAB
…set-and-binary fix(email): honour part charset and skip binary parts in list snippets
Discovery subtracted every container env entry whose exact KEY=VALUE matched the
image's baked-in default, so a migrated service imported only the vars an
operator had provably overridden. That holds for a hand-run `docker run`, but not
for Coolify: it resolves and injects every runtime variable explicitly —
service-level, shared `{{team.*}}`, linked-resource and secret alike — and its
Nixpacks builds bake those same values into the image it then runs. The two sets
overlap almost completely, so the subtraction discarded real production
configuration, frequently all of it.
Measured through `reconcileStack` with a Coolify-labelled container whose
NODE_ENV and DATABASE_URL match its image defaults:
before -> env {}
after -> env { NODE_ENV, DATABASE_URL } (PATH still denylisted)
Coolify stamps `coolify.managed=true` on every container it manages
(bootstrap/helpers/docker.php), so keying off that label scopes the change to the
platform that needs it — plain Docker and Compose migrations keep the base-image
denoising and the test that covers it.
Build-time variables and BuildKit secrets are genuinely absent from a running
container's config and stay unrecoverable; discovery now says so per service
instead of leaving the gap silent.
`DiscoveredService.warnings` was computed during discovery and sent to the client, but no step ever rendered it. Everything discovery could not carry over — bind mounts whose data stays on the host, host ports dropped as duplicates, and now Coolify's build-time variables — stayed invisible until after adoption. Render them on the service config card, beside the env editor an operator would use to fill those gaps, reusing the card's existing warning treatment.
A compose file declaring a required variable (`${VAR:?message}`) was
scanned against the repo `.env` alone. Env the user had configured in
Openship never reached the parser, so the scan reported the file as
unparseable even though the deploy would resolve the same variable:
Could not parse the Docker Compose file at "deploy/docker-compose":
Set POSTGRES_PASSWORD in .env
`parseComposeFile` already accepts explicit interpolation values that
override the ones loaded from `envFileContent`. Thread the caller's env
to it: `Source` -> `ResolveOptions` -> `resolveFromReader` ->
`toProjectInfo`. `ResolveOptions` already reaches both the GitHub and
local resolvers, so neither needed changing.
Omitting the env keeps the existing behaviour — a genuinely unset
required variable is still reported rather than silently dropped.
Expose the resolver's new interpolation env on the scan route so the wizard can send what the user already entered. Declared on `PrepareDeployBody` as a string->string record, so the shape is validated at the trust boundary like every other env map on the API. The value is interpolation-only: it is not persisted here, and the response already masks every service env via `maskScanService`, so a supplied secret cannot be echoed back unmasked.
`rescanWithComposePath` re-reads the source because projectType, the service list and each service's env can only come from the compose file. On a file with required variables that re-scan failed, since the env the user had just entered was not part of the request. Carry `config.envVars` through both initialize paths into the prepare body. Empty maps are dropped so a blank value never reaches the API, matching how `scanComposePath` handles an unset pin.
…-env fix(api/dashboard): interpolate Compose vars from the configured deploy env
…import fix(api/dashboard): import Coolify's runtime env during server migration
…pport Add TanStack Start stack detection
…to the client
detectPackageManager() legitimately returns "unknown" when no manifest is
found anywhere in the scanned root (e.g. a docker-compose-only subfolder
in a monorepo, no package.json/go.mod/requirements.txt/etc). That's a
valid internal sentinel — applyWorkspaceContext already special-cases it
— but it isn't a real package manager, and PackageManagerEnum
(project.schema.ts, derived from the STACKS registry) never included it
as a literal.
Two builders echoed the raw value straight into API responses the
dashboard persists verbatim into subsequent project-creation requests:
ProjectInfo (prepare.service.ts, the single-root/declared-compose path)
and MonorepoApp (project-root-detector.ts, the monorepo sub-app path).
Either one 400s with "Expected union value" the moment a user deploys a
compose stack that lives in a subfolder with no manifest of its own —
which is the common case for a pure-infra monorepo (a repo of several
infra/<service>/docker-compose.yml folders, one Openship project per
folder).
Fixes both leak points by normalizing "unknown" to "npm" right where the
value is produced — the same default the dashboard's own `|| "npm"`
fallbacks already assume, now applied where it's actually needed instead
of scattered across every read site.
Added a regression test in prepare.service.test.ts reproducing the exact
shape that broke: a docker-compose.yml in a subfolder with zero manifest
files, scanned via a declared composePath. Confirmed it fails without the
fix ('unknown' !== 'npm') and passes with it.
…nknown-400 Fix: normalize packageManager "unknown" to "npm" before it hits PackageManagerEnum
Three conflicts, all "both sides fixed the same code path" — resolved by keeping both fixes rather than picking a side: - docker-reconcile: main stopped subtracting image defaults for Coolify-managed containers; test started reporting which defaults were dropped. Now the guard feeds the reporting call, so Coolify containers drop nothing and therefore report nothing. - backup.orchestrator: kept test's whole-payloadConfig forward (main's explicit sourceIds/command/exclude picks are redundant beside its own spread), and kept main's header-safe metadata filter ahead of the put whose result test needs for the etag/sha256 comparison. - nginx.test: import-block union; every name is used by the merged body.
… resolve 400 Bad Request (oblien#427)
… creates a new per-org custom app (no :id in the URL), so it\nmust be flagged as collection-scoped for the permission middleware to\nskip the leaf id check. The sibling POST / route already does this.\n\nFixes oblien#440
…ection fix(apps): add collection: true to POST /api/apps/custom
…ramework-400 fix(api): accept framework display labels in project ensure schema to…
Fix bugs for 0.5.5 patch
…nchers npm reads the shebang of the bin file to generate openship.cmd and openship.ps1 on Windows. The main dist/index.js uses a sh/JS polyglot shebang (#!/usr/bin/env sh) intentionally — it lets the official Bun-based installer run the CLI on Bun-only Unix boxes without Node. But on Windows, npm sees 'sh' and emits sh.exe calls in the PowerShell launcher, breaking every Windows npm install with: The term 'sh.exe' is not recognized... Fix: add dist/node-entry.js with #!/usr/bin/env node as the npm bin entry point. It simply imports dist/index.js. npm now generates node.exe launchers on Windows. The polyglot dist/index.js and the official installer path are completely unchanged. tsup.config.ts is split from a single defineConfig() into an array of two configs — one per entry — so each gets its own banner string.
…ws npm Regression guard: if the tsup.config.ts banner for node-entry is ever changed back to the polyglot shebang (#!/usr/bin/env sh), npm would again generate broken Windows launchers (sh.exe). This check catches the regression before publish by asserting dist/node-entry.js has #!/usr/bin/env node as its first line.
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
11 participants
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.
Fixes #460
Problem
npm i -g openshipgenerates a broken Windows launcher. npm reads theshebang of the
binfile (dist/index.js) which is#!/usr/bin/env sh(intentional polyglot for Bun-only Unix boxes), then emits
sh.execallsin
openship.ps1— which doesn't exist on standard Windows.Fix
Add
src/node-entry.ts→dist/node-entry.jswith#!/usr/bin/env nodeas the npm
bintarget. It simply doesimport "./index.js". npm now readsnodeand generates correct Windows launchers. The polyglotdist/index.jsand the official Bun-based installer path are completely untouched.
tsup.config.tsis split into a two-config array so each entry gets its ownbannerstring.Tested on Windows
npm uninstall -g openship && npm install -g .openship --version→0.5.0✅ (was:sh.exe not recognized❌)openship.ps1now callsnode$exenotsh$exe✅Changes
apps/cli/src/node-entry.tsimport "./index.js"apps/cli/tsup.config.tsapps/cli/package.jsonbin.openship→./dist/node-entry.jsapps/cli/scripts/release-smoke.sh