fix(scope): return 400 on missing/invalid push-options header

[davidfirst]

Parse the push-options header in the scope put route safely: return a clear 400 when the header is missing or isn't valid JSON, instead of surfacing a cryptic 500 from deep in the stack.

Background

This PR originally added zod validation across the scope/lanes remote-scope protocol routes. We decided against that: these routes are a versioned wire protocol (client and server upgrade independently), so each schema becomes a second source of truth that must stay backward-compatible forever, with asymmetric downside (a too-strict server rejects lagging clients) and little upside given the schemas were deliberately permissive. So it's trimmed down to the one contract-free robustness fix.

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Add shared zod validation to remote scope/lanes HTTP routes
✨ Enhancement 🐞 Bug fix 🕐 20-40 Minutes

Description

• Add shared zod validation middleware and HttpError in @teambit/express.
• Validate scope/lanes protocol route inputs to return clear 400s for bad requests.
• Harden scope put by safely parsing and validating the push-options header JSON.

Diagram

graph TD
  client["Remote client"] --> routes["Scope/Lanes routes"] --> validate["express validate"] --> zod{{"zod"}} --> err["errorHandle"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Keep per-route ad-hoc 400 checks

• ➕ No new shared API surface
• ➕ Very explicit checks per handler
• ➖ Duplication and inconsistent error messages
• ➖ Easy to miss malformed JSON/header cases leading to 500s

2. Central schema registry + generic route wrapper

• ➕ Uniform validation for body/headers/query across routes
• ➕ Single place to evolve protocol input contracts
• ➖ More framework work and indirection for a small set of routes
• ➖ Harder incremental adoption and debugging

3. Adopt a third-party zod/express middleware package

• ➕ Less custom code and potentially richer error shaping
• ➖ Adds external dependency behavior surface
• ➖ May not integrate cleanly with existing errorHandle(status) convention

Recommendation: The PR’s approach is the best trade-off: small shared helpers that integrate with the existing error handler via HttpError(status), while keeping schemas permissive (.passthrough()) to preserve cross-version protocol compatibility. More generic wrappers/registries or third-party middleware would add complexity or dependency risk without clear benefit for this scope.

Files changed (11) +147 / -25

Enhancement (4) +66 / -1

index.tsRe-export validation helpers from @teambit/express +1/-0

Re-export validation helpers from @teambit/express

• Exposes validateBody, validateData, and HttpError from the package entrypoint for route consumption.

scopes/harmony/express/index.ts

index.tsExport validate middleware from middlewares barrel +1/-0

Export validate middleware from middlewares barrel

• Adds validateBody, validateData, and HttpError exports alongside existing error utilities.

scopes/harmony/express/middlewares/index.ts

validate.tsAdd zod validation helpers and HttpError(400) +53/-0

Add zod validation helpers and HttpError(400)

• Introduces HttpError carrying an HTTP status, plus validateData() (throws 400 with readable Zod issue formatting) and validateBody() middleware that replaces req.body with the parsed result.

scopes/harmony/express/middlewares/validate.ts

action.route.tsValidate scope action body (name + optional options) +11/-1

Validate scope action body (name + optional options)

• Adds a permissive schema enforcing only action name (options intentionally left unconstrained) and wires validateBody() middleware.

scopes/scope/scope/routes/action.route.ts

Bug fix (7) +81 / -24

lanes.create.route.tsValidate lanes create body with zod schema +9/-4

Validate lanes create body with zod schema

• Adds a permissive schema requiring a non-empty lane name and wires validateBody() middleware, removing manual missing-field checks.

scopes/lanes/lanes/lanes.create.route.ts

lanes.delete.route.tsValidate lanes delete body; keep empty-array 204 behavior +9/-4

Validate lanes delete body; keep empty-array 204 behavior

• Adds a schema for names: string[] and uses validateBody() middleware, while preserving the special case of returning 204 when names is empty.

scopes/lanes/lanes/lanes.delete.route.ts

lanes.restore.route.tsValidate lanes restore body with zod schema +9/-4

Validate lanes restore body with zod schema

• Adds a permissive schema requiring a non-empty hash and wires validateBody() middleware, removing manual missing-field checks.

scopes/lanes/lanes/lanes.restore.route.ts

lanes-check-conflicts.route.tsValidate lanes check-conflicts body with zod schema +10/-7

Validate lanes check-conflicts body with zod schema

• Adds a schema requiring non-empty sourceLane and targetLane and uses validateBody() middleware instead of manual 400 checks.

scopes/lanes/merge-lanes/lanes-check-conflicts.route.ts

delete.route.tsValidate scope delete body (ids + optional flags) +11/-1

Validate scope delete body (ids + optional flags)

• Adds schema validation for ids and optional force/lanes flags and wires validateBody() middleware to normalize 400 behavior.

scopes/scope/scope/routes/delete.route.ts

fetch.route.tsValidate scope fetch body while allowing versioned fields +15/-1

Validate scope fetch body while allowing versioned fields

• Adds schema validation for ids and fetchOptions.type while allowing additional fetchOptions fields via passthrough for cross-version compatibility.

scopes/scope/scope/routes/fetch.route.ts

put.route.tsHarden push-options header parsing + validate via validateData() +18/-3

Harden push-options header parsing + validate via validateData()

• Changes missing/invalid push-options header failures to HttpError(400), safely JSON.parses the header, and validates only the fields read (clientId/persist) with a permissive schema.

scopes/scope/scope/routes/put.route.ts

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Fetch type too strict ✓ Resolved 🐞 Bug ≡ Correctness

Description

FetchRoute now requires fetchOptions.type in the request body, returning 400 if it's missing. This
breaks backward compatibility because the legacy fetch() implementation explicitly defaults a
missing fetchOptions.type to 'component'.

Code

scopes/scope/scope/routes/fetch.route.ts[R17-21]

+      fetchOptions: z
+        .object({
+          type: z.string(),
+        })
+        .passthrough(),

Evidence

The new schema makes fetchOptions.type mandatory, but the legacy fetch implementation explicitly
supports requests without fetchOptions.type by defaulting it for backward compatibility, so the
route-level validation will reject previously-supported clients/payloads before legacy handling
runs.

scopes/scope/scope/routes/fetch.route.ts[12-23]
components/legacy/scope-api/lib/fetch.ts[137-142]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`FetchRoute` validates request bodies with a Zod schema that requires `fetchOptions.type`. Older clients (and the legacy fetch implementation) allow `type` to be omitted, defaulting it to `'component'`, so this validation introduces a backward-compatibility breaking 400.
## Issue Context
The legacy implementation contains an explicit backward-compatibility default (`if (!fetchOptions.type) fetchOptions.type = 'component'`). The route-level schema should not reject this historically-valid shape.
## Fix Focus Areas
- scopes/scope/scope/routes/fetch.route.ts[12-23]
## Suggested fix
Update `fetchBodySchema` to accept `fetchOptions.type` as optional (or provide a default via Zod), while still requiring `fetchOptions` itself to be an object:
- Change `type: z.string()` to `type: z.string().optional()` (or `.default('component')`).
- Keep `.passthrough()` for cross-version compatibility.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Plain-text 400 response 🐞 Bug ≡ Correctness

Description

PutRoute now returns res.status(400).send(...) for missing/invalid push-options, which produces
a non-JSON error body. The Bit HTTP client’s throwForNonOkStatus() attempts res.json() and
ignores non-JSON bodies, so callers lose the specific server error message and may only see a
generic network error.

Code

scopes/scope/scope/routes/put.route.ts[R21-29]

+      if (!pushOptionsStr) {
+        return res.status(400).send('http is missing the push-options header');
+      }
+      let pushOptions;
+      try {
+        pushOptions = JSON.parse(pushOptionsStr as string);
+      } catch {
+        return res.status(400).send('the push-options header is not a valid JSON');
+      }

Evidence

PutRoute now emits plain-text 400s, but the Express error handler and other 4xx responses use JSONP;
meanwhile the Bit HTTP client attempts res.json() for non-OK responses and ignores the body if it
is not JSON. This combination causes the specific 400 message to be dropped client-side.

scopes/scope/scope/routes/put.route.ts[17-29]
scopes/harmony/express/middlewares/error.ts[14-29]
scopes/harmony/express/express.main.runtime.ts[143-152]
scopes/scope/network/http/http.ts[443-460]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`PutRoute` returns plain-text 400 responses via `res.send()`. This bypasses the shared Express error/response convention (`errorHandle()`/`verbValidation()` respond with JSON), and the Bit HTTP client drops non-JSON bodies for non-OK responses.
## Issue Context
The server already has a standard JSON error shape (`{ message, error }`). Keeping 400 responses in that shape preserves backwards compatibility for clients and makes the error message visible.
## Fix Focus Areas
- scopes/scope/scope/routes/put.route.ts[18-29]
## Suggested fix
Replace the `res.status(400).send(...)` responses with a JSON/JSONP response that matches the server convention, e.g.:
- `return res.status(400).jsonp({ message: '...', error: { status: 400, message: '...' } });`
Alternatively, throw an error object carrying `status = 400` (so `catchErrors()` routes it through `errorHandle()` which already formats JSON).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Zod loaded at bootstrap ✓ Resolved 🐞 Bug ➹ Performance

Description

Route modules now import z from zod at module top-level, so zod is still loaded during CLI
bootstrap when routes are registered, despite validateBody() documenting thunks as a way to keep
zod out of bootstrap. This can regress startup performance and makes comments like PutRoute’s “built
lazily … so zod stays out of the cli bootstrap” incorrect.

Code

scopes/scope/scope/routes/put.route.ts[R5-16]

+import { z } from 'zod';
import type { OnPostPutSlot, ScopeMain } from '../scope.main.runtime';
+// permissive on purpose - validate the fields we read, tolerate anything else for cross-version compatibility.
+// built lazily (thunk) so `zod` stays out of the cli bootstrap - see validateBody() in @teambit/express.
+const pushOptionsSchema = () =>
+  z
+    .object({
+      clientId: z.string().optional(),
+      persist: z.boolean().optional(),
+    })
+    .passthrough();

Evidence

The new validate middleware explicitly warns that building schemas at module top-level would eagerly
pull zod into CLI bootstrap, but the PR adds runtime import { z } from 'zod' in route modules
(example shown in PutRoute), which still loads zod during module evaluation even if schema objects
are created lazily.

scopes/harmony/express/middlewares/validate.ts[44-52]
scopes/scope/scope/routes/put.route.ts[1-16]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`validateBody()` documents a thunk-based approach to avoid pulling `zod` into CLI bootstrap, but the updated routes import `zod` at module top-level (e.g. `import { z } from 'zod'`), which defeats that goal and leaves misleading comments.
## Issue Context
Route modules are evaluated during CLI bootstrap to register routes. A top-level `import { z } from 'zod'` will load the zod module immediately during that evaluation.
## Fix Focus Areas
- scopes/harmony/express/middlewares/validate.ts[44-60]
- scopes/scope/scope/routes/put.route.ts[1-16]
- scopes/scope/scope/routes/fetch.route.ts[1-25]
- scopes/scope/scope/routes/delete.route.ts[1-14]
- scopes/scope/scope/routes/action.route.ts[1-16]
- scopes/lanes/lanes/lanes.create.route.ts[1-13]
- scopes/lanes/lanes/lanes.delete.route.ts[1-13]
- scopes/lanes/lanes/lanes.restore.route.ts[1-13]
- scopes/lanes/merge-lanes/lanes-check-conflicts.route.ts[1-14]
## What to change
Pick one of the two consistent directions:
1) **Actually keep zod out of bootstrap** (preferred if startup time matters):
- Update `validateBody()` to accept an **async** schema thunk (e.g. `() => Promise<ZodType>`), memoize the promise, and `await` it in the middleware.
- In each route module, remove the top-level `import { z } from 'zod'` and build the schema via `await import('zod')` inside the thunk.
2) **Accept eager zod import** (preferred if simplicity matters):
- Keep the top-level `zod` imports, but remove/adjust the misleading comments in `validateBody()` and the route modules so the code and docs match reality.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. validateBody error handling gap ✓ Resolved 🐞 Bug ☼ Reliability

Description

validateBody() throws HttpError from an async middleware without handling it locally, so it only
reliably produces a 400 when the middleware is installed via the route path that wraps handlers with
catchErrors. If validateBody() is ever used via middlewareSlot/app.use() (which ExpressMain
installs without catchErrors), validation failures bypass errorHandle() and can degrade into
default Express error responses or unhandled async errors.

Code

scopes/harmony/express/middlewares/validate.ts[R53-59]

+export function validateBody(schema: ZodType | (() => ZodType)): Middleware {
+  let resolved: ZodType | undefined;
+  return async (req, _res, next) => {
+    resolved ??= typeof schema === 'function' ? schema() : schema;
+    req.body = validateData(resolved, req.body, 'request body');
+    next();
+  };

Evidence

validateBody() calls validateData() (which throws HttpError) inside an async middleware and
then calls next(); it doesn’t write a 400 response itself. ExpressMain wraps only the route
middlewares with catchErrors, while middlewares from middlewareSlot are mounted directly with
app.use(middleware) and are not wrapped, so thrown/rejected async errors from such a middleware
won’t go through the project’s errorHandle() path.

scopes/harmony/express/middlewares/validate.ts[36-42]
scopes/harmony/express/middlewares/validate.ts[53-59]
scopes/harmony/express/express.main.runtime.ts[107-113]
scopes/harmony/express/express.main.runtime.ts[115-120]
scopes/harmony/express/express.main.runtime.ts[155-157]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`validateBody()` currently relies on the outer routing framework (`catchErrors`) to convert thrown `HttpError`s into HTTP responses. This works for route middlewares (wrapped by `catchErrors`), but ExpressMain installs `middlewareSlot` middlewares directly via `app.use()` without wrapping, so the same exported middleware can behave inconsistently depending on how it’s mounted.
### Issue Context
- `ExpressMain` wraps **route** middlewares with `catchErrors(...)`, but does **not** wrap `middlewareSlot` middlewares.
- `validateBody()` throws (via `validateData()`) and does not respond/call the shared error handler itself.
### Fix Focus Areas
- scopes/harmony/express/middlewares/validate.ts[53-59]
- scopes/harmony/express/express.main.runtime.ts[107-113]
- scopes/harmony/express/express.main.runtime.ts[155-157]
### Suggested fix
Update `validateBody()` to handle validation failures internally (e.g., `try/catch` around `validateData(...)`) and respond with status 400 directly (or call the shared `errorHandle()`), instead of relying on being wrapped by `catchErrors`.
This keeps behavior consistent whether the middleware is used in route middleware arrays or installed via `middlewareSlot`/`app.use()`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View more (3)

5. Zod types leak publicly ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

@teambit/express now publicly re-exports validateBody/validateData, and their TypeScript
signatures reference ZodType from zod, which can break TS consumers if zod is not resolvable
in their dependency graph. This is a public API surface change (even though runtime code uses only
structural safeParse calls).

Code

scopes/harmony/express/middlewares/validate.ts[R1-3]

+import type { ZodError, ZodType } from 'zod';
+import type { Middleware } from '../types';
+

Evidence

The new helpers are exported from the main @teambit/express entrypoint, and their declarations
depend on zod types (ZodType, ZodError), which become part of the public TypeScript API
surface.

scopes/harmony/express/index.ts[1-8]
scopes/harmony/express/middlewares/validate.ts[1-3]
scopes/harmony/express/middlewares/validate.ts[36-42]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`@teambit/express` exports `validateBody`/`validateData`, whose public TS types import and expose `ZodType`/`ZodError` from `zod`. This makes `zod` part of the public type surface and can cause downstream TypeScript builds to fail if `zod` is not present/resolvable.
### Issue Context
Runtime behavior is fine (the code only relies on `schema.safeParse()`), but the `.d.ts` surface will still reference `zod`.
### Fix options
Choose one:
1) **Make the dependency explicit**: ensure `zod` is declared appropriately for `teambit.harmony/express` (dependency or peerDependency) so TS consumers always resolve it.
2) **Avoid leaking `zod` types**: replace `ZodType`/`ZodError` in the public signatures with small structural types (e.g., a `SchemaLike` with `safeParse()` and an `ErrorLike` with `issues`) so consumers don’t need `zod` unless they choose to use it.
### Fix Focus Areas
- scopes/harmony/express/middlewares/validate.ts[1-60]
- scopes/harmony/express/index.ts[1-8]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

6. Missing ESM exports ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

validateBody, validateData, and HttpError are exported from scopes/harmony/express/index.ts
but are not re-exported from scopes/harmony/express/esm.mjs, so ESM consumers using named imports
may hit runtime “does not provide an export” errors for these new helpers.

Code

scopes/harmony/express/index.ts[5]

+export { validateBody, validateData, HttpError } from './middlewares';

Evidence

The PR adds the new exports to the main barrel (index.ts), but the ESM wrapper’s explicit named
exports list does not include them, so ESM named imports won’t resolve these symbols.

scopes/harmony/express/index.ts[1-8]
scopes/harmony/express/esm.mjs[1-7]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`@teambit/express` now exposes new runtime helpers (`validateBody`, `validateData`, `HttpError`) via `index.ts`, but the ESM wrapper (`esm.mjs`) still only re-exports `ExpressAspect` and `Verb`. This makes named ESM imports of the new helpers unavailable.
### Issue Context
The repo uses ESM wrapper files (`esm.mjs`) as the named-export surface for ESM consumers, so new runtime exports should be added there when introduced.
### Fix Focus Areas
- scopes/harmony/express/esm.mjs[1-7]
- scopes/harmony/express/index.ts[1-8]
### Proposed fix
In `scopes/harmony/express/esm.mjs`, add:
- `export const validateBody = cjsModule.validateBody;`
- `export const validateData = cjsModule.validateData;`
- `export const HttpError = cjsModule.HttpError;`

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

7. 4xx logged as errors ✓ Resolved 🐞 Bug ◔ Observability

Description

validateData() throws HttpError(400) for invalid input, which is handled by errorHandle() that
always logs via logger.error; this will record malformed client requests as server errors and can
add significant noise to error logs.

Code

scopes/harmony/express/middlewares/validate.ts[R36-41]

+export function validateData<T>(schema: ZodType<T>, data: unknown, label = 'request body'): T {
+  const result = schema.safeParse(data);
+  if (!result.success) {
+    throw new HttpError(`invalid ${label}: ${formatZodError(result.error)}`, 400);
+  }
+  return result.data;

Evidence

Zod validation failures are converted into HttpError(400) by validateData(); route middlewares are
wrapped with catchErrors(), which forwards rejected middleware promises into errorHandle().
errorHandle() logs every such error using logger.error, so 4xx validation errors will be logged as
errors.

scopes/harmony/express/middlewares/validate.ts[36-41]
scopes/harmony/express/middlewares/error.ts[9-12]
scopes/harmony/express/middlewares/error.ts[22-28]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Validation failures now throw `HttpError` with status `400`, but the shared `errorHandle()` logs *all* errors at error level. This causes client-side input mistakes to appear as server errors in logs, reducing signal for true 5xx failures.
## Issue Context
- `validateData()` throws `HttpError(..., 400)` when Zod parsing fails.
- `catchErrors()` routes promise rejections into `errorHandle()`.
- `errorHandle()` currently always calls `logger.error(...)` regardless of `err.status`.
## Fix Focus Areas
- scopes/harmony/express/middlewares/error.ts[14-28]
### Suggested fix
Update `errorHandle()` to log based on status code, e.g.:
- `status >= 500` -> `logger.error(...)`
- `status >= 400 && status < 500` -> `logger.warn(...)` (or `debug` if you expect frequent validation failures)
Optionally avoid dumping the full error object for 4xx to reduce log volume (log just `message`, `status`, and `url`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 891c351

[davidfirst]

@qodo Thanks — addressed the 4xx logged as errors finding in 987958b.

errorHandle() now branches on status code: 4xx (client errors, e.g. validation failures) are logged via logger.warn with a minimal payload (url + status + message), while genuine 5xx server failures keep the full logger.error dump. This prevents malformed client requests from adding noise to the error logs.

[qodo-free-for-open-source-projects]

Thanks — addressed the 4xx logged as errors finding in 987958b.

errorHandle() now branches on status code: 4xx (client errors, e.g. validation failures) are logged via logger.warn with a minimal payload (url + status + message), while genuine 5xx server failures keep the full logger.error dump. This prevents malformed client requests from adding noise to the error logs.

Looks good — thanks for the update.

• 4xx now stay out of the error logs, which should reduce noise from validation/client issues.
• Keeping the full logger.error path for 5xx preserves the useful debugging detail.

No further action needed from my side.

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 987958b

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 812084a

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 145bd35

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 8e23da2

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit ea5e648

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit d0347da

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 783dbd0

[Copilot]

Pull request overview

This PR improves robustness of the scope /scope/put HTTP route by avoiding an internal 500 when the push-options header is missing or malformed, instead returning a clear 400 response.

Changes:

• Return 400 when push-options header is missing.
• Catch JSON parsing errors for push-options and return 400 instead of throwing.

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit cc80f12