feat(cli): support exclude patterns for compute build context - #6658
Open
johnstonmatt wants to merge 9 commits into
Open
johnstonmatt wants to merge 9 commits into
johnstonmatt wants to merge 9 commits into
Conversation
`[compute.<name>]` gains an `exclude` list of patterns naming paths to leave out of the build context a deploy uploads, so a project can keep secrets and generated trees out of the image. Patterns are carried verbatim rather than validated here: unlike `instances`, a pattern this layer cannot read is never silently dropped, so the CLI is free to refuse a specific one by name instead of failing the whole config load.
`push` reads the recorded patterns and leaves whatever they match out of the `.tar.gz` build context it uploads, so a secret or a generated tree no longer has to reach the platform to be ignored by the build. Patterns are read the way `.gitignore` reads them, because that is the vocabulary the paths people want gone are already written in: bare names match at any depth, a `/` anchors to the source directory, a trailing `/` matches directories only, `**` spans them, and one segment is matched by the glob matcher the seed globber already uses. An excluded directory is not descended into, so nothing beneath it is read at all. Re-inclusion (`!`) is refused rather than read as a literal filename: pruning at the directory means a pattern re-admitting something beneath it could never be reached, and a setting that silently does nothing is worse than one that isn't offered. Empty patterns, empty path segments and malformed character classes are refused the same way, before anything is packaged or uploaded, beside the runtime and exposure checks. A matched symlink is excluded before it is vetted for escaping the context, so excluding a hoisted `node_modules` link is now an answer to `ComputeSourceEscapingLinkError` rather than something that failure pre-empts. The packaged line carries an excluded count when patterns are configured, and a source whose every file is excluded fails naming the patterns instead of reporting the empty-directory case.
Adds the mechanism only: `COMPUTE_RUNTIME_EXCLUSIONS` is keyed by runtime the way the description and marker-file tables already are, `compute new` writes whatever a runtime declares into `[compute.<name>] exclude`, and the TOML section writer learns to render a list of strings as an array. Every runtime declares an empty list here, so no scaffold changes behavior yet and no `exclude` key is written — what each runtime should leave out depends on how it resolves dependencies, and is decided per runtime rather than alongside the plumbing. Recording the list in `config.toml` rather than applying it silently at push time keeps it visible, editable, and the same on every machine. A built-in default nobody could see would be a second, invisible source of truth for what ships, and `push` stays free of defaults of its own.
Fills in the per-runtime lists the previous commit left empty, so a scaffolded compute starts with the patterns its runtime actually wants. Every runtime keeps environment files and version-control metadata out, since both are secrets or noise in an image whose context is uploaded to the platform. Beyond that the lists diverge by how each runtime resolves what it depends on. `node` drops the locally installed tree's contents with `node_modules/*` while keeping the directory, so the runtime still finds the resolution root it expects and the build resolves dependencies itself rather than inheriting one machine's platform-specific binaries. `deno` caches remote dependencies outside the project and so has no installed tree to drop. And `dockerfile` assumes nothing further about a context the user's own `Dockerfile` decides how to copy. This is the commit that changes what a scaffolded compute deploys, and the one to revert if the build does not resolve dependencies.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🤖 AI Review
All 10 deduplicated findings are confirmed. The main issue is a likely runtime-breaking Node default that excludes dependencies despite the existing no-install deployment contract. The remaining findings cover misleading empty-package errors, matcher correctness/performance, stale generated schema output, type-safety, and documentation/convention violations.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🟠 MAJOR | apps/cli/src/shared/compute/compute-runtimes.ts:66 |
correctness |
claude+codex | The Node scaffold excludes installed dependencies even though the existing catalog-runtime deployment contract has no server-side install step. |
| 🟡 MINOR | apps/cli/src/commands/experimental/compute/push/push.handler.ts:351 |
error-handling |
claude+codex | A configured exclusion list misdiagnoses any zero-file package as having every file excluded, even when no pattern matched. |
| 🟡 MINOR | apps/cli/src/shared/compute/toml-section.ts:79 |
type-safety |
claude | The production as-cast used to render string values violates the repository's explicit type-safety convention and masks incomplete narrowing. |
| 🟡 MINOR | apps/cli/src/shared/compute/compute-exclude.ts:65 |
correctness |
claude+codex | A terminal /** matches and removes its parent directory, contrary to the documented gitignore-style behavior. |
| 🟡 MINOR | apps/cli/src/shared/compute/compute-exclude.ts:65 |
performance |
codex | Globstar matching repeatedly evaluates the same suffix states, producing combinatorial work for valid patterns with multiple globstars. |
| 🟡 MINOR | apps/docs/public/cli/config.schema.json:2383 |
generated-artifacts |
claude | The published generated config schema omits the new compute exclude property, causing schema validation and editor tooling to reject it. |
| ⚪ NIT | apps/cli/src/shared/compute/compute-exclude.ts:115 |
correctness |
claude | Repeated trailing slashes unexpectedly change an unanchored directory pattern into an anchored one. |
| ⚪ NIT | apps/cli/src/shared/compute/compute-runtimes.ts:66 |
documentation |
claude | The runtime-default explanation does not account for *.log being present only in the Node and Deno lists. |
| ⚪ NIT | apps/cli/src/shared/compute/compute-exclude.ts:135 |
error-handling |
codex | Malformed glob syntax unrelated to character classes is incorrectly reported as a malformed character class. |
| ⚪ NIT | apps/cli/src/shared/compute/compute-exclude.ts:5 |
maintainability |
codex | The new matcher file violates the repository's comment-density policy and duplicates semantics maintained in command documentation. |
Findings outside the diff
- 🟡 MINOR
apps/docs/public/cli/config.schema.json:2383— The published generated config schema omits the new compute exclude property, causing schema validation and editor tooling to reject it.
Stats
Claude findings: 7 · Codex findings: 6 · Confirmed: 10 · Refuted: 0 · Uncertain: 0
Models: claude-opus-5 + gpt-5.6-sol · Trigger: auto · Workflow run
This review runs once per PR. A maintainer can request another with a /ai-review comment.
The refusal for a symlink leaving the build context still told users to install the compute's dependencies inside its own directory. That was the third and last place carrying the old no-server-install contract, and the only one that was a user-facing string rather than a comment, so it survived the earlier sweep and now contradicted both the `node` scaffold's own defaults and this error's rationale one file over. Excluding the link is the cheaper recovery and the one `exclude` exists to offer, so the suggestion names it first. Pinned by the escaping-link cases, which asserted only the error type before and so let the guidance drift while staying green.
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
Review follow-ups on the `exclude` matcher, all behaviour-visible: A trailing `**` now has to consume a segment. `cache/**` matching `cache` itself meant the walk pruned the directory the pattern was written to empty, taking it out of the archive rather than emptying it; a spanner anywhere else may still span nothing, so `build/**/cache` keeps matching `build/cache`. Adjacent spanners collapse to one. `**/**` spans exactly what `**` spans, so leaving the repeats in let a pattern retry the same suffixes once per spanner — combinatorial work on a matcher that runs for every entry in the walk. Anchoring is decided after trailing separators come off, so `dist//` is the same unanchored directory pattern as `dist/` rather than silently becoming a root-only one. A malformed pattern no longer claims to be a malformed character class. `pathMatch` returns one verdict for every bad operator, a trailing escape included, so the message names the offending segment instead of guessing. `push` picks the empty-source message from what was actually excluded rather than from whether patterns were configured: a tree of nothing but empty directories packages to zero files whatever `exclude` says, and blaming a pattern that matched nothing sent the user to edit a line doing its job. Also drops the `as string` in the TOML section writer by narrowing on what each branch is rather than what it isn't, explains why only the catalog runtimes exclude `*.log`, and moves the matcher's prose into the command's SIDE_EFFECTS.md, which is the doc of record for the semantics.
The scaffolded pattern dropped the directory's contents while keeping the directory itself; exclude the directory outright.
…11/feat/compute/exclude-config-property
A worktree or submodule checkout has `.git` as a file holding an absolute gitdir path, which the directory-only `.git/` pattern passed over.
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.
Adds a
[compute.<name>] excludelist so a compute's build context can leave out secrets and generated trees instead of uploading them for the platform to ignore, and gives each runtime scaffolded bycompute newa sensible default list.exclude: string[]field to the compute entry schema inpackages/config. The schema checks only that the key holds a list of strings; pattern syntax is validated bypush, which can then refuse a single bad pattern by name instead of failing the whole config load.pushto compile the recorded patterns with.gitignoresemantics (name-anywhere,/-anchoring, trailing-/for directories,**spanning, glob syntax per segment) and skip matched paths — including excluded symlinks, checked before the existing escaping-link validation — while pruning excluded directories entirely rather than descending into them.!...) and malformed/empty patterns up front, before any packaging or upload, and report the packaged line's excluded count; a source excluded down to nothing now fails naming the offending patterns instead of reporting an empty directory.excludepatterns viaCOMPUTE_RUNTIME_EXCLUSIONS, written intoconfig.tomlbycompute new(extending the TOML section writer to render string-list values) rather than applied invisibly at push time, so the list stays visible and editable.nodealso dropsnode_modules/and*.log,denodrops*.log, anddockerfileadds nothing further since the user's ownDockerfiledecides what it copies.