diff --git a/docs/rfcs/0001-notte-project-scaffolding-and-deploy.md b/docs/rfcs/0001-notte-project-scaffolding-and-deploy.md new file mode 100644 index 0000000..12e1da6 --- /dev/null +++ b/docs/rfcs/0001-notte-project-scaffolding-and-deploy.md @@ -0,0 +1,687 @@ +# RFC 0001 — A `notte` project: scaffolding, bundling, and declarative deploys + +| | | +|---|---| +| **Status** | Draft — for discussion, nothing implemented | +| **Author** | Lucas Giordano | +| **Date** | 2026-08-27 | +| **v1 scope** | functions, secrets, schedules. Managed-auth connections later. | + +--- + +## Context + +We have built the same framework twice, by hand: + +| | `anything-api/marketplace` | `monorepo/apps/back/managed-auth` | +|---|---|---| +| What | 2,049 `.py` functions across 670 domains | 9 connectors = 18 functions (login + verifier) | +| Config | one 2.2 MB `marketplace/manifest.json` | one `connectors/.json` per connector | +| Deploy | `scripts/marketplace-catalog.ts` (2,404 lines TS) | `scripts/deploy.py` (996 lines, stdlib-only) | +| Driver | `make marketplace push prod` | `make deploy google.com staging` | +| Env selection | `$(filter …,$(MAKECMDGOALS))` + failing no-op rules | `%::` catch-all so URLs with `:` survive | +| Create vs update | manifest lockfile: `envs[env].code_sha256 != local` | server decides from `revision` + per-file sha256 | +| Shared code | **none — 0 relative imports in 2,049 files**; `has_value`/`clean_*` reimplemented hundreds of times | `contract.py` spliced in by one regex; `_verification_code()` hand-copied across 4 connectors | +| CI | designed for, never wired up | full 6-job promotion pipeline | + +Both converged on the same shape. Both spent their worst code on the same two problems: **faking subcommands in `make`**, and **not having a bundler**. Meanwhile the CLI already owns auth, config, output formatting, and every `functions` API call these scripts shell out to. + +Goal: fold the framework into `notte`, so a functions repo is `notte stack init` + `notte stack deploy`, and so `from ._shared.http import get` actually works. + +--- + +## Scope + +Three kinds of thing, and only the first two belong in git: + +| | Owner | In v1 | +|---|---|---| +| **Code** — function sources, shared modules | git, bundled by the CLI | ✅ | +| **Config** — function name/description/shared, cron, *which* secrets are required | `notte.toml` | ✅ | +| **Secret values** | gitignored `.env.`, pushed explicitly | ✅ (`notte secrets push/diff`) | +| **Runtime/user data** — managed-auth connections, sessions, personas, vaults, runs | the API; never reconciled from a file | ❌ (operational commands only) | + +Managed-auth **templates** are already declarative and could join later (see below). Managed-auth **connections** should probably never be — creating one runs a real browser login, spends money, and provisions a vault + a profile as side effects. + +--- + +## The hard constraint nobody can design around + +There are **two** gates, with different scopes, and conflating them is easy — an earlier draft of this document did exactly that. Getting the distinction right matters, because it changes which arguments are load-bearing: + +| | Upload | Runtime | +|---|---|---| +| Where | `POST /functions`, `?restricted=` **defaults to `True`** (`notte_api/functions/endpoints.py:353,411`) | `workflows-lambda/worker.py:891` calls `run_script(..., restricted=False)` | +| RestrictedPython AST policy | **enforced** — `FORBIDDEN_NODES`, `FORBIDDEN_CALLS`, the lot | **not applied at all** — `worker.py:608` takes the `restricted=False` branch and uses plain `compile()` | +| Import allowlist | enforced statically over the source | **still enforced**, dynamically: `__import__` is `runner.safe_import`, which calls `check_valid_import(name)` on every import (`worker.py:572-578`) | +| Which list | `notte_api.ast.ALLOWED_IMPORTS` | `_LAMBDA_ALLOWED_IMPORTS` + `{httpx, httpcloak}`, minus `tempfile` (`worker.py:520`) | + +So the AST policy is an upload-time quality gate, while **the import allowlist is a genuine runtime guard** — enforced by name, on every import, by the last guard standing once `restricted=False`. + +**1. Local imports are structurally impossible server-side.** +`notte-api/src/notte_api/ast.py`, `visit_ImportFrom`: +```python +if node.module is None: + raise SyntaxError("Relative imports are not allowed") +``` +`from . import util` dies there. `from .util import x` survives that check (module is `"util"`) but then hits `check_valid_import("util")` → not in `ALLOWED_IMPORTS` → rejected. Plain `import util` likewise. **There is no server-side path to multi-file. Bundling must be client-side.** + +**2. Off-the-shelf bundlers don't help — and `os`/`sys` isn't the reason.** + +An earlier draft said stickytape and pinliner were impossible because RestrictedPython forbids `sys`, `exec`, `compile`, `__import__` and `os`. **That was the wrong mechanism** — the AST policy isn't applied at execution time, so pointing at `FORBIDDEN_CALLS` proves nothing about what a deployed function can do. The real blockers are further down, and they bite in this order: + +1. **`import tempfile` fails first.** `worker.py:520` does `ALLOWED_IMPORTS.discard("tempfile")`, so stickytape's `mkdtemp()` dies before a single module is written. +2. **`import util` — the entire point — is rejected by name at run time.** stickytape exists to make a written-to-disk module importable; that import goes through `safe_import("util")` → `check_valid_import("util")` → not in the allowlist. The one thing it does is the one thing that's gated. +3. `os`, `sys` and `shutil` are on the **runtime** denylist too (`_LAMBDA_DISALLOWED_STDLIB_IMPORTS`), so none of this is an upload-time gate a query parameter can switch off. + +**On the obvious counter-proposal — "just allow `os` and `sys`":** it is neither necessary nor sufficient. It doesn't touch (1) or (2). Making stickytape work means disabling `safe_import`, i.e. permitting arbitrary imports by name at run time. That is a materially bigger decision than allowing two modules, and unlike the original framing it carries a real security dimension, because `safe_import` is the only import guard left once `restricted=False`. It deserves to be decided on its own merits, not adopted as a side effect of a bundling convenience. + +**Three reasons to flatten that don't depend on any allowlist.** These are the actual argument, and the earlier draft buried them beneath a claim that turned out to be wrong: + +- **The artifact stops being readable.** stickytape emits a prelude plus every module as an escaped bytes literal passed to `__stickytape_write_module`. The deployed file is what the console renders, what `functions show` downloads, and what marketplace's `push`/`check` diff against. A blob kills the diff model, and makes tracebacks worse rather than better. +- **stickytape disclaims itself.** Its README: *"bodged together… for a specific use case"*, no `from __future__` imports, `__file__` unreliable, dynamic imports need manual flags. That is an unmaintained third-party dependency sitting in the deploy path. +- **It's Python.** `notte stack` requires an interpreter anyway, so this is no longer a dependency argument — it is a separation one. Flattening stays a pure text transformation with no interpreter in it, which is what lets it be tested offline against thousands of files and reasoned about without a runtime. Python does the part that genuinely needs Python: validating the result. + +So: the bundler must be a **static flattener** emitting plain, ordinary Python — one module namespace, no runtime machinery. **That recommendation stands even in a fully permissive runtime**, which is how it should have been argued in the first place. + +**3. Dependencies are a fixed allowlist, so there is no dependency resolution to build.** +`ALLOWED_IMPORTS = set(sys.stdlib_module_names) - DISALLOWED_STDLIB_IMPORTS | {notte, notte_sdk, notte_core, notte_browser, notte_agent, notte_llm, pydantic, loguru, requests, httpx, httpcloak, playwright, gspread, google, litellm, bs4, pipedream, tqdm, typing_extensions}`. + +No `pip install`, no `requirements.txt`, no PEP-723 block, and no dependency resolver to write. The set is closed, so `notte stack sync` installs the intersection of that list with what your functions import — which makes the venv itself the check, since anything outside it fails to resolve. + +Two more upload-time contracts: +- `extract_env_requirements(source)` (`notte_api/functions/requirements.py`) AST-scans for literal `os.environ[...]`/`os.getenv(...)`/`.get`/`.setdefault`/`.pop` plus aliases, unions with an optional module-level `NOTTE_REQUIRED_SECRETS = [...]` list, subtracts reserved names, and persists to `functions.required_secrets`. Env vars are read via `from notte_sdk.types import os` (bare `import os` is blocked). **This is a ready-made input for the secrets planner.** +- `response_format` (a JSON Schema) is accepted only if `check_run_returns_pydantic_model` finds `def run(...) -> Model` with `class Model(BaseModel)` in the same file. + +--- + +## What the CLI has today + +- Cobra, all commands in `internal/cmd` as package globals (`internal/cmd/root.go`). +- `internal/config/config.go`: one global `~/.notte/cli/config.json` = `{api_key, api_url}`, plus bare-string state files `current_session`, `current_function`. **No per-directory config anywhere in the repo** — `notte stack init` would introduce the first. +- `internal/auth/env.go` **already has an environment notion**: `KeyringKeyForEnv(label)` namespaces keyring entries, and `hostToEnvLabel` maps `api.notte.cc`/`us-prod`→prod, `us-staging`→staging, `us-dev*`→dev. That's the hook `--env` hangs off. +- `internal/cmd/functions.go` covers list/create/show/update/delete/fork/run/runs/schedule/secrets. Create/update send a single multipart `file` part. No zip/tar/directory support anywhere in the repo. +- **Unexposed wins already in the generated client:** `response_format` and `restricted` (create/update), `version` (update — server-assigned `v%Y%m%d_%H%M%S`), `decryption_key` (download). The marketplace script had to hand-roll `fetch` + re-derive `sha256("api_key:{k}:workflow_id:{id}:dumb")[:64]` because `--decryption-key` doesn't exist. +- No `//go:embed` in the repo. Scaffolding would be the first. + +Two things to verify rather than assume: +- `marketplace-catalog.ts` carries a `detectCliError()` workaround because *"the CLI reports API errors as a JSON body on stdout with exit 0"*. `internal/output/json.go` looks like it writes errors to stderr and `Execute()` exits 1, so this may be path-specific — reproduce before designing around it. +- The README documents `notte functions schedule --cron "0 9 * * *"` — five fields. `validate_and_format_cron` wants the six-field AWS EventBridge form (`cron(m h dom mon dow year)`). Either the README is wrong or the server is lenient; worth checking, and worth validating client-side either way. + +--- + +## Prior art, and what each one gets right + +| | Project marker | Config | Per-unit dir | Shared code | Envs | +|---|---|---|---|---|---| +| **Supabase** | `supabase/` + `config.toml` | TOML, `[functions.]` | `functions//index.ts` | `_shared/` (underscore = not a function) | `supabase link --project-ref`, `[remotes]` | +| **Vercel** | `.vercel/project.json` (gitignored) | `vercel.json` | `api/.py` — file *or* dir | bundler resolves it | preview vs production; `vercel promote ` | +| **Cloudflare** | `wrangler.toml` | TOML | one worker per config | esbuild | `[env.staging]` + `--env staging` | +| **dbt** | `dbt_project.yml` | YAML | `models/**.sql` | `macros/` | `profiles.yml` targets + `--target prod` | +| **Modal** | none | Python | `-m src.app` module mode | `Image.add_local_python_source` (explicit since 1.0) | — | +| **Val Town** | `.vt/` | — | one val | — | `vt clone` / `push` / `watch` | + +Worth stealing: + +1. **Underscore = not a unit** (Supabase `_shared`). Zero config, instantly legible. +2. **File *or* directory** (Vercel). `functions/quick.py` for a one-liner; promote to `functions/quick/` when it grows helpers. +3. **`--env` + a config block per env** (wrangler, dbt). Deletes both Makefile hacks outright. +4. **`promote` moves the *artifact*, not the source** (Vercel). Byte-identical staging→prod. +5. **Explicit local sources** (Modal 1.0). Modal *removed* automounting because it was unpredictable. Ours is explicit by construction — only what's reachable from `main.py` via relative import gets bundled. +6. **Secret values via a gitignored `.env`, never in the config** (Supabase). `config.toml` is safe to commit; `supabase secrets set` pushes from `.env`. + +And four ideas from your own frameworks that beat anything in that table: + +7. **The manifest is a lockfile with per-environment state; the path is the identity.** + ```jsonc + {"envs":{"prod":{"function_id":"a0b8…","functions_version":"v20260821_162138","code_sha256":"ea2e…"}, + "dev":{"function_id":"7cc4…","functions_version":"v20260826_081715","code_sha256":"ea2e…"}}, + "path":"1001tracklists.com/fetch_batch_tracklists.py", …} + ``` + Per-env `code_sha256` is what lets **one tree serve all three environments** — a tree-wide hash would mean pushing to prod marks dev as up to date. No env-scoped id ever appears in a filename. +8. **The write and the bookkeeping are separate failure domains.** `entryAfterPush` advances `code_sha256` to the pushed bytes even when the confirmation read-back fails; only version strings go stale. Tying the hash to the read-back meant *"a transient error on the second request minted a duplicate upstream version on the next run."* +9. **A run is authoritative only for what it inspected.** `--limit 20` must never orphan the other 2,029. A *complete* walk that no longer lists something is the only thing that may remove it. And **never delete remote things absent from the tree** — report them (`reportExtraRemote`). +10. **Preview → guard → apply** (managed-auth): dry-run returns `target_state_sha256`; the apply sends it back as `expected_target_state_sha256`. Optimistic concurrency for free. + +--- + +## Proposed layout + +``` +my-functions/ +├── notte.toml # committed. intent: envs, defaults, per-function config +├── notte.lock.json # committed, machine-written. per-env ids + hashes +├── .env.dev / .env.prod # GITIGNORED. secret values only +├── .notte/ # gitignored. build output, caches +│ └── build/prod/amazon_search.py +├── pyrightconfig.json # written by `notte stack init` +├── AGENTS.md # written by `notte stack init` — the authoring contract +└── functions/ # ← real python package, importable from repo root + ├── __init__.py + ├── _shared/ # `_` prefix = library, never deployed + │ ├── __init__.py + │ ├── http.py + │ └── contract.py + ├── amazon_search/ # a function + │ ├── __init__.py + │ ├── main.py # entrypoint: must define exactly one `run()` + │ ├── parse.py # local helper, bundled in + │ └── test_main.py # colocated test, never bundled + └── quick_check.py # single-file function — no directory needed +``` + +`functions/amazon_search/main.py`: +```python +from pydantic import BaseModel +from .parse import parse_rows # sibling +from .._shared.http import fetch_json # shared + +class Response(BaseModel): + items: list[dict] + +def run(query: str = "laptop") -> Response: + return Response(items=parse_rows(fetch_json(query))) +``` + +**Why `functions/` at the repo root and not `notte/functions/`.** `notte` is a real PyPI package and is in `ALLOWED_IMPORTS`. A top-level `notte/` directory in a repo whose root lands on `sys.path` — which happens the moment you run pytest — shadows it, and pyright resolves your empty directory instead of the SDK. `functions/` has no such collision. Make it configurable (`[project] functions_dir = "functions"`) so you *can* have `notte/functions/`, and document the caveat there rather than defaulting into it. + +Everything is a real package (`__init__.py` generated by `notte stack new`), so relative imports resolve identically for pyright, pytest, and the bundler. Discovery rule, one sentence: **anything directly under `functions_dir` whose name doesn't start with `_` is a function — a `/main.py` or a `.py`.** + +--- + +## Config file format + +Two files, two owners, two formats. That split matters more than which format wins. + +### Which format for the hand-written config + +| | Comments | Editor schema/autocomplete | Go parsing | Deep nesting | Ecosystem | +|---|---|---|---|---|---| +| **JSON** | ✗ — fatal for a config you want to annotate | ✓✓ best in class: `$schema`, works in every editor with zero setup | stdlib | fine | JS/TS (`vercel.json`, `tsconfig.json`) | +| **JSONC / JSON5** | ✓ | partial | third-party | fine | VS Code only; no cross-editor story | +| **YAML** | ✓ | ✓ via `# yaml-language-server: $schema=` | third-party | ✓✓ | k8s, CI | +| **TOML** | ✓ | ✓ via taplo's `#:schema` directive + Even Better TOML | third-party | ✗ awkward past 2 levels | Python & Rust (`pyproject.toml`, `Cargo.toml`, `wrangler.toml`, `fly.toml`, `netlify.toml`) | + +**Recommendation: TOML, as `notte.toml`.** Three reasons, in order: + +1. **The users are Python developers.** They read `pyproject.toml` every day. A functions CLI whose config looks like `pyproject.toml` needs no explanation. +2. **Comments are the point.** Both existing frameworks are ~40% prose comments explaining *why* — `deploy.py`'s `NOTTE_API_URLS` carries a war story about `NOTTE_API_URL` silently pointing `pull prod` at dev. JSON cannot hold that, and it's exactly what belongs next to `[env.prod] api_url = …`. +3. **The nesting here is shallow by construction** — `[env.prod]`, `[functions.amazon_search]`. That's TOML's sweet spot, and it's the one place TOML is weak, so the weakness never bites. + +YAML is rejected on safety: whitespace significance, and the Norway problem (`no` → `false`) in a config whose values include country codes — `proxy_country = "no"` is a real value in the managed-auth template schema. + +Bare `.notte` is rejected outright: no extension means no highlighting, no schema association, no declared format until you open it. But `.notte/` as a **directory** for gitignored local state is right, and mirrors `.vercel/`, `.vt/`, `.terraform/`. + +Also rejected: `[tool.notte]` inside `pyproject.toml` (most Python-native, but the repo isn't a distributable package and it couples project config to a file uv/pip also own); `.notte.toml` (hidden files are for user-level config — you want this discoverable in `ls`); `notte.config.toml` (the JS `*.config.*` convention disambiguates; there's nothing to disambiguate from). + +### Ship a JSON Schema regardless + +TOML's schema story is real but needs a directive on line 1: +```toml +#:schema https://notte.cc/schema/notte-v1.json +``` +`notte stack init` writes it; `notte schema` prints it for vendoring. Validate against the same schema in `notte stack check`, so a typo'd key errors instead of doing nothing — TOML's failure mode for an unknown key is silence. + +### The rule that follows from Go's TOML libraries + +Every Go TOML library either drops comments on write or exposes them read-only; `pelletier/go-toml` v2 explicitly **dropped document editing from its requirements**. Comment-preserving edits need a niche lossless-AST library (`creachadair/tomledit`, `smm-h/go-toml-edit`). + +Make it a rule instead of a dependency: **`notte.toml` is never machine-rewritten.** `notte stack init` and `notte stack new` *render templates* (text, not a marshal round-trip); after that only humans edit it. Everything the CLI writes back goes to `notte.lock.json` — JSON, stdlib-parsed, machine-owned. That's precisely the line `marketplace/manifest.json` failed to draw: it mixed generated state (`pulled_at`, `run_count`) with copy (`name`, `description`, `categories`) that a human wants to own, and the result is a 2.2 MB file every sync dirties. + +### One interpolation syntax + +Config must reference things it may not contain. Supabase solved this with `env("MY_KEY")`. Use a uniform `${namespace:key}` so there's one rule: + +```toml +[env.prod] +api_url = "https://api.notte.cc" +api_key = "${env:NOTTE_API_KEY_PROD}" # resolved at use, never stored + +[env.preview] +extends = "dev" +headers = { "x-db-preview" = "${git:branch}" } +``` +Namespaces: `env:` (process environment, optionally from a gitignored `.env.`), `git:` (`branch`, `sha`, `short_sha`), `keyring:` (the existing `KeyringKeyForEnv` entries). **Fail loudly on an unresolved reference** — `deploy.py`'s `--db-preview` guard exists because a header silently ignored by staging meant *"a silent wrong write"*, and unresolved-to-empty-string is the same bug class. + +--- + +## Naming conventions + +| Thing | Choice | Why not the alternative | +|---|---|---| +| Project config | `notte.toml` | See above. | +| Lockfile | `notte.lock.json` | One entry per line — marketplace proved this keeps diffs reviewable at 2,049 entries. Separate from `notte.toml` because it's machine-written. | +| Local state | `.notte/` (gitignored) | Mirrors `.vercel/`. Build output lives here, never next to sources. | +| Secret values | `.env.` (gitignored) | Supabase's split. Never in `notte.toml`. | +| Package root | `functions/`, configurable | See layout section. | +| Entrypoint | `main.py` | Not `function.py` (redundant inside `functions//`), not `index.py` (a JS import), not `route.py` — a Notte function has exactly one `run()`; there is no route/handler split to encode. | +| Shared code | `_`-prefixed anything; `_shared/` by convention | Supabase's rule, and it doubles as the "not a function" marker. | +| Function name | directory or file stem, `[a-z0-9_-]+` | Never contains the function id — ids are per-env. | +| Build output | `.notte/build//.py` | Per-env because per-env config can change the bytes. | +| Envs | `dev`, `staging`, `prod`, `preview`, `local` | Both frameworks already use exactly these. | +| Deploy verb | `deploy` | Not `push`. `push` implies a round trip; bundling makes the tree source-of-truth and the flow one-way. Reserve `pull` for adoption/import. | + +--- + +## Command surface + +``` +notte stack init [dir] # scaffold notte.toml, functions/, pyrightconfig, .gitignore, AGENTS.md +notte stack init --from-session # bootstrap from `sessions workflow-code` — record, then scaffold +notte stack new # one function directory from a template + +notte stack deploy [] # build → diff → confirm → create/update → schedule → write lock +notte stack check [] # build + validate + diff vs remote. writes NOTHING. the CI gate. +notte stack status # what's drifted, and what a `_shared` edit would touch +notte stack pull # adopt existing remote functions into the tree + lock + +notte stack sync # create/refresh .notte/venv: Python 3.12, latest notte-sdk, + # plus the allowlisted packages your functions import +notte stack doctor # what is installed, resolved SDK vs latest, ty version, org +``` + +Eight commands. `` is a name, a glob, `all`, or a path — so `notte stack deploy functions/amazon_search` tab-completes. + +### Why `stack`, and the two rules that keep it honest + +The namespace exists because `notte functions` today is **id-centric and imperative** — `--function-id`, a global `~/.notte/cli/current_function`, one file at a time — while these are **directory-centric and declarative**. Two mental models under one name is the confusion worth preventing. Whether a command needs Python is not: validation degrades rather than being required, so a toolchain prefix would advertise a dependency that does not exist. No CLI in the prior art groups by toolchain anyway — Supabase needs Docker for `db` and `functions serve` and marks neither; `docker compose` and `git lfs` are namespaced because they are separate binaries. + +`stack` over the alternatives: `project` is generic and says nothing; `workspace` is **taken**, since Notte already means org by it (`workspaceIdFromUrl` in marketplace resolves an org id); `app` implies a running application; `fn`/`fns` is too close to `functions` to disambiguate anything. And `functions` itself becomes actively wrong the moment managed-auth connectors join, because you would be deploying connectors from a namespace called functions. `stack` absorbs them, which is the point — it names where this is going rather than only where it is. + +The one real cost is that in Pulumi and CloudFormation — the prior art users arrive with — **a stack is an environment** (`pulumi stack select dev`). Two rules keep that from surfacing, and they are constraints rather than preferences: + +1. **No stack selector, ever.** No `notte stack select prod`, no `notte stack prod deploy`. `--env` is the only way an environment is ever named. The ambiguity requires both spellings to exist. +2. **No `notte stack destroy`.** It is the command a Pulumi user reaches for, and it would promise resource-lifecycle semantics this design explicitly does not own. If teardown is ever needed, give it a name that does not imply cascade. + +### Environments are opt-in + +Almost every user deploys to prod and nothing else. Multi-environment support exists for `marketplace` and `managed-auth`, both internal, so it has to be **possible** without being **prominent**: + +- **`--env` defaults to `prod`** and appears in no quickstart example. +- **`notte stack init` scaffolds no `[env.*]` blocks.** Just `[project]`. An environments section gets added when a second environment actually exists. +- **`notte stack status` hides the environment column** for a single-environment project. +- **The lockfile keeps its per-environment shape regardless.** That costs a single-environment user one key, and marketplace established that a tree-wide hash silently marks dev up to date when you push to prod. + +An earlier draft had this inverted — scaffolding three `[env.*]` blocks and three API keys into every new project. That generalised from the two internal frameworks in the wrong direction, and would have left a first-time user believing three credentials were a prerequisite for deploying anything. + + +### Why `pull` is v1 and not a migration nicety + +It is tempting to file `pull` under "only needed to adopt an existing tree." That is wrong twice over. + +**An org with existing functions is the normal case, not the migration case.** Functions already arrive from `sessions workflow-code`, from the Anything API build agent, and from the console. The expected Notte flow is *author in the browser, then decide you want it in git* — which is `pull`, and which is also why `notte stack init --from-session` is really just a single-function `pull` wearing a different name. The machinery is required either way. + +**Without it, `deploy` is unsafe in a non-empty org.** Create-vs-update is decided from the lock: no `function_id` for this env → create. A fresh `notte stack init` against an org that already has `amazon_search` produces a lock that believes nothing exists, so the first `deploy` **creates a second `amazon_search`** rather than updating the first. `functions.name` has no unique constraint (`text NOT NULL DEFAULT 'default'`), so the API accepts it silently, and now two functions share a name while callers hold the id of the one that stopped being updated. That is the worst failure mode in this document, and `pull` is what prevents it. + +Which implies a companion rule: **`deploy` must refuse to create a function whose name already exists remotely but is absent from the lock**, and say `run 'notte stack pull' first`. Fail closed, same as the credential rule below. `--force-create` exists for the genuine case where you do want a second one. + +**`pull` is not the inverse of `deploy`, and must not pretend to be.** What comes back over the wire is the *artifact*, and a bundled artifact cannot be un-flattened into the package that produced it. So: + +- A function with no entry in the lock is written as a **single-file function** — `functions/.py` — because that is genuinely what it is. It can be promoted to a directory with helpers later, by hand, at which point it starts bundling. +- A function already in the lock and already deployed *from this tree* is **left alone**. Overwriting `functions/amazon_search/{main,parse}.py` with one flattened file would destroy the sources to "sync" them, which is the opposite of the intent. If its `artifact_sha256` doesn't match what's deployed, that's drift — report it, and let `status`/`deploy` handle it. +- Following marketplace: a run is authoritative only for what it inspected, so `--limit` or a failed download never prunes; and remote functions absent from the tree are **reported, never deleted**. + +**There is no bulk download, and no download command at all.** This is the part that makes `pull` a real batch job rather than one request, and it needs saying because the shape isn't obvious: + +- `GET /functions` returns `PaginatedResponseFunctionResponse{Items []FunctionResponse}`, and `FunctionResponse` **has no URL field**. Only `FunctionWithLinkResponse` carries `Url`, and that comes from `GET /functions/{id}`. +- So the code for each function costs **two more requests**: one to get the signed URL, one to fetch it. A full pull is `⌈N/100⌉ + 2N` — roughly 4,120 requests for marketplace's 2,049 functions. It ran that at concurrency 48. +- The URL is a Fernet token for Notte-managed functions, decrypted with a key derived client-side: `sha256(f"api_key:{key}:workflow_id:{id}:dumb")[:64]`, passed as `?decryption_key=`. +- **`notte functions download` does not exist.** `functions show` already calls `FunctionDownloadUrl`, prints the metadata, and throws the URL away. So today this is entirely hand-rolled — marketplace reimplements the key derivation and the fetch, which is how one secret-derivation rule ended up living in two repos. + +**This needs no backend change — parallelise it.** An earlier draft asked for `url` on the list endpoint to halve the request count. That ask is unnecessary, and marketplace is the evidence: it runs the full walk at `concurrency: 48` (`marketplace-catalog.ts:2103`, overridable with `--concurrency`) over the largest corpus that exists, and the comment on its retry classifier records the measurement: + +> *"Nothing hit a 429 while this was being measured, but a full pull is several times larger than any sample taken, and a retry is much cheaper than a half-written tree."* + +4,120 requests, no rate limiting. The retry logic is defensive rather than a response to observed throttling. For a realistic project — tens of functions, not thousands — `1 + 2N` at concurrency 48 is a second or two. Adding a backend dependency to optimise that would be trading real coordination cost for an imperceptible win. + +So what's actually needed is client-side, and all of it is CLI work: + +- **`notte functions download --function-id `** as a primitive in its own right, deriving the decryption key internally rather than exposing a flag users must understand. `pull` becomes a bounded-concurrency loop over it, and marketplace's hand-rolled fetch and duplicated key derivation both get deleted. +- Retry with backoff on 429/5xx honouring `Retry-After` — cheap, and the failure it prevents is a half-written tree. +- A **complete** page walk before anything is reported as a remote extra. A listing that stops early is indistinguishable from one where functions were deleted. + +**And `check` shouldn't pay this cost at all in the common case.** The lock already stores `artifact_sha256` per env, so the default gate — *"you changed sources and didn't deploy"* — is a local build plus a hash comparison, with no downloads and no page walk. `--verify-remote` does the full download walk to catch out-of-band edits made in the console. marketplace always downloads in `check` because it is a mirror with no separate source hash to trust; we have one, so we can be cheap by default and thorough on request. That also matches its own observation that a scheduled `check` against prod is a staleness alarm rather than a build gate. + +### Deferred, and why + +An earlier draft proposed eighteen commands. Roughly half were aspirational or gated on backend work that doesn't exist yet, and a large surface is its own cost — it has to be documented, tab-completed, kept coherent, and lived with. Everything below is deliberately *not* in v1: + +| Command | Why deferred | +|---|---| +| `run`, `logs`, `secrets`, `schedule` | already exist under `notte functions`; project-aware twins aren't needed on day one | +| `build` | folded into `deploy` and `check`. Expose separately only once someone actually wants the artifact without the diff | +| `promote`, `rollback` | need `--version` and `versions[]` exposed on the CLI first | +| `dev` | genuinely valuable, but it's a second execution model and deserves its own design rather than a line in this table | +| `whoami`, `auth login --env` | blocked on `GET /me` (backend ask #3). These ship *with* that endpoint — `auth login --env` is a prerequisite for the fail-closed credential rule below, not an optional extra | + +The credential resolution rules in the next section apply to all six commands. `--env` is not deferred — it just defaults to prod and stays out of the way. + +`notte.toml`: +```toml +#:schema https://notte.cc/schema/notte-v1.json + +[project] +name = "anything-api" +functions_dir = "functions" + +[functions.amazon_search] +name = "Amazon AE product search" +description = "…" +shared = true +cron = "cron(0 9 * * ? *)" +secrets = ["AMAZON_PARTNER_TAG"] # in addition to what the AST scan finds +``` + +That is the whole file for the common case: no environments, no credentials, prod implied. A project that genuinely has several adds them, and only then: + +```toml +[env.dev] +api_url = "https://us-dev.notte.cc" +api_key = "${env:NOTTE_API_KEY_DEV}" + +[env.staging] +api_url = "https://us-staging.notte.cc" +api_key = "${env:NOTTE_API_KEY_STAGING}" + +[env.preview] +extends = "dev" +headers = { "x-db-preview" = "${git:branch}" } # generalizes managed-auth's preview mode +``` + +### Credentials resolve *from* the environment, never beside it + +API keys are **not** literals in `notte.toml`. More importantly, **the key and the URL must be resolved as one unit.** Selecting an env fixes `api_url`, and every candidate key is then derived from that same `api_url` — there is no step in the chain that can hand back a credential belonging to a different endpoint: + +1. `--api-key` (explicit, and the operator owns the consequences) +2. the `api_key = "${env:…}"` reference declared **inside that env's own block** +3. the keyring entry under `KeyringKeyForEnv(hostToEnvLabel(api_url))` — the label computed from the resolved URL, not from ambient state +4. **stop.** Fail with `no credential for env 'staging' (https://us-staging.notte.cc) — set NOTTE_API_KEY_STAGING or run 'notte auth login --env staging'`. + +The two fallbacks the global CLI uses today — a bare `NOTTE_API_KEY` and `~/.notte/cli/config.json` — are **deliberately not in this chain**, because neither is tied to an endpoint. A developer with `NOTTE_API_KEY` exported for prod running `notte stack deploy --env staging` would otherwise authenticate to staging with a prod key: it fails closed if the orgs differ, but it succeeds and writes to the *wrong org* whenever they don't. `notte stack deploy` is the command where that matters most. + +This is the same bug `marketplace-catalog.ts` already documents having hit from the other direction — its `NOTTE_API_URLS` table exists precisely because reusing a helper that read ambient `NOTTE_API_URL` made `pull prod` silently read dev. Its `createNotteRunner` then passes `NOTTE_API_KEY` and `NOTTE_API_URL` to the subprocess explicitly, together, never ambient. Same rule, enforced one level up. + +`notte auth login --env ` and `notte whoami --env ` are the paired ergonomics that make failing closed tolerable, and `notte stack status` should print the resolved org for each configured env so a misconfiguration is visible before a deploy rather than after. + +--- + +## The bundler + +### Algorithm + +1. Parse `main.py`; collect relative imports (`from .x import a`, `from ..y.z import b`). +2. Resolve to files inside `functions_dir`; recurse. Anything outside the package, or non-relative, is left alone and checked against the allowlist. +3. Topologically sort. Cycle → error naming the cycle. +4. Emit: header, then `from __future__ import annotations` if any module had it (exactly one, first statement), then hoisted+deduped third-party imports, then each dependency's body in topological order with its relative-import lines **replaced** (see aliases below), then `main.py`'s body last. +5. Hash the artifact. Write `.notte/build//.py` + a source map. + +**Aliased relative imports keep their binding.** A relative import line is not simply deleted — it is replaced in place by an assignment per aliased name: + +```python +from .parse import parse_rows as pr, clean # source +pr = parse_rows # artifact (clean needs nothing) +``` + +Deleting the line outright would drop `pr` and the artifact would die with `NameError` at run time, which is the worst possible failure mode: it passes the bundler, passes upload validation, and fails in production. Unaliased names need no assignment because the flattened definition already carries that name, and dependency bodies are emitted before the body that imports them, so the right-hand side is always bound by the time the assignment runs. + +**Collisions are an error, not a rename.** If `_shared/http.py` and `parse.py` both define `clean`, fail with `_shared/http.py:12 and parse.py:8 both define 'clean' — rename one`. This is the pivotal simplification: **no reference rewriting is ever needed**, so no full-fidelity Python parser is needed, and the artifact stays byte-readable — which matters because that artifact is what the console shows and what tracebacks point at. + +The collision set is every top-level binding **plus every alias introduced above** — `from .parse import clean as fetch` collides with a `fetch` defined in `_shared/http.py` exactly as a second `def fetch` would, and must be reported the same way. + +Rejected in v1, each with a fix-it message: +- `from . import mod` then `mod.f()` → *"use `from .mod import f`"*. Neither existing codebase does this. +- `from .x import *` → *"star imports can't be flattened"*. +- Import cycles. +- Relative imports inside a function body or `if TYPE_CHECKING`. + +### Where it runs + +| | Go-native | Shell out to Python/`uv` | Server-side | +|---|---|---|---| +| Parse fidelity | Purpose-built tokenizer. Sufficient **because collisions error out**. `go-python/gpython` is a Python 3.4 grammar — no f-strings, no walrus, no `match` — so it isn't an option. | Perfect: Python's own `ast`. | Perfect. | +| Runtime deps | None. Brew binary works in CI, in a bare container, everywhere. | Needs `python3`/`uv` present. | None. | +| local build offline | Yes | Yes | **No** — you lose local preview and the CI gate | +| Agreement with `ScriptValidator` | Must mirror the allowlist as data (drifts) | Same problem — the validator lives in `notte-api`, not in a pip package | Authoritative by construction | +| Cost | ~600 lines Go + tests | ~200 lines Python, `//go:embed`-ed, run via `uv run --script` | Backend work: accept a tar, bundle, validate | +| Failure mode | A weird import form is rejected with a clear message | "python3: not found" on a machine where the CLI otherwise works | Slow loop; can't check in a PR without credentials | + +**Recommendation: Go flattens, Python validates.** Not either column alone — each does the half it is actually good at, and neither reimplements the other. + +**Go does the flattening**, with no toolchain requirement. Resolution, topological sort, concatenation, import hoisting and alias preservation are a text transformation, and the collisions-error-out rule shrinks the parsing problem to import discovery plus top-level binding extraction, all line-oriented at indent zero. Validated against `anything-api/marketplace`: 2,524 production files, zero bundle errors, zero artifacts failing `py_compile`, zero lost definitions. + +**Python validates**, when it is available — and this is where the real leverage is. Mirroring the server's rules in Go means maintaining a copy of `ALLOWED_IMPORTS`, a denylist, and a stdlib set generated from a pinned CPython. Every one of those drifts. With an interpreter present you stop mirroring and run the real thing: + +| Problem | Cost of mirroring in Go | With Python present | +|---|---|---| +| Tokenizer edge cases | a hand-written scanner | `ast`, the parser the server uses | +| Import allowlist drift | vendored copy of two lists | import the real `ScriptValidator` | +| stdlib version mismatch | pinned generator, version-guarded | the runtime's own interpreter | +| Semantic errors | not detectable at all | `ty check` | + +That last row is the one the flattener cannot cover by itself. A *missed* collision produces valid Python with the wrong meaning, so `py_compile` passes — but a type checker reports it as a redefinition. The gate closes exactly the hole the bundler can leave open. + +### `notte stack` requires Python; the rest of the CLI does not + +`notte sessions`, `notte page` and friends stay pure Go with no toolchain. `notte stack` requires an interpreter, and requires rather than prefers it. + +An earlier draft made it optional, degrading to a vendored copy of the server's rules when uv was absent. That copy was the problem. Mirroring `ALLOWED_IMPORTS`, a denylist, and a stdlib set generated from a pinned CPython means maintaining three things that drift from a backend this repo does not control — and two of them drifted within a week of being written, one of them shipping a list from CPython 3.14 that would have rejected `telnetlib`, `cgi` and 17 other modules the 3.12 runner actually has. + +Requiring Python deletes the mirror rather than maintaining it: + +| Deleted | Replaced by | +|---|---| +| a vendored `ALLOWED_IMPORTS` + denylist | the real `ScriptValidator.parse_script(source, restricted=True)` | +| a stdlib set generated from a pinned CPython | the interpreter's own `sys.stdlib_module_names` | +| `make generate-stdlib` and its script | nothing | +| "is this check current?" | not a question that can be asked | + +The ask is small because **uv downloads the interpreter itself**, so the requirement is "have uv" — one binary. `managed-auth` already works this way. It needs one line in `--help` and a good error, not a redesign. + +### The venv is the enforcement + +`notte stack sync` builds `.notte/venv` with Python 3.12, the latest `notte-sdk`, and **the allowlisted packages your functions actually import** — not all of them, and nothing else. + +That last constraint does more work than it looks. The runtime allowlist is *closed*: `requests`, `httpx`, `httpcloak`, `pydantic`, `loguru`, `playwright`, `bs4`, `litellm`, `gspread`, `google`, `tqdm`, `typing_extensions`, `notte_*`. So detection is not open-ended dependency resolution, it is intersecting your imports with a known set. And once the venv mirrors the runtime image, **`ty`'s `unresolved-import` *is* the allowlist violation** — no separate third-party check has to exist, because the environment enforces it. + +Standard-library denials (`os`, `sys`, `subprocess`) still resolve fine in a venv, so those remain `ScriptValidator`'s job. Between the two, every rule the runtime applies is checked by the thing that applies it. + +An import of something not on the allowlist is an error at sync time, naming the file and line. Never a silent install. + +Two consequences for the rest of the design: + +- **`sync` is implicit.** `deploy` and `check` build the venv if it is missing rather than erroring, the way `uv run` does. `sync` is the explicit refresh. +- **`notte stack init` writes `ty.toml` and `pyrightconfig.json` pointing at that venv**, which is where the day-to-day win lands: clone a functions repo, run one command, and the editor resolves `notte_sdk`, `pydantic` and `session.page`. `managed-auth` currently reconstructs this by hand in a Makefile target that scrapes an SDK commit out of another project's lockfile. + +**`sync` does not generate a `pyproject.toml`.** It is tempting — the project would become a normal Python project and `pytest` would work unconfigured — but a generated `pyproject.toml` is clobbered the moment someone adds `pytest` or `ruff` to it. That is exactly the mistake `marketplace/manifest.json` made by mixing generated state with hand-owned content. `pyproject.toml` stays entirely the user's; the CLI owns `.notte/` and nothing else in the repo root. + +### `ty`, and the way it fails open + +Use `ty`, not basedpyright. `anything-api` already gates its build agent on `ty check client.py` before every create or update, so there is in-house precedent and a working configuration; and ty's 10–100x advantage on *cold* checks is precisely the case a CLI hits on every invocation. It is on `0.0.x` with no stable API, which argues for pinning the version, not for avoiding it. + +The trap is documented in `anything-api`'s `ty-config.ts`, and it is worth quoting because the CLI would walk straight into it: + +> ty does NOT resolve imports against the environment it was pip-installed into. With no `VIRTUAL_ENV`, no `.venv` and no `--python`, it falls back to the first `python` on PATH — the system 3.9 in the sandbox, not the 3.11 the snapshot installed notte-sdk into. So every generated client.py reported `unresolved-import` for `requests`, `pydantic` AND `notte_sdk`, and **the agent deployed straight through the mandatory type check.** + +A type checker that cannot resolve imports does not fail. It emits `unresolved-import` and everything downstream reads green — the worst shape a gate can have, since it looks like coverage and provides none. So: + +- **Write a `ty.toml` naming the interpreter explicitly.** Never rely on ambient `python`. Under `uv` the CLI owns the venv, so this is a known path rather than a probe — but the config must still be written. +- **`unresolved-import` for an allowlisted package is a hard error**, never an environment artefact to wave through. That is already the rule in the build agent's prompt. +- ty treats a wrong `environment.python` as fatal for the whole run, and rejects an `extra-paths` entry that is not a directory. Both are worse than the bug being fixed, so both are checked before the file is written. + +### Which `notte-sdk` to check against + +Latest. The runner image pins `notte-sdk` to a commit extracted from `notte-api/uv.lock` at build time (`build-docker.sh:54`), which sounds like it could lag PyPI — but the monorepo has a hard check that the latest SDK is installed before every release, so the lock is bumped and the image rebuilt on every SDK release. Latest tracks the runtime. + +So the CLI pins nothing and resolves fresh, and **`deploy` fails on SDK skew**, because a green check against an SDK the runtime does not run is exactly the drift this section removes. Two guards keep that from being outage-shaped: + +- **Never fail on unreachable PyPI.** Fall back to the uv cache and say so. An offline runner or a registry blip must not block a hotfix. +- **`--allow-sdk-skew`**, with the message naming installed versus latest. A door exists, or people route around the tool entirely. + +One consequence worth stating plainly: because the runtime moves, **a deployed function can break without anyone touching it.** A scheduled `notte stack check --verify-remote` is the only thing that would notice, which makes it more than the staleness alarm it is described as above. + +### Two hashes, because bundling breaks the round trip + +marketplace's core invariant — *"files are byte-identical to what prod serves"* — cannot survive a bundler. Replace it with two hashes in the lock: + +- `source_sha256` — over the canonicalized *set* of contributing source files (sorted `path:sha256` pairs). Drives create-vs-update. +- `artifact_sha256` — the bundled bytes. Drives the diff shown before confirming, against what's deployed now. + +And now that byte-fidelity is gone, a generated header is free and should be there: +```python +# generated by notte 0.1.0 — do not edit +# sources: functions/amazon_search/{main,parse}.py, functions/_shared/http.py +# source-sha256: 4f2a… +``` + +--- + +## Secrets + +The API shape (`notte-api/src/notte_api/secrets/endpoints.py`, table `tenant_secrets`): + +- `POST /secrets {namespace, name, value}` → 201. Namespaces: `function_env`, `llm_provider`. +- `GET /secrets?namespace=` → metadata only: `{id, namespace, name, key_hint, created_at, last_used_at}`. +- `GET /secrets/{name}?namespace=` → **plaintext value**. +- `DELETE /secrets/{secret_id}` → 204. + +Four facts that shape the design: + +1. **`(namespace, name)` is UNIQUE** — enforced by partial indexes, scoped org-or-user. That is the best natural key in the whole API and it makes secrets genuinely declarable. +2. **`function_env` names must match `^[A-Z_][A-Z0-9_]*$`**, ≤128, and must not be in `{NOTTE_API_KEY, NOTTE_API_URL, NOTTE_BASE_URL, NOTTE_ENV, ENVIRONMENT, NOTTE_DB_PREVIEW_BRANCH}`. Validate all of that client-side. +3. **`POST` is not an upsert.** It's a bare insert; a duplicate is a **409**. And **delete is by UUID, not name.** So "update a secret" means LIST → map name→id → DELETE → POST. Non-atomic, and there's a window where the secret doesn't exist. +4. **Secrets are org-scoped, not per-function.** Since each env is a different key/org, per-env separation comes free — but it also means the CLI's current placement of these under `notte functions secrets` is misleading. Promote to top-level `notte secrets`. + +### The model + +**Names in git, values in a gitignored `.env.`** (the same split Supabase uses). + +The declared set for an env = union of every deployed function's `required_secrets` (which the server already computes from the AST scan and returns on `FunctionResponse`) **plus** any explicit `[functions.x] secrets = [...]` for names computed at runtime that no scanner can see. + +``` +$ notte secrets diff --env prod + missing on prod (declared, not set): + AMAZON_PARTNER_TAG required by amazon_search + STOCKX_TOKEN required by stockx_search, stockx_bid + set on prod, not declared: + OLD_SCRAPER_KEY (not deleted — pass --prune to remove) +``` + +- `notte secrets push --env prod` reads `.env.prod`, POSTs what's missing. For a name that exists with a different `key_hint`, it DELETEs then POSTs — and **says so explicitly**, because that sequence is not atomic. +- **Diff on `key_hint`, never on values.** Reading a value back writes an `audit_events` row (`entity_type="tenant_secret"`, `action="read"`) and bumps `last_used_at`. A `diff` that silently audits every secret on every CI run is a bad neighbour. +- **Never prune by default.** Same rule as `reportExtraRemote`: report extras, delete only on `--prune`. +- Never print values, in any output mode. + +### Deploy preflight + +`preflight_required_secrets` already raises **422** with the missing names at run time, before a session starts (so nothing is charged). Pull that forward: after `notte stack deploy` writes a function, read back `required_secrets`, diff against the env, and warn — **especially before setting a cron**, since otherwise the first sign of trouble is a 09:00 job failing on a Sunday. + +--- + +## Schedules + +`POST /functions/{id}/schedule {cron, variables}` is genuinely good for reconciliation: an upsert, idempotent for the same cron, guarded by a Postgres advisory lock plus optimistic `schedule_revision` CAS, with EventBridge rollback compensation. It validates `variables` keys against the function's declared `variables`. + +**The blocker: there is no read endpoint.** The data exists — `functions.schedule_cron`, `schedule_variables`, `schedule_state`, `schedule_paused_reason`, `schedule_paused_at`, `schedule_revision` are all on the row and all on `SupabaseFunctionResponse` — but the API's `FunctionResponse` model drops them, so FastAPI never serialises them, so the generated Go client can't see them. The CLI has `schedule`/`unschedule` and nothing else. + +Consequence: `notte stack status` cannot show the current cron, and `notte stack deploy` cannot distinguish "already correct" from "about to change". Two options: + +- **Preferred:** add the three fields to `FunctionResponse` (`functions/endpoints.py:50-63`) — additive, mirrors exactly how `published` and `required_secrets` were added, and regenerates through OpenAPI → Go client. Then cron reconciles properly. +- **Until then:** record the last-applied cron in `notte.lock.json` and treat the lock as truth, printing a caveat that a change made in the console is invisible to `status`. + +Two rules regardless: + +- **Validate the cron client-side.** It must be six-field AWS EventBridge form (`cron(m h dom mon dow year)`) or `rate(...)`. A five-field crontab string is the natural thing to write and the docs currently show one. +- **Never fight `schedule_state`.** The system pauses schedules for `credit_exhausted` or `function_inactive`. A reconciler that re-POSTs because state ≠ enabled will thrash against the billing system. Reconcile on `cron` + `variables` only; surface `schedule_paused_reason` in `status` as information. + +--- + +## Managed auth — deferred, and why it will be easy + +**Templates are already the declarative design you're describing.** `POST /managed-auth/templates/import?dry_run=true` returns a real field-level diff — `{slug, action: create|update|no_change, previous_revision, revision, metadata_changes[], login_changed, verifier_changed, bundle_sha256, target_state_sha256}` — and the apply is guarded by `expected_target_state_sha256` from the preview. `GET /managed-auth/templates/{slug}/export` round-trips it. `slug` is UNIQUE. It is strictly the best-designed surface in the API, and `connectors/.json` is already a git-versioned manifest. + +Three things to know before adopting it: +- The router is **`include_in_schema=False`** (`main.py:719`), so managed-auth is absent from the OpenAPI spec and therefore absent from the generated Go client entirely. Flip that, or hand-write the client. +- Import is gated to one hard-coded org: `_require_connector_organization()` demands `org_id == "4dbf683a-…"`. Fine for you; a blocker if customers should ever declare their own connectors. +- A **connection** is not a template. Creating one runs a real browser login, spends money, and provisions a vault + a browser profile as side effects; `PATCH` covers only `label` and `schedule`; `credentials`, `vault_id`, `mailbox_id`, `two_fa_method`, `domain` are all create-only and immutable; and most of its fields (`status`, `last_login_at`, `last_failure_code`) are observed runtime state. Reconciling a connection means delete-and-re-login. **Keep connections imperative.** What a CLI can usefully add is operational commands (`list`, `check`, `reauthenticate`, `reset-profile`) and CI fixtures — the `ci-longlived-` / `ci-smoke--` pattern `smoke.py` already implements, including its hard refusal to delete anything with the long-lived prefix. + +Related: vaults, personas, and profiles all have server-generated UUIDs with non-unique names, so none of them are name-addressable. Profiles are the closest — `GET /profiles?name=` supports filtering, which makes find-or-create viable. Vault *credentials* are keyed by `(vault_id, root domain of url)`, which is a real natural key and effectively an upsert. Worth knowing, not worth building yet. + +--- + +## The DX ideas worth building + +**Source maps.** Emit `.notte/build//.map.json` mapping artifact line ranges → `source:line`, and have `notte logs` / `run-metadata` rewrite tracebacks through it. A traceback that says `line 612` in a 900-line concatenated file is the single worst thing about any bundler, and nobody in Python serverless fixes it. Highest-leverage item here. + +**Blast radius in `notte stack status`.** `_shared/contract.py` is inlined into every function that imports it, so editing it changes N artifacts. managed-auth papered over exactly this with `scripts/check_revision_bumps.py` (122 lines) plus a note in four separate docs. The CLI knows the import graph: +``` +$ notte stack status + functions/_shared/contract.py changed → 9 functions affected + ✗ google_login drifted (source 4f2a… ≠ deployed 8c31…) + ✗ bluesky_login drifted + … +``` +Auto-derived. `check_revision_bumps.py` and the manual `revision` field both disappear. + +**`notte promote` moves bytes, not source.** Download the artifact deployed to staging, upload those exact bytes to prod, record both hashes. Guarantees what you tested is what ships — stronger than re-running the build, and it's Vercel's model. + +**`notte stack check` as the CI gate `anything-api` designed and never wired up.** Writes nothing, exits non-zero on drift; `notte stack init` drops the GitHub Action in. Heed marketplace's warning: on a PR it's a genuine gate; on a schedule against prod it's a *staleness alarm*, since the catalog changes whenever anyone publishes. + +**`notte dev `.** Run the entrypoint locally against real cloud sessions, `--var` → `run()` kwargs. The current inner loop is deploy-to-test. This is `supabase functions serve` / `wrangler dev`, and it's where `drive_login.py` and the whole `login/*.py` exploratory-recording corpus want to live. + +**Confirm on the diff, not on the destination.** An earlier draft had `[env.prod] confirm = true`, which makes sense only when prod is the exceptional target. For almost every user it is the *only* target, so it degrades into friction on every deploy. The diff-then-confirm step already covers the real risk and keys off *what changed* rather than *where it is going*, which is the better signal anyway. marketplace's `push` refuses without a TTY, naming all three ways out (`--yes`, `--apply`, `--dry-run`); keep that wording. + +**Expose what's already in the generated client.** `--version` on update and `versions[]` on show → `notte rollback --to v20260821_162138`. `--decryption-key`, or just derive it automatically → deletes the duplicated `sha256("api_key:{k}:workflow_id:{id}:dumb")[:64]` that currently lives in two repos. + +**Agent-native scaffolding.** `notte stack init` writes `AGENTS.md` with the real contract — `run()` returns a `BaseModel`, the import allowlist, `from notte_sdk.types import os`, six-field cron — and registers the `notte-browser` skill. Encore does exactly this (`encore app create` asks which AI tool and writes the rules file). The material exists as `notte-skills/plugins/notte-cli/skills/notte-browser/references/function-management.md`; it needs the project layout added and to stay in sync (it's a submodule). + +**`notte init --from-session `.** `sessions workflow-code` already emits a deployable `run()`. Record a workflow in a browser → scaffold a project around it. An onboarding path nothing else in the prior-art table can offer. + +**Testing.** Two layers. + +*For user projects:* `test_*.py` colocated in the function dir, never bundled, run with pytest; `notte stack check --test` runs them. managed-auth's `ShippedConnectorsTest` — asserting properties of the real checked-in catalog, e.g. *"every connector still parses once inlined"* — generalizes into `notte stack check` itself and stops being something each repo hand-writes. + +*For the CLI itself:* the bundler is the part where a wrong answer is silent, so it ships with a golden-file suite before it ships at all — `internal/bundle/testdata//{in/,want.py}`, one directory per case, following `marketplace-catalog.ts`'s convention of naming each test after the invariant it protects. The cases that must exist on day one: + +| Case | Asserts | +|---|---| +| `alias-preserved` | `from .parse import f as g` emits `g = f`; the artifact defines `g` | +| `alias-collides` | an alias colliding with another module's top-level name is reported, not silently shadowed | +| `collision-reported` | two modules defining `clean` fail with both file:line locations | +| `topo-order` | a dependency's body precedes every body that imports it | +| `diamond` | a module reached by two paths is emitted exactly once | +| `cycle-rejected` | the error names the cycle | +| `future-annotations` | emitted once, first statement, even when three modules declare it | +| `import-hoist-dedup` | `import requests` in four modules yields one line | +| `star-import`, `from-dot-import`, `import-in-function` | each rejected with its fix-it message | +| `disallowed-import` | `import os` fails locally with the `notte_sdk.types` hint, before any upload | +| `deterministic` | bundling twice byte-identical — `artifact_sha256` is load-bearing for the whole diff model | +| `source-map` | every artifact line maps to a real `source:line` | + +`alias-preserved` and `alias-collides` exist because that gap was found in review of this document rather than in a test — which is the argument for the table. + +--- + +## Migration + +**`managed-auth`** — the cleaner fit for the bundler. `contract.py` → `functions/_shared/contract.py`; `login/bluesky_login.py` → `functions/bluesky_login/main.py`; `verifier/bluesky.py` → `functions/bluesky_verify/main.py`. `from contract import LoginResult` becomes `from .._shared.contract import LoginResult`, and `inline_contract()` — including its `re.sub` escape-expansion war story — is deleted. `login/email_2fa.py` becomes `functions/_shared/email_2fa.py` and the four hand-copied `_verification_code()` loops collapse into one import. `revision` and `check_revision_bumps.py` are replaced by `source_sha256`. What doesn't map in v1: the connector concept (one slug = a login + verifier deployed transactionally) and `/managed-auth/templates/import`. Keep a thin `deploy.py` for the template bundle, let `notte` own the two functions, and revisit when managed-auth joins the project model. + +**`marketplace`** — 2,049 files, zero relative imports, so bundling is a no-op for every one of them. The value is deleting `marketplace-catalog.ts`: the `MAKECMDGOALS` filtering, `createNotteRunner` + `detectCliError`, `redact()`, the org preflight, the decryption-key derivation, the `pool`/`retry` helpers — all CLI-native. `manifest.json` maps almost field-for-field onto `notte.lock.json`; `envs[env].{function_id, functions_version, versions, code_sha256}` is already the right shape. The gap it exposes: **`name`, `description`, `categories` are only editable upstream** — `push` only ever sends `--file`. `[functions.]` should own them and `notte stack deploy` should push them, which is a real capability gain over what exists. + +--- + +## Backend asks (ordered by how much they unblock) + +1. **Add `schedule_cron` / `schedule_variables` / `schedule_state` to `FunctionResponse`** (`functions/endpoints.py:50-63`). ~2 lines, additive, exactly how `published` and `required_secrets` were added. Without it, cron cannot be reconciled — only blindly re-applied. +2. **Make secrets updatable by name**: `PUT /secrets/{namespace}/{name}` as an upsert, and `DELETE` by `(namespace, name)`. Today a secret rotation is LIST → DELETE-by-uuid → POST, which is three calls and a window where the secret doesn't exist. +3. **`GET /me`** → `{user_id, org_id, org_name, org_role, plan_type}`. A `notte.toml` committed to git gets applied by different keys; without this, "am I about to deploy to the right org?" is unanswerable. It also deletes marketplace's hack of reading the org id out of the first path segment of a signed download URL. +4. **`POST /functions?dry_run=true`** returning a managed-auth-style diff (`action`, `previous_version`, `changed`, `target_state_sha256`). Enables preview→guard→apply for functions and makes `notte stack check` server-authoritative. +5. *(later, for managed auth)* Flip `include_in_schema` on the managed-auth router so the Go client can be generated, and decide whether template import stays gated to the connectors org. + +--- + +## Open questions + +1. **Should `safe_import` remain the runtime import guard?** Execution already runs with `restricted=False`, so the RestrictedPython AST policy is off and `safe_import` is the only thing standing between a deployed function and arbitrary imports. It is also, incidentally, what makes off-the-shelf bundlers unusable. Relaxing it would make them work — but that trade should be evaluated on its own merits, not taken as a side effect of a bundling convenience, and this RFC does not need it either way. +2. **`notte functions` vs the project commands.** The current commands are function-id-centric with global `~/.notte/cli/current_function` state; the new ones are project-centric. Proposal: `GetCurrentFunctionID()` gains a fourth source — the project lock, resolved from cwd — ahead of the global state file, so both surfaces stay coherent. +3. **Is `preview` (dev + `x-db-preview: `) Notte-wide or managed-auth-specific?** Modeled above as generic `[env.*] headers`, which may be over-general. +4. **Function grouping.** managed-auth needs "these two deploy together, transactionally." Does a `[bundle]` concept belong in the model now, or is it deferred with managed auth? diff --git a/internal/bundle/bundle.go b/internal/bundle/bundle.go new file mode 100644 index 0000000..b5e5617 --- /dev/null +++ b/internal/bundle/bundle.go @@ -0,0 +1,304 @@ +// Package bundle flattens a Python package into the single file the Notte +// functions API accepts. +// +// The API rejects every form of local import: `from . import x` is refused +// outright by the upload validator, and `from .util import x` then fails the +// import allowlist by name — at run time as well, because the Lambda runner +// keeps __import__ bound to safe_import even though it disables the +// RestrictedPython AST policy. So a multi-module function has to become one +// module before it is uploaded, and it has to do so statically: the sys.modules +// prelude every off-the-shelf bundler emits would need imports that are +// themselves blocked. +// +// Flattening is concatenation in dependency order with relative imports +// removed. Names that would collide are an error rather than something to +// mangle, which is what keeps a full Python parser out of this package: nothing +// is ever rewritten, so nothing has to be understood well enough to rewrite. +// It also keeps the artifact readable, which matters because the artifact is +// what the console shows and what tracebacks point at. +// +// This package deliberately does not check imports against the runtime +// allowlist. It used to, by vendoring a copy of the server's lists and a stdlib +// set generated from a pinned CPython — and both drifted within a week of being +// written. `notte stack` requires Python, so the real ScriptValidator runs +// against the artifact instead, and a copy that can disagree with it is worse +// than no copy at all. +package bundle + +import ( + "fmt" + "io/fs" + "path" + "strings" +) + +// Result is a successful bundle. +type Result struct { + // Code is the artifact: valid Python defining exactly the names the + // entrypoint's module would have had. + Code string + // Sources are the contributing files in emission order, entrypoint last. + Sources []string + // SourceSHA256 covers the set of inputs. It answers "does this need + // deploying?" and is stable across bundler changes that do not change the + // sources. + SourceSHA256 string + // ArtifactSHA256 covers Code. It answers "what changed upstream?" and is + // what a remote diff compares against. + ArtifactSHA256 string + // Map resolves an artifact line back to the file and line it came from. + Map *SourceMap +} + +// Error is a bundling failure carrying the location that caused it. +type Error struct { + Path string + Line int + Msg string + Hint string +} + +func (e *Error) Error() string { + loc := e.Path + if e.Line > 0 { + loc = fmt.Sprintf("%s:%d", e.Path, e.Line) + } + if e.Hint != "" { + return fmt.Sprintf("%s: %s — %s", loc, e.Msg, e.Hint) + } + return fmt.Sprintf("%s: %s", loc, e.Msg) +} + +func errAt(p string, line int, msg, hint string) *Error { + return &Error{Path: p, Line: line, Msg: msg, Hint: hint} +} + +// Options tunes the emitted artifact. +type Options struct { + // Header is prepended verbatim. Callers pass provenance here; the bundler + // does not invent one so that output stays a pure function of the input. + Header string +} + +// module is one parsed source file. +type module struct { + path string // slash-separated, relative to the package root + src string + lines []string + stmts []Stmt + imports []Import + deps []string // resolved paths of relative imports, in source order +} + +// Bundle flattens the package reachable from entrypoint into one file. +// +// fsys is rooted at the functions directory, and entrypoint is a path within +// it such as "amazon_search/main.py". Only relative imports are followed; +// absolute ones are hoisted and left for the allowlist check. +func Bundle(fsys fs.FS, entrypoint string, opts Options) (*Result, error) { + mods := map[string]*module{} + order, err := collect(fsys, entrypoint, mods, nil) + if err != nil { + return nil, err + } + if err := checkCollisions(order, mods); err != nil { + return nil, err + } + return emit(order, mods, opts) +} + +// collect loads the entrypoint and everything it reaches, returning modules in +// dependency-first order. stack carries the current resolution path so a cycle +// can be reported as the cycle it is rather than as a stack overflow. +func collect(fsys fs.FS, p string, mods map[string]*module, stack []string) ([]string, error) { + for i, s := range stack { + if s == p { + return nil, errAt(p, 0, "import cycle: "+strings.Join(append(stack[i:], p), " -> "), + "break the cycle by moving the shared names into their own module") + } + } + if _, done := mods[p]; done { + return nil, nil + } + + m, err := load(fsys, p) + if err != nil { + return nil, err + } + mods[p] = m + + var order []string + for _, dep := range m.deps { + sub, err := collect(fsys, dep, mods, append(stack, p)) + if err != nil { + return nil, err + } + order = append(order, sub...) + } + return append(order, p), nil +} + +func load(fsys fs.FS, p string) (*module, error) { + raw, err := fs.ReadFile(fsys, p) + if err != nil { + return nil, errAt(p, 0, "cannot read module", "") + } + src := string(raw) + m := &module{ + path: p, + src: src, + lines: strings.Split(src, "\n"), + stmts: Scan(src), + } + + for _, s := range m.stmts { + im, ok := ParseImport(s) + if !ok { + continue + } + if !s.TopLevel() { + // A relative import inside a function body would have to be + // rewritten in place, and rewriting is the thing this design + // avoids. Absolute ones are harmless where they are. + if im.Kind == ImportRelative { + return nil, errAt(p, s.StartLine, "relative import inside an indented block", + "move it to the top of the file") + } + continue + } + // Emission drops or rewrites whole lines, so an import sharing a line + // with anything else would take its neighbour with it. Rejecting is + // cheap for the author to fix and PEP 8 asks for it anyway; a sub-line + // rewriter is a lot of machinery for `import os; x = 1`. + if sharesLine(m.stmts, s) { + return nil, errAt(p, s.StartLine, "import shares a line with another statement", + "put each import on its own line") + } + m.imports = append(m.imports, im) + + switch im.Kind { + case ImportFuture: + if len(im.Names) != 1 || im.Names[0].Name != "annotations" { + return nil, errAt(p, s.StartLine, "only 'from __future__ import annotations' is allowed", "") + } + case ImportRelative: + if im.Star { + return nil, errAt(p, s.StartLine, "star imports cannot be flattened", + "import the names explicitly") + } + if im.Module == "" { + return nil, errAt(p, s.StartLine, "'from . import ' cannot be flattened", + fmt.Sprintf("use 'from .%s import ' instead", firstName(im))) + } + dep, err := resolve(p, im) + if err != nil { + return nil, err + } + m.deps = append(m.deps, dep) + } + } + return m, nil +} + +// sharesLine reports whether any other statement begins inside this one's span. +func sharesLine(stmts []Stmt, target Stmt) bool { + for _, other := range stmts { + if other == target { + continue + } + if other.StartLine >= target.StartLine && other.StartLine <= target.EndLine { + return true + } + } + return false +} + +func firstName(im Import) string { + if len(im.Names) > 0 { + return im.Names[0].Name + } + return "mod" +} + +// resolve turns a relative import into a path within the package root. +// +// Level 1 is the importing module's own package, level 2 its parent, and so +// on — the same rule Python uses, so a layout that resolves here resolves in +// the editor too. +func resolve(from string, im Import) (string, error) { + pkg := path.Dir(from) + if pkg == "." { + pkg = "" + } + for i := 1; i < im.Level; i++ { + if pkg == "" { + return "", errAt(from, im.Stmt.StartLine, + "relative import goes above the functions directory", "") + } + pkg = path.Dir(pkg) + if pkg == "." { + pkg = "" + } + } + rel := strings.ReplaceAll(im.Module, ".", "/") + ".py" + if pkg == "" { + return rel, nil + } + return pkg + "/" + rel, nil +} + +// checkCollisions rejects two modules defining the same module-level name. +// +// Concatenation makes the later definition win silently, so this is reported +// rather than resolved. Aliases count: `from .parse import clean as fetch` +// introduces `fetch` exactly as a def would. +func checkCollisions(order []string, mods map[string]*module) error { + type owner struct { + path string + line int + } + seen := map[string]owner{} + + for _, p := range order { + m := mods[p] + for _, s := range m.stmts { + if !s.TopLevel() { + continue + } + for _, name := range bindingsOwnedBy(s) { + if prev, dup := seen[name]; dup && prev.path != p { + return errAt(p, s.StartLine, + fmt.Sprintf("%s:%d and %s:%d both define %q", prev.path, prev.line, p, s.StartLine, name), + "rename one of them") + } + seen[name] = owner{path: p, line: s.StartLine} + } + } + } + return nil +} + +// bindingsOwnedBy are the names a statement introduces into the flattened +// namespace. +// +// An unaliased relative import is excluded: `from .parse import clean` refers +// to the very definition that will be concatenated in, so counting it would +// report every shared helper as colliding with itself. Absolute imports are +// excluded because they are hoisted and deduplicated, so four modules importing +// requests produce one binding rather than four. +func bindingsOwnedBy(s Stmt) []string { + im, isImport := ParseImport(s) + if !isImport { + return TopLevelBindings(s) + } + if im.Kind != ImportRelative { + return nil + } + var out []string + for _, n := range im.Names { + if n.Alias != "" { + out = append(out, n.Alias) + } + } + return out +} diff --git a/internal/bundle/corpus_test.go b/internal/bundle/corpus_test.go new file mode 100644 index 0000000..ae24969 --- /dev/null +++ b/internal/bundle/corpus_test.go @@ -0,0 +1,73 @@ +package bundle + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +// TestRealCorpus bundles every .py file in a real functions tree and checks +// that it survives: the bundler accepts it, the artifact compiles, and no +// top-level definition is lost. +// +// Opt-in via NOTTE_CORPUS because it needs a checkout and takes a couple of +// minutes. It is the highest-value test here by some distance — a hand-written +// suite covers the cases its author thought of, while anything-api/marketplace +// is 2.5k files of production Python written by other people and by an agent, +// full of constructs nobody would think to write down. Point it at a +// marketplace checkout before trusting a change to the scanner: +// +// NOTTE_CORPUS=~/path/to/anything-api/marketplace go test ./internal/bundle -run TestRealCorpus +func TestRealCorpus(t *testing.T) { + root := os.Getenv("NOTTE_CORPUS") + if root == "" { + t.Skip("set NOTTE_CORPUS") + } + var files []string + _ = filepath.WalkDir(root, func(p string, d os.DirEntry, err error) error { + if err == nil && !d.IsDir() && strings.HasSuffix(p, ".py") { + files = append(files, p) + } + return nil + }) + t.Logf("found %d files", len(files)) + + py, _ := exec.LookPath("python3") + var bundleErrs, compileErrs, mismatched int + for _, f := range files { + rel, _ := filepath.Rel(root, f) + res, err := Bundle(os.DirFS(root), filepath.ToSlash(rel), Options{}) + if err != nil { + bundleErrs++ + if bundleErrs <= 8 { + t.Logf("BUNDLE ERR %s: %v", rel, err) + } + continue + } + // Zero relative imports in this corpus, so every def must survive. + orig, _ := os.ReadFile(f) + for _, line := range strings.Split(string(orig), "\n") { + if strings.HasPrefix(line, "def ") && !strings.Contains(res.Code, line) { + mismatched++ + if mismatched <= 5 { + t.Logf("LOST DEF %s: %q", rel, line) + } + break + } + } + if py != "" { + tmp := filepath.Join(t.TempDir(), "a.py") + _ = os.WriteFile(tmp, []byte(res.Code), 0o644) + if out, err := exec.Command(py, "-m", "py_compile", tmp).CombinedOutput(); err != nil { + compileErrs++ + if compileErrs <= 8 { + t.Logf("COMPILE ERR %s: %s", rel, out) + } + } + } + } + t.Logf("RESULT files=%d bundleErrs=%d compileErrs=%d lostDefs=%d", + len(files), bundleErrs, compileErrs, mismatched) +} diff --git a/internal/bundle/emit.go b/internal/bundle/emit.go new file mode 100644 index 0000000..69e2844 --- /dev/null +++ b/internal/bundle/emit.go @@ -0,0 +1,263 @@ +package bundle + +import ( + "crypto/sha256" + "encoding/hex" + "fmt" + "sort" + "strings" +) + +// SourceMap maps 1-based artifact lines back to where they came from. +// +// Without it a traceback points into a concatenated file and the reader has to +// guess which module line 612 belonged to. Entries are sorted by ArtifactLine +// and cover every emitted source line; generated lines (the header, hoisted +// imports, alias assignments) have an empty Path. +type SourceMap struct { + Entries []MapEntry `json:"entries"` +} + +// MapEntry is one contiguous run of artifact lines from one source file. +type MapEntry struct { + ArtifactLine int `json:"artifact_line"` + Path string `json:"path"` + SourceLine int `json:"source_line"` + Count int `json:"count"` +} + +// Lookup resolves an artifact line to its source location. ok is false for +// generated lines, which have no source. +func (sm *SourceMap) Lookup(artifactLine int) (path string, line int, ok bool) { + i := sort.Search(len(sm.Entries), func(i int) bool { + return sm.Entries[i].ArtifactLine > artifactLine + }) - 1 + if i < 0 { + return "", 0, false + } + e := sm.Entries[i] + if e.Path == "" || artifactLine >= e.ArtifactLine+e.Count { + return "", 0, false + } + return e.Path, e.SourceLine + (artifactLine - e.ArtifactLine), true +} + +// writer accumulates artifact lines and the map alongside them. +type writer struct { + lines []string + entries []MapEntry +} + +// generated appends lines with no source counterpart. +func (w *writer) generated(lines ...string) { + w.lines = append(w.lines, lines...) +} + +// fromSource appends one source line and records where it came from, extending +// the previous run when it is contiguous. +func (w *writer) fromSource(path string, srcLine int, text string) { + artifactLine := len(w.lines) + 1 + if n := len(w.entries); n > 0 { + last := &w.entries[n-1] + if last.Path == path && last.ArtifactLine+last.Count == artifactLine && last.SourceLine+last.Count == srcLine { + last.Count++ + w.lines = append(w.lines, text) + return + } + } + w.entries = append(w.entries, MapEntry{ + ArtifactLine: artifactLine, + Path: path, + SourceLine: srcLine, + Count: 1, + }) + w.lines = append(w.lines, text) +} + +// emit concatenates the modules in dependency order. +// +// Per module: relative imports become alias assignments or vanish, absolute +// imports are lifted to a single deduplicated block at the top, and every other +// line is copied verbatim so the artifact still reads like the sources. +func emit(order []string, mods map[string]*module, opts Options) (*Result, error) { + w := &writer{} + + if opts.Header != "" { + for _, line := range strings.Split(strings.TrimRight(opts.Header, "\n"), "\n") { + w.generated(line) + } + w.generated("") + } + + // `from __future__` must precede every other statement, so it is emitted + // once here no matter which module asked for it. + if anyFutureAnnotations(order, mods) { + w.generated("from __future__ import annotations", "") + } + + if hoisted := hoistImports(order, mods); len(hoisted) > 0 { + w.generated(hoisted...) + w.generated("") + } + + for _, p := range order { + m := mods[p] + drop, replace := rewritePlan(m) + + w.generated("# ── " + p + " ──") + started := false + for i, text := range m.lines { + lineNo := i + 1 + if lineNo == len(m.lines) && text == "" { + continue // trailing newline artefact of the split + } + if drop[lineNo] { + continue + } + if aliases, ok := replace[lineNo]; ok { + w.generated(aliases...) + started = true + continue + } + // Removing a module's imports strands the blank lines that + // separated them from the first definition, under the header + // comment. Skip forward to real content. + if !started && strings.TrimSpace(text) == "" { + continue + } + started = true + w.fromSource(p, lineNo, text) + } + w.generated("") + } + + code := strings.Join(w.lines, "\n") + if !strings.HasSuffix(code, "\n") { + code += "\n" + } + + return &Result{ + Code: code, + Sources: order, + SourceSHA256: sourceHash(order, mods), + ArtifactSHA256: sha256Hex(code), + Map: &SourceMap{Entries: w.entries}, + }, nil +} + +// rewritePlan decides, per physical line, what emission does with it. +// +// drop covers lines that leave entirely (hoisted absolute imports, __future__, +// unaliased relative imports). replace maps the first line of an aliased +// relative import to the assignments that preserve its bindings. +func rewritePlan(m *module) (drop map[int]bool, replace map[int][]string) { + drop = map[int]bool{} + replace = map[int][]string{} + + for _, im := range m.imports { + switch im.Kind { + case ImportAbsolute, ImportFrom, ImportFuture: + for l := im.Stmt.StartLine; l <= im.Stmt.EndLine; l++ { + drop[l] = true + } + case ImportRelative: + // The definition arrives by concatenation, so the unaliased name is + // already bound. An alias is not, and deleting the line without + // recreating it is a NameError at run time that nothing before + // production would catch. + var assigns []string + for _, n := range im.Names { + if n.Alias != "" { + assigns = append(assigns, fmt.Sprintf("%s = %s", n.Alias, n.Name)) + } + } + for l := im.Stmt.StartLine; l <= im.Stmt.EndLine; l++ { + drop[l] = true + } + if len(assigns) > 0 { + delete(drop, im.Stmt.StartLine) + replace[im.Stmt.StartLine] = assigns + } + } + } + return drop, replace +} + +func anyFutureAnnotations(order []string, mods map[string]*module) bool { + for _, p := range order { + for _, im := range mods[p].imports { + if im.Kind == ImportFuture { + return true + } + } + } + return false +} + +// hoistImports collects every absolute import, deduplicated and sorted. +// +// Sorting is what makes the artifact reproducible: the same sources must yield +// the same bytes, because ArtifactSHA256 drives the remote diff. +func hoistImports(order []string, mods map[string]*module) []string { + seen := map[string]bool{} + var plain, from []string + for _, p := range order { + for _, im := range mods[p].imports { + if im.Kind != ImportAbsolute && im.Kind != ImportFrom { + continue + } + text := normalizeImport(im) + if seen[text] { + continue + } + seen[text] = true + if im.Kind == ImportAbsolute { + plain = append(plain, text) + } else { + from = append(from, text) + } + } + } + sort.Strings(plain) + sort.Strings(from) + return append(plain, from...) +} + +// normalizeImport rebuilds an import from its parsed form so that two spellings +// of the same import deduplicate. +func normalizeImport(im Import) string { + names := make([]string, 0, len(im.Names)) + for _, n := range im.Names { + if n.Alias != "" { + names = append(names, n.Name+" as "+n.Alias) + } else { + names = append(names, n.Name) + } + } + sort.Strings(names) + if im.Kind == ImportAbsolute { + return "import " + strings.Join(names, ", ") + } + if im.Star { + return "from " + im.Module + " import *" + } + return "from " + im.Module + " import " + strings.Join(names, ", ") +} + +// sourceHash covers the inputs rather than the output, so it is stable when the +// bundler changes but the sources do not. +func sourceHash(order []string, mods map[string]*module) string { + paths := append([]string(nil), order...) + sort.Strings(paths) + h := sha256.New() + for _, p := range paths { + // hash.Hash.Write is documented never to return an error. + _, _ = fmt.Fprintf(h, "%s:%s\n", p, sha256Hex(mods[p].src)) + } + return hex.EncodeToString(h.Sum(nil)) +} + +func sha256Hex(s string) string { + sum := sha256.Sum256([]byte(s)) + return hex.EncodeToString(sum[:]) +} diff --git a/internal/bundle/errors_test.go b/internal/bundle/errors_test.go new file mode 100644 index 0000000..9f2d557 --- /dev/null +++ b/internal/bundle/errors_test.go @@ -0,0 +1,223 @@ +package bundle + +import ( + "strings" + "testing" + "testing/fstest" +) + +// mapFS builds an in-memory package. Keys are paths under the functions +// directory; values are file bodies. +func mapFS(files map[string]string) fstest.MapFS { + fsys := fstest.MapFS{} + for p, body := range files { + fsys[p] = &fstest.MapFile{Data: []byte(body)} + } + return fsys +} + +// wantErr bundles and requires failure, returning the message. +func wantErr(t *testing.T, files map[string]string) string { + t.Helper() + res, err := Bundle(mapFS(files), "fn/main.py", Options{}) + if err == nil { + t.Fatalf("expected an error, got a bundle:\n%s", res.Code) + } + return err.Error() +} + +func mustContain(t *testing.T, got string, wants ...string) { + t.Helper() + for _, w := range wants { + if !strings.Contains(got, w) { + t.Fatalf("error %q does not mention %q", got, w) + } + } +} + +// Two modules defining the same name: concatenation would silently let the +// later one win. +func TestCollisionIsReportedWithBothLocations(t *testing.T) { + msg := wantErr(t, map[string]string{ + "fn/main.py": "from .a import a\nfrom .b import b\n\n\ndef run():\n return a() + b()\n", + "fn/a.py": "def clean(s):\n return s\n\n\ndef a():\n return clean(1)\n", + "fn/b.py": "def clean(s):\n return s\n\n\ndef b():\n return clean(2)\n", + }) + mustContain(t, msg, "clean", "fn/a.py", "fn/b.py", "rename") +} + +// An alias occupies a name exactly as a definition does. +func TestAliasCollidesWithDefinition(t *testing.T) { + msg := wantErr(t, map[string]string{ + "fn/main.py": "from .a import helper as fetch\nfrom .b import fetch as other\n\n\ndef run():\n return fetch, other\n", + "fn/a.py": "def helper():\n return 1\n", + "fn/b.py": "def fetch():\n return 2\n", + }) + mustContain(t, msg, "fetch") +} + +// The same name imported unaliased by two modules is one definition, not a +// collision. A naive binding count reports every shared helper as conflicting. +func TestSharedHelperImportedTwiceIsNotACollision(t *testing.T) { + res, err := Bundle(mapFS(map[string]string{ + "fn/main.py": "from .a import a\nfrom .b import b\n\n\ndef run():\n return a() + b()\n", + "fn/a.py": "from .shared import shared\n\n\ndef a():\n return shared()\n", + "fn/b.py": "from .shared import shared\n\n\ndef b():\n return shared()\n", + "fn/shared.py": "def shared():\n return 1\n", + }), "fn/main.py", Options{}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if n := strings.Count(res.Code, "def shared()"); n != 1 { + t.Fatalf("shared emitted %d times, want 1", n) + } +} + +func TestImportCycleIsReported(t *testing.T) { + msg := wantErr(t, map[string]string{ + "fn/main.py": "from .a import a\n\n\ndef run():\n return a()\n", + "fn/a.py": "from .b import b\n\n\ndef a():\n return b()\n", + "fn/b.py": "from .a import a\n\n\ndef b():\n return a()\n", + }) + mustContain(t, msg, "cycle", "fn/a.py", "fn/b.py") +} + +func TestSelfImportIsReportedAsCycle(t *testing.T) { + msg := wantErr(t, map[string]string{ + "fn/main.py": "from .main import run\n\n\ndef run():\n return 1\n", + }) + mustContain(t, msg, "cycle") +} + +func TestStarImportIsRejected(t *testing.T) { + msg := wantErr(t, map[string]string{ + "fn/main.py": "from .helpers import *\n\n\ndef run():\n return 1\n", + "fn/helpers.py": "def h():\n return 1\n", + }) + mustContain(t, msg, "star", "explicitly") +} + +// `from . import mod` then `mod.f()` needs the module to survive as an object, +// which flattening cannot provide. The message has to name the alternative. +func TestFromDotImportIsRejectedWithAFix(t *testing.T) { + msg := wantErr(t, map[string]string{ + "fn/main.py": "from . import parse\n\n\ndef run():\n return parse.clean(1)\n", + "fn/parse.py": "def clean(s):\n return s\n", + }) + mustContain(t, msg, "from .parse import") +} + +func TestRelativeImportInsideFunctionIsRejected(t *testing.T) { + msg := wantErr(t, map[string]string{ + "fn/main.py": "def run():\n from .parse import clean\n return clean(1)\n", + "fn/parse.py": "def clean(s):\n return s\n", + }) + mustContain(t, msg, "indented", "top of the file") +} + +// An absolute import inside a function is harmless where it is: it needs no +// rewriting, so there is no reason to reject it. +func TestAbsoluteImportInsideFunctionIsAllowed(t *testing.T) { + if _, err := Bundle(mapFS(map[string]string{ + "fn/main.py": "def run():\n import json\n return json.dumps({})\n", + }), "fn/main.py", Options{}); err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + +func TestNonAnnotationsFutureImportIsRejected(t *testing.T) { + msg := wantErr(t, map[string]string{ + "fn/main.py": "from __future__ import division\n\n\ndef run():\n return 1\n", + }) + mustContain(t, msg, "__future__") +} + +func TestMissingModuleIsReported(t *testing.T) { + msg := wantErr(t, map[string]string{ + "fn/main.py": "from .nope import x\n\n\ndef run():\n return x\n", + }) + mustContain(t, msg, "fn/nope.py") +} + +// Climbing above the functions directory has no meaning; it must not silently +// resolve to something outside the tree. +func TestImportAboveRootIsReported(t *testing.T) { + msg := wantErr(t, map[string]string{ + "fn/main.py": "from ...outside import x\n\n\ndef run():\n return x\n", + }) + mustContain(t, msg, "above the functions directory") +} + +func TestMissingEntrypointIsReported(t *testing.T) { + _, err := Bundle(mapFS(map[string]string{"fn/other.py": "x = 1\n"}), "fn/main.py", Options{}) + if err == nil { + t.Fatal("expected an error") + } + mustContain(t, err.Error(), "fn/main.py") +} + +// Errors carry a location so the message can be acted on without searching. +func TestErrorCarriesPathAndLine(t *testing.T) { + _, err := Bundle(mapFS(map[string]string{ + "fn/main.py": "import requests\n\nfrom .helpers import *\n", + "fn/helpers.py": "def h():\n return 1\n", + }), "fn/main.py", Options{}) + if err == nil { + t.Fatal("expected an error") + } + be, ok := err.(*Error) + if !ok { + t.Fatalf("error is %T, want *bundle.Error", err) + } + if be.Path != "fn/main.py" || be.Line != 3 { + t.Fatalf("location = %s:%d, want fn/main.py:3", be.Path, be.Line) + } +} + +// `import json; import re` used to parse as a single import of a module named +// "json;", drop the whole line, and never hoist re — a NameError from an +// artifact that compiled and passed upload validation. +func TestSemicolonSeparatedImportsAreRejected(t *testing.T) { + msg := wantErr(t, map[string]string{ + "fn/main.py": "import json; import re\n\n\ndef run():\n return json, re\n", + }) + mustContain(t, msg, "own line") +} + +func TestImportSharingALineWithCodeIsRejected(t *testing.T) { + msg := wantErr(t, map[string]string{ + "fn/main.py": "import json; x = 1\n\n\ndef run():\n return x\n", + }) + mustContain(t, msg, "own line") +} + +// Semicolons elsewhere are fine; only imports constrain the rewriter. +func TestSemicolonInOrdinaryCodeIsAllowed(t *testing.T) { + res, err := Bundle(mapFS(map[string]string{ + "fn/main.py": "A = 1; B = 2\n\n\ndef run():\n return A + B\n", + }), "fn/main.py", Options{}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !strings.Contains(res.Code, "A = 1; B = 2") { + t.Fatalf("line altered:\n%s", res.Code) + } +} + +// Collisions must be caught for names bound without an '=' too. +func TestClauseBoundNamesCollide(t *testing.T) { + for _, b := range []struct{ name, src string }{ + {"for", "for item in [1]:\n pass\n"}, + {"with", "with open(\"f\") as item:\n pass\n"}, + {"walrus", "if (item := 1):\n pass\n"}, + } { + t.Run(b.name, func(t *testing.T) { + msg := wantErr(t, map[string]string{ + "fn/main.py": "from .a import a\nfrom .b import b\n\n\ndef run():\n return a() + b()\n", + "fn/a.py": b.src + "\n\ndef a():\n return 1\n", + "fn/b.py": "item = 2\n\n\ndef b():\n return item\n", + }) + mustContain(t, msg, "item") + }) + } +} diff --git a/internal/bundle/golden_test.go b/internal/bundle/golden_test.go new file mode 100644 index 0000000..1463abd --- /dev/null +++ b/internal/bundle/golden_test.go @@ -0,0 +1,190 @@ +package bundle + +import ( + "flag" + "os" + "path/filepath" + "strings" + "testing" +) + +// -update rewrites the golden files. Review the diff it produces: these files +// are the specification of what the bundler emits. +var update = flag.Bool("update", false, "rewrite golden files") + +// goldenCases are the fixtures under testdata//in, bundled from +// fn/main.py and compared against testdata//want.py. +var goldenCases = []string{ + "single-file", + "alias-preserved", + "topo-order", + "diamond", + "import-hoist-dedup", + "future-annotations", + "shared-parent", + "docstring-not-an-import", +} + +func bundleCase(t *testing.T, name string) *Result { + t.Helper() + res, err := Bundle(os.DirFS(filepath.Join("testdata", name, "in")), "fn/main.py", Options{}) + if err != nil { + t.Fatalf("%s: %v", name, err) + } + return res +} + +func TestGolden(t *testing.T) { + for _, name := range goldenCases { + t.Run(name, func(t *testing.T) { + res := bundleCase(t, name) + goldenPath := filepath.Join("testdata", name, "want.py") + + if *update { + if err := os.WriteFile(goldenPath, []byte(res.Code), 0o644); err != nil { + t.Fatal(err) + } + return + } + want, err := os.ReadFile(goldenPath) + if err != nil { + t.Fatalf("missing golden file (run: go test ./internal/bundle -update): %v", err) + } + if res.Code != string(want) { + t.Errorf("artifact differs from golden\n--- got ---\n%s\n--- want ---\n%s", res.Code, want) + } + }) + } +} + +// The artifact hash drives the remote diff, so identical inputs must produce +// identical bytes. Map iteration order is the obvious way for that to break. +func TestBundleIsDeterministic(t *testing.T) { + for _, name := range goldenCases { + t.Run(name, func(t *testing.T) { + first := bundleCase(t, name) + for i := 0; i < 8; i++ { + again := bundleCase(t, name) + if again.Code != first.Code { + t.Fatalf("run %d differs from run 0", i+1) + } + if again.ArtifactSHA256 != first.ArtifactSHA256 { + t.Fatalf("ArtifactSHA256 unstable: %s vs %s", again.ArtifactSHA256, first.ArtifactSHA256) + } + if again.SourceSHA256 != first.SourceSHA256 { + t.Fatalf("SourceSHA256 unstable") + } + } + }) + } +} + +// The whole point of the alias rule: the artifact must still bind pr. +func TestAliasBindingSurvivesInArtifact(t *testing.T) { + res := bundleCase(t, "alias-preserved") + if !strings.Contains(res.Code, "pr = parse_rows") { + t.Fatalf("alias assignment missing:\n%s", res.Code) + } + if strings.Contains(res.Code, "from .parse import") { + t.Fatalf("relative import survived into the artifact:\n%s", res.Code) + } + // clean is unaliased, so it needs no assignment — the definition carries it. + if strings.Contains(res.Code, "clean = clean") { + t.Fatalf("emitted a redundant self-assignment:\n%s", res.Code) + } +} + +func TestTopologicalOrder(t *testing.T) { + res := bundleCase(t, "topo-order") + if want := []string{"fn/base.py", "fn/mid.py", "fn/main.py"}; !equal(res.Sources, want) { + t.Fatalf("sources = %v, want %v", res.Sources, want) + } + base := strings.Index(res.Code, "def base()") + middle := strings.Index(res.Code, "def middle()") + run := strings.Index(res.Code, "def run()") + if base >= middle || middle >= run { + t.Fatalf("definitions out of dependency order: base=%d middle=%d run=%d", base, middle, run) + } +} + +// A module reached by two paths must appear once; twice would be a redefinition +// and, for a class, a different object than the one already captured. +func TestDiamondEmitsSharedModuleOnce(t *testing.T) { + res := bundleCase(t, "diamond") + if n := strings.Count(res.Code, "def shared()"); n != 1 { + t.Fatalf("shared emitted %d times, want 1:\n%s", n, res.Code) + } + if n := countString(res.Sources, "fn/shared.py"); n != 1 { + t.Fatalf("shared listed %d times in Sources", n) + } +} + +func TestHoistedImportsAreDeduplicated(t *testing.T) { + res := bundleCase(t, "import-hoist-dedup") + if n := strings.Count(res.Code, "import requests"); n != 1 { + t.Fatalf("`import requests` appears %d times, want 1:\n%s", n, res.Code) + } + if n := strings.Count(res.Code, "from pydantic import BaseModel"); n != 1 { + t.Fatalf("pydantic import appears %d times, want 1:\n%s", n, res.Code) + } +} + +// Only one __future__ import, and it has to be the first statement or Python +// raises SyntaxError. +func TestFutureAnnotationsEmittedOnceAndFirst(t *testing.T) { + res := bundleCase(t, "future-annotations") + if n := strings.Count(res.Code, "from __future__ import annotations"); n != 1 { + t.Fatalf("appears %d times, want 1:\n%s", n, res.Code) + } + for _, line := range strings.Split(res.Code, "\n") { + if strings.TrimSpace(line) == "" || strings.HasPrefix(line, "#") { + continue + } + if line != "from __future__ import annotations" { + t.Fatalf("first real statement is %q, want the __future__ import", line) + } + break + } +} + +func TestParentPackageImportResolves(t *testing.T) { + res := bundleCase(t, "shared-parent") + if want := []string{"_shared/http.py", "fn/main.py"}; !equal(res.Sources, want) { + t.Fatalf("sources = %v, want %v", res.Sources, want) + } + if !strings.Contains(res.Code, "def fetch_json(q):") { + t.Fatalf("shared helper not inlined:\n%s", res.Code) + } +} + +// A single-file function has nothing to flatten; it must survive intact. +func TestSingleFileFunctionIsUnchangedApartFromHoisting(t *testing.T) { + res := bundleCase(t, "single-file") + if len(res.Sources) != 1 { + t.Fatalf("sources = %v", res.Sources) + } + if !strings.Contains(res.Code, `return requests.get("https://x.test").text`) { + t.Fatalf("body altered:\n%s", res.Code) + } +} + +// Import-looking text inside a docstring must not be followed. +func TestDocstringImportsAreNotResolved(t *testing.T) { + res := bundleCase(t, "docstring-not-an-import") + if want := []string{"fn/real.py", "fn/main.py"}; !equal(res.Sources, want) { + t.Fatalf("sources = %v, want %v — a docstring import was followed", res.Sources, want) + } + if !strings.Contains(res.Code, "from .ghost import missing") { + t.Fatalf("docstring content was stripped; it should be copied verbatim:\n%s", res.Code) + } +} + +func countString(xs []string, want string) int { + n := 0 + for _, x := range xs { + if x == want { + n++ + } + } + return n +} diff --git a/internal/bundle/imports.go b/internal/bundle/imports.go new file mode 100644 index 0000000..a78f6ae --- /dev/null +++ b/internal/bundle/imports.go @@ -0,0 +1,317 @@ +package bundle + +import "strings" + +// ImportKind classifies a top-level import statement. The distinction that +// matters is Relative versus everything else: relative imports are resolved +// and inlined, absolute ones are hoisted verbatim and checked against the +// runtime allowlist. +type ImportKind int + +const ( + NotImport ImportKind = iota + ImportAbsolute + ImportFrom + ImportRelative + ImportFuture +) + +// Name is one imported name, with the alias it was bound under if any. +type Name struct { + Name string + Alias string +} + +// Binding is the module-level name this import actually defines. +// +// `import a.b` binds a, not a.b — the submodule is reached through the parent. +// Aliasing changes that to the alias in every form. +func (n Name) Binding() string { + if n.Alias != "" { + return n.Alias + } + if i := strings.IndexByte(n.Name, '.'); i >= 0 { + return n.Name[:i] + } + return n.Name +} + +// Import is a parsed top-level import statement. +type Import struct { + Kind ImportKind + Level int // leading dots; relative imports only + Module string // "" for `import x` and for `from . import x` + Names []Name + Star bool + Stmt Stmt +} + +// Bindings are the module-level names this statement introduces. +func (im Import) Bindings() []string { + out := make([]string, 0, len(im.Names)) + for _, n := range im.Names { + out = append(out, n.Binding()) + } + return out +} + +// ParseImport reads an import statement, or reports false if the statement is +// not one. Input is Stmt.Text, so continuations are already joined. +func ParseImport(s Stmt) (Import, bool) { + text := s.Text + switch { + case strings.HasPrefix(text, "import "): + names, star := parseNameList(text[len("import "):]) + return Import{Kind: ImportAbsolute, Names: names, Star: star, Stmt: s}, true + case strings.HasPrefix(text, "from "): + rest := text[len("from "):] + idx := strings.Index(rest, " import ") + if idx < 0 { + return Import{}, false + } + spec := strings.TrimSpace(rest[:idx]) + names, star := parseNameList(rest[idx+len(" import "):]) + + level := 0 + for level < len(spec) && spec[level] == '.' { + level++ + } + module := strings.TrimSpace(spec[level:]) + + im := Import{Level: level, Module: module, Names: names, Star: star, Stmt: s} + switch { + case module == "__future__" && level == 0: + im.Kind = ImportFuture + case level > 0: + im.Kind = ImportRelative + default: + im.Kind = ImportFrom + } + return im, true + } + return Import{}, false +} + +// parseNameList reads the comma-separated tail of an import statement, +// tolerating the parenthesised multi-line form the scanner has already joined. +func parseNameList(s string) ([]Name, bool) { + s = strings.TrimSpace(s) + s = strings.TrimPrefix(s, "(") + s = strings.TrimSuffix(strings.TrimSpace(s), ")") + + var names []Name + star := false + for _, part := range strings.Split(s, ",") { + fields := strings.Fields(part) + switch { + case len(fields) == 0: + continue + case fields[0] == "*": + star = true + case len(fields) >= 3 && fields[1] == "as": + names = append(names, Name{Name: fields[0], Alias: fields[2]}) + default: + names = append(names, Name{Name: fields[0]}) + } + } + return names, star +} + +// TopLevelBindings are the module-level names a statement defines, for +// collision detection across concatenated modules. +// +// Attribute and subscript targets (a.b = ..., a[0] = ...) mutate an existing +// object rather than binding a module name, so they are deliberately absent. +func TopLevelBindings(s Stmt) []string { + text := s.Text + switch { + case strings.HasPrefix(text, "def "): + return []string{identAfter(text, "def ")} + case strings.HasPrefix(text, "async def "): + return []string{identAfter(text, "async def ")} + case strings.HasPrefix(text, "class "): + return []string{identAfter(text, "class ")} + } + if im, ok := ParseImport(s); ok { + if im.Kind == ImportFuture { + return nil + } + return im.Bindings() + } + if names := clauseTargets(text); len(names) > 0 { + return names + } + if names := walrusTargets(text); len(names) > 0 { + return names + } + return assignTargets(text) +} + +// clauseTargets covers the top-level statements that bind a name without an +// '=': a for loop's variable, a with block's alias, and an except clause's +// exception. They are rare at module level but bind exactly as a def does, so +// omitting them means a real collision goes unreported. +func clauseTargets(text string) []string { + switch { + case strings.HasPrefix(text, "for "): + rest := text[len("for "):] + idx := strings.Index(rest, " in ") + if idx < 0 { + return nil + } + return splitTargets(rest[:idx]) + case strings.HasPrefix(text, "with "), strings.HasPrefix(text, "async with "), + strings.HasPrefix(text, "except "), strings.HasPrefix(text, "except* "): + // Every `as NAME` in the clause; `with` may carry several. + var out []string + for _, part := range strings.Split(text, " as ")[1:] { + name := strings.TrimSpace(part) + end := 0 + for end < len(name) && isIdentByte(name[end]) { + end++ + } + if n := name[:end]; isIdentifier(n) { + out = append(out, n) + } + } + return out + } + return nil +} + +// walrusTargets covers `if (found := f()):` and friends at module level. +func walrusTargets(text string) []string { + var out []string + for i := 0; i+1 < len(text); i++ { + if text[i] != ':' || text[i+1] != '=' { + continue + } + end := i + for end > 0 && text[end-1] == ' ' { + end-- + } + start := end + for start > 0 && isIdentByte(text[start-1]) { + start-- + } + if n := text[start:end]; isIdentifier(n) { + out = append(out, n) + } + } + return out +} + +// splitTargets pulls plain identifiers out of a possibly-tupled target list. +func splitTargets(s string) []string { + var out []string + for _, part := range strings.Split(s, ",") { + name := strings.TrimSpace(strings.Trim(strings.TrimSpace(part), "()[]")) + if isIdentifier(name) { + out = append(out, name) + } + } + return out +} + +// identAfter reads the identifier following a keyword. +func identAfter(text, keyword string) string { + rest := strings.TrimSpace(text[len(keyword):]) + end := 0 + for end < len(rest) && isIdentByte(rest[end]) { + end++ + } + return rest[:end] +} + +// assignTargets extracts names bound by a top-level assignment, covering the +// plain, annotated, tuple and chained forms. +func assignTargets(text string) []string { + eq := topLevelAssign(text) + if eq < 0 { + return nil + } + lhs := text[:eq] + + // Chained assignment: every segment left of the final value is a target. + var out []string + for _, segment := range strings.Split(lhs, "=") { + // Annotated form: the type follows a colon and binds nothing. + if colon := strings.IndexByte(segment, ':'); colon >= 0 { + segment = segment[:colon] + } + for _, target := range strings.Split(segment, ",") { + name := strings.TrimSpace(strings.Trim(strings.TrimSpace(target), "()[]")) + if name == "" || !isIdentifier(name) { + continue + } + out = append(out, name) + } + } + return out +} + +// topLevelAssign returns the index of the assignment '=' that separates +// targets from the value, or -1 if the statement assigns nothing. +// +// It returns the *last* such '=' so that chained assignment (A = B = 1) keeps +// every target on the left; taking the first silently drops all but one. +// Comparisons are stepped over rather than rejected, because `A = B == C` is a +// perfectly good binding, while an augmented operator rebinds an existing name +// and introduces none. +func topLevelAssign(text string) int { + depth := 0 + last := -1 + var quote byte + for i := 0; i < len(text); i++ { + c := text[i] + if quote != 0 { + switch c { + case '\\': + i++ + case quote: + quote = 0 + } + continue + } + switch c { + case '"', '\'': + quote = c + case '(', '[', '{': + depth++ + case ')', ']', '}': + depth-- + case '=': + if depth != 0 { + continue + } + if i+1 < len(text) && text[i+1] == '=' { + i++ // comparison; step over both characters + continue + } + if i > 0 && strings.IndexByte("=!<>+-*/%&|^@", text[i-1]) >= 0 { + return -1 // augmented assignment rebinds, it does not bind + } + last = i + } + } + return last +} + +func isIdentifier(s string) bool { + if s == "" || (s[0] >= '0' && s[0] <= '9') { + return false + } + for i := 0; i < len(s); i++ { + if !isIdentByte(s[i]) { + return false + } + } + return true +} + +func isIdentByte(c byte) bool { + return c == '_' || + (c >= 'a' && c <= 'z') || + (c >= 'A' && c <= 'Z') || + (c >= '0' && c <= '9') +} diff --git a/internal/bundle/imports_test.go b/internal/bundle/imports_test.go new file mode 100644 index 0000000..ee893c1 --- /dev/null +++ b/internal/bundle/imports_test.go @@ -0,0 +1,188 @@ +package bundle + +import "testing" + +// parseOne scans a single statement and parses it as an import. +func parseOne(t *testing.T, src string) Import { + t.Helper() + stmts := Scan(src) + if len(stmts) != 1 { + t.Fatalf("expected 1 statement from %q, got %d", src, len(stmts)) + } + im, ok := ParseImport(stmts[0]) + if !ok { + t.Fatalf("%q did not parse as an import", src) + } + return im +} + +func TestParseImportAbsolute(t *testing.T) { + im := parseOne(t, "import requests\n") + if im.Kind != ImportAbsolute { + t.Fatalf("kind = %v", im.Kind) + } + if len(im.Names) != 1 || im.Names[0].Name != "requests" { + t.Fatalf("names = %+v", im.Names) + } +} + +func TestParseImportDottedBindsFirstSegment(t *testing.T) { + im := parseOne(t, "import os.path\n") + if got := im.Names[0].Binding(); got != "os" { + t.Fatalf("binding = %q, want %q", got, "os") + } +} + +func TestParseImportAliasBindsAlias(t *testing.T) { + im := parseOne(t, "import numpy.linalg as la\n") + if got := im.Names[0].Binding(); got != "la" { + t.Fatalf("binding = %q, want %q", got, "la") + } +} + +func TestParseImportMultipleNames(t *testing.T) { + im := parseOne(t, "import json, re as regex\n") + if len(im.Names) != 2 { + t.Fatalf("names = %+v", im.Names) + } + if im.Names[0].Binding() != "json" || im.Names[1].Binding() != "regex" { + t.Fatalf("bindings = %v", im.Bindings()) + } +} + +func TestParseFromImport(t *testing.T) { + im := parseOne(t, "from pydantic import BaseModel\n") + if im.Kind != ImportFrom { + t.Fatalf("kind = %v", im.Kind) + } + if im.Module != "pydantic" || im.Level != 0 { + t.Fatalf("module = %q level = %d", im.Module, im.Level) + } +} + +func TestParseRelativeImportLevels(t *testing.T) { + one := parseOne(t, "from .parse import f\n") + if one.Kind != ImportRelative || one.Level != 1 || one.Module != "parse" { + t.Fatalf("got kind=%v level=%d module=%q", one.Kind, one.Level, one.Module) + } + + two := parseOne(t, "from .._shared.http import fetch\n") + if two.Level != 2 || two.Module != "_shared.http" { + t.Fatalf("level = %d module = %q", two.Level, two.Module) + } +} + +// `from . import mod` has no module part; the bundler rejects it, but the +// parser still has to represent it so the rejection can name it. +func TestParseFromDotImportHasEmptyModule(t *testing.T) { + im := parseOne(t, "from . import parse\n") + if im.Kind != ImportRelative || im.Level != 1 || im.Module != "" { + t.Fatalf("got kind=%v level=%d module=%q", im.Kind, im.Level, im.Module) + } +} + +func TestParseStarImport(t *testing.T) { + im := parseOne(t, "from .parse import *\n") + if !im.Star { + t.Fatal("star not detected") + } +} + +func TestParseFutureImport(t *testing.T) { + im := parseOne(t, "from __future__ import annotations\n") + if im.Kind != ImportFuture { + t.Fatalf("kind = %v", im.Kind) + } +} + +func TestParseParenthesisedRelativeImport(t *testing.T) { + im := parseOne(t, "from .parse import (\n parse_rows,\n clean as scrub,\n)\n") + if len(im.Names) != 2 { + t.Fatalf("names = %+v", im.Names) + } + if im.Names[0].Binding() != "parse_rows" || im.Names[1].Binding() != "scrub" { + t.Fatalf("bindings = %v", im.Bindings()) + } + if im.Names[1].Name != "clean" { + t.Fatalf("aliased name = %q, want clean", im.Names[1].Name) + } +} + +func TestParseImportRejectsNonImports(t *testing.T) { + for _, src := range []string{"x = 1\n", "def f():\n pass\n", "important = 1\n"} { + stmts := Scan(src) + if _, ok := ParseImport(stmts[0]); ok { + t.Fatalf("%q parsed as an import", src) + } + } +} + +func bindingsOf(t *testing.T, src string) []string { + t.Helper() + stmts := Scan(src) + if len(stmts) == 0 { + t.Fatalf("no statements in %q", src) + } + return TopLevelBindings(stmts[0]) +} + +func TestTopLevelBindingsDefClass(t *testing.T) { + if got := bindingsOf(t, "def clean(x):\n return x\n"); !equal(got, []string{"clean"}) { + t.Fatalf("got %q", got) + } + if got := bindingsOf(t, "async def fetch(x):\n return x\n"); !equal(got, []string{"fetch"}) { + t.Fatalf("got %q", got) + } + if got := bindingsOf(t, "class Response(BaseModel):\n pass\n"); !equal(got, []string{"Response"}) { + t.Fatalf("got %q", got) + } + if got := bindingsOf(t, "class Bare:\n pass\n"); !equal(got, []string{"Bare"}) { + t.Fatalf("got %q", got) + } +} + +func TestTopLevelBindingsAssignments(t *testing.T) { + cases := []struct { + src string + want []string + }{ + {"TARGET = 1\n", []string{"TARGET"}}, + {"TARGET: str = \"x\"\n", []string{"TARGET"}}, + {"A, B = 1, 2\n", []string{"A", "B"}}, + {"A = B = 1\n", []string{"A", "B"}}, + } + for _, tc := range cases { + if got := bindingsOf(t, tc.src); !equal(got, tc.want) { + t.Fatalf("%q: got %q, want %q", tc.src, got, tc.want) + } + } +} + +// These bind nothing at module level. Counting them would produce phantom +// collisions between modules that merely mutate the same kind of object. +func TestTopLevelBindingsIgnoresNonBindingForms(t *testing.T) { + for _, src := range []string{ + "obj.attr = 1\n", + "items[0] = 1\n", + "COUNT += 1\n", + "if a == b:\n pass\n", + "print(\"x = 1\")\n", + } { + if got := bindingsOf(t, src); len(got) != 0 { + t.Fatalf("%q bound %q, want nothing", src, got) + } + } +} + +// A default argument containing '=' must not be read as an assignment target. +func TestTopLevelBindingsIgnoresEqualsInsideBrackets(t *testing.T) { + if got := bindingsOf(t, "CONFIG = dict(a=1, b=2)\n"); !equal(got, []string{"CONFIG"}) { + t.Fatalf("got %q", got) + } +} + +func TestTopLevelBindingsFutureImportBindsNothing(t *testing.T) { + if got := bindingsOf(t, "from __future__ import annotations\n"); len(got) != 0 { + t.Fatalf("got %q", got) + } +} diff --git a/internal/bundle/python_test.go b/internal/bundle/python_test.go new file mode 100644 index 0000000..52aecb3 --- /dev/null +++ b/internal/bundle/python_test.go @@ -0,0 +1,118 @@ +package bundle + +import ( + "os" + "os/exec" + "path/filepath" + "strconv" + "strings" + "testing" +) + +// python locates an interpreter, or skips. These tests assert that the artifact +// is real Python rather than merely plausible-looking text, which no amount of +// string matching in Go can establish. +func python(t *testing.T) string { + t.Helper() + p, err := exec.LookPath("python3") + if err != nil { + t.Skip("python3 not available") + } + return p +} + +// writeTemp puts code in a file named so tracebacks are legible. +func writeTemp(t *testing.T, code string) string { + t.Helper() + path := filepath.Join(t.TempDir(), "artifact.py") + if err := os.WriteFile(path, []byte(code), 0o644); err != nil { + t.Fatal(err) + } + return path +} + +// Every golden artifact must compile. A bundler that emits a syntax error is +// worse than one that refuses, because the failure surfaces after upload. +func TestGoldenArtifactsCompile(t *testing.T) { + py := python(t) + for _, name := range goldenCases { + t.Run(name, func(t *testing.T) { + code, err := os.ReadFile(filepath.Join("testdata", name, "want.py")) + if err != nil { + t.Skipf("no golden file yet: %v", err) + } + path := writeTemp(t, string(code)) + out, err := exec.Command(py, "-m", "py_compile", path).CombinedOutput() + if err != nil { + t.Fatalf("artifact does not compile: %v\n%s\n--- code ---\n%s", err, out, code) + } + }) + } +} + +// runArtifact executes the artifact and evaluates expr against its namespace. +func runArtifact(t *testing.T, code, expr string) string { + t.Helper() + py := python(t) + path := writeTemp(t, code) + script := "import runpy; ns = runpy.run_path(" + strconv.Quote(path) + "); print(" + expr + ")" + out, err := exec.Command(py, "-c", script).CombinedOutput() + if err != nil { + t.Fatalf("executing artifact failed: %v\n%s\n--- code ---\n%s", err, out, code) + } + return strings.TrimSpace(string(out)) +} + +// The alias rule, proven rather than asserted: deleting the import without +// recreating the binding raises NameError here, which is exactly the failure +// that would otherwise reach production. +func TestAliasedImportArtifactActuallyRuns(t *testing.T) { + res := bundleCase(t, "alias-preserved") + if got := runArtifact(t, res.Code, `ns["run"](" hi ")`); got != "['hi']" { + t.Fatalf("run() returned %q, want %q", got, "['hi']") + } +} + +func TestFlattenedDependencyChainRuns(t *testing.T) { + res := bundleCase(t, "topo-order") + if got := runArtifact(t, res.Code, `ns["run"]()`); got != "2" { + t.Fatalf("run() returned %q, want 2", got) + } +} + +func TestDiamondArtifactRuns(t *testing.T) { + res := bundleCase(t, "diamond") + if got := runArtifact(t, res.Code, `ns["run"]()`); got != "1" { + t.Fatalf("run() returned %q, want 1", got) + } +} + +// __future__ must be the first statement or Python refuses the file outright, +// so this compiles only if the emitter got the ordering right. +func TestFutureAnnotationsArtifactCompiles(t *testing.T) { + res := bundleCase(t, "future-annotations") + path := writeTemp(t, res.Code) + out, err := exec.Command(python(t), "-m", "py_compile", path).CombinedOutput() + if err != nil { + t.Fatalf("misplaced __future__ import: %v\n%s\n%s", err, out, res.Code) + } +} + +// Property test: every module in the package must survive into the artifact +// with its top-level definitions intact. +func TestAllGoldenArtifactsDefineRun(t *testing.T) { + py := python(t) + for _, name := range goldenCases { + t.Run(name, func(t *testing.T) { + res := bundleCase(t, name) + if strings.Contains(res.Code, "import requests") || strings.Contains(res.Code, "pydantic") { + t.Skip("artifact needs third-party packages that are not installed for tests") + } + path := writeTemp(t, res.Code) + script := "import runpy; ns = runpy.run_path(" + strconv.Quote(path) + "); assert callable(ns.get('run')), 'run() missing'" + if out, err := exec.Command(py, "-c", script).CombinedOutput(); err != nil { + t.Fatalf("%v\n%s", err, out) + } + }) + } +} diff --git a/internal/bundle/scanner.go b/internal/bundle/scanner.go new file mode 100644 index 0000000..0a166e2 --- /dev/null +++ b/internal/bundle/scanner.go @@ -0,0 +1,174 @@ +package bundle + +import "strings" + +// Stmt is one logical Python statement: a physical line plus any lines joined +// to it by an open bracket or a trailing backslash. +// +// Text has comments stripped and continuation newlines collapsed to single +// spaces, which is what the import parser wants. It is deliberately not +// faithful to the source; emission works from StartLine/EndLine against the +// original file so that everything the bundler does not rewrite survives +// byte-for-byte. +type Stmt struct { + Text string + StartLine int // 1-based, inclusive + EndLine int // 1-based, inclusive + Indent int // leading space/tab count of the first physical line +} + +// TopLevel reports whether the statement starts at column zero. Only +// column-zero statements bind module-level names, so everything the bundler +// analyses is filtered through this. +func (s Stmt) TopLevel() bool { return s.Indent == 0 } + +// Scan splits Python source into logical statements. +// +// It is a tokenizer, not a parser: it understands strings, comments, brackets +// and continuations well enough to know where statements begin and end, and +// nothing else. That is sufficient because the bundler rejects every construct +// whose handling would need a real parse — see Bundle. +// +// Iteration is over bytes rather than runes on purpose. Every byte it reacts to +// is ASCII, and a UTF-8 continuation byte is never ASCII, so multi-byte +// characters pass through untouched. +func Scan(src string) []Stmt { + var out []Stmt + var buf strings.Builder + + line := 1 + depth := 0 + inStmt := false + stmtStart := 0 + stmtIndent := 0 + atLineStart := true + indent := 0 + + var quote byte // 0 when not inside a string literal + var triple bool + + flush := func(end int) { + if !inStmt { + return + } + if text := strings.TrimSpace(buf.String()); text != "" { + out = append(out, Stmt{ + Text: text, + StartLine: stmtStart, + EndLine: end, + Indent: stmtIndent, + }) + } + buf.Reset() + inStmt = false + } + + for i := 0; i < len(src); i++ { + c := src[i] + + if quote != 0 { + buf.WriteByte(c) + switch { + case c == '\n': + line++ + case c == '\\' && i+1 < len(src): + // Backslash consumes the next byte in raw literals too — r"\"" + // is a two-character string, not an unterminated one — so the + // prefix never changes where a string ends and is not parsed. + i++ + buf.WriteByte(src[i]) + if src[i] == '\n' { + line++ + } + case c == quote && triple: + if i+2 < len(src) && src[i+1] == quote && src[i+2] == quote { + buf.WriteByte(src[i+1]) + buf.WriteByte(src[i+2]) + i += 2 + quote = 0 + } + case c == quote: + quote = 0 + } + continue + } + + if c == '\n' { + if depth > 0 && inStmt { + buf.WriteByte(' ') + } else { + flush(line) + } + line++ + atLineStart = true + indent = 0 + continue + } + + if atLineStart { + if c == ' ' || c == '\t' { + indent++ + continue + } + atLineStart = false + if !inStmt { + inStmt = true + stmtStart = line + stmtIndent = indent + } + } + + if c == '#' { + for i < len(src) && src[i] != '\n' { + i++ + } + i-- // hand the newline back to the loop + continue + } + + if c == '\\' && i+1 < len(src) && src[i+1] == '\n' { + i++ + line++ + atLineStart = true + indent = 0 + buf.WriteByte(' ') + continue + } + + switch c { + case ';': + // A semicolon separates statements on one physical line. Without + // this the parser reads `import json; import re` as a single import + // of a module literally named "json;", drops the line as an import, + // and never hoists re — a NameError from an artifact that compiled. + if depth == 0 { + end := line + flush(end) + inStmt = true + stmtStart = end + stmtIndent = indent + continue + } + case '(', '[', '{': + depth++ + case ')', ']', '}': + if depth > 0 { + depth-- + } + case '"', '\'': + quote = c + triple = i+2 < len(src) && src[i+1] == c && src[i+2] == c + buf.WriteByte(c) + if triple { + buf.WriteByte(src[i+1]) + buf.WriteByte(src[i+2]) + i += 2 + } + continue + } + + buf.WriteByte(c) + } + flush(line) + return out +} diff --git a/internal/bundle/scanner_test.go b/internal/bundle/scanner_test.go new file mode 100644 index 0000000..4135116 --- /dev/null +++ b/internal/bundle/scanner_test.go @@ -0,0 +1,203 @@ +package bundle + +import ( + "strings" + "testing" +) + +// texts is the statement text of every scanned statement, for terse assertions. +func texts(stmts []Stmt) []string { + out := make([]string, len(stmts)) + for i, s := range stmts { + out[i] = s.Text + } + return out +} + +func equal(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} + +func TestScanSplitsSimpleStatements(t *testing.T) { + got := texts(Scan("import os\nx = 1\n")) + want := []string{"import os", "x = 1"} + if !equal(got, want) { + t.Fatalf("got %q, want %q", got, want) + } +} + +func TestScanIgnoresBlankAndCommentOnlyLines(t *testing.T) { + got := texts(Scan("\n# a comment\n\nx = 1\n # indented comment\n")) + if want := []string{"x = 1"}; !equal(got, want) { + t.Fatalf("got %q, want %q", got, want) + } +} + +func TestScanStripsTrailingComments(t *testing.T) { + got := texts(Scan("from .parse import f # keep f\n")) + if want := []string{"from .parse import f"}; !equal(got, want) { + t.Fatalf("got %q, want %q", got, want) + } +} + +// A '#' inside a string is not a comment. Getting this wrong truncates the +// statement and can make an import look like it imports fewer names. +func TestScanDoesNotTreatHashInStringAsComment(t *testing.T) { + got := texts(Scan(`url = "https://x.test/#frag"` + "\n")) + if want := []string{`url = "https://x.test/#frag"`}; !equal(got, want) { + t.Fatalf("got %q, want %q", got, want) + } +} + +// The whole reason the scanner exists rather than a line split: an import can +// be parenthesised across many lines. +func TestScanJoinsParenthesisedImport(t *testing.T) { + src := "from .parse import (\n a,\n b as c,\n)\nx = 1\n" + stmts := Scan(src) + if len(stmts) != 2 { + t.Fatalf("expected 2 statements, got %d: %q", len(stmts), texts(stmts)) + } + if !strings.Contains(stmts[0].Text, "a,") || !strings.Contains(stmts[0].Text, "b as c") { + t.Fatalf("continuation not joined: %q", stmts[0].Text) + } + if stmts[0].StartLine != 1 || stmts[0].EndLine != 4 { + t.Fatalf("span = %d..%d, want 1..4", stmts[0].StartLine, stmts[0].EndLine) + } + if stmts[1].StartLine != 5 { + t.Fatalf("second statement starts at %d, want 5", stmts[1].StartLine) + } +} + +func TestScanJoinsBackslashContinuation(t *testing.T) { + stmts := Scan("x = 1 + \\\n 2\ny = 3\n") + if len(stmts) != 2 { + t.Fatalf("expected 2 statements, got %d: %q", len(stmts), texts(stmts)) + } + if stmts[0].EndLine != 2 { + t.Fatalf("EndLine = %d, want 2", stmts[0].EndLine) + } + if stmts[1].Text != "y = 3" { + t.Fatalf("got %q", stmts[1].Text) + } +} + +// A triple-quoted docstring containing import-like text must not produce +// statements. This is the failure that would make the bundler chase imports +// that do not exist. +func TestScanSkipsTripleQuotedContent(t *testing.T) { + src := "\"\"\"Module doc.\n\nfrom .nope import ghost\nimport nothing\n\"\"\"\nimport requests\n" + got := texts(Scan(src)) + if len(got) != 2 { + t.Fatalf("expected 2 statements, got %d: %q", len(got), got) + } + if got[1] != "import requests" { + t.Fatalf("second statement = %q, want %q", got[1], "import requests") + } + if strings.Contains(got[0], "ghost") && !strings.HasPrefix(got[0], `"""`) { + t.Fatalf("docstring body leaked out of its literal: %q", got[0]) + } +} + +func TestScanHandlesSingleQuotedTripleStrings(t *testing.T) { + src := "x = '''\nimport ghost\n'''\ny = 1\n" + got := texts(Scan(src)) + if len(got) != 2 || got[1] != "y = 1" { + t.Fatalf("got %q", got) + } +} + +// An escaped quote must not close the string. If it does, everything after is +// mis-tokenized. +func TestScanHandlesEscapedQuote(t *testing.T) { + src := `x = "she said \"hi\" # not a comment"` + "\ny = 1\n" + got := texts(Scan(src)) + if len(got) != 2 { + t.Fatalf("expected 2 statements, got %d: %q", len(got), got) + } + if got[1] != "y = 1" { + t.Fatalf("got %q", got) + } +} + +// r"\" is a raw string whose backslash still pairs with the closing quote in +// the tokenizer. Treating raw strings as backslash-free ends the literal early. +func TestScanHandlesRawStringWithEscapedQuote(t *testing.T) { + src := `p = r"\""` + "\n" + "y = 1\n" + got := texts(Scan(src)) + if len(got) != 2 || got[1] != "y = 1" { + t.Fatalf("got %q", got) + } +} + +func TestScanRecordsIndent(t *testing.T) { + src := "def f():\n import inner\n return 1\nx = 2\n" + stmts := Scan(src) + if len(stmts) != 4 { + t.Fatalf("expected 4 statements, got %d: %q", len(stmts), texts(stmts)) + } + if !stmts[0].TopLevel() { + t.Fatal("def should be top level") + } + if stmts[1].TopLevel() { + t.Fatalf("indented import reported as top level (indent=%d)", stmts[1].Indent) + } + if !stmts[3].TopLevel() { + t.Fatal("x = 2 should be top level") + } +} + +func TestScanHandlesFileWithoutTrailingNewline(t *testing.T) { + got := texts(Scan("x = 1")) + if want := []string{"x = 1"}; !equal(got, want) { + t.Fatalf("got %q, want %q", got, want) + } +} + +func TestScanHandlesEmptyInput(t *testing.T) { + if got := Scan(""); len(got) != 0 { + t.Fatalf("got %d statements, want 0", len(got)) + } +} + +// Line numbers are what the source map and every error message depend on, so +// they are asserted directly rather than via a golden file. +func TestScanLineNumbersSurviveStringsAndComments(t *testing.T) { + src := "# c\n\"\"\"\ndoc\n\"\"\"\n\nimport requests\n" + stmts := Scan(src) + last := stmts[len(stmts)-1] + if last.Text != "import requests" { + t.Fatalf("last statement = %q", last.Text) + } + if last.StartLine != 6 { + t.Fatalf("StartLine = %d, want 6", last.StartLine) + } +} + +func TestScanNestedBracketsStayOpen(t *testing.T) { + src := "x = [\n (1,\n 2),\n]\ny = 1\n" + stmts := Scan(src) + if len(stmts) != 2 { + t.Fatalf("expected 2 statements, got %d: %q", len(stmts), texts(stmts)) + } + if stmts[0].EndLine != 4 { + t.Fatalf("EndLine = %d, want 4", stmts[0].EndLine) + } +} + +// f-strings may contain braces and quotes; they must not desynchronise the +// bracket depth or the string state. +func TestScanHandlesFString(t *testing.T) { + src := "msg = f\"value={x['k']} #\"\ny = 1\n" + got := texts(Scan(src)) + if len(got) != 2 || got[1] != "y = 1" { + t.Fatalf("got %q", got) + } +} diff --git a/internal/bundle/sourcemap_test.go b/internal/bundle/sourcemap_test.go new file mode 100644 index 0000000..a667251 --- /dev/null +++ b/internal/bundle/sourcemap_test.go @@ -0,0 +1,113 @@ +package bundle + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// Every line the bundler copied from a source file must map back to the exact +// text it came from. This is the invariant the whole source map exists for; if +// it drifts by even one line, a traceback points at the wrong statement, which +// is worse than pointing at nothing. +func TestSourceMapResolvesEveryCopiedLine(t *testing.T) { + for _, name := range goldenCases { + t.Run(name, func(t *testing.T) { + res := bundleCase(t, name) + root := filepath.Join("testdata", name, "in") + + artifactLines := strings.Split(res.Code, "\n") + checked := 0 + for i, text := range artifactLines { + path, srcLine, ok := res.Map.Lookup(i + 1) + if !ok { + continue + } + raw, err := os.ReadFile(filepath.Join(root, path)) + if err != nil { + t.Fatalf("mapped to unreadable file %q: %v", path, err) + } + srcLines := strings.Split(string(raw), "\n") + if srcLine < 1 || srcLine > len(srcLines) { + t.Fatalf("artifact line %d maps to %s:%d, out of range", i+1, path, srcLine) + } + if got := srcLines[srcLine-1]; got != text { + t.Fatalf("artifact line %d = %q but %s:%d = %q", i+1, text, path, srcLine, got) + } + checked++ + } + if checked == 0 { + t.Fatal("no artifact line mapped to a source; the map is empty") + } + }) + } +} + +// Generated lines — the header, hoisted imports, alias assignments — have no +// source. Reporting a location for them would be a fabricated traceback. +func TestSourceMapReturnsNotOkForGeneratedLines(t *testing.T) { + res := bundleCase(t, "alias-preserved") + lines := strings.Split(res.Code, "\n") + + var aliasLine int + for i, l := range lines { + if strings.TrimSpace(l) == "pr = parse_rows" { + aliasLine = i + 1 + break + } + } + if aliasLine == 0 { + t.Fatalf("alias assignment not found:\n%s", res.Code) + } + if path, line, ok := res.Map.Lookup(aliasLine); ok { + t.Fatalf("generated alias line mapped to %s:%d, want no mapping", path, line) + } +} + +func TestSourceMapHeaderCommentIsNotMapped(t *testing.T) { + res := bundleCase(t, "topo-order") + for i, l := range strings.Split(res.Code, "\n") { + if strings.HasPrefix(l, "# ── ") { + if _, _, ok := res.Map.Lookup(i + 1); ok { + t.Fatalf("module header at line %d claims a source location", i+1) + } + } + } +} + +func TestSourceMapLookupOutOfRange(t *testing.T) { + res := bundleCase(t, "single-file") + if _, _, ok := res.Map.Lookup(0); ok { + t.Fatal("line 0 should not resolve") + } + if _, _, ok := res.Map.Lookup(1 << 20); ok { + t.Fatal("a line past the artifact should not resolve") + } +} + +// Contiguous runs are coalesced, so the map stays small on a large bundle. +func TestSourceMapCoalescesContiguousRuns(t *testing.T) { + res := bundleCase(t, "topo-order") + if len(res.Map.Entries) > len(res.Sources)*2 { + t.Fatalf("map has %d entries for %d sources; runs are not being merged", + len(res.Map.Entries), len(res.Sources)) + } +} + +// A run's Count must not overstate its extent, or lines belonging to the next +// module resolve to the previous one. +func TestSourceMapRunsDoNotOverlap(t *testing.T) { + for _, name := range goldenCases { + t.Run(name, func(t *testing.T) { + res := bundleCase(t, name) + prevEnd := 0 + for _, e := range res.Map.Entries { + if e.ArtifactLine <= prevEnd { + t.Fatalf("entry at artifact line %d overlaps a run ending at %d", e.ArtifactLine, prevEnd) + } + prevEnd = e.ArtifactLine + e.Count - 1 + } + }) + } +} diff --git a/internal/bundle/testdata/alias-preserved/in/fn/main.py b/internal/bundle/testdata/alias-preserved/in/fn/main.py new file mode 100644 index 0000000..4977784 --- /dev/null +++ b/internal/bundle/testdata/alias-preserved/in/fn/main.py @@ -0,0 +1,5 @@ +from .parse import parse_rows as pr, clean + + +def run(q: str = "x"): + return pr(clean(q)) diff --git a/internal/bundle/testdata/alias-preserved/in/fn/parse.py b/internal/bundle/testdata/alias-preserved/in/fn/parse.py new file mode 100644 index 0000000..05415ea --- /dev/null +++ b/internal/bundle/testdata/alias-preserved/in/fn/parse.py @@ -0,0 +1,6 @@ +def clean(s): + return s.strip() + + +def parse_rows(s): + return [s] diff --git a/internal/bundle/testdata/alias-preserved/want.py b/internal/bundle/testdata/alias-preserved/want.py new file mode 100644 index 0000000..da00651 --- /dev/null +++ b/internal/bundle/testdata/alias-preserved/want.py @@ -0,0 +1,14 @@ +# ── fn/parse.py ── +def clean(s): + return s.strip() + + +def parse_rows(s): + return [s] + +# ── fn/main.py ── +pr = parse_rows + + +def run(q: str = "x"): + return pr(clean(q)) diff --git a/internal/bundle/testdata/diamond/in/fn/left.py b/internal/bundle/testdata/diamond/in/fn/left.py new file mode 100644 index 0000000..6fd2567 --- /dev/null +++ b/internal/bundle/testdata/diamond/in/fn/left.py @@ -0,0 +1,5 @@ +from .shared import shared + + +def left(): + return shared() diff --git a/internal/bundle/testdata/diamond/in/fn/main.py b/internal/bundle/testdata/diamond/in/fn/main.py new file mode 100644 index 0000000..ad848b5 --- /dev/null +++ b/internal/bundle/testdata/diamond/in/fn/main.py @@ -0,0 +1,6 @@ +from .left import left +from .right import right + + +def run(): + return left() + right() diff --git a/internal/bundle/testdata/diamond/in/fn/right.py b/internal/bundle/testdata/diamond/in/fn/right.py new file mode 100644 index 0000000..63a796f --- /dev/null +++ b/internal/bundle/testdata/diamond/in/fn/right.py @@ -0,0 +1,5 @@ +from .shared import shared + + +def right(): + return shared() + 1 diff --git a/internal/bundle/testdata/diamond/in/fn/shared.py b/internal/bundle/testdata/diamond/in/fn/shared.py new file mode 100644 index 0000000..8dff7cb --- /dev/null +++ b/internal/bundle/testdata/diamond/in/fn/shared.py @@ -0,0 +1,2 @@ +def shared(): + return 0 diff --git a/internal/bundle/testdata/diamond/want.py b/internal/bundle/testdata/diamond/want.py new file mode 100644 index 0000000..a19e1fd --- /dev/null +++ b/internal/bundle/testdata/diamond/want.py @@ -0,0 +1,15 @@ +# ── fn/shared.py ── +def shared(): + return 0 + +# ── fn/left.py ── +def left(): + return shared() + +# ── fn/right.py ── +def right(): + return shared() + 1 + +# ── fn/main.py ── +def run(): + return left() + right() diff --git a/internal/bundle/testdata/docstring-not-an-import/in/fn/main.py b/internal/bundle/testdata/docstring-not-an-import/in/fn/main.py new file mode 100644 index 0000000..f020f4e --- /dev/null +++ b/internal/bundle/testdata/docstring-not-an-import/in/fn/main.py @@ -0,0 +1,10 @@ +"""Doc. + +from .ghost import missing +""" + +from .real import real + + +def run(): + return real() diff --git a/internal/bundle/testdata/docstring-not-an-import/in/fn/real.py b/internal/bundle/testdata/docstring-not-an-import/in/fn/real.py new file mode 100644 index 0000000..59c1402 --- /dev/null +++ b/internal/bundle/testdata/docstring-not-an-import/in/fn/real.py @@ -0,0 +1,2 @@ +def real(): + return 1 diff --git a/internal/bundle/testdata/docstring-not-an-import/want.py b/internal/bundle/testdata/docstring-not-an-import/want.py new file mode 100644 index 0000000..d55e1eb --- /dev/null +++ b/internal/bundle/testdata/docstring-not-an-import/want.py @@ -0,0 +1,14 @@ +# ── fn/real.py ── +def real(): + return 1 + +# ── fn/main.py ── +"""Doc. + +from .ghost import missing +""" + + + +def run(): + return real() diff --git a/internal/bundle/testdata/future-annotations/in/fn/helper.py b/internal/bundle/testdata/future-annotations/in/fn/helper.py new file mode 100644 index 0000000..b83d912 --- /dev/null +++ b/internal/bundle/testdata/future-annotations/in/fn/helper.py @@ -0,0 +1,5 @@ +from __future__ import annotations + + +def helper() -> dict: + return {} diff --git a/internal/bundle/testdata/future-annotations/in/fn/main.py b/internal/bundle/testdata/future-annotations/in/fn/main.py new file mode 100644 index 0000000..e50dec5 --- /dev/null +++ b/internal/bundle/testdata/future-annotations/in/fn/main.py @@ -0,0 +1,7 @@ +from __future__ import annotations + +from .helper import helper + + +def run() -> dict: + return helper() diff --git a/internal/bundle/testdata/future-annotations/want.py b/internal/bundle/testdata/future-annotations/want.py new file mode 100644 index 0000000..7afd80e --- /dev/null +++ b/internal/bundle/testdata/future-annotations/want.py @@ -0,0 +1,9 @@ +from __future__ import annotations + +# ── fn/helper.py ── +def helper() -> dict: + return {} + +# ── fn/main.py ── +def run() -> dict: + return helper() diff --git a/internal/bundle/testdata/import-hoist-dedup/in/fn/a.py b/internal/bundle/testdata/import-hoist-dedup/in/fn/a.py new file mode 100644 index 0000000..00f6677 --- /dev/null +++ b/internal/bundle/testdata/import-hoist-dedup/in/fn/a.py @@ -0,0 +1,5 @@ +import requests + + +def a(): + return requests is not None diff --git a/internal/bundle/testdata/import-hoist-dedup/in/fn/b.py b/internal/bundle/testdata/import-hoist-dedup/in/fn/b.py new file mode 100644 index 0000000..2f5b59d --- /dev/null +++ b/internal/bundle/testdata/import-hoist-dedup/in/fn/b.py @@ -0,0 +1,6 @@ +import requests +from pydantic import BaseModel + + +def b(): + return BaseModel is not None and requests is not None diff --git a/internal/bundle/testdata/import-hoist-dedup/in/fn/main.py b/internal/bundle/testdata/import-hoist-dedup/in/fn/main.py new file mode 100644 index 0000000..b6d9deb --- /dev/null +++ b/internal/bundle/testdata/import-hoist-dedup/in/fn/main.py @@ -0,0 +1,13 @@ +import requests +from pydantic import BaseModel + +from .a import a +from .b import b + + +class Response(BaseModel): + ok: bool + + +def run(): + return Response(ok=bool(requests) and a() and b()) diff --git a/internal/bundle/testdata/import-hoist-dedup/want.py b/internal/bundle/testdata/import-hoist-dedup/want.py new file mode 100644 index 0000000..e2caea1 --- /dev/null +++ b/internal/bundle/testdata/import-hoist-dedup/want.py @@ -0,0 +1,18 @@ +import requests +from pydantic import BaseModel + +# ── fn/a.py ── +def a(): + return requests is not None + +# ── fn/b.py ── +def b(): + return BaseModel is not None and requests is not None + +# ── fn/main.py ── +class Response(BaseModel): + ok: bool + + +def run(): + return Response(ok=bool(requests) and a() and b()) diff --git a/internal/bundle/testdata/shared-parent/in/_shared/http.py b/internal/bundle/testdata/shared-parent/in/_shared/http.py new file mode 100644 index 0000000..4bb461a --- /dev/null +++ b/internal/bundle/testdata/shared-parent/in/_shared/http.py @@ -0,0 +1,5 @@ +import requests + + +def fetch_json(q): + return requests.get(q).json() diff --git a/internal/bundle/testdata/shared-parent/in/fn/main.py b/internal/bundle/testdata/shared-parent/in/fn/main.py new file mode 100644 index 0000000..d7d4e25 --- /dev/null +++ b/internal/bundle/testdata/shared-parent/in/fn/main.py @@ -0,0 +1,5 @@ +from .._shared.http import fetch_json + + +def run(q: str = "x"): + return fetch_json(q) diff --git a/internal/bundle/testdata/shared-parent/want.py b/internal/bundle/testdata/shared-parent/want.py new file mode 100644 index 0000000..83e5249 --- /dev/null +++ b/internal/bundle/testdata/shared-parent/want.py @@ -0,0 +1,9 @@ +import requests + +# ── _shared/http.py ── +def fetch_json(q): + return requests.get(q).json() + +# ── fn/main.py ── +def run(q: str = "x"): + return fetch_json(q) diff --git a/internal/bundle/testdata/single-file/in/fn/main.py b/internal/bundle/testdata/single-file/in/fn/main.py new file mode 100644 index 0000000..ac260ff --- /dev/null +++ b/internal/bundle/testdata/single-file/in/fn/main.py @@ -0,0 +1,5 @@ +import requests + + +def run(): + return requests.get("https://x.test").text diff --git a/internal/bundle/testdata/single-file/want.py b/internal/bundle/testdata/single-file/want.py new file mode 100644 index 0000000..2eb3073 --- /dev/null +++ b/internal/bundle/testdata/single-file/want.py @@ -0,0 +1,5 @@ +import requests + +# ── fn/main.py ── +def run(): + return requests.get("https://x.test").text diff --git a/internal/bundle/testdata/topo-order/in/fn/base.py b/internal/bundle/testdata/topo-order/in/fn/base.py new file mode 100644 index 0000000..322f737 --- /dev/null +++ b/internal/bundle/testdata/topo-order/in/fn/base.py @@ -0,0 +1,2 @@ +def base(): + return 1 diff --git a/internal/bundle/testdata/topo-order/in/fn/main.py b/internal/bundle/testdata/topo-order/in/fn/main.py new file mode 100644 index 0000000..1a13d28 --- /dev/null +++ b/internal/bundle/testdata/topo-order/in/fn/main.py @@ -0,0 +1,5 @@ +from .mid import middle + + +def run(): + return middle() diff --git a/internal/bundle/testdata/topo-order/in/fn/mid.py b/internal/bundle/testdata/topo-order/in/fn/mid.py new file mode 100644 index 0000000..1aa2c10 --- /dev/null +++ b/internal/bundle/testdata/topo-order/in/fn/mid.py @@ -0,0 +1,5 @@ +from .base import base + + +def middle(): + return base() + 1 diff --git a/internal/bundle/testdata/topo-order/want.py b/internal/bundle/testdata/topo-order/want.py new file mode 100644 index 0000000..47f90df --- /dev/null +++ b/internal/bundle/testdata/topo-order/want.py @@ -0,0 +1,11 @@ +# ── fn/base.py ── +def base(): + return 1 + +# ── fn/mid.py ── +def middle(): + return base() + 1 + +# ── fn/main.py ── +def run(): + return middle()