feat(network-config): add framework auto-detection and migrate config parsing to Effect services#268
Merged
Merged
Conversation
Add a detection module that automatically identifies web frameworks (13 supported: Next.js, Nuxt, NestJS, Remix, SvelteKit, Astro, Laravel, Symfony, Hono, Elysia, Fastify, Express, Vite) and generates deployment config defaults, eliminating the need for a gigadrive.yaml in common projects. - Language-aware dependency detection (package.json / composer.json) - Lockfile-based package manager detection - Priority-based framework matching with AND-logic detectors - Config merging (user config takes precedence over framework defaults) - Extract postProcessConfig() for Vercel Build Output v3 compatibility - Integrate auto-detection fallback into CLI ProjectConfigService - Fix condition order bug in empty-deployment validation - Fix typos in regions.ts and v4/index.ts Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ervices Replace raw node:fs I/O and plain Error throws with Effect-native layered services, typed tagged errors, and @effect/platform FileSystem abstraction. New services: RawConfigReader, SchemaValidator, V4ConfigParser, VercelBuildOutputParser — composed into NetworkConfigLive layer. CLI updated to consume services natively instead of wrapping in Effect.tryPromise. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: e40d279 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds an Effect-based framework auto-detection and config pipeline to packages/network-config, new detection APIs/types/framework definitions/tests, refactors parsing into Effect services and a NetworkConfigLive layer, updates CLI to consume detection results (framework metadata and excludeFiles), removes mock-fs dev deps and several legacy sync parsing modules. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(240,248,255,0.5)
participant Client as Client
participant PCS as ProjectConfigService
participant Raw as RawConfigReader
participant DF as detectFramework
participant DPM as detectPackageManager
participant GC as generateConfig
participant MC as mergeWithFrameworkDefaults
end
Client->>PCS: resolve(projectFolder)
PCS->>Raw: findConfig(projectFolder)
alt config found
Raw-->>PCS: configPath
PCS->>Raw: readRawConfig(configPath)
Raw-->>PCS: rawConfig
else no config
Raw-->>PCS: null
end
PCS->>DF: detectFramework(projectFolder)
alt framework detected
DF->>DPM: detectPackageManager(projectFolder)
DPM-->>DF: packageManager
DF->>GC: generateConfig(framework, packageManager)
GC-->>DF: frameworkConfig
DF-->>PCS: DetectionResult
else none
DF-->>PCS: FrameworkNotDetectedError
end
alt config + framework
PCS->>MC: mergeWithFrameworkDefaults(parsedConfig, frameworkConfig)
MC-->>PCS: mergedConfig
else framework only
PCS->>GC: postProcessConfig(frameworkConfig)
GC-->>PCS: processedConfig
end
PCS-->>Client: { config, configPath: string | null, framework? }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
♻️ Duplicate comments (3)
packages/network-config/src/detection/frameworks/nestjs.ts (1)
9-9: SamegetDefaultConfigsignature drift as already flagged.This file has the same pattern already called out in
packages/network-config/src/detection/frameworks/sveltekit.ts(Line 9).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/detection/frameworks/nestjs.ts` at line 9, The getDefaultConfig function here uses the inconsistent zero-arg signature; update the getDefaultConfig declaration in this file to match the canonical signature used by other framework detectors (same shape as packages/network-config/src/detection/frameworks/sveltekit.ts), i.e. change the function signature to accept the same parameters (options/context) and return the same typed config form; locate and edit the exported getDefaultConfig arrow function to accept the matching parameter list and propagate those parameters into its config computation so signatures are consistent across detectors.packages/network-config/src/detection/frameworks/remix.ts (1)
9-9: SamegetDefaultConfigsignature drift as already flagged.This file repeats the same arity mismatch pattern noted in
packages/network-config/src/detection/frameworks/sveltekit.ts(Line 9).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/detection/frameworks/remix.ts` at line 9, getDefaultConfig in this file has the wrong function signature (arity) compared to the expected detection API; update the getDefaultConfig declaration in remix.ts to match the same signature used in the other framework detectors (the one fixed in sveltekit.ts) — i.e., accept the same parameters (e.g., add the missing argument such as projectRoot or options) and return the same config shape, and ensure any calls or exported types referencing getDefaultConfig are updated to the new signature so types and consumers remain consistent.packages/network-config/src/detection/frameworks/nextjs.ts (1)
9-9: SamegetDefaultConfigsignature drift as already flagged.This repeats the same arity mismatch pattern noted earlier in
packages/network-config/src/detection/frameworks/sveltekit.ts(Line 9).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/detection/frameworks/nextjs.ts` at line 9, getDefaultConfig is declared with zero parameters (getDefaultConfig: () => {...}) but other framework detectors expect the same factory to accept a context/arity (as flagged in sveltekit.ts); change the signature of getDefaultConfig in nextjs.ts to match the expected arity (e.g., getDefaultConfig: (projectRoot?: string) => {...} or the shared context type used by other detectors) and update any internal references to use the passed parameter so the function signature is consistent across framework detectors.
🟡 Minor comments (4)
packages/network-config/src/detection/frameworks/sveltekit.ts-9-9 (1)
9-9:⚠️ Potential issue | 🟡 MinorAlign
getDefaultConfigparameter with the interface signature across all frameworks.The
FrameworkDefinitioninterface declaresgetDefaultConfig(packageManager: PackageManager), but this implementation omits the parameter. The caller ingenerate-config.tspassespackageManagerto every framework'sgetDefaultConfig, so the signature should accept it explicitly for consistency—even if unused, this clarifies intent.This pattern is consistent across all 13 framework definitions (astro, elysia, express, fastify, hono, laravel, nestjs, nextjs, nuxt, remix, symfony, sveltekit, vite). Update this one with
(_packageManager) => ({and apply the same fix to all other frameworks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/detection/frameworks/sveltekit.ts` at line 9, Update the sveltekit FrameworkDefinition implementation so its getDefaultConfig matches the interface signature by accepting the packageManager parameter; change the function to declare the parameter (e.g., _packageManager) even if unused, aligning with FrameworkDefinition and the caller in generate-config.ts, and apply the same change to the other framework definitions (astro, elysia, express, fastify, hono, laravel, nestjs, nextjs, nuxt, remix, symfony, vite) so all getDefaultConfig functions accept a PackageManager parameter.packages/network-config/src/parse-config.ts-17-23 (1)
17-23:⚠️ Potential issue | 🟡 MinorComplete exported JSDoc blocks with required
@returns/@exampletags.Both exported functions are missing required JSDoc tags (
parseConfigis missing@returnsand@example;postProcessConfigis missing@example).📝 Proposed JSDoc patch
/** * Parses a config file at the given path, validates it against the appropriate * JSON Schema, converts it to a NormalizedConfig, and applies post-processing. * * `@param` filePath - Absolute path to the config file * `@param` projectFolder - Absolute path to the project root + * `@returns` Effect that resolves to a post-processed NormalizedConfig + * `@example` + * await Effect.runPromise(parseConfig('/abs/project/gigadrive.yaml', '/abs/project')); */ export const parseConfig = Effect.fn('parseConfig')(function* (filePath: string, projectFolder: string) { ... }); /** * Applies post-processing transformers to a NormalizedConfig: * - Vercel Build Output v3 (merges functions/routes from .vercel/output if present) * - Empty deployment validation * - Filter functions from assets * * `@param` config - The parsed NormalizedConfig * `@param` projectFolder - Absolute path to the project root * `@returns` The post-processed NormalizedConfig + * `@example` + * await Effect.runPromise(postProcessConfig(config, '/abs/project')); */ export const postProcessConfig = Effect.fn('postProcessConfig')(function* (As per coding guidelines,
Document exported functions with JSDoc comments including@param,@returns, and@exampletags.Also applies to: 55-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/parse-config.ts` around lines 17 - 23, Add missing JSDoc tags to the exported functions parseConfig and postProcessConfig: update the comment for parseConfig to include an `@returns` describing the Promise<NormalizedConfig> (or the actual return type) and an `@example` showing typical usage with filePath and projectFolder, and update postProcessConfig to include an `@example` demonstrating input and returned normalized config; ensure both JSDoc blocks keep existing `@param` entries, accurately describe types and thrown errors (if any), and follow the project's JSDoc style conventions so the exported functions are fully documented.packages/network-config/src/detection/ADDING_FRAMEWORKS.md-9-18 (1)
9-18:⚠️ Potential issue | 🟡 MinorSpecify a language for the fenced code block.
Line 9 opens a fenced block without a language, which triggers markdownlint MD040.
📝 Proposed fix
-``` +```text packages/network-config/src/detection/ frameworks/ index.ts ← barrel: register your framework here nextjs.ts ← example: meta-framework (high priority) elysia.ts ← example: minimal server (low priority) laravel.ts ← example: PHP framework (multi-detector) <your-framework>.ts ← you add this types.ts ← FrameworkDefinition interface</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@packages/network-config/src/detection/ADDING_FRAMEWORKS.mdaround lines 9 -
18, The fenced code block in ADDING_FRAMEWORKS.md is missing a language
specifier which triggers markdownlint MD040; update the opening fence to include
a language (e.g., changetotext) for the block that lists the
packages/network-config/src/detection/ frameworks so the linter recognizes it as
a fenced code block with language; ensure only the opening fence is changed and
the content of the block (the list with index.ts, nextjs.ts, elysia.ts,
laravel.ts, .ts, types.ts) remains unchanged.</details> </blockquote></details> <details> <summary>packages/network-config/src/detection/detect-framework.ts-78-78 (1)</summary><blockquote> `78-78`: _⚠️ Potential issue_ | _🟡 Minor_ **Validate `projectFolder` before starting detection.** Line 78 accepts empty/whitespace paths without a guard, which can lead to probing unintended paths and confusing failures. Add an early input guard. <details> <summary>🛡️ Suggested change</summary> ```diff export const detectFramework = Effect.fn('detectFramework')(function* (projectFolder: string) { + if (projectFolder.trim().length === 0) { + return yield* Effect.fail(new Error('projectFolder must be a non-empty path')); + } + yield* Effect.logDebug(`Detecting framework in ${projectFolder}`); ``` </details> As per coding guidelines, "Use guard clauses with early `throw new Error(...)` for invalid inputs". <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/detection/detect-framework.ts` at line 78, The detectFramework generator (export const detectFramework = Effect.fn('detectFramework')(function* (projectFolder: string) { ... }) ) lacks validation for projectFolder, allowing empty or whitespace-only paths; add an early guard clause at the top of that function which trims projectFolder and throws a new Error(...) if it's empty/undefined/only whitespace before any detection logic runs so the function fails-fast on invalid input. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (12)</summary><blockquote> <details> <summary>packages/network-config/src/v4/parse.test.ts (3)</summary><blockquote> `93-119`: **Prefer a typed cast over `config as any`.** `readFixture` returns `Record<string, unknown>`, which is incompatible with `getFunctionSettings`'s expected parameter type. Casting to `any` silently disables all type checks on the argument, meaning future signature changes to `getFunctionSettings` won't be caught here. Cast to the actual config type (e.g., `config as V4Config` or whatever the parameter type of `getFunctionSettings` is) to preserve compile-time safety. <details> <summary>♻️ Suggested approach</summary> Either narrow the return type of `readFixture` for YAML configs: ```diff -async function readFixture(filePath: string): Promise<Record<string, unknown>> { +async function readFixture(filePath: string): Promise<V4Config> { ``` Or use the precise cast at call sites instead of `as any`: ```diff -expect(getFunctionSettings('src/index.ts', config as any)).toEqual({ +expect(getFunctionSettings('src/index.ts', config as V4Config)).toEqual({ ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/v4/parse.test.ts` around lines 93 - 119, The tests currently cast the parsed config with "config as any", which disables type checks; instead cast to the concrete config type expected by getFunctionSettings (e.g., config as V4Config) or adjust readFixture to return Record<string, V4Config> so the calls like getFunctionSettings('src/index.ts', config as V4Config) are correctly typed; update the four calls that pass config to getFunctionSettings to use the precise cast and import/alias the V4Config type where needed. ``` </details> --- `32-33`: **Inconsistent fixture loading: use `readFixture` for the schema file too.** Line 30 reads `example.yaml` through the new `readFixture` helper, but lines 32–33 still use `fs.readFileSync` + `JSON.parse` directly for `schema.json`. Since `readFixture` handles JSON already (line 20–22), it can replace this inline block for consistency. <details> <summary>♻️ Proposed refactor</summary> ```diff - const schemaFile = fs.readFileSync(path.join(__dirname, 'schema.json'), 'utf8'); - const schema = JSON.parse(schemaFile); + const schema = await readFixture(path.join(__dirname, 'schema.json')); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/v4/parse.test.ts` around lines 32 - 33, Replace the inline fs.readFileSync + JSON.parse block that builds the schema (currently producing schema from schemaFile) with the existing readFixture helper; i.e., assign const schema = readFixture('schema.json') so the test uses the same fixture loader as the example.yaml path and benefits from readFixture's JSON parsing behavior (ensure readFixture is in scope where schema is created). ``` </details> --- `17-25`: **`readFileSync` blocks the event loop inside an async function; prefer `fs.promises.readFile`.** The function is async solely because of `await import('yaml')` on the YAML branch. The `readFileSync` call on line 18 still blocks the Node.js event loop for every invocation. While this is tolerable in tests, it's inconsistent with the function's async signature. <details> <summary>♻️ Proposed refactor</summary> ```diff async function readFixture(filePath: string): Promise<Record<string, unknown>> { - const content = fs.readFileSync(filePath, 'utf8'); + const content = await fs.promises.readFile(filePath, 'utf8'); const ext = path.extname(filePath).toLowerCase(); if (ext === '.json') { return JSON.parse(content) as Record<string, unknown>; } const { parse: parseYaml } = await import('yaml'); return parseYaml(content) as Record<string, unknown>; } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/v4/parse.test.ts` around lines 17 - 25, readFixture currently uses blocking fs.readFileSync inside an async function; switch to the promise-based API so the function is fully non-blocking. Replace the synchronous read with an awaited read from fs.promises (e.g., const content = await fs.promises.readFile(filePath, 'utf8')) in the readFixture function and keep the dynamic await import('yaml') branch unchanged; ensure types remain Record<string, unknown> and behavior for JSON and YAML parsing is preserved. ``` </details> </blockquote></details> <details> <summary>packages/network-config/src/services/v4-config-parser.test.ts (1)</summary><blockquote> `39-52`: **Add parse-level assertions for entrypoint settings to prevent overlap regressions.** Current parse test only checks regions and commands. It should also assert that parsed entrypoints preserve runtime/schedule/symlink behavior for specific files. <details> <summary>🧪 Suggested assertion extension</summary> ```diff expect(result.regions).toEqual(AVAILABLE_REGIONS); expect(result.commands).toEqual(['bun install']); + expect(result.entrypoints).toEqual( + expect.arrayContaining([ + expect.objectContaining({ path: 'src/index.ts', runtime: 'bun-1', memory: 128, maxDuration: 15 }), + expect.objectContaining({ path: 'src/cron.ts', schedule: 'rate(5 minutes)' }), + ]) + ); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/services/v4-config-parser.test.ts` around lines 39 - 52, The test for V4ConfigParser.parse only asserts regions and commands but misses entrypoint-level assertions; update the test (in v4-config-parser.test.ts where loadExample(), V4ConfigParser.parse and result are used) to also assert that result.entrypoints (or the parsed entrypoint structure returned by parse) contains the expected entries and that each targeted file preserves its runtime, schedule and symlink flags (e.g., check runtime === expectedRuntime, schedule === expectedSchedule, symlink === expectedSymlink for the specific entrypoint keys from the example ConfigV4) to prevent regressions in entrypoint parsing. ``` </details> </blockquote></details> <details> <summary>packages/network-config/src/services/schema-validator.ts (1)</summary><blockquote> `20-23`: **`new Ajv()` and `addFormats` instantiated inside `validate` — hoisting to service scope avoids repeated allocation** A new `Ajv` instance (and `addFormats` registration) is created on every `validate` call. This is wasteful — `Ajv` is designed to be instantiated once and reused. Move the instantiation into the `Effect.gen` body so it is created once at service initialization. <details> <summary>♻️ Proposed refactor</summary> ```diff export class SchemaValidator extends Effect.Service<SchemaValidator>()('SchemaValidator', { effect: Effect.gen(function* () { + const ajv = new Ajv(); + addFormats(ajv); + const validate = Effect.fn('SchemaValidator.validate')(function* ( config: Record<string, unknown>, schema: object, filePath: string ) { - const ajv = new Ajv(); - addFormats(ajv); - const compiledValidate = ajv.compile(schema); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/services/schema-validator.ts` around lines 20 - 23, The code currently creates a new Ajv instance and calls addFormats on every invocation of validate; move Ajv instantiation and addFormats registration out of the validate function and into the service initialization inside the Effect.gen body so a single Ajv instance is reused. Specifically, create ajv = new Ajv() and call addFormats(ajv) once at the top-level of the Effect.gen block that constructs the validator service, then in the validate function use ajv.compile(schema) (or cache compiled validators) instead of new Ajv()/addFormats per call; reference symbols: Ajv, addFormats, validate, Effect.gen, and compiledValidate to locate and change the allocation site. ``` </details> </blockquote></details> <details> <summary>packages/network-config/src/detection/generate-config.ts (1)</summary><blockquote> `25-32`: **Add an `@example` section to the exported function JSDoc.** The `generateConfig` JSDoc is missing an example usage block. <details> <summary>🧾 Suggested doc update</summary> ```diff /** * Generates a NormalizedConfig from a framework definition and detected package manager. * Prepends the install command and builds a complete deployment configuration. * * `@param` framework - The detected framework definition * `@param` packageManager - The detected package manager * `@returns` A complete NormalizedConfig with framework defaults + * `@example` + * const config = await Effect.runPromise(generateConfig(nextjs, 'pnpm')); */ ``` </details> As per coding guidelines, "Document exported functions with JSDoc comments including `@param`, `@returns`, and `@example` tags". <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/detection/generate-config.ts` around lines 25 - 32, Add an `@example` block to the JSDoc for the exported function generateConfig showing how to call it and what it returns; update the doc above the generateConfig declaration to include a short usage example (e.g., importing or referencing a FrameworkDefinition, passing a packageManager like "npm" or "yarn", calling generateConfig(framework, packageManager), and a brief example of the returned NormalizedConfig shape) so the JSDoc contains `@param`, `@returns`, and `@example` tags for quick reference. ``` </details> </blockquote></details> <details> <summary>packages/network-config/src/detection/detect-package-manager.ts (1)</summary><blockquote> `28-28`: **Use `path.join` for consistent path construction.** This uses string interpolation (`\`${projectFolder}/${file}\``) while `raw-config-reader.ts` uses `path.join(projectFolder, name)` for the same operation. Using `path.join` is more robust (handles trailing slashes, etc.) and keeps the codebase consistent. <details> <summary>♻️ Suggested fix</summary> ```diff import { FileSystem } from '@effect/platform'; import { Effect } from 'effect'; +import path from 'node:path'; import type { PackageManager } from './types'; ``` ```diff - const exists = yield* fs.exists(`${projectFolder}/${file}`).pipe(Effect.catchAll(() => Effect.succeed(false))); + const exists = yield* fs.exists(path.join(projectFolder, file)).pipe(Effect.catchAll(() => Effect.succeed(false))); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/detection/detect-package-manager.ts` at line 28, The file uses string interpolation when calling fs.exists in detect-package-manager.ts (const exists = yield* fs.exists(`${projectFolder}/${file}`)...); change this to use path.join(projectFolder, file) for robust path construction, update the import to include Node's path module if not already imported, and ensure the same pattern as raw-config-reader.ts is followed so trailing slashes and platform separators are handled consistently. ``` </details> </blockquote></details> <details> <summary>packages/network-config/src/services/raw-config-reader.ts (2)</summary><blockquote> `64-72`: **Unreachable fallback branch for unknown file extensions.** The `else` branch (lines 70-71) silently falls back to YAML parsing for unknown extensions, but `ALLOWED_CONFIG_NAMES` only contains `.yaml`, `.yml`, and `.json` files — so this branch is dead code. If a non-standard extension somehow reaches this code in the future, silently treating it as YAML could mask bugs. Consider either removing the branch and failing explicitly, or adding a comment explaining the intentional fallback. <details> <summary>♻️ Option: fail explicitly on unexpected extensions</summary> ```diff if (fileExtension === '.json') { parsed = JSON.parse(fileContents) as Record<string, unknown>; } else if (['.yml', '.yaml'].includes(fileExtension)) { parsed = parseYaml(fileContents) as Record<string, unknown>; } else { - parsed = parseYaml(fileContents) as Record<string, unknown>; + return yield* Effect.fail( + new ConfigFileParseError({ + message: `Unsupported config file extension "${fileExtension}" at ${filePath}`, + filePath, + }) + ); } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/services/raw-config-reader.ts` around lines 64 - 72, The current parsing block in raw-config-reader.ts silently falls back to parseYaml for any unknown fileExtension (dead code behind ALLOWED_CONFIG_NAMES); change the final else branch to explicitly throw a descriptive error instead of calling parseYaml — reference the local variables fileExtension and parsed and the functions JSON.parse and parseYaml (and the ALLOWED_CONFIG_NAMES constant) so the error message can state the unexpected extension and list allowed extensions; ensure callers of the function will surface the thrown error (or adjust signature) rather than silently accepting YAML parsing for unknown types. ``` </details> --- `7-7`: **Consider `as const` for `ALLOWED_CONFIG_NAMES`.** Marking this array `as const` would provide a narrower tuple type and enable deriving a union type of allowed filenames if needed downstream. ```diff -export const ALLOWED_CONFIG_NAMES = ['gigadrive.yaml', 'gigadrive.yml', 'nebula.yaml', 'nebula.yml', 'nebula.json']; +export const ALLOWED_CONFIG_NAMES = ['gigadrive.yaml', 'gigadrive.yml', 'nebula.yaml', 'nebula.yml', 'nebula.json'] as const; ``` As per coding guidelines: "Prefer `as const` assertions for literal arrays that derive union types (e.g., AVAILABLE_REGIONS, AVAILABLE_RUNTIMES)." <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/services/raw-config-reader.ts` at line 7, ALLOWED_CONFIG_NAMES is currently a mutable string array; change its declaration to a readonly tuple by appending as const so the type becomes a literal tuple and you can derive a union of allowed filenames downstream (update any code that relied on it being string[] to accept readonly string[] or derive a union via typeof ALLOWED_CONFIG_NAMES[number]); locate ALLOWED_CONFIG_NAMES in raw-config-reader.ts and replace its declaration with the as const assertion and adjust any type usages accordingly. ``` </details> </blockquote></details> <details> <summary>packages/network-config/src/detection/detect-framework.test.ts (1)</summary><blockquote> `9-14`: **Duplicate `makeTestFs` — prefer the shared helper from `test-utils.ts`.** A richer `makeTestFs` already exists in `packages/network-config/src/test-utils.ts` with directory-existence handling, `readDirectory`, and `stat` support. This local copy only stubs `exists` and `readFileString`, which means any future detection logic touching `stat` or `readDirectory` will silently receive `undefined` methods, failing at runtime rather than at compile time (because of the `as unknown as` cast). <details> <summary>♻️ Suggested fix</summary> ```diff -import { FileSystem } from '@effect/platform'; -import { Effect, Layer } from 'effect'; +import { Effect } from 'effect'; import { describe, expect, it } from 'vitest'; import { detectFramework } from './detect-framework'; +import { makeTestFs } from '../test-utils'; - -/** - * Creates a test FileSystem layer with in-memory files and existence checks. - */ -const makeTestFs = (files: Record<string, string>) => - Layer.succeed(FileSystem.FileSystem, { - exists: (path: string) => Effect.succeed(path in files), - readFileString: (path: string) => - path in files ? Effect.succeed(files[path]) : Effect.fail(new Error(`File not found: ${path}`)), - } as unknown as FileSystem.FileSystem); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/detection/detect-framework.test.ts` around lines 9 - 14, The local duplicate makeTestFs in detect-framework.test.ts should be removed and replaced by the shared helper from packages/network-config/src/test-utils.ts; import the consolidated makeTestFs (the version that implements exists, readFileString, readDirectory, and stat) and use it in this test so detection logic gets full FS behavior instead of the stubbed two-method implementation (avoid casting with as unknown as FileSystem.FileSystem and rely on the typed helper from test-utils.ts). ``` </details> </blockquote></details> <details> <summary>packages/cli/src/services/project-config.test.ts (1)</summary><blockquote> `43-49`: **Consider adding a type-safe stub instead of `as any`.** The `StubRawConfigReader` bypasses type safety with `as any`. A narrower approach would be to type-assert only the missing methods, making it easier to catch breakage if the `RawConfigReader` interface changes. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/services/project-config.test.ts` around lines 43 - 49, Replace the unsafe "as any" cast on the StubRawConfigReader with a type-safe stub that matches RawConfigReader's shape: create a stub object typed as Partial<RawConfigReader> (e.g. const rawConfigReaderStub: Partial<RawConfigReader> = { findConfig: () => Effect.succeed(findConfigResult), readRawConfig: () => Effect.succeed({}), readConfigFile: () => Effect.succeed(null) }) and then use either a narrow cast (rawConfigReaderStub as RawConfigReader) or, better, use TypeScript's "satisfies RawConfigReader" on the object literal to ensure missing/renamed methods are caught; then pass that typed object into Layer.succeed(RawConfigReader, ... ) instead of using "as any". ``` </details> </blockquote></details> <details> <summary>packages/network-config/src/detection/detect-framework.ts (1)</summary><blockquote> `68-77`: **Add an `@example` block to exported function docs.** The exported `detectFramework` JSDoc is missing `@example`. As per coding guidelines, "Document exported functions with JSDoc comments including `@param`, `@returns`, and `@example` tags". <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/detection/detect-framework.ts` around lines 68 - 77, Add an `@example` tag to the JSDoc for the exported detectFramework function to illustrate typical usage: show calling detectFramework with a projectFolder string (absolute or relative), awaiting the promise, and inspecting the returned DetectionResult (framework, packageManager, generated config). Update the comment block above the detectFramework export in detect-framework.ts to include a short runnable example (e.g., async call within an IIFE or top-level await) that demonstrates input and how to read the returned object's properties. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@packages/cli/src/index.ts:
- Around line 39-48: ProjectConfigService currently requires RawConfigReader but
its dependency is being provided at the usage site; modify the service
definition (ProjectConfigService) to declare NetworkConfig as a dependency (add
a dependencies field that includes NetworkConfig/RawConfigReader binding) so the
required RawConfigReader/NetworkConfig is owned by ProjectConfigService.Default;
then update index.ts to use ProjectConfigService.Default directly (remove
Layer.provide(ProjectConfigService.Default, NetworkConfigLive)) and remove the
duplicate NetworkConfigLive from the Layer.mergeAll call that builds
BaseServices to avoid redundant composition.In
@packages/cli/src/services/project-config.ts:
- Around line 79-88: postProcessConfig is being wrapped with Effect.catchAll
which violates the guideline; replace the generic catchAll on the pipeline that
calls postProcessConfig(detection.config, cwd) with a targeted Effect.catchTag
or Effect.catchTags that handles the specific error tag(s) that
postProcessConfig can emit, and only map those to a new ConfigParseError
(preserving the original error/cause); leave other unexpected errors unhandled
(or re-throw) so they aren’t swallowed. Ensure you update the pipeline where
config is assigned (the result of postProcessConfig(detection.config,
cwd).pipe(...)) and reference the same ConfigParseError construction used today
when mapping the specific tag(s).- Around line 49-58: The current use of Effect.catchAll around parseConfig
flattens distinct parse errors and violates the CLI guideline; replace the
catchAll usage in the parseConfig block (and the identical block later) with
Effect.catchTags/Effect.catchTag that match the specific error tags
(ConfigFileNotFoundError, ConfigFileParseError, ConfigVersionError,
ConfigSchemaValidationError) and map each to an appropriate ConfigParseError
while preserving the original error as the cause, and extract the repeated logic
into a shared helper (e.g., parseConfigWithDetailedErrors or wrapParseConfig)
that calls parseConfig(configPath, cwd) and applies the catchTags-based mapping
so both sites reuse the helper.- Around line 19-20: ProjectConfigService is using RawConfigReader via yield*
RawConfigReader inside its effect but RawConfigReader is not declared in the
service definition; update the ProjectConfigService declaration to include
dependencies: [RawConfigReader.Default] (mirroring AuthService's pattern) so
RawConfigReader is provided as an explicit dependency, keeping the effect
(effect: Effect.gen(function* () { const rawReader = yield* RawConfigReader; ...
})) unchanged.In
@packages/network-config/src/detection/frameworks/astro.ts:
- Around line 9-20: The getDefaultConfig object currently sets commands: ['astro
build'] which omits installing dependencies; update getDefaultConfig to prepend
the appropriate install step using the packageManager parameter (e.g. determine
the install command from packageManager and insert it before 'astro build' in
the commands array) so that dependencies (including astro) are installed prior
to running the build; modify the configuration construction that returns
commands to use packageManager to generate the full commands array.In
@packages/network-config/src/detection/frameworks/express.ts:
- Around line 9-18: getDefaultConfig currently sets commands: [] which skips
dependency installation; update getDefaultConfig in
packages/network-config/src/detection/frameworks/express.ts to add an install
step driven by the existing packageManager logic (e.g., inject the
packageManager's install command into the commands array, and optionally a build
step if a build script is present) so that node_modules are installed during
deployment; ensure you reference getDefaultConfig and the commands property when
adding the install command and preserve existing streaming/entrypoint values.In
@packages/network-config/src/detection/frameworks/laravel.ts:
- Line 6: The language field in the Laravel framework descriptor is set
incorrectly to 'node'; update the language property in
packages/network-config/src/detection/frameworks/laravel.ts to 'php' so it
aligns with runtime: 'php-84' and the PHP detectors ('laravel/framework' and
'artisan') and ensures detectFramework reads composer.json (similar to
symfony.ts); locate and change the language: 'node' entry inside the exported
framework descriptor for Laravel to language: 'php'.- Line 14: The build commands in getDefaultConfig currently hardcode "composer
require bref/laravel-bridge --update-with-dependencies" (which mutates
composer.lock) and hardcode Bun steps; change getDefaultConfig to accept a
packageManager parameter (getDefaultConfig: (packageManager) => ({...})), remove
the composer require from the commands array so bref/laravel-bridge is not
injected at deploy time, and branch the install/build commands using the
packageManager value (e.g., npm/yarn/pnpm vs bun) to construct the appropriate
install and build commands instead of always using 'bun install'/'bun run
build'; update the commands array reference in the returned config and mirror
the pattern used in symfony.ts for packageManager handling.In
@packages/network-config/src/detection/frameworks/nuxt.ts:
- Around line 9-20: getDefaultConfig currently ignores the packageManager
parameter and only runs 'nuxt build', which will fail without installing deps;
update the getDefaultConfig implementation in
packages/network-config/src/detection/frameworks/nuxt.ts (the getDefaultConfig
function) to accept and use the packageManager argument and prepend the
appropriate install command (map packageManager values to 'npm install', 'yarn',
'pnpm install', or 'bun install') to the commands array before 'nuxt build',
ensuring the install step is first and preserving existing entries like 'nuxt
build'.In
@packages/network-config/src/detection/frameworks/symfony.ts:
- Around line 21-26: The default APP_SECRET value in the environmentVariables
object is a known public placeholder and must not be shipped; update
detection/frameworks/symfony.ts so the environmentVariables map does not include
a hard-coded APP_SECRET key (omit APP_SECRET entirely) or replace it with an
explicit sentinel that forces users to supply a value at deploy time, and ensure
any code that merges defaults (the environmentVariables constant/object) will
fail or warn if APP_SECRET is missing; specifically modify the
environmentVariables export (remove the APP_SECRET entry) and add a clear
runtime validation/warning where defaults are consumed to require the user to
set APP_SECRET.- Line 10: The default Symfony runtime value currently uses the unsupported
string 'php-84'; update that runtime value to 'php-83' so generated Symfony
configs validate against the V4 schema. Locate the runtime field (runtime:
'php-84') in the Symfony framework definition in frameworks/symfony.ts and
replace the literal 'php-84' with 'php-83' (preserving surrounding formatting
and any related export/const names).- Line 14: The commands array in the Symfony framework definition currently
hardcodes "bun install" and "bun run build", ignoring the provided
packageManager argument from the FrameworkDefinition; update the commands
construction to use the packageManager variable (e.g.,${packageManager} installand${packageManager} run buildor a small mapping that chooses the
appropriate install/build commands per package manager) so the install/build
steps follow the detected package manager; apply the same change to the
analogous commands array in laravel.ts.In
@packages/network-config/src/detection/generate-config.ts:
- Around line 61-67: The routes mapping always forces handler:
'SERVERLESS_FUNCTION', so when the configuration enables streaming those routes
are not emitted as streaming; update the routes creation in generate-config.ts
(the routes: defaults.routes.map(...) block) to choose the handler based on the
streaming flag (e.g., use handler: defaults.streaming ? 'STREAMING_FUNCTION' as
const : 'SERVERLESS_FUNCTION' as const) so frameworks with streaming: true emit
streaming route handlers.In
@packages/network-config/src/detection/read-dependencies.ts:
- Line 57: The current catch wipes every ManifestReadError (readManifest ->
ManifestReadError) and returns null, hiding parse/I/O failures; change the error
handling so only the "manifest not found" variant is swallowed: replace the
broad Effect.catchTag('ManifestReadError', ...) with a targeted handler that
catches the specific not-found variant (e.g., 'ManifestNotFound' or whatever tag
your ManifestReadError uses) and returns null, while allowing other
ManifestReadError variants (parse errors, I/O errors) to propagate (or re-throw
with a descriptive message) so they surface to the caller instead of being
silently ignored.In
@packages/network-config/src/services/v4-config-parser.ts:
- Around line 95-133: The code currently calls getFunctionSettings(fnPath,
config) once per pattern key, causing broad patterns to apply the wrong settings
to matched files; move the settings lookup into the matchedFiles loop and call
getFunctionSettings(file, config) so runtime/memory/max_duration/streaming are
resolved per actual file path, while still using func.schedule and func.symlinks
from the pattern; update any references to settings in the entrypoints.push
(runtime, memory, maxDuration, streaming) to use the per-file settings variable
(and keep the existing FunctionConfigError behavior for resolution failures).- Around line 24-25: In getFunctionSettings, avoid unguarded dynamic RegExp
construction from user-supplied keys: validate/escape or sandbox the pattern
before calling new RegExp(key) (or prefer minimatch), and wrap the RegExp
creation/test in a try-catch that skips invalid patterns; additionally enforce
simple safeguards (e.g., max pattern length) to reduce ReDoS risk—update the
loop that iterates Object.entries(config.functions ?? {}) (and the line using
minimatch(path, key) || new RegExp(key).test(path)) to first try minimatch, then
safely attempt RegExp construction inside try-catch or use an escaped/validated
pattern, and skip or log patterns that throw SyntaxError.In
@packages/network-config/src/services/vercel-build-output-parser.ts:
- Around line 65-69: The code currently overwrites result.regions with
parseResult.regions each iteration, dropping regions from earlier functions;
change the logic that assigns to result.regions (where result:
Partial and parseResult is the per-function parse result) to
append/merge the new regions instead of replacing them (e.g., concat or push the
parseResult.regions array, guarding for undefined) so all functions' regions
accumulate rather than being rebuilt per iteration.- Around line 125-160: The route destination fields are using just the handler
filename which can collide; update the destination in both the defaultRoute and
the mapped config.routes (the objects created where destination: handler is set)
to use the same full entrypoint path you pushed into result.entrypoints (the
path produced by path.join(functionDirectory, handler)). In other words, replace
the simple handler reference with the full entrypoint path (the same value used
when constructing result.entrypoints) so defaultRoute and the objects inside
configRoutes reference the identical path.
Minor comments:
In@packages/network-config/src/detection/ADDING_FRAMEWORKS.md:
- Around line 9-18: The fenced code block in ADDING_FRAMEWORKS.md is missing a
language specifier which triggers markdownlint MD040; update the opening fence
to include a language (e.g., changetotext) for the block that lists the
packages/network-config/src/detection/ frameworks so the linter recognizes it as
a fenced code block with language; ensure only the opening fence is changed and
the content of the block (the list with index.ts, nextjs.ts, elysia.ts,
laravel.ts, .ts, types.ts) remains unchanged.In
@packages/network-config/src/detection/detect-framework.ts:
- Line 78: The detectFramework generator (export const detectFramework =
Effect.fn('detectFramework')(function* (projectFolder: string) { ... }) ) lacks
validation for projectFolder, allowing empty or whitespace-only paths; add an
early guard clause at the top of that function which trims projectFolder and
throws a new Error(...) if it's empty/undefined/only whitespace before any
detection logic runs so the function fails-fast on invalid input.In
@packages/network-config/src/detection/frameworks/sveltekit.ts:
- Line 9: Update the sveltekit FrameworkDefinition implementation so its
getDefaultConfig matches the interface signature by accepting the packageManager
parameter; change the function to declare the parameter (e.g., _packageManager)
even if unused, aligning with FrameworkDefinition and the caller in
generate-config.ts, and apply the same change to the other framework definitions
(astro, elysia, express, fastify, hono, laravel, nestjs, nextjs, nuxt, remix,
symfony, vite) so all getDefaultConfig functions accept a PackageManager
parameter.In
@packages/network-config/src/parse-config.ts:
- Around line 17-23: Add missing JSDoc tags to the exported functions
parseConfig and postProcessConfig: update the comment for parseConfig to include
an@returnsdescribing the Promise (or the actual return type)
and an@exampleshowing typical usage with filePath and projectFolder, and
update postProcessConfig to include an@exampledemonstrating input and returned
normalized config; ensure both JSDoc blocks keep existing@paramentries,
accurately describe types and thrown errors (if any), and follow the project's
JSDoc style conventions so the exported functions are fully documented.
Duplicate comments:
In@packages/network-config/src/detection/frameworks/nestjs.ts:
- Line 9: The getDefaultConfig function here uses the inconsistent zero-arg
signature; update the getDefaultConfig declaration in this file to match the
canonical signature used by other framework detectors (same shape as
packages/network-config/src/detection/frameworks/sveltekit.ts), i.e. change the
function signature to accept the same parameters (options/context) and return
the same typed config form; locate and edit the exported getDefaultConfig arrow
function to accept the matching parameter list and propagate those parameters
into its config computation so signatures are consistent across detectors.In
@packages/network-config/src/detection/frameworks/nextjs.ts:
- Line 9: getDefaultConfig is declared with zero parameters (getDefaultConfig:
() => {...}) but other framework detectors expect the same factory to accept a
context/arity (as flagged in sveltekit.ts); change the signature of
getDefaultConfig in nextjs.ts to match the expected arity (e.g.,
getDefaultConfig: (projectRoot?: string) => {...} or the shared context type
used by other detectors) and update any internal references to use the passed
parameter so the function signature is consistent across framework detectors.In
@packages/network-config/src/detection/frameworks/remix.ts:
- Line 9: getDefaultConfig in this file has the wrong function signature (arity)
compared to the expected detection API; update the getDefaultConfig declaration
in remix.ts to match the same signature used in the other framework detectors
(the one fixed in sveltekit.ts) — i.e., accept the same parameters (e.g., add
the missing argument such as projectRoot or options) and return the same config
shape, and ensure any calls or exported types referencing getDefaultConfig are
updated to the new signature so types and consumers remain consistent.
Nitpick comments:
In@packages/cli/src/services/project-config.test.ts:
- Around line 43-49: Replace the unsafe "as any" cast on the StubRawConfigReader
with a type-safe stub that matches RawConfigReader's shape: create a stub object
typed as Partial (e.g. const rawConfigReaderStub:
Partial = { findConfig: () => Effect.succeed(findConfigResult),
readRawConfig: () => Effect.succeed({}), readConfigFile: () =>
Effect.succeed(null) }) and then use either a narrow cast (rawConfigReaderStub
as RawConfigReader) or, better, use TypeScript's "satisfies RawConfigReader" on
the object literal to ensure missing/renamed methods are caught; then pass that
typed object into Layer.succeed(RawConfigReader, ... ) instead of using "as
any".In
@packages/network-config/src/detection/detect-framework.test.ts:
- Around line 9-14: The local duplicate makeTestFs in detect-framework.test.ts
should be removed and replaced by the shared helper from
packages/network-config/src/test-utils.ts; import the consolidated makeTestFs
(the version that implements exists, readFileString, readDirectory, and stat)
and use it in this test so detection logic gets full FS behavior instead of the
stubbed two-method implementation (avoid casting with as unknown as
FileSystem.FileSystem and rely on the typed helper from test-utils.ts).In
@packages/network-config/src/detection/detect-framework.ts:
- Around line 68-77: Add an
@exampletag to the JSDoc for the exported
detectFramework function to illustrate typical usage: show calling
detectFramework with a projectFolder string (absolute or relative), awaiting the
promise, and inspecting the returned DetectionResult (framework, packageManager,
generated config). Update the comment block above the detectFramework export in
detect-framework.ts to include a short runnable example (e.g., async call within
an IIFE or top-level await) that demonstrates input and how to read the returned
object's properties.In
@packages/network-config/src/detection/detect-package-manager.ts:
- Line 28: The file uses string interpolation when calling fs.exists in
detect-package-manager.ts (const exists = yield*
fs.exists(${projectFolder}/${file})...); change this to use
path.join(projectFolder, file) for robust path construction, update the import
to include Node's path module if not already imported, and ensure the same
pattern as raw-config-reader.ts is followed so trailing slashes and platform
separators are handled consistently.In
@packages/network-config/src/detection/generate-config.ts:
- Around line 25-32: Add an
@exampleblock to the JSDoc for the exported
function generateConfig showing how to call it and what it returns; update the
doc above the generateConfig declaration to include a short usage example (e.g.,
importing or referencing a FrameworkDefinition, passing a packageManager like
"npm" or "yarn", calling generateConfig(framework, packageManager), and a brief
example of the returned NormalizedConfig shape) so the JSDoc contains@param,
@returns, and@exampletags for quick reference.In
@packages/network-config/src/services/raw-config-reader.ts:
- Around line 64-72: The current parsing block in raw-config-reader.ts silently
falls back to parseYaml for any unknown fileExtension (dead code behind
ALLOWED_CONFIG_NAMES); change the final else branch to explicitly throw a
descriptive error instead of calling parseYaml — reference the local variables
fileExtension and parsed and the functions JSON.parse and parseYaml (and the
ALLOWED_CONFIG_NAMES constant) so the error message can state the unexpected
extension and list allowed extensions; ensure callers of the function will
surface the thrown error (or adjust signature) rather than silently accepting
YAML parsing for unknown types.- Line 7: ALLOWED_CONFIG_NAMES is currently a mutable string array; change its
declaration to a readonly tuple by appending as const so the type becomes a
literal tuple and you can derive a union of allowed filenames downstream (update
any code that relied on it being string[] to accept readonly string[] or derive
a union via typeof ALLOWED_CONFIG_NAMES[number]); locate ALLOWED_CONFIG_NAMES in
raw-config-reader.ts and replace its declaration with the as const assertion and
adjust any type usages accordingly.In
@packages/network-config/src/services/schema-validator.ts:
- Around line 20-23: The code currently creates a new Ajv instance and calls
addFormats on every invocation of validate; move Ajv instantiation and
addFormats registration out of the validate function and into the service
initialization inside the Effect.gen body so a single Ajv instance is reused.
Specifically, create ajv = new Ajv() and call addFormats(ajv) once at the
top-level of the Effect.gen block that constructs the validator service, then in
the validate function use ajv.compile(schema) (or cache compiled validators)
instead of new Ajv()/addFormats per call; reference symbols: Ajv, addFormats,
validate, Effect.gen, and compiledValidate to locate and change the allocation
site.In
@packages/network-config/src/services/v4-config-parser.test.ts:
- Around line 39-52: The test for V4ConfigParser.parse only asserts regions and
commands but misses entrypoint-level assertions; update the test (in
v4-config-parser.test.ts where loadExample(), V4ConfigParser.parse and result
are used) to also assert that result.entrypoints (or the parsed entrypoint
structure returned by parse) contains the expected entries and that each
targeted file preserves its runtime, schedule and symlink flags (e.g., check
runtime === expectedRuntime, schedule === expectedSchedule, symlink ===
expectedSymlink for the specific entrypoint keys from the example ConfigV4) to
prevent regressions in entrypoint parsing.In
@packages/network-config/src/v4/parse.test.ts:
- Around line 93-119: The tests currently cast the parsed config with "config as
any", which disables type checks; instead cast to the concrete config type
expected by getFunctionSettings (e.g., config as V4Config) or adjust readFixture
to return Record<string, V4Config> so the calls like
getFunctionSettings('src/index.ts', config as V4Config) are correctly typed;
update the four calls that pass config to getFunctionSettings to use the precise
cast and import/alias the V4Config type where needed.- Around line 32-33: Replace the inline fs.readFileSync + JSON.parse block that
builds the schema (currently producing schema from schemaFile) with the existing
readFixture helper; i.e., assign const schema = readFixture('schema.json') so
the test uses the same fixture loader as the example.yaml path and benefits from
readFixture's JSON parsing behavior (ensure readFixture is in scope where schema
is created).- Around line 17-25: readFixture currently uses blocking fs.readFileSync inside
an async function; switch to the promise-based API so the function is fully
non-blocking. Replace the synchronous read with an awaited read from fs.promises
(e.g., const content = await fs.promises.readFile(filePath, 'utf8')) in the
readFixture function and keep the dynamic await import('yaml') branch unchanged;
ensure types remain Record<string, unknown> and behavior for JSON and YAML
parsing is preserved.</details> --- <details> <summary>ℹ️ Review info</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 774909d4f134df91502dc1476bddf989e78242c4 and 7a54f5f0f2db5d6c8a59ef929accbcaf25b5beb5. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `pnpm-lock.yaml` is excluded by `!**/pnpm-lock.yaml` </details> <details> <summary>📒 Files selected for processing (59)</summary> * `AGENTS.md` * `package.json` * `packages/cli/src/index.ts` * `packages/cli/src/services/project-config.test.ts` * `packages/cli/src/services/project-config.ts` * `packages/network-config/AGENTS.md` * `packages/network-config/package.json` * `packages/network-config/src/build-output-v3/get-default-path-map.test.ts` * `packages/network-config/src/build-output-v3/get-default-path-map.ts` * `packages/network-config/src/build-output-v3/parse.ts` * `packages/network-config/src/build-output-v3/read-config-file.test.ts` * `packages/network-config/src/build-output-v3/read-config-file.ts` * `packages/network-config/src/detection/ADDING_FRAMEWORKS.md` * `packages/network-config/src/detection/detect-framework.test.ts` * `packages/network-config/src/detection/detect-framework.ts` * `packages/network-config/src/detection/detect-package-manager.test.ts` * `packages/network-config/src/detection/detect-package-manager.ts` * `packages/network-config/src/detection/frameworks/astro.ts` * `packages/network-config/src/detection/frameworks/elysia.ts` * `packages/network-config/src/detection/frameworks/express.ts` * `packages/network-config/src/detection/frameworks/fastify.ts` * `packages/network-config/src/detection/frameworks/hono.ts` * `packages/network-config/src/detection/frameworks/index.ts` * `packages/network-config/src/detection/frameworks/laravel.ts` * `packages/network-config/src/detection/frameworks/nestjs.ts` * `packages/network-config/src/detection/frameworks/nextjs.ts` * `packages/network-config/src/detection/frameworks/nuxt.ts` * `packages/network-config/src/detection/frameworks/remix.ts` * `packages/network-config/src/detection/frameworks/sveltekit.ts` * `packages/network-config/src/detection/frameworks/symfony.ts` * `packages/network-config/src/detection/frameworks/vite.ts` * `packages/network-config/src/detection/generate-config.test.ts` * `packages/network-config/src/detection/generate-config.ts` * `packages/network-config/src/detection/index.ts` * `packages/network-config/src/detection/merge-config.test.ts` * `packages/network-config/src/detection/merge-config.ts` * `packages/network-config/src/detection/read-dependencies.test.ts` * `packages/network-config/src/detection/read-dependencies.ts` * `packages/network-config/src/detection/types.ts` * `packages/network-config/src/errors.ts` * `packages/network-config/src/find-config.ts` * `packages/network-config/src/index.ts` * `packages/network-config/src/parse-config.ts` * `packages/network-config/src/parse-raw-config.test.ts` * `packages/network-config/src/parse-raw-config.ts` * `packages/network-config/src/regions.ts` * `packages/network-config/src/services/index.ts` * `packages/network-config/src/services/raw-config-reader.test.ts` * `packages/network-config/src/services/raw-config-reader.ts` * `packages/network-config/src/services/schema-validator.test.ts` * `packages/network-config/src/services/schema-validator.ts` * `packages/network-config/src/services/v4-config-parser.test.ts` * `packages/network-config/src/services/v4-config-parser.ts` * `packages/network-config/src/services/vercel-build-output-parser.test.ts` * `packages/network-config/src/services/vercel-build-output-parser.ts` * `packages/network-config/src/test-utils.ts` * `packages/network-config/src/v4/index.ts` * `packages/network-config/src/v4/parse.test.ts` * `packages/network-config/src/v4/parse.ts` </details> <details> <summary>💤 Files with no reviewable changes (8)</summary> * package.json * packages/network-config/src/parse-raw-config.ts * packages/network-config/src/build-output-v3/parse.ts * packages/network-config/src/parse-raw-config.test.ts * packages/network-config/src/v4/parse.ts * packages/network-config/src/build-output-v3/read-config-file.ts * packages/network-config/src/build-output-v3/read-config-file.test.ts * packages/network-config/src/find-config.ts </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
…che dependency reads - Add excludeFiles field to NormalizedConfig for archive-level file exclusion - Map excludeFiles from FrameworkDefaultConfig through generateConfig - Merge excludeFiles in mergeWithFrameworkDefaults (user takes precedence) - Pass excludeFiles to ArchiveService for deployment archive filtering - Memoize readDependencies via Effect.cachedFunction to avoid redundant I/O - Add clarifying comment for Case A postProcessConfig asymmetry
- Replace catchAll with catchTags in ProjectConfigService for specific error handling - Add dependencies: [NetworkConfigLive] to ProjectConfigService and simplify layer composition - Split ManifestReadError into ManifestNotFoundError, ManifestReadError, ManifestParseError - Use streaming-aware handler (SERVERLESS_FUNCTION_STREAMING) in generateConfig - Guard new RegExp(key) with try/catch in getFunctionSettings for glob patterns - Fix region aggregation in VercelBuildOutputParser to preserve earlier functions' regions - Sync v4 schema runtime enum with runtime.ts (add php-84, node-22; remove php-82, php-80) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
packages/network-config/src/services/v4-config-parser.ts (2)
100-141: Settings are resolved per pattern key, not per matched file — overlapping patterns apply incorrect settings.This was flagged in a prior review and still applies.
getFunctionSettings(fnPath, config)at Line 102 resolves settings for the pattern string itself, not for each matched file. With overlapping patterns (e.g.,api/**/*.ts+api/special.ts), a broad match processes files first and the specific pattern's settings never apply because the file is already inentrypoints(Line 123 skip).Additionally, Lines 136–137 take
schedule/symlinksdirectly fromfuncrather than fromsettings, creating an inconsistency:runtime/memory/max_durationcome from the merged settings object, butschedule/symlinksbypass the merge entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/services/v4-config-parser.ts` around lines 100 - 141, The code resolves settings once per pattern (getFunctionSettings(fnPath, config)) causing broad patterns to claim files before more specific patterns can apply and also reads schedule/symlinks from the raw func instead of the merged settings. Fix by resolving settings per matched file: inside the matchedFiles loop call getFunctionSettings(file, config) (or otherwise merge config for that specific file) and use those returned settings for runtime/memory/max_duration as well as schedule and symlinks; keep the existing dedupe check (entrypoints.find...) but ensure it uses the per-file settings to decide inclusion so specific-pattern settings can override broad patterns.
25-30: ReDoS risk from dynamicRegExpconstruction remains.The
try-catchnow prevents crashes from invalid regex syntax (addressing the prior feedback), but a valid-yet-pathological pattern (e.g.,(a+)+$) can still cause exponential backtracking. Since these patterns come from the project's owngigadrive.yaml, the practical risk is low, but consider adding a timeout or length cap if this ever accepts less-trusted input.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/services/v4-config-parser.ts` around lines 25 - 30, The dynamic RegExp construction for matching rules (the new RegExp(key) used to set regexMatch) can still allow catastrophic backtracking for valid-but-pathological patterns; update v4-config-parser.ts to guard RegExp(key) by first rejecting or sanitizing unsafe/oversized patterns — e.g., introduce a MAX_PATTERN_LENGTH constant and skip/ignore patterns longer than that, or run a safety check using a library like safe-regex before constructing the RegExp, and only call new RegExp(key).test(path) when the pattern passes the safety/length check; ensure the fallback behavior remains the same (skip regex matching) when a pattern is rejected.packages/network-config/src/services/vercel-build-output-parser.ts (1)
125-160:⚠️ Potential issue | 🟠 MajorRoute destinations should use the same full entrypoint path as the function entrypoint.
Line 146 and Line 159 currently use only
handler, which can collide across multiple functions with identical handler filenames.🐛 Proposed fix
+ const functionEntrypointPath = path.join(functionDirectory, handler); + result.entrypoints.push({ displayName: functionName, runtime, - path: path.join(functionDirectory, handler), + path: functionEntrypointPath, memory: memory ?? 1024, maxDuration: maxDuration ?? 15, environmentVariables, streaming, package: { filePathMap, }, }); @@ const defaultRoute: NormalizedConfigRoute = { path: `/${functionName}`, methods: ['ANY'], headers: {}, - destination: handler, + destination: functionEntrypointPath, handler: handlerType, }; @@ path: ('src' in r ? r.src : undefined) ?? `/${functionName}`, methods: ['ANY'] as NormalizedConfigRouteMethod[], headers: {}, - destination: handler, + destination: functionEntrypointPath, handler: handlerType, }) as NormalizedConfigRoute🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/services/vercel-build-output-parser.ts` around lines 125 - 160, The route destination currently uses only the handler filename which can collide; compute and reuse the full entrypoint path (the same value pushed into result.entrypoints via path.join(functionDirectory, handler)) and replace the destinations that use just handler (in defaultRoute and in the config.routes.map) with that full entrypoint path; update references near result.entrypoints, defaultRoute, and the config.routes mapping so destination points to the full entrypoint path instead of handler, keeping handlerType, functionName, and other fields unchanged.
🧹 Nitpick comments (6)
packages/network-config/src/services/v4-config-parser.ts (2)
157-162: Naming nit:disallowedExtensionscontains filenames, not extensions.
.htaccessand.htpasswdare full filenames (dotfiles), not file extensions. TheendsWithcheck works correctly (e.g., it matchespath/to/.htaccess), but the variable name is slightly misleading. A name likedisallowedFileSuffixesordisallowedFileswould be more accurate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/services/v4-config-parser.ts` around lines 157 - 162, Rename the misleading variable disallowedExtensions to something that reflects they are full filenames/suffixes (e.g., disallowedFiles or disallowedFileSuffixes) and update all references in this function (the array declaration and the check using disallowedExtensions.some((ext) => assetName.toLowerCase().endsWith(ext.toLowerCase()))) so the loop over allFiles and the assetName.endsWith check continue to work unchanged.
189-211: Consider narrowing theas NormalizedConfig/as NormalizedConfigRoutecasts.The two
ascasts (Lines 209, 211) suppress compile-time checking for missing or mistyped fields. IfNormalizedConfiggains a required field later, the cast will silently hide the omission. Consider usingsatisfies NormalizedConfig(or declaring the return type onparse) so TypeScript structurally validates the object shape.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/services/v4-config-parser.ts` around lines 189 - 211, The object and route entries are force-cast with "as NormalizedConfig" and "as NormalizedConfigRoute", which silences missing/mistyped field errors; remove those casts and instead make TypeScript validate the shape by using "satisfies NormalizedConfig" on the top-level object (or declaring the parse function's return type as NormalizedConfig) and ensure each entry returned from routes.map satisfies NormalizedConfigRoute (the mapped route object and the handler variable should be typed accordingly) so the compiler enforces required fields and catches future shape changes.packages/network-config/src/detection/generate-config.ts (1)
25-32: Add@exampleto exported function JSDoc.The exported
generateConfigdocs include@param/@returnsbut miss@example.As per coding guidelines: "Document exported functions with JSDoc comments including
@param,@returns, and@exampletags".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/detection/generate-config.ts` around lines 25 - 32, The JSDoc for the exported function generateConfig is missing an `@example` tag; update the comment block above generateConfig to include a concise `@example` showing typical usage (e.g., calling generateConfig with a detected framework object and packageManager string and using the returned NormalizedConfig), keeping the existing `@param` and `@returns` tags and ensuring the example demonstrates input shape and expected invocation so readers understand how to call generateConfig.packages/network-config/src/detection/read-dependencies.test.ts (1)
61-68: Assert the specific error type, not just generic failure.Line 67 only checks that the effect failed; it should verify
ManifestParseErrorto lock the API contract.♻️ Proposed test hardening
- const result = await Effect.runPromiseExit(readDependencies('/project', 'node').pipe(Effect.provide(fs))); - expect(result._tag).toBe('Failure'); + const error = await Effect.runPromise( + readDependencies('/project', 'node').pipe(Effect.provide(fs), Effect.flip) + ); + expect(error._tag).toBe('ManifestParseError');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/detection/read-dependencies.test.ts` around lines 61 - 68, The test currently only asserts the Effect failed; update the assertion to check for the specific ManifestParseError: call readDependencies('/project', 'node') with the invalid JSON fs (from makeTestFs) and after running Effect.runPromiseExit assert that result._tag === 'Failure' and that result.cause (or result.failure) contains an instance or kind of ManifestParseError (use the same ManifestParseError symbol/type used by readDependencies) so the test verifies the exact error type rather than a generic failure.packages/network-config/src/detection/detect-framework.ts (1)
82-91: Add an@examplesection todetectFrameworkJSDoc.The exported function docs should include a concrete usage example alongside
@param/@returns.As per coding guidelines:
packages/network-config/**/*.{ts,tsx}requires exported functions to include JSDoc with@param,@returns, and@exampletags.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/detection/detect-framework.ts` around lines 82 - 91, Add an `@example` block to the JSDoc for the exported detectFramework function in detect-framework.ts showing concrete usage: call detectFramework(projectFolder) with an example absolute path, await the Promise, and illustrate the shape of the returned DetectionResult (framework name, packageManager, generated config object). Keep it brief and realistic (e.g., show using async/await, sample path like "/path/to/project", and example returned object fields) and include it alongside the existing `@param` and `@returns` tags.packages/network-config/src/detection/read-dependencies.ts (1)
40-49: Add an@exampleblock to the exportedreadDependenciesdocs.The exported API docs are missing a usage example; please include a short
@examplesnippet.As per coding guidelines:
packages/network-config/**/*.{ts,tsx}requires exported functions to include JSDoc with@param,@returns, and@exampletags.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/detection/read-dependencies.ts` around lines 40 - 49, The JSDoc for the exported function readDependencies is missing an `@example`; update the comment block for readDependencies to include a brief `@example` snippet showing typical usage (e.g., calling await readDependencies(projectFolder, language) and demonstrating the returned Set of dependency names or iterating it), while keeping the existing `@param` and `@returns` tags intact; ensure the example uses realistic variable names (projectFolder, language) and is short (one to three lines) to satisfy the packages/network-config JSDoc requirement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/services/project-config.ts`:
- Around line 67-69: If a configPath is provided, skip running detectFramework
so manifest read/parse failures cannot abort config-only resolution; update the
logic around detectFramework (the detection variable) to check for configPath
first and only call
detectFramework(cwd).pipe(Effect.catchTag('FrameworkNotDetectedError', () =>
Effect.succeed(null))) when configPath is absent, otherwise set detection to
null (or Effect.succeed(null)) so the config-only fallback path remains
non-blocking.
In `@packages/network-config/src/detection/generate-config.ts`:
- Around line 73-79: The assets config is discarding the detected assets
directory by setting paths: [] when defaults.assetsDir is present; update the
block that sets config.assets (the if that checks defaults.assetsDir) to
populate paths with the detected directory (e.g., include defaults.assetsDir)
rather than an empty array, preserving any existing asset paths if present and
keeping prefixToStrip, dynamicRoutes and populateCache logic intact so the
detected assetsDir is actually included in config.assets.paths.
In `@packages/network-config/src/services/v4-config-parser.ts`:
- Around line 221-247: collectFilesRecursively can recurse infinitely on symlink
loops because it calls fs.stat (which follows symlinks); update the function to
avoid following symlinks by using fs.lstat if the FileSystem API supports it
(replace fs.stat(...) with fs.lstat(...) and treat symlink entries as files or
resolve them carefully), or add a visited tracker: add a Set parameter (e.g.,
visitedPaths or visitedInodes) passed through recursive calls and skip directory
entries whose resolved path/inode (use stat/realpath/inode) is already in the
set before recursing; reference symbols: collectFilesRecursively, fs.stat,
fs.lstat, entryRelative, basePath, and ensure the visited Set is updated when
descending into a Directory.
- Around line 88-91: The fallback for undefined config.regions is currently
returning ['global'] which is not a valid Region and breaks type safety; update
the regions assignment (the const regions variable that inspects config.regions)
so that both the explicit 'global' case and the undefined/falsy fallback resolve
to AVAILABLE_REGIONS instead of ['global'], ensuring the result is a Region[];
use config.regions?.includes('global') to map to AVAILABLE_REGIONS and replace
the nullish fallback (?? ['global']) with (?? AVAILABLE_REGIONS) or equivalent
logic that filters out 'global' when present and returns AVAILABLE_REGIONS for
undefined.
---
Duplicate comments:
In `@packages/network-config/src/services/v4-config-parser.ts`:
- Around line 100-141: The code resolves settings once per pattern
(getFunctionSettings(fnPath, config)) causing broad patterns to claim files
before more specific patterns can apply and also reads schedule/symlinks from
the raw func instead of the merged settings. Fix by resolving settings per
matched file: inside the matchedFiles loop call getFunctionSettings(file,
config) (or otherwise merge config for that specific file) and use those
returned settings for runtime/memory/max_duration as well as schedule and
symlinks; keep the existing dedupe check (entrypoints.find...) but ensure it
uses the per-file settings to decide inclusion so specific-pattern settings can
override broad patterns.
- Around line 25-30: The dynamic RegExp construction for matching rules (the new
RegExp(key) used to set regexMatch) can still allow catastrophic backtracking
for valid-but-pathological patterns; update v4-config-parser.ts to guard
RegExp(key) by first rejecting or sanitizing unsafe/oversized patterns — e.g.,
introduce a MAX_PATTERN_LENGTH constant and skip/ignore patterns longer than
that, or run a safety check using a library like safe-regex before constructing
the RegExp, and only call new RegExp(key).test(path) when the pattern passes the
safety/length check; ensure the fallback behavior remains the same (skip regex
matching) when a pattern is rejected.
In `@packages/network-config/src/services/vercel-build-output-parser.ts`:
- Around line 125-160: The route destination currently uses only the handler
filename which can collide; compute and reuse the full entrypoint path (the same
value pushed into result.entrypoints via path.join(functionDirectory, handler))
and replace the destinations that use just handler (in defaultRoute and in the
config.routes.map) with that full entrypoint path; update references near
result.entrypoints, defaultRoute, and the config.routes mapping so destination
points to the full entrypoint path instead of handler, keeping handlerType,
functionName, and other fields unchanged.
---
Nitpick comments:
In `@packages/network-config/src/detection/detect-framework.ts`:
- Around line 82-91: Add an `@example` block to the JSDoc for the exported
detectFramework function in detect-framework.ts showing concrete usage: call
detectFramework(projectFolder) with an example absolute path, await the Promise,
and illustrate the shape of the returned DetectionResult (framework name,
packageManager, generated config object). Keep it brief and realistic (e.g.,
show using async/await, sample path like "/path/to/project", and example
returned object fields) and include it alongside the existing `@param` and
`@returns` tags.
In `@packages/network-config/src/detection/generate-config.ts`:
- Around line 25-32: The JSDoc for the exported function generateConfig is
missing an `@example` tag; update the comment block above generateConfig to
include a concise `@example` showing typical usage (e.g., calling generateConfig
with a detected framework object and packageManager string and using the
returned NormalizedConfig), keeping the existing `@param` and `@returns` tags and
ensuring the example demonstrates input shape and expected invocation so readers
understand how to call generateConfig.
In `@packages/network-config/src/detection/read-dependencies.test.ts`:
- Around line 61-68: The test currently only asserts the Effect failed; update
the assertion to check for the specific ManifestParseError: call
readDependencies('/project', 'node') with the invalid JSON fs (from makeTestFs)
and after running Effect.runPromiseExit assert that result._tag === 'Failure'
and that result.cause (or result.failure) contains an instance or kind of
ManifestParseError (use the same ManifestParseError symbol/type used by
readDependencies) so the test verifies the exact error type rather than a
generic failure.
In `@packages/network-config/src/detection/read-dependencies.ts`:
- Around line 40-49: The JSDoc for the exported function readDependencies is
missing an `@example`; update the comment block for readDependencies to include a
brief `@example` snippet showing typical usage (e.g., calling await
readDependencies(projectFolder, language) and demonstrating the returned Set of
dependency names or iterating it), while keeping the existing `@param` and
`@returns` tags intact; ensure the example uses realistic variable names
(projectFolder, language) and is short (one to three lines) to satisfy the
packages/network-config JSDoc requirement.
In `@packages/network-config/src/services/v4-config-parser.ts`:
- Around line 157-162: Rename the misleading variable disallowedExtensions to
something that reflects they are full filenames/suffixes (e.g., disallowedFiles
or disallowedFileSuffixes) and update all references in this function (the array
declaration and the check using disallowedExtensions.some((ext) =>
assetName.toLowerCase().endsWith(ext.toLowerCase()))) so the loop over allFiles
and the assetName.endsWith check continue to work unchanged.
- Around line 189-211: The object and route entries are force-cast with "as
NormalizedConfig" and "as NormalizedConfigRoute", which silences
missing/mistyped field errors; remove those casts and instead make TypeScript
validate the shape by using "satisfies NormalizedConfig" on the top-level object
(or declaring the parse function's return type as NormalizedConfig) and ensure
each entry returned from routes.map satisfies NormalizedConfigRoute (the mapped
route object and the handler variable should be typed accordingly) so the
compiler enforces required fields and catches future shape changes.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
packages/cli/src/commands/platform/deploy/index.tspackages/cli/src/index.tspackages/cli/src/services/archive.tspackages/cli/src/services/project-config.test.tspackages/cli/src/services/project-config.tspackages/network-config/src/detection/detect-framework.tspackages/network-config/src/detection/generate-config.test.tspackages/network-config/src/detection/generate-config.tspackages/network-config/src/detection/merge-config.test.tspackages/network-config/src/detection/merge-config.tspackages/network-config/src/detection/read-dependencies.test.tspackages/network-config/src/detection/read-dependencies.tspackages/network-config/src/detection/types.tspackages/network-config/src/normalized-config.tspackages/network-config/src/services/v4-config-parser.tspackages/network-config/src/services/vercel-build-output-parser.tspackages/network-config/src/v4/schema.jsonpackages/network-config/src/v4/schema.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/network-config/src/detection/merge-config.test.ts
- packages/network-config/src/detection/merge-config.ts
- packages/cli/src/index.ts
- packages/network-config/src/detection/generate-config.test.ts
- Make framework detection non-blocking: catch ManifestReadError and ManifestParseError so manifest issues don't abort config-only resolution - Fix undefined regions fallback: use AVAILABLE_REGIONS instead of ['global'] which is not a valid Region type Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/network-config/src/services/v4-config-parser.ts (3)
221-247:⚠️ Potential issue | 🟡 MinorCircular-symlink infinite recursion still unguarded.
fs.statfollows symlinks; a directory containing a symlink that points back to an ancestor will recurse until the stack overflows. The previous comment proposing avisited: Set<string>guard has not been addressed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/services/v4-config-parser.ts` around lines 221 - 247, collectFilesRecursively can infinite-recurse on circular symlinks because fs.stat follows links; fix by adding a visited set of resolved absolute paths and skip revisiting them: update collectFilesRecursively to accept an optional visited: Set<string> (or create one at the top call), call fs.realpath (or equivalent) on fullPath before recursing, check if the resolved path is in visited and skip if so, otherwise add it to visited and proceed; also handle symlinks by using lstat/realpath semantics so you only follow links intentionally and avoid re-traversing ancestors.
100-142:⚠️ Potential issue | 🟠 MajorPer-pattern settings lookup still not resolved per matched file.
getFunctionSettings(fnPath, config)at Line 102 resolves settings against the pattern string (e.g.,api/**/*.ts), not against each resolvedfile. For overlapping patterns the wrong settings are applied. Additionally, Lines 136-137 consumefunc.schedule/func.symlinksfrom the raw pattern value instead of the aggregatedsettingsobject.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/services/v4-config-parser.ts` around lines 100 - 142, The code currently calls getFunctionSettings(fnPath, config) once per pattern, so overlapping patterns get incorrect settings; move the per-file settings resolution into the matchedFiles loop by calling getFunctionSettings(file, config) for each resolved file (and fail with FunctionConfigError if null, mirroring the existing error shape), then use the resulting settings object for runtime, memory, maxDuration and also for schedule and symlinks (replace func.schedule / func.symlinks with settings.schedule / settings.symlinks); ensure streaming continues to derive from settings.runtime.
26-30:⚠️ Potential issue | 🟠 MajorReDoS risk from user-supplied regex patterns remains.
The
try/catchprevents aSyntaxErrorcrash, but a syntactically valid yet pathological pattern (e.g.,(a+)+) still causes catastrophic backtracking at test time. Becausekeyis user-controlled config, the ReDoS surface is real.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/services/v4-config-parser.ts` around lines 26 - 30, The current try/catch around new RegExp(key).test(path) only avoids syntax errors but still allows catastrophic backtracking for malicious or pathological patterns; update the logic in v4-config-parser.ts so you do not call new RegExp(key).test(path) on unchecked user input (the key variable). Instead, validate key first with a safe-regex check (or use a library like safe-regex or regexpp to detect exponential-backtracking patterns) and only construct the RegExp if it passes; for unsafe patterns fallback to an escaped-literal match or a glob matcher (e.g., convert to minimatch) so regexMatch is set without executing a potentially unsafe RegExp. Ensure you reference the existing regexMatch assignment and the new RegExp(key) usage when making the change.
🧹 Nitpick comments (3)
packages/network-config/src/services/v4-config-parser.ts (3)
157-161: HoistdisallowedExtensionsto module scope asDISALLOWED_ASSET_EXTENSIONS.It's a stable constant literal array recreated on every
parseinvocation. Per coding guidelines, constant literal arrays should useUPPER_SNAKE_CASEand be declared at module scope (and preferas constto derive the union type).♻️ Proposed refactor
+const DISALLOWED_ASSET_EXTENSIONS = ['.htaccess', '.htpasswd'] as const; + export class V4ConfigParser extends Effect.Service<V4ConfigParser>()('V4ConfigParser', { ... - const disallowedExtensions = ['.htaccess', '.htpasswd']; - - for (const assetName of allFiles) { - if (disallowedExtensions.some((ext) => assetName.toLowerCase().endsWith(ext.toLowerCase()))) { + for (const assetName of allFiles) { + if (DISALLOWED_ASSET_EXTENSIONS.some((ext) => assetName.toLowerCase().endsWith(ext.toLowerCase()))) {As per coding guidelines: "Use UPPER_SNAKE_CASE for constants" and "Prefer
as constassertions for literal arrays that derive union types."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/services/v4-config-parser.ts` around lines 157 - 161, Move the local const disallowedExtensions out of the parse function to module scope and rename it to DISALLOWED_ASSET_EXTENSIONS (use UPPER_SNAKE_CASE); make it a literal readonly array using "as const" so its union type is preserved, then update the loop that currently references disallowedExtensions (inside the parse logic that iterates allFiles) to use DISALLOWED_ASSET_EXTENSIONS instead. Ensure any type annotations that previously referred to the local variable are adjusted to use typeof DISALLOWED_ASSET_EXTENSIONS[number] if needed.
221-225: Use function declaration syntax forcollectFilesRecursively.The function is a non-trivial multi-statement
Effect.genbody and should follow the project convention for larger functions.♻️ Proposed refactor
-const collectFilesRecursively = ( +function collectFilesRecursively( fs: FileSystem.FileSystem, basePath: string, - relativePath: string = '' -): Effect.Effect<string[], never, never> => - Effect.gen(function* () { + relativePath = '' +): Effect.Effect<string[], never, never> { + return Effect.gen(function* () { ... - }); + }); +}Based on learnings: "Use function declaration syntax (not arrow functions) for larger or async functions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/services/v4-config-parser.ts` around lines 221 - 225, Convert the arrow function collectFilesRecursively to a function declaration using the same name, parameters (fs: FileSystem.FileSystem, basePath: string, relativePath: string = ''), and return type Effect.Effect<string[], never, never>; move the existing Effect.gen multi-statement body intact into the new function declaration body so behavior is unchanged, preserving all internal references and exports/usages of collectFilesRecursively.
209-211: Unsafeascasts may hide missing required fields.
as NormalizedConfigRoute(Line 209) andas NormalizedConfig(Line 211) suppress compiler checks. If either type gains a required property, these casts will silently produce incomplete objects at runtime.♻️ Proposed refactor
- } as NormalizedConfigRoute; + }; ... - } as NormalizedConfig; + };Let TypeScript verify the shapes structurally — drop the casts and fix any resulting type errors explicitly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/services/v4-config-parser.ts` around lines 209 - 211, The code is using unsafe "as NormalizedConfigRoute" and "as NormalizedConfig" casts which hide missing required fields; remove those casts and let TypeScript enforce the shapes for the mapped route object and the returned config object. Edit the mapping that currently returns the route (the object being cast to NormalizedConfigRoute) to explicitly include all required properties of NormalizedConfigRoute (or transform/validate source fields so they match), and update the outer object returned (the one cast to NormalizedConfig) to include all required NormalizedConfig members; run the compiler, fix any resulting type errors by adding or deriving the missing properties or using explicit validation/constructors, and prefer returning a properly-typed value rather than forcing with "as".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/network-config/src/services/v4-config-parser.ts`:
- Around line 104-111: The null-check branch for "settings" in
v4-config-parser.ts is dead because getFunctionSettings(fnPath, config) always
returns a match for keys from config.functions; remove the unreachable if
(settings == null) { ... } block (including the FunctionConfigError
construction) and rely on the invariant that getFunctionSettings returns a
value, or alternatively replace it with a concise internal assertion (e.g., an
invariant/error with clear message) if you want an explicit guard for future
changes; update references around getFunctionSettings, settings, and
FunctionConfigError accordingly.
---
Duplicate comments:
In `@packages/network-config/src/services/v4-config-parser.ts`:
- Around line 221-247: collectFilesRecursively can infinite-recurse on circular
symlinks because fs.stat follows links; fix by adding a visited set of resolved
absolute paths and skip revisiting them: update collectFilesRecursively to
accept an optional visited: Set<string> (or create one at the top call), call
fs.realpath (or equivalent) on fullPath before recursing, check if the resolved
path is in visited and skip if so, otherwise add it to visited and proceed; also
handle symlinks by using lstat/realpath semantics so you only follow links
intentionally and avoid re-traversing ancestors.
- Around line 100-142: The code currently calls getFunctionSettings(fnPath,
config) once per pattern, so overlapping patterns get incorrect settings; move
the per-file settings resolution into the matchedFiles loop by calling
getFunctionSettings(file, config) for each resolved file (and fail with
FunctionConfigError if null, mirroring the existing error shape), then use the
resulting settings object for runtime, memory, maxDuration and also for schedule
and symlinks (replace func.schedule / func.symlinks with settings.schedule /
settings.symlinks); ensure streaming continues to derive from settings.runtime.
- Around line 26-30: The current try/catch around new RegExp(key).test(path)
only avoids syntax errors but still allows catastrophic backtracking for
malicious or pathological patterns; update the logic in v4-config-parser.ts so
you do not call new RegExp(key).test(path) on unchecked user input (the key
variable). Instead, validate key first with a safe-regex check (or use a library
like safe-regex or regexpp to detect exponential-backtracking patterns) and only
construct the RegExp if it passes; for unsafe patterns fallback to an
escaped-literal match or a glob matcher (e.g., convert to minimatch) so
regexMatch is set without executing a potentially unsafe RegExp. Ensure you
reference the existing regexMatch assignment and the new RegExp(key) usage when
making the change.
---
Nitpick comments:
In `@packages/network-config/src/services/v4-config-parser.ts`:
- Around line 157-161: Move the local const disallowedExtensions out of the
parse function to module scope and rename it to DISALLOWED_ASSET_EXTENSIONS (use
UPPER_SNAKE_CASE); make it a literal readonly array using "as const" so its
union type is preserved, then update the loop that currently references
disallowedExtensions (inside the parse logic that iterates allFiles) to use
DISALLOWED_ASSET_EXTENSIONS instead. Ensure any type annotations that previously
referred to the local variable are adjusted to use typeof
DISALLOWED_ASSET_EXTENSIONS[number] if needed.
- Around line 221-225: Convert the arrow function collectFilesRecursively to a
function declaration using the same name, parameters (fs: FileSystem.FileSystem,
basePath: string, relativePath: string = ''), and return type
Effect.Effect<string[], never, never>; move the existing Effect.gen
multi-statement body intact into the new function declaration body so behavior
is unchanged, preserving all internal references and exports/usages of
collectFilesRecursively.
- Around line 209-211: The code is using unsafe "as NormalizedConfigRoute" and
"as NormalizedConfig" casts which hide missing required fields; remove those
casts and let TypeScript enforce the shapes for the mapped route object and the
returned config object. Edit the mapping that currently returns the route (the
object being cast to NormalizedConfigRoute) to explicitly include all required
properties of NormalizedConfigRoute (or transform/validate source fields so they
match), and update the outer object returned (the one cast to NormalizedConfig)
to include all required NormalizedConfig members; run the compiler, fix any
resulting type errors by adding or deriving the missing properties or using
explicit validation/constructors, and prefer returning a properly-typed value
rather than forcing with "as".
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/cli/src/services/project-config.tspackages/network-config/src/services/v4-config-parser.ts
Replace visited-set approach with a depth limit (MAX_ASSET_DEPTH=100) to prevent infinite recursion from symlink loops where paths grow monotonically. Add tests for nested directory asset collection, disallowed extension filtering, and symlink loop termination. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fixes 12 review issues: ReDoS protection with safe-regex2, consistent Path.Path usage across all modules, depth-limited directory recursion, Ajv instance reuse, install command deduplication for PHP frameworks, Vite static-only config, error handling in Case C, mutation fix in loadFunctions, deduplicated test utilities, and type-safe CLI test mocks. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace dependency-injection-via-arguments anti-pattern in collectFilesRecursively with proper Effect context yielding and convert it to Effect.fn for tracing. Add comment explaining why processDirectory uses closure instead. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Extract pure helpers (matchesPattern, resolveRouteHandler, resolveRegions, mapRoute) as plain functions outside Effect context - Decompose monolithic parse method into focused Effect.fn helpers (parseEntrypoints, collectAssets) - Use yieldable errors (yield* new FunctionConfigError) instead of yield* Effect.fail(new ...) - Simplify getFunctionSettings with object spread over repetitive null-checks - Move constants above their consumers - Add accessors: true to service for direct static method access - Update tests to use accessor pattern (V4ConfigParser.parse directly) - Replace as any casts with as ConfigV4 in v4/parse.test.ts Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
packages/network-config/src/services/v4-config-parser.ts (1)
44-45: ReDoS guard viasafe-regex2— LGTM.
safeRegex(pattern)gates thenew RegExpcall, and the surroundingtry/catchhandles any remainingSyntaxError. The ast-grep warning is a false positive here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/services/v4-config-parser.ts` around lines 44 - 45, False positive ReDoS warning: safeRegex(pattern) already guards the new RegExp(pattern) call and the surrounding try/catch handles SyntaxError, so no functional change is needed; retain the safeRegex(pattern) guard and the try/catch around new RegExp in v4-config-parser (referencing safeRegex and the RegExp construction) and, if you want to silence the ast-grep alert, add a short inline comment explaining the guard or an explicit linter suppression next to the check.
🧹 Nitpick comments (9)
packages/network-config/src/v4/parse.test.ts (1)
18-26: Convert the dynamicyamlimport to a static top-level import.
yamlis a declared project dependency — there is no reason to dynamicallyawait import('yaml')inside the helper. The dynamic import is the sole reasonreadFixtureisasync, which forces every call site toawaitand adds unnecessary overhead in tests. A static import allows the function to be synchronous.♻️ Proposed refactor
+import { parse as parseYaml } from 'yaml'; ... -async function readFixture(filePath: string): Promise<Record<string, unknown>> { +function readFixture(filePath: string): Record<string, unknown> { const content = fs.readFileSync(filePath, 'utf8'); const ext = path.extname(filePath).toLowerCase(); if (ext === '.json') { return JSON.parse(content) as Record<string, unknown>; } - const { parse: parseYaml } = await import('yaml'); return parseYaml(content) as Record<string, unknown>; }Call sites (
await readFixture(...)) can then drop theawait.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/v4/parse.test.ts` around lines 18 - 26, The helper readFixture is unnecessarily async due to the dynamic await import('yaml'); replace that with a static top-level import (e.g., import { parse as parseYaml } from 'yaml'), remove the async keyword from readFixture and the internal await, and make it return synchronously for non-JSON files; update call sites that do "await readFixture(...)" to call readFixture(...) without awaiting. Ensure the function still handles .json and YAML by using parseYaml for YAML parsing and keep the return type Record<string, unknown>.packages/network-config/src/build-output-v3/get-default-path-map.ts (2)
28-47: Prefer function declaration for the larger helper.
processDirectoryis substantial enough that a function declaration is clearer and matches the project rule.♻️ Suggested refactor
- const processDirectory = (currentPath: string, basePath: string = '', depth: number = 0): Effect.Effect<void> => - Effect.gen(function* () { - if (depth > MAX_DEPTH) return; - - const entries = yield* fs.readDirectory(currentPath).pipe(Effect.catchAll(() => Effect.succeed([] as string[]))); - - for (const name of entries) { - const fullPath = pathService.join(currentPath, name); - const relativePath = basePath ? pathService.join(basePath, name) : name; - - const stat = yield* fs.stat(fullPath).pipe(Effect.catchAll(() => Effect.succeed(null))); - if (!stat) continue; - - if (stat.type === 'Directory') { - yield* processDirectory(fullPath, relativePath, depth + 1); - } else { - result[fullPath] = relativePath; - } - } - }); + function processDirectory(currentPath: string, basePath: string = '', depth: number = 0): Effect.Effect<void> { + return Effect.gen(function* () { + if (depth > MAX_DEPTH) return; + + const entries = yield* fs.readDirectory(currentPath).pipe(Effect.catchAll(() => Effect.succeed([] as string[]))); + + for (const name of entries) { + const fullPath = pathService.join(currentPath, name); + const relativePath = basePath ? pathService.join(basePath, name) : name; + + const stat = yield* fs.stat(fullPath).pipe(Effect.catchAll(() => Effect.succeed(null))); + if (!stat) continue; + + if (stat.type === 'Directory') { + yield* processDirectory(fullPath, relativePath, depth + 1); + } else { + result[fullPath] = relativePath; + } + } + }); + }As per coding guidelines: "Use function declaration syntax for larger/async functions:
export async function fn(...) { }".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/build-output-v3/get-default-path-map.ts` around lines 28 - 47, The helper processDirectory is large and should be converted from a const-assigned Effect.gen arrow to a named function declaration to follow the project's style; refactor the const processDirectory (and its Effect.gen body) into a function declaration (e.g., function processDirectory(currentPath: string, basePath = '', depth = 0): Effect.Effect<void>) preserving the same logic and references to MAX_DEPTH, fs.readDirectory, fs.stat, pathService.join and the result mapping, and ensure callers still invoke processDirectory(...) as before.
4-13: Add@exampleto exported function JSDoc.The exported function docs are missing the required
@examplesection.✍️ Suggested doc update
/** * Creates a mapping of absolute file paths to their relative paths * starting from the provided directory. * * This is the default path map that will be used for Vercel-based functions, * when the function does not specify a filePathMap. * * `@param` directory - The absolute path of the directory to process * `@returns` A record where keys are absolute file paths and values are relative paths + * `@example` + * const pathMap = yield* getDefaultPathMap('/workspace/.vercel/output/functions'); */As per coding guidelines: "Document exported functions with JSDoc comments including
@param,@returns, and@exampletags".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/build-output-v3/get-default-path-map.ts` around lines 4 - 13, The JSDoc for the exported function (getDefaultPathMap) is missing an `@example`; update the JSDoc block above the exported function (getDefaultPathMap) to include an `@example` that shows typical usage: calling the function with a sample absolute directory path and the expected shape of the returned Record<string,string> (absolute -> relative) so readers know input/output format; keep the example concise and use realistic values and a short note about asynchronous/sync behavior if applicable.packages/network-config/src/detection/detect-framework.ts (1)
90-100: CompletedetectFrameworkJSDoc with an@example.The exported API doc is missing the required example section.
As per coding guidelines: "Document exported functions with JSDoc comments including
@param,@returns, and@exampletags."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/detection/detect-framework.ts` around lines 90 - 100, The JSDoc for the exported function detectFramework is missing an `@example`; add a concise example to the existing comment showing typical usage (call detectFramework with a project root string and handling the returned DetectionResult), keeping it brief and illustrating input and expected shape of output; ensure the `@example` appears alongside the existing `@param` and `@returns` tags in the JSDoc for export detectFramework.packages/network-config/src/detection/read-dependencies.ts (1)
40-53: Add@exampletoreadDependenciesJSDoc.The exported API docs are missing the
@examplesection.As per coding guidelines: "Document exported functions with JSDoc comments including
@param,@returns, and@exampletags."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/detection/read-dependencies.ts` around lines 40 - 53, Add an `@example` section to the JSDoc for the exported function readDependencies showing a minimal usage example: call readDependencies with a sample projectFolder path and a language constant (or string) and demonstrate awaiting/handling the returned Set of dependency names; include both sync/async usage form appropriate for Effect.fn (e.g., yielding/awaiting the effect) and show expected return shape (Set<string>) so consumers understand how to call readDependencies and what it returns. Ensure the new `@example` is placed in the existing JSDoc block above the readDependencies declaration and follows the same style as `@param/`@returns tags.packages/network-config/src/detection/generate-config.ts (1)
25-33: Add an@exampleblock to the exportedgenerateConfigJSDoc.The exported function docs currently include
@param/@returnsbut miss@example.✍️ Suggested doc update
/** * Generates a NormalizedConfig from a framework definition and detected package manager. * Prepends the install command and builds a complete deployment configuration. * * `@param` framework - The detected framework definition * `@param` packageManager - The detected package manager * `@returns` A complete NormalizedConfig with framework defaults + * `@example` + * const cfg = yield* generateConfig(framework, 'pnpm'); */As per coding guidelines: "Document exported functions with JSDoc comments including
@param,@returns, and@exampletags."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/detection/generate-config.ts` around lines 25 - 33, Add an `@example` JSDoc block to the exported generateConfig function showing a minimal usage example: call generateConfig with a sample framework object (matching the framework type used in the file) and a packageManager string (e.g., "npm" or "yarn"), demonstrating how to yield/await the Effect result and showing the shape of the returned NormalizedConfig; update the comment above export const generateConfig = Effect.fn('generateConfig')(function* (...) to include this `@example` so consumers see how to invoke generateConfig and what the resolved config looks like.packages/network-config/src/services/raw-config-reader.test.ts (1)
58-177: Add a regression test for YAML values that parse tonull.Current cases miss the
nullparse path ('null'or whitespace-only YAML). Adding one protects against uncaught runtime errors inreadRawConfig.✅ Suggested test case
+ it('should fail with ConfigFileParseError when YAML parses to null', async () => { + const result = await Effect.runPromise( + Effect.gen(function* () { + const reader = yield* RawConfigReader; + return yield* reader.readRawConfig('/project/config.yaml'); + }).pipe( + Effect.catchTag('ConfigFileParseError', (err) => + Effect.succeed({ _tag: 'caught' as const, filePath: err.filePath }) + ), + Effect.provide(RawConfigReader.Default), + Effect.provide(Layer.merge(makeTestFs({ '/project/config.yaml': 'null' }), TestPathLayer)) + ) + ); + expect(result).toMatchObject({ _tag: 'caught', filePath: '/project/config.yaml' }); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/services/raw-config-reader.test.ts` around lines 58 - 177, Add a regression test to ensure YAML values that parse to null are handled: create a new it() in the readRawConfig suite that uses runWithFs (or Effect.runPromise + Layer.merge(makeTestFs(...), TestPathLayer)) to supply a file (e.g., '/project/config.yaml') whose contents are 'null' and another case with whitespace-only content, then call RawConfigReader.readRawConfig and assert it fails with ConfigFileEmptyError (or the expected error handling path) or returns an empty object when disableVersionCheck is true; reference RawConfigReader.readRawConfig, RawConfigReader.Default, runWithFs, makeTestFs and TestPathLayer to locate where to add the test.packages/network-config/src/detection/detect-framework.test.ts (1)
206-223: The invalid-regex test doesn't currently exercise an invalid regex path.This case never injects an invalid
matchContentpattern, so it doesn't verify the behavior the test name describes. Either inject a bad detector pattern in the fixture setup or rename the test to match the actual assertion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/detection/detect-framework.test.ts` around lines 206 - 223, The test name claims to exercise an invalid matchContent regex but never injects one; update the test that calls detectFramework('/project') (which currently composes fs via Layer.merge, makeTestFs and TestPathLayer) to inject a framework detector with an invalid matchContent pattern (e.g., a malformed regex string) into the detection fixtures before calling detectFramework, or alternatively rename the test to reflect the current behavior; target symbols: detectFramework, Layer.merge, makeTestFs, and TestPathLayer so the injected bad detector is picked up by the detection pipeline.packages/network-config/src/detection/generate-config.test.ts (1)
145-168: Harden install-command assertions to prevent hidden secondary installers.Lines 156-167 only guard duplicate
composer install. They won’t fail if another install step (e.g.,bun install) slips into framework commands. Add a check that commands after index 0 do not contain any install command.♻️ Suggested test hardening
expect(result.commands[0]).toBe('composer install --prefer-dist --optimize-autoloader --no-dev'); expect(result.commands).not.toContain('composer install'); + expect( + result.commands.slice(1).some((command) => /\b(?:npm|pnpm|yarn|bun|composer)\s+install\b/.test(command)) + ).toBe(false);Based on learnings: framework
commandsinpackages/network-config/src/detection/are intended to be framework-specific build commands only, while install commands are prepended bygenerateConfig().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/detection/generate-config.test.ts` around lines 145 - 168, The test allows a hidden secondary installer in later commands; after calling generateConfig (used in tests referencing generateConfig, symfony, and customInstallFramework) assert that every command after index 0 does not contain any install operation (e.g., does not include the word "install" or start with "composer install" / other installer verbs); specifically check result.commands[0] remains the intended install entry and then assert result.commands.slice(1).every(...) is true to ensure no subsequent command is an install step.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/services/project-config.ts`:
- Around line 90-112: The current Case A calls parseConfig (which runs
postProcessConfig) before mergeWithFrameworkDefaults, causing stale pre-merge
errors to persist; change the flow so you parse the user config without
post-processing, then call mergeWithFrameworkDefaults(userConfig,
detection.config), and only after merging run postProcessConfig on the merged
config (wrapped with wrapParseErrors) to produce the final config used for
validation; update the branch where config is set (the code that currently calls
parseConfig and then mergeWithFrameworkDefaults) to instead obtain a raw parsed
user config, merge framework defaults, then postProcessConfig the merged result
and assign that to config (and keep resolvedConfigPath and framework assignment
as-is).
In `@packages/network-config/src/detection/ADDING_FRAMEWORKS.md`:
- Around line 112-113: Update the "commands" guidance row in
ADDING_FRAMEWORKS.md to clarify that the general rule is to omit the install
command (do not include npm/pnpm/yarn install) but allow explicit install/build
commands in exceptional framework definitions that require platform
compatibility (e.g., Laravel and Symfony entries which intentionally include
commands like `bun install`); reference the `commands` table row and the
Laravel/Symfony examples around the `entrypoint`/framework definitions so the
text states "omit install commands unless a framework's definition requires a
hardcoded install for platform compatibility (see Laravel/Symfony examples)".
- Around line 9-18: The fenced code block that shows the directory tree (the
block starting with ``` and containing "packages/network-config/src/detection/")
is missing a language tag and triggers MD040; fix it by changing the opening
fence to include a language (e.g., replace ``` with ```text) so the block is
treated as plain text and the linter warning is resolved.
In `@packages/network-config/src/services/raw-config-reader.ts`:
- Around line 64-95: The parsed result from JSON/YAML can be null; update the
post-parse guards around the parsed variable (the local parsed in
raw-config-reader.ts produced by parseYaml/JSON.parse) to treat null the same as
undefined and non-object values: if parsed is null or typeof parsed !== 'object'
then return Effect.fail(new ConfigFileParseError(...)); and only check
parsed.version when parsed is a non-null object (i.e. after that guard),
preserving the existing ConfigVersionError logic and honoring
options?.disableVersionCheck.
In `@packages/network-config/src/services/v4-config-parser.ts`:
- Around line 225-254: The outer Effect.gen wrapper is an unused generator (no
yield*) around the parse Effect; replace it with Effect.succeed to avoid the
lint error: move the const parse = Effect.fn('V4ConfigParser.parse')(function*
(config: ConfigV4, projectFolder: string) { ... }) out of the Effect.gen block
(i.e., export/return parse directly) and wrap the outer return in
Effect.succeed({ parse }) instead of Effect.gen(function* () { ... }), ensuring
the parse definition (and its internal yields like yield* parseEntrypoints)
remains unchanged.
- Line 87: The conditional return uses an unnecessary type assertion which CI
flags; in the function referencing configRegions, replace the asserted return
"AVAILABLE_REGIONS as Region[]" with a plain return of AVAILABLE_REGIONS (i.e.,
remove "as Region[]") since AVAILABLE_REGIONS is already typed as Region[];
update the return in the branch that checks configRegions?.includes('global')
and ensure no other redundant assertions remain for AVAILABLE_REGIONS or Region
in v4-config-parser.
---
Duplicate comments:
In `@packages/network-config/src/services/v4-config-parser.ts`:
- Around line 44-45: False positive ReDoS warning: safeRegex(pattern) already
guards the new RegExp(pattern) call and the surrounding try/catch handles
SyntaxError, so no functional change is needed; retain the safeRegex(pattern)
guard and the try/catch around new RegExp in v4-config-parser (referencing
safeRegex and the RegExp construction) and, if you want to silence the ast-grep
alert, add a short inline comment explaining the guard or an explicit linter
suppression next to the check.
---
Nitpick comments:
In `@packages/network-config/src/build-output-v3/get-default-path-map.ts`:
- Around line 28-47: The helper processDirectory is large and should be
converted from a const-assigned Effect.gen arrow to a named function declaration
to follow the project's style; refactor the const processDirectory (and its
Effect.gen body) into a function declaration (e.g., function
processDirectory(currentPath: string, basePath = '', depth = 0):
Effect.Effect<void>) preserving the same logic and references to MAX_DEPTH,
fs.readDirectory, fs.stat, pathService.join and the result mapping, and ensure
callers still invoke processDirectory(...) as before.
- Around line 4-13: The JSDoc for the exported function (getDefaultPathMap) is
missing an `@example`; update the JSDoc block above the exported function
(getDefaultPathMap) to include an `@example` that shows typical usage: calling the
function with a sample absolute directory path and the expected shape of the
returned Record<string,string> (absolute -> relative) so readers know
input/output format; keep the example concise and use realistic values and a
short note about asynchronous/sync behavior if applicable.
In `@packages/network-config/src/detection/detect-framework.test.ts`:
- Around line 206-223: The test name claims to exercise an invalid matchContent
regex but never injects one; update the test that calls
detectFramework('/project') (which currently composes fs via Layer.merge,
makeTestFs and TestPathLayer) to inject a framework detector with an invalid
matchContent pattern (e.g., a malformed regex string) into the detection
fixtures before calling detectFramework, or alternatively rename the test to
reflect the current behavior; target symbols: detectFramework, Layer.merge,
makeTestFs, and TestPathLayer so the injected bad detector is picked up by the
detection pipeline.
In `@packages/network-config/src/detection/detect-framework.ts`:
- Around line 90-100: The JSDoc for the exported function detectFramework is
missing an `@example`; add a concise example to the existing comment showing
typical usage (call detectFramework with a project root string and handling the
returned DetectionResult), keeping it brief and illustrating input and expected
shape of output; ensure the `@example` appears alongside the existing `@param` and
`@returns` tags in the JSDoc for export detectFramework.
In `@packages/network-config/src/detection/generate-config.test.ts`:
- Around line 145-168: The test allows a hidden secondary installer in later
commands; after calling generateConfig (used in tests referencing
generateConfig, symfony, and customInstallFramework) assert that every command
after index 0 does not contain any install operation (e.g., does not include the
word "install" or start with "composer install" / other installer verbs);
specifically check result.commands[0] remains the intended install entry and
then assert result.commands.slice(1).every(...) is true to ensure no subsequent
command is an install step.
In `@packages/network-config/src/detection/generate-config.ts`:
- Around line 25-33: Add an `@example` JSDoc block to the exported generateConfig
function showing a minimal usage example: call generateConfig with a sample
framework object (matching the framework type used in the file) and a
packageManager string (e.g., "npm" or "yarn"), demonstrating how to yield/await
the Effect result and showing the shape of the returned NormalizedConfig; update
the comment above export const generateConfig =
Effect.fn('generateConfig')(function* (...) to include this `@example` so
consumers see how to invoke generateConfig and what the resolved config looks
like.
In `@packages/network-config/src/detection/read-dependencies.ts`:
- Around line 40-53: Add an `@example` section to the JSDoc for the exported
function readDependencies showing a minimal usage example: call readDependencies
with a sample projectFolder path and a language constant (or string) and
demonstrate awaiting/handling the returned Set of dependency names; include both
sync/async usage form appropriate for Effect.fn (e.g., yielding/awaiting the
effect) and show expected return shape (Set<string>) so consumers understand how
to call readDependencies and what it returns. Ensure the new `@example` is placed
in the existing JSDoc block above the readDependencies declaration and follows
the same style as `@param/`@returns tags.
In `@packages/network-config/src/services/raw-config-reader.test.ts`:
- Around line 58-177: Add a regression test to ensure YAML values that parse to
null are handled: create a new it() in the readRawConfig suite that uses
runWithFs (or Effect.runPromise + Layer.merge(makeTestFs(...), TestPathLayer))
to supply a file (e.g., '/project/config.yaml') whose contents are 'null' and
another case with whitespace-only content, then call
RawConfigReader.readRawConfig and assert it fails with ConfigFileEmptyError (or
the expected error handling path) or returns an empty object when
disableVersionCheck is true; reference RawConfigReader.readRawConfig,
RawConfigReader.Default, runWithFs, makeTestFs and TestPathLayer to locate where
to add the test.
In `@packages/network-config/src/v4/parse.test.ts`:
- Around line 18-26: The helper readFixture is unnecessarily async due to the
dynamic await import('yaml'); replace that with a static top-level import (e.g.,
import { parse as parseYaml } from 'yaml'), remove the async keyword from
readFixture and the internal await, and make it return synchronously for
non-JSON files; update call sites that do "await readFixture(...)" to call
readFixture(...) without awaiting. Ensure the function still handles .json and
YAML by using parseYaml for YAML parsing and keep the return type Record<string,
unknown>.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (27)
packages/cli/CLAUDE.mdpackages/cli/src/services/project-config.test.tspackages/cli/src/services/project-config.tspackages/network-config/package.jsonpackages/network-config/src/build-output-v3/get-default-path-map.test.tspackages/network-config/src/build-output-v3/get-default-path-map.tspackages/network-config/src/detection/ADDING_FRAMEWORKS.mdpackages/network-config/src/detection/detect-framework.test.tspackages/network-config/src/detection/detect-framework.tspackages/network-config/src/detection/detect-package-manager.test.tspackages/network-config/src/detection/detect-package-manager.tspackages/network-config/src/detection/frameworks/symfony.tspackages/network-config/src/detection/frameworks/vite.tspackages/network-config/src/detection/generate-config.test.tspackages/network-config/src/detection/generate-config.tspackages/network-config/src/detection/read-dependencies.test.tspackages/network-config/src/detection/read-dependencies.tspackages/network-config/src/detection/types.tspackages/network-config/src/services/raw-config-reader.test.tspackages/network-config/src/services/raw-config-reader.tspackages/network-config/src/services/schema-validator.tspackages/network-config/src/services/v4-config-parser.test.tspackages/network-config/src/services/v4-config-parser.tspackages/network-config/src/services/vercel-build-output-parser.test.tspackages/network-config/src/services/vercel-build-output-parser.tspackages/network-config/src/test-utils.tspackages/network-config/src/v4/parse.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/cli/CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/network-config/src/build-output-v3/get-default-path-map.test.ts
- packages/network-config/src/services/vercel-build-output-parser.test.ts
- packages/network-config/package.json
- packages/network-config/src/services/schema-validator.ts
- packages/network-config/src/detection/detect-package-manager.test.ts
- packages/network-config/src/detection/detect-package-manager.ts
- packages/network-config/src/services/v4-config-parser.test.ts
- packages/network-config/src/detection/frameworks/vite.ts
- packages/network-config/src/detection/frameworks/symfony.ts
- packages/network-config/src/detection/types.ts
- packages/network-config/src/detection/read-dependencies.test.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/network-config/src/services/v4-config-parser.ts (1)
225-254:⚠️ Potential issue | 🟡 MinorYield-less
Effect.gen— Biome CI still failing.The outer
Effect.gen(function* () { ... })at Line 225 never callsyield*, so Biome reportslint/correctness/useYield. Replace withEffect.succeed.🐛 Proposed fix
- effect: Effect.gen(function* () { - const parse = Effect.fn('V4ConfigParser.parse')(function* (config: ConfigV4, projectFolder: string) { - const entrypoints = yield* parseEntrypoints(config, projectFolder); - const assets = yield* collectAssets(config, projectFolder); - - return { - regions: resolveRegions(config.regions), - assets: { - paths: assets.sort(), - prefixToStrip: (config.assets ?? '') + '/', - dynamicRoutes: true, - populateCache: config.populateAssetCache ?? false, - }, - environmentVariables: config.env ?? {}, - commands: config.build_commands ?? [], - entrypoints, - errors: [], - warnings: [], - routes: (config.routes ?? []).map(mapRoute), - } as NormalizedConfig; - }); - - return { parse }; - }), + effect: Effect.succeed({ + parse: Effect.fn('V4ConfigParser.parse')(function* (config: ConfigV4, projectFolder: string) { + const entrypoints = yield* parseEntrypoints(config, projectFolder); + const assets = yield* collectAssets(config, projectFolder); + + return { + regions: resolveRegions(config.regions), + assets: { + paths: assets.sort(), + prefixToStrip: (config.assets ?? '') + '/', + dynamicRoutes: true, + populateCache: config.populateAssetCache ?? false, + }, + environmentVariables: config.env ?? {}, + commands: config.build_commands ?? [], + entrypoints, + errors: [], + warnings: [], + routes: (config.routes ?? []).map(mapRoute), + } as NormalizedConfig; + }), + }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/services/v4-config-parser.ts` around lines 225 - 254, The outer wrapper uses Effect.gen(function* () { ... }) but never yields, causing the lint error; replace the generator wrapper with a simple Effect.succeed returning the parse effect. Concretely, keep the inner const parse = Effect.fn('V4ConfigParser.parse')(function* (...) { ... }) as-is, then return Effect.succeed({ parse }) instead of using Effect.gen; ensure the final exported object is the Effect created by Effect.succeed and that parseEntrypoints, collectAssets and mapRoute usages remain unchanged.
🧹 Nitpick comments (1)
packages/network-config/src/services/v4-config-parser.ts (1)
117-145: Non-null assertions (settings!) are avoidable withreturn yield*.Because
yield* new FunctionConfigError(...)(Line 118) lacks areturn, TypeScript's control-flow analysis doesn't treat it as a terminator, leavingsettingsasConfigV4FunctionSettings | undefinedin the rest of the loop body. Addingreturnmakes TypeScript narrow correctly and eliminates the five!assertions.♻️ Proposed refactor
if (settings == null) { - yield* new FunctionConfigError({ + return yield* new FunctionConfigError({ message: `Settings invalid for function at path '${fnPath}'`, functionPath: fnPath, }); } ... entrypoints.push({ path: file, - runtime: settings!.runtime ?? 'node-20', - memory: settings!.memory ?? 128, - maxDuration: settings!.max_duration ?? 30, + runtime: settings.runtime ?? 'node-20', + memory: settings.memory ?? 128, + maxDuration: settings.max_duration ?? 30, schedule: func.schedule, symlinks: func.symlinks, streaming: - settings!.runtime == null || settings!.runtime.startsWith('node-') || settings!.runtime.startsWith('bun-'), + settings.runtime == null || settings.runtime.startsWith('node-') || settings.runtime.startsWith('bun-'), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/services/v4-config-parser.ts` around lines 117 - 145, The null-check branch that currently does "yield* new FunctionConfigError({...})" doesn't terminate control flow so TypeScript can't narrow "settings" and the code uses several non-null assertions ("settings!"); change that line to "return yield* new FunctionConfigError({...})" so the function exits on error and TypeScript will treat "settings" as defined, then remove the five "settings!" assertions in the entrypoint push (references: settings, FunctionConfigError, entrypoints, matchedFiles, fnPath) so runtime/defaults are used without non-null bangs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/network-config/src/services/v4-config-parser.ts`:
- Around line 225-254: The outer wrapper uses Effect.gen(function* () { ... })
but never yields, causing the lint error; replace the generator wrapper with a
simple Effect.succeed returning the parse effect. Concretely, keep the inner
const parse = Effect.fn('V4ConfigParser.parse')(function* (...) { ... }) as-is,
then return Effect.succeed({ parse }) instead of using Effect.gen; ensure the
final exported object is the Effect created by Effect.succeed and that
parseEntrypoints, collectAssets and mapRoute usages remain unchanged.
---
Nitpick comments:
In `@packages/network-config/src/services/v4-config-parser.ts`:
- Around line 117-145: The null-check branch that currently does "yield* new
FunctionConfigError({...})" doesn't terminate control flow so TypeScript can't
narrow "settings" and the code uses several non-null assertions ("settings!");
change that line to "return yield* new FunctionConfigError({...})" so the
function exits on error and TypeScript will treat "settings" as defined, then
remove the five "settings!" assertions in the entrypoint push (references:
settings, FunctionConfigError, entrypoints, matchedFiles, fnPath) so
runtime/defaults are used without non-null bangs.
- Guard parseYaml result against null (loose equality check) - Use full entrypoint path for route destinations in Vercel parser - Replace yield-less Effect.gen with Effect.succeed in V4ConfigParser - Add language tag to fenced code block in ADDING_FRAMEWORKS.md - Clarify install-command exception for platform-specific frameworks Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/network-config/src/services/raw-config-reader.ts (1)
64-95:⚠️ Potential issue | 🟠 Major
parsedstill typed asRecord<string, unknown>, making thenullguard on Line 83 invisible to the type-checker.Both
JSON.parseandparseYamlcan return non-object primitives (e.g.null, a bare string), but all three branches cast the resultas Record<string, unknown>. Understrict: true/strictNullChecks, TypeScript then treatsparsedas never-null, so theif (parsed == null)guard on Line 83 is dead code from the compiler's perspective — defeating the safety it provides at runtime. The previous review already recommended changing the declared type tounknown.🐛 Proposed fix
- let parsed: Record<string, unknown>; + let parsed: unknown; try { if (fileExtension === '.json') { - parsed = JSON.parse(fileContents) as Record<string, unknown>; + parsed = JSON.parse(fileContents); } else { - parsed = parseYaml(fileContents) as Record<string, unknown>; + parsed = parseYaml(fileContents); } } catch (error) { ... } - if (parsed == null) { + if (parsed == null || typeof parsed !== 'object' || Array.isArray(parsed)) { return yield* Effect.fail( new ConfigFileParseError({ message: `Config file could not be parsed at ${filePath}`, filePath }) ); } + const parsedRecord = parsed as Record<string, unknown>; - if (!options?.disableVersionCheck && typeof parsed.version !== 'number') { + if (!options?.disableVersionCheck && typeof parsedRecord.version !== 'number') { ... } - return parsed; + return parsedRecord;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/services/raw-config-reader.ts` around lines 64 - 95, Change the declared type of parsed from Record<string, unknown> to unknown and stop force-casting JSON.parse/parseYaml results; after parsing, narrow parsed with proper runtime checks (e.g., ensure parsed is non-null and typeof parsed === 'object' before treating it as a record), then use a type-guard or local const typed as Record<string, unknown> to check parsed.version (used by the existing ConfigVersionError branch) and to satisfy the subsequent null and version checks; keep the existing error handling using ConfigFileParseError and ConfigVersionError but rely on these runtime narrows instead of the initial cast so the if (parsed == null) guard is effective to the TypeScript compiler.
🧹 Nitpick comments (5)
packages/network-config/src/services/raw-config-reader.ts (2)
24-27: Redundant type assertions on return values.
filePathis alreadystringandnullis alreadynull; theas string | nullcasts convey no additional information.♻️ Proposed fix
- return filePath as string | null; + return filePath; } } - return null as string | null; + return null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/services/raw-config-reader.ts` around lines 24 - 27, Remove the redundant type assertions on the return statements: simply return filePath and return null instead of using "as string | null". Locate the return statements that currently cast filePath and null (references to the filePath variable and the return of null in raw-config-reader) and delete the "as string | null" casts so the values are returned with their natural types.
66-72: Collapse redundantelse if/elsebranches — both invokeparseYaml.The
else if (['.yml', '.yaml'].includes(fileExtension))and the finalelseblock are identical. A singleelseis sufficient (JSON path explicit, everything else YAML).♻️ Proposed refactor
if (fileExtension === '.json') { parsed = JSON.parse(fileContents); - } else if (['.yml', '.yaml'].includes(fileExtension)) { - parsed = parseYaml(fileContents); } else { parsed = parseYaml(fileContents); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/services/raw-config-reader.ts` around lines 66 - 72, The branch in raw-config-reader.ts that checks fileExtension has redundant clauses; simplify the logic in the function where parsed is assigned by replacing the chain "if (fileExtension === '.json') { parsed = JSON.parse(...) } else if (['.yml', '.yaml'].includes(fileExtension)) { parsed = parseYaml(...) } else { parsed = parseYaml(...) }" with a clear two-way branch: keep the explicit JSON check using JSON.parse, and use a single else to call parseYaml(fileContents). Ensure references to fileExtension, parsed, JSON.parse and parseYaml remain intact and behavior unchanged for non-JSON inputs.packages/network-config/src/services/vercel-build-output-parser.ts (1)
125-127: Redundant null guards —entrypointsandroutesare always initialized.Both
result.entrypoints(Line 67) andresult.routes(Line 68) are initialized as empty arrays at the top ofloadFunctions. The guards on Lines 125-127 and 168-170 are unreachable dead code.♻️ Suggested cleanup
- if (!result.entrypoints) { - result.entrypoints = []; - } - result.entrypoints.push({- if (!result.routes) { - result.routes = []; - } - result.routes.push(defaultRoute, ...configRoutes);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/services/vercel-build-output-parser.ts` around lines 125 - 127, The null guards for result.entrypoints and result.routes are redundant because loadFunctions initializes them as empty arrays; remove the if (!result.entrypoints) { result.entrypoints = []; } and the equivalent guard for result.routes to clean up unreachable dead code in the loadFunctions function while leaving the initial assignments intact.packages/network-config/src/services/v4-config-parser.ts (2)
248-250: Prefersatisfies NormalizedConfigoveras NormalizedConfigto retain structural type checking.The
asassertion suppresses TypeScript errors if any field is missing, renamed, or has an incompatible type. Usingsatisfies(or an explicit return type on theEffect.fncallback) keeps the cast-free structural check.♻️ Proposed refactor
- return { + return ({ regions: resolveRegions(config.regions), assets: { paths: assets.sort(), prefixToStrip: (config.assets ?? '') + '/', dynamicRoutes: true, populateCache: config.populateAssetCache ?? false, }, environmentVariables: config.env ?? {}, commands: config.build_commands ?? [], entrypoints, errors: [], warnings: [], routes: (config.routes ?? []).map(mapRoute), - } as NormalizedConfig; + }) satisfies NormalizedConfig;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/services/v4-config-parser.ts` around lines 248 - 250, The object literal currently being forced with "as NormalizedConfig" should use a type-preserving check: replace the "as NormalizedConfig" assertion with "satisfies NormalizedConfig" (or alternatively give the enclosing Effect.fn callback an explicit return type of NormalizedConfig) so missing/incorrect fields like warnings, routes or the mapped entries from mapRoute/config.routes are caught by the compiler; update the expression that builds { warnings: [], routes: (config.routes ?? []).map(mapRoute) } to use the satisfies form or ensure the function return type is NormalizedConfig.
225-252: Yield-lessEffect.gencorrectly replaced withEffect.succeed— LGTM.The prior Biome
lint/correctness/useYielderror is resolved.Prefer
satisfies NormalizedConfigoveras NormalizedConfigto preserve structural type checking.The
asassertion on Line 250 suppresses TypeScript errors if any required field ofNormalizedConfigis missing or type-incompatible.satisfiesprovides the same hint to readers while keeping the compiler check intact.♻️ Proposed refactor
- return { + return ({ regions: resolveRegions(config.regions), assets: { paths: assets.sort(), prefixToStrip: (config.assets ?? '') + '/', dynamicRoutes: true, populateCache: config.populateAssetCache ?? false, }, environmentVariables: config.env ?? {}, commands: config.build_commands ?? [], entrypoints, errors: [], warnings: [], routes: (config.routes ?? []).map(mapRoute), - } as NormalizedConfig; + }) satisfies NormalizedConfig;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/services/v4-config-parser.ts` around lines 225 - 252, Replace the unsafe type assertion that suppresses structural checks: in the Effect.fn('V4ConfigParser.parse') implementation return object, change the trailing "as NormalizedConfig" to use TypeScript's "satisfies NormalizedConfig" so the returned object retains full compiler structural checking while keeping the intended type hint for the parser function (refer to the parse function inside the V4ConfigParser.parse Effect and the NormalizedConfig type).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/network-config/src/services/raw-config-reader.ts`:
- Line 6: The exported constant ALLOWED_CONFIG_NAMES should be a readonly tuple
of string literals so callers can derive a literal union; update the declaration
of ALLOWED_CONFIG_NAMES (the exported const in raw-config-reader.ts) to use an
"as const" assertion on the array (making it readonly and preserving each
element's literal type) so downstream code can infer the union type
`'gigadrive.yaml' | 'gigadrive.yml' | 'nebula.yaml' | 'nebula.yml' |
'nebula.json'`.
In `@packages/network-config/src/services/vercel-build-output-parser.ts`:
- Line 49: deepMerge is currently concatenating arrays from parseResult and the
object returned by loadFunctions, causing duplicate entries (notably
parseResult.regions) after the merge; update the merge logic so array fields are
deduplicated: either remove regions/entrypoints/routes from the object returned
by loadFunctions and merge them separately with a deduplication step, or run
deduplication immediately after deepMerge (e.g., produce result =
deepMerge(parseResult, functionData, assetOverrides) and then set
result.regions/entrypoints/routes to unique arrays built from their values);
focus changes around the deepMerge call, loadFunctions output, and ensure you
reference parseResult.regions and result.regions when implementing
deduplication.
---
Duplicate comments:
In `@packages/network-config/src/services/raw-config-reader.ts`:
- Around line 64-95: Change the declared type of parsed from Record<string,
unknown> to unknown and stop force-casting JSON.parse/parseYaml results; after
parsing, narrow parsed with proper runtime checks (e.g., ensure parsed is
non-null and typeof parsed === 'object' before treating it as a record), then
use a type-guard or local const typed as Record<string, unknown> to check
parsed.version (used by the existing ConfigVersionError branch) and to satisfy
the subsequent null and version checks; keep the existing error handling using
ConfigFileParseError and ConfigVersionError but rely on these runtime narrows
instead of the initial cast so the if (parsed == null) guard is effective to the
TypeScript compiler.
---
Nitpick comments:
In `@packages/network-config/src/services/raw-config-reader.ts`:
- Around line 24-27: Remove the redundant type assertions on the return
statements: simply return filePath and return null instead of using "as string |
null". Locate the return statements that currently cast filePath and null
(references to the filePath variable and the return of null in
raw-config-reader) and delete the "as string | null" casts so the values are
returned with their natural types.
- Around line 66-72: The branch in raw-config-reader.ts that checks
fileExtension has redundant clauses; simplify the logic in the function where
parsed is assigned by replacing the chain "if (fileExtension === '.json') {
parsed = JSON.parse(...) } else if (['.yml', '.yaml'].includes(fileExtension)) {
parsed = parseYaml(...) } else { parsed = parseYaml(...) }" with a clear two-way
branch: keep the explicit JSON check using JSON.parse, and use a single else to
call parseYaml(fileContents). Ensure references to fileExtension, parsed,
JSON.parse and parseYaml remain intact and behavior unchanged for non-JSON
inputs.
In `@packages/network-config/src/services/v4-config-parser.ts`:
- Around line 248-250: The object literal currently being forced with "as
NormalizedConfig" should use a type-preserving check: replace the "as
NormalizedConfig" assertion with "satisfies NormalizedConfig" (or alternatively
give the enclosing Effect.fn callback an explicit return type of
NormalizedConfig) so missing/incorrect fields like warnings, routes or the
mapped entries from mapRoute/config.routes are caught by the compiler; update
the expression that builds { warnings: [], routes: (config.routes ??
[]).map(mapRoute) } to use the satisfies form or ensure the function return type
is NormalizedConfig.
- Around line 225-252: Replace the unsafe type assertion that suppresses
structural checks: in the Effect.fn('V4ConfigParser.parse') implementation
return object, change the trailing "as NormalizedConfig" to use TypeScript's
"satisfies NormalizedConfig" so the returned object retains full compiler
structural checking while keeping the intended type hint for the parser function
(refer to the parse function inside the V4ConfigParser.parse Effect and the
NormalizedConfig type).
In `@packages/network-config/src/services/vercel-build-output-parser.ts`:
- Around line 125-127: The null guards for result.entrypoints and result.routes
are redundant because loadFunctions initializes them as empty arrays; remove the
if (!result.entrypoints) { result.entrypoints = []; } and the equivalent guard
for result.routes to clean up unreachable dead code in the loadFunctions
function while leaving the initial assignments intact.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/network-config/src/detection/ADDING_FRAMEWORKS.mdpackages/network-config/src/services/raw-config-reader.tspackages/network-config/src/services/v4-config-parser.tspackages/network-config/src/services/vercel-build-output-parser.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/network-config/src/detection/ADDING_FRAMEWORKS.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/network-config/src/services/vercel-build-output-parser.ts (1)
49-49:⚠️ Potential issue | 🟠 MajorRegion merge currently re-introduces duplicates through
deepMerge.At Line 69,
result.regionsstarts fromparseResult.regions, then Line 49 merges back intoparseResultusingdeepMerge(array concatenation), so existing regions are duplicated.🔧 Proposed fix
- const result: Partial<NormalizedConfig> = { + const result: Partial<NormalizedConfig> = { entrypoints: [], routes: [], - regions: [...parseResult.regions], + regions: [], errors: [], }; @@ - return deepMerge(parseResult, functionData, assetOverrides); + const merged = deepMerge(parseResult, functionData, assetOverrides); + merged.regions = Array.from(new Set(merged.regions ?? [])) as Region[]; + return merged;Use this script to verify the behavior from source:
#!/bin/bash set -euo pipefail # Confirm seeded regions and deepMerge call site in parser rg -n -C2 'regions:\s*\[\.\.\.parseResult\.regions\]|deepMerge\(parseResult' packages/network-config/src/services/vercel-build-output-parser.ts # Confirm deepMerge concatenates arrays (pushes source into target) rg -n -C3 'Array\.isArray\(targetValue\)\s*&&\s*Array\.isArray\(sourceValue\)|push\(\.\.\.' packages/commons/src/deep-merge.tsAlso applies to: 66-70, 105-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/services/vercel-build-output-parser.ts` at line 49, The merge reintroduces duplicate regions because deepMerge concatenates arrays when merging parseResult back into result; to fix, avoid merging the regions array—either remove/omit parseResult.regions before calling deepMerge or perform the deepMerge into an object that already has result.regions so deepMerge doesn’t append parseResult.regions; update the call site that uses deepMerge(parseResult, functionData, assetOverrides) to exclude the regions key (or use a replace-array merge strategy) so result.regions remains the seeded unique array (referencing deepMerge, parseResult, and result.regions in vercel-build-output-parser.ts).packages/network-config/src/services/raw-config-reader.ts (1)
64-95:⚠️ Potential issue | 🟠 MajorReject non-object parse results before version checks/return.
At Line 64 and Line 83,
parsedis assumed to be a record, but onlynullis rejected. Arrays/scalars can still flow through (especially viareadConfigFilewhere version checks are disabled), which breaks the declared return shape.🔧 Proposed fix
- let parsed: Record<string, unknown>; + let parsed: unknown; try { if (fileExtension === '.json') { - parsed = JSON.parse(fileContents) as Record<string, unknown>; + parsed = JSON.parse(fileContents); } else if (['.yml', '.yaml'].includes(fileExtension)) { - parsed = parseYaml(fileContents) as Record<string, unknown>; + parsed = parseYaml(fileContents); } else { - parsed = parseYaml(fileContents) as Record<string, unknown>; + parsed = parseYaml(fileContents); } } catch (error) { return yield* Effect.fail( @@ - if (parsed == null) { + if (parsed == null || typeof parsed !== 'object' || Array.isArray(parsed)) { return yield* Effect.fail( new ConfigFileParseError({ message: `Config file could not be parsed at ${filePath}`, filePath }) ); } + const parsedRecord = parsed as Record<string, unknown>; - if (!options?.disableVersionCheck && typeof parsed.version !== 'number') { + if (!options?.disableVersionCheck && typeof parsedRecord.version !== 'number') { return yield* Effect.fail( new ConfigVersionError({ message: `Config file is missing version at ${filePath}`, filePath }) ); } - return parsed; + return parsedRecord;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/services/raw-config-reader.ts` around lines 64 - 95, The parsed result from JSON/YAML may be null, an array, or a scalar so update the validation after parsing in raw-config-reader.ts to reject any non-object (i.e., not an object, null, or Array) before running version checks or returning; specifically change the check around the parsed variable to assert typeof parsed === 'object' && parsed !== null && !Array.isArray(parsed) and, if that fails, yield* Effect.fail with a ConfigFileParseError (similar to the existing error cases) so that functions like readConfigFile and the subsequent ConfigVersionError only see actual record objects.
🧹 Nitpick comments (3)
packages/network-config/src/services/v4-config-parser.ts (3)
54-60: Complete JSDoc tags for exported APIs.
getFunctionSettingsand the exportedparsemethod are missing required JSDoc tags (@param,@returns,@example) in their current blocks.📝 Suggested doc updates
/** * Resolves merged function settings for a given path by iterating all function * patterns in the config. Later patterns override earlier ones. + * + * `@param` path - File path to resolve settings for. + * `@param` config - Parsed V4 config object. * * `@returns` The merged settings, or undefined when no pattern matches the path. + * + * `@example` + * const settings = getFunctionSettings('api/hello.ts', config); */ export const getFunctionSettings = (path: string, config: ConfigV4): ConfigV4FunctionSettings | undefined => {/** * Parses a ConfigV4 into a NormalizedConfig. * * `@param` config - The raw V4 config object * `@param` projectFolder - Absolute path to the project root + * `@returns` A normalized config object for downstream deployment/runtime consumers. + * + * `@example` + * const normalized = yield* parser.parse(config, '/workspace/project'); */ parse: Effect.fn('V4ConfigParser.parse')(function* (config: ConfigV4, projectFolder: string) {As per coding guidelines: Document exported functions with JSDoc comments including
@param,@returns, and@exampletags.Also applies to: 226-231
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/services/v4-config-parser.ts` around lines 54 - 60, Update the JSDoc for the exported functions to include complete tags: add `@param` entries describing the parameters (their names and types) and expected values for getFunctionSettings(path: string, config: ConfigV4) and the exported parse function, add `@returns` describing the return type and when undefined is returned, and add a short `@example` showing typical usage for each function (calling getFunctionSettings with a sample path/config and calling parse with a sample input). Make sure the param names match the function signatures (path, config) and reference the concrete return type ConfigV4FunctionSettings (or undefined) so tools can pick up the types.
47-49: Avoid barecatch {}inmatchesPattern.Line 47 uses bare
catch {}without re-throwing. Switch tocatch (_error)(or handleerror) to comply with repository error-handling rules.As per coding guidelines: Use bare
catch { }without a variable only when re-throwing with a custom message.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/services/v4-config-parser.ts` around lines 47 - 49, In matchesPattern, replace the bare catch block with a catch that declares the error (e.g. catch (_error) or catch (error)) so the exception variable is captured per repo rules; update the catch in the matchesPattern function to use a named parameter (and optionally ignore or log it) instead of bare `catch {}` to satisfy error-handling guidelines.
114-136: Precompute function keys once inparseEntrypoints.Line 135 recomputes/scans function keys for every matched file. A precomputed
Setkeeps lookup O(1) and simplifies the inner loop.♻️ Optional micro-optimization
const parseEntrypoints = Effect.fn('parseEntrypoints')(function* (config: ConfigV4, projectFolder: string) { const entrypoints: NormalizedConfigEntrypoint[] = []; if (config.functions == null) return entrypoints; + const functionKeys = new Set(Object.keys(config.functions)); for (const [fnPath, func] of Object.entries(config.functions)) { @@ for (const file of matchedFiles) { if (entrypoints.some((ep) => ep.path === file)) continue; - if (Object.keys(config.functions).some((f) => f === file && f !== fnPath)) continue; + if (functionKeys.has(file) && file !== fnPath) continue;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/services/v4-config-parser.ts` around lines 114 - 136, The inner loop in parseEntrypoints currently recalculates Object.keys(config.functions) for every matched file (seen in the check Object.keys(config.functions).some((f) => f === file && f !== fnPath)), causing repeated scans; precompute a Set of function keys (e.g., functionPathsSet or functionKeys) once before iterating functions in parseEntrypoints and replace that check with a constant-time lookup (functionKeys.has(file) && file !== fnPath) to avoid repeated Object.keys scans and simplify the inner loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/network-config/src/services/v4-config-parser.ts`:
- Around line 86-89: The resolveRegions function currently coerces string[] to
Region[] with an unsafe as Region[] cast; remove that cast and instead validate
input strings against AVAILABLE_REGIONS and return a properly narrowed Region[]
(or the full AVAILABLE_REGIONS default). Implement a type guard like isRegion(r:
string): r is Region that checks AVAILABLE_REGIONS.includes(r as Region), then
use configRegions?.filter(isRegion) to produce a Region[]; if no valid regions
remain, return AVAILABLE_REGIONS. Update resolveRegions to use this filtering
and drop the unsafe assertion.
In `@packages/network-config/src/services/vercel-build-output-parser.ts`:
- Around line 42-44: The version gate in vercel-build-output-parser.ts is too
lax: replace the current check that uses "config.version < 3" with a hardened
guard that also rejects non-numeric or missing versions; specifically update the
conditional around the existing config check (the block that returns
parseResult) to verify typeof config.version === 'number' (or
!isNaN(Number(config.version))) and only allow numeric versions >= 3 to proceed,
so functions that reference config and the parseResult early-return path behave
safely.
---
Duplicate comments:
In `@packages/network-config/src/services/raw-config-reader.ts`:
- Around line 64-95: The parsed result from JSON/YAML may be null, an array, or
a scalar so update the validation after parsing in raw-config-reader.ts to
reject any non-object (i.e., not an object, null, or Array) before running
version checks or returning; specifically change the check around the parsed
variable to assert typeof parsed === 'object' && parsed !== null &&
!Array.isArray(parsed) and, if that fails, yield* Effect.fail with a
ConfigFileParseError (similar to the existing error cases) so that functions
like readConfigFile and the subsequent ConfigVersionError only see actual record
objects.
In `@packages/network-config/src/services/vercel-build-output-parser.ts`:
- Line 49: The merge reintroduces duplicate regions because deepMerge
concatenates arrays when merging parseResult back into result; to fix, avoid
merging the regions array—either remove/omit parseResult.regions before calling
deepMerge or perform the deepMerge into an object that already has
result.regions so deepMerge doesn’t append parseResult.regions; update the call
site that uses deepMerge(parseResult, functionData, assetOverrides) to exclude
the regions key (or use a replace-array merge strategy) so result.regions
remains the seeded unique array (referencing deepMerge, parseResult, and
result.regions in vercel-build-output-parser.ts).
---
Nitpick comments:
In `@packages/network-config/src/services/v4-config-parser.ts`:
- Around line 54-60: Update the JSDoc for the exported functions to include
complete tags: add `@param` entries describing the parameters (their names and
types) and expected values for getFunctionSettings(path: string, config:
ConfigV4) and the exported parse function, add `@returns` describing the return
type and when undefined is returned, and add a short `@example` showing typical
usage for each function (calling getFunctionSettings with a sample path/config
and calling parse with a sample input). Make sure the param names match the
function signatures (path, config) and reference the concrete return type
ConfigV4FunctionSettings (or undefined) so tools can pick up the types.
- Around line 47-49: In matchesPattern, replace the bare catch block with a
catch that declares the error (e.g. catch (_error) or catch (error)) so the
exception variable is captured per repo rules; update the catch in the
matchesPattern function to use a named parameter (and optionally ignore or log
it) instead of bare `catch {}` to satisfy error-handling guidelines.
- Around line 114-136: The inner loop in parseEntrypoints currently recalculates
Object.keys(config.functions) for every matched file (seen in the check
Object.keys(config.functions).some((f) => f === file && f !== fnPath)), causing
repeated scans; precompute a Set of function keys (e.g., functionPathsSet or
functionKeys) once before iterating functions in parseEntrypoints and replace
that check with a constant-time lookup (functionKeys.has(file) && file !==
fnPath) to avoid repeated Object.keys scans and simplify the inner loop.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/network-config/src/detection/ADDING_FRAMEWORKS.mdpackages/network-config/src/services/raw-config-reader.tspackages/network-config/src/services/v4-config-parser.tspackages/network-config/src/services/vercel-build-output-parser.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/network-config/src/detection/ADDING_FRAMEWORKS.md
…t to ALLOWED_CONFIG_NAMES The deepMerge call in VercelBuildOutputParser was duplicating regions because loadFunctions pre-seeded its regions array with parseResult.regions, and deepMerge concatenates arrays. Initialize with an empty array instead and deduplicate after the merge. Also add `as const` to ALLOWED_CONFIG_NAMES so downstream code can derive a string literal union type. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extract parseConfigRaw (parse + validate without post-processing) so Case A can parse the user config raw, merge with framework defaults, then run postProcessConfig on the merged result. This prevents stale pre-merge errors (e.g. "no functions or assets") from persisting after framework defaults fill in the gaps. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…veRegions Add isRegion type guard to validate config strings against AVAILABLE_REGIONS, replacing the unsafe `as Region[]` assertion. Invalid region strings are now filtered out, falling back to AVAILABLE_REGIONS when no valid regions remain. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…eric versions Add typeof check so missing or non-numeric version fields (e.g. undefined, string "3") are rejected alongside versions below 3. Add tests covering both cases. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/network-config/src/detection/ADDING_FRAMEWORKS.md (1)
36-36: Add at least one example showingpmin use.Every
getDefaultConfigexample (lines 36, 209, 234, 264, 298) writes() => ({...})and silently ignores thepmargument. The reference table on Line 101 documents the signature as(pm: PackageManager) => FrameworkDefaultConfigand says the argument "lets you customise install commands per package manager if needed" — but there is no worked example of how to do that. A contributor adding a framework that needs, for example,yarn buildvspnpm buildhas nothing to copy.Consider adding a short inline snippet, e.g., in the Step-by-step example or as a dedicated sub-section:
- getDefaultConfig: () => ({ + getDefaultConfig: (pm) => ({ runtime: 'node-22', memory: 128, maxDuration: 30, streaming: true, - commands: ['my-framework build'], + commands: [`${pm} run build`], // use pm when the command differs per package manager entrypoint: 'dist/index.js',Also applies to: 101-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/network-config/src/detection/ADDING_FRAMEWORKS.md` at line 36, The examples for getDefaultConfig currently ignore the pm argument; update at least one getDefaultConfig example (and ideally the others) to accept pm: PackageManager and show using pm to choose install/build commands (e.g., conditionally returning install/build strings based on pm === 'yarn' vs 'pnpm' or using a helper like pm.run). Modify the function signature in the example to getDefaultConfig: (pm) => ({...}) and include a short snippet in the Step-by-step or a new subsection demonstrating how to map pm to package-manager-specific commands so contributors can copy the pattern; reference getDefaultConfig, PackageManager, FrameworkDefaultConfig and pm in the added example.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/services/project-config.ts`:
- Around line 70-78: The current detectFramework pipeline unconditionally
converts ManifestReadError and ManifestParseError to null which masks real
detection failures and later surfaces as ConfigNotFoundError; update the
Effect.catchTag handlers for ManifestReadError and ManifestParseError inside the
detectFramework pipeline to only swallow the error when it clearly indicates a
missing config/manifest (e.g., an underlying ENOENT/file-not-found flag or a
specific "manifest not present" flag on the error), otherwise rethrow the
original error (use Effect.fail(e) or rethrowing within the handler) so genuine
read/parse failures propagate instead of becoming null and turning into
ConfigNotFoundError. Ensure you reference detectFramework, ManifestReadError,
ManifestParseError, and ConfigNotFoundError when making the change.
---
Nitpick comments:
In `@packages/network-config/src/detection/ADDING_FRAMEWORKS.md`:
- Line 36: The examples for getDefaultConfig currently ignore the pm argument;
update at least one getDefaultConfig example (and ideally the others) to accept
pm: PackageManager and show using pm to choose install/build commands (e.g.,
conditionally returning install/build strings based on pm === 'yarn' vs 'pnpm'
or using a helper like pm.run). Modify the function signature in the example to
getDefaultConfig: (pm) => ({...}) and include a short snippet in the
Step-by-step or a new subsection demonstrating how to map pm to
package-manager-specific commands so contributors can copy the pattern;
reference getDefaultConfig, PackageManager, FrameworkDefaultConfig and pm in the
added example.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/cli/src/services/project-config.test.tspackages/cli/src/services/project-config.tspackages/network-config/src/detection/ADDING_FRAMEWORKS.mdpackages/network-config/src/parse-config.tspackages/network-config/src/services/raw-config-reader.tspackages/network-config/src/services/v4-config-parser.tspackages/network-config/src/services/vercel-build-output-parser.test.tspackages/network-config/src/services/vercel-build-output-parser.ts
…g them ManifestReadError and ManifestParseError always indicate genuine problems (file exists but is unreadable or malformed JSON). Previously these were swallowed and converted to null, masking real failures and producing a misleading ConfigNotFoundError. Now they propagate as ConfigParseError with an actionable message. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Merged
This was referenced Jul 3, 2026
This was referenced Jul 20, 2026
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
1 participant
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
detection/module in@gigadrive/network-configthat automatically detects web frameworks (Next.js, Nuxt, Remix, SvelteKit, Astro, Hono, Elysia, Express, Fastify, NestJS, Vite, Laravel, Symfony) from project dependencies and generates deployment configs without requiring agigadrive.yaml.RawConfigReader,SchemaValidator,V4ConfigParser,VercelBuildOutputParser) to Effect services with@effect/platformFileSystem, replacing directnode:fsusage andmock-fstest dependency with an in-memory FileSystem layer.ProjectConfigServiceto handle four resolution cases: config file + detected framework (merge), config file only, framework only (auto-detect), and neither (error) — enabling zero-config deployments for supported frameworks.Changes
New: Framework detection (
packages/network-config/src/detection/)detect-framework.ts— reads project manifests, matches against framework definitions, returns configdetect-package-manager.ts— detects npm/yarn/pnpm/bun from lockfilesread-dependencies.ts— readspackage.json/composer.jsondependencies via Effect FileSystemgenerate-config.ts— convertsFrameworkDefaultConfigtoNormalizedConfigmerge-config.ts— merges user config with framework defaults (user always wins)types.ts—FrameworkDefinition,FrameworkDefaultConfig, detection typesframeworks/— 13 framework definitions (nextjs, nuxt, remix, sveltekit, astro, vite, hono, elysia, express, fastify, nestjs, laravel, symfony)ADDING_FRAMEWORKS.md— contributor guide for adding new frameworksRefactored: Config parsing as Effect services (
packages/network-config/src/services/)RawConfigReader— finds and reads config files via FileSystemSchemaValidator— AJV-based schema validationV4ConfigParser— parses v4 config formatVercelBuildOutputParser— parses Vercel Build Output v3 (replaces deletedbuild-output-v3/parse.tsandread-config-file.ts)Updated
parse-config.ts— now an Effect function; addedpostProcessConfig()for Vercel BOv3 post-processingpackages/cli/src/services/project-config.ts— four-case resolution logic with framework mergingpackages/cli/src/index.ts— wiresNetworkConfigLivelayermock-fsdependency; tests use in-memoryFileSystemlayer (test-utils.ts)AGENTS.mdwith Effect design decisions and detection architecture docsSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests