From 5ad2de91a26b312bf27124ceca16ef331621bde8 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Sat, 4 Jul 2026 16:07:39 +0000 Subject: [PATCH] feat(sdk): enforce s3:// URIs for string S3 params + ingestion (EL) docs (#9912) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(pipelines): ingestion (EL) templates + docs Co-Authored-By: Claude Fable 5 * fix(pipelines): review nits — draft collision guard, template-mode selection reset, invariant test Co-Authored-By: Claude Fable 5 * fix(pipelines): lead the insert menu with ingestion templates Co-Authored-By: Claude Fable 5 * refactor(pipelines): ingestion story as docs-only — drop editor template UI The insert-menu template section mixed two selection grammars in one popover and confused more than it helped. The three E2E-verified example pipelines now live verbatim in docs/pipeline-ingestion.md; the Python bare-string S3 key fix in pipelineTemplates.ts stays. Co-Authored-By: Claude Fable 5 * feat(sdk): bare string S3 keys in py/ts clients + asset parsers A plain string passed where an S3Object is expected is now a bare key in the default storage — previously the py client silently degraded it to s3="" (auto-generated key) and both asset parsers canonicalized it without the leading slash, splitting lineage. parseS3Object moves to s3Types.ts so it is unit-testable without the generated services. The pipeline template fix from the earlier commit is superseded (bare strings are the supported spelling again); docs examples flipped to bare keys. Co-Authored-By: Claude Fable 5 * refactor(sdk): enforce s3:// URIs for string S3Object params Bare strings now raise/throw with a hint pointing at the s3:/// spelling instead of being treated as keys (previous commit) or silently degrading to an empty key (original behavior). One string spelling everywhere: SDK calls, // on annotations, and DuckDB SQL all use s3:///. TS regains the s3://-template-literal type; the asset parsers record no asset for a bare string (the call can only error); templates emit the URI form. Co-Authored-By: Claude Fable 5 * docs(pipelines): move ingestion (EL) guide to windmilldocs, keep design constraints User-facing how-to (engine choice, cursor recipes, schema drift, worked examples) moves to windmilldocs core_concepts/63_pipelines (windmilldocs#1462); the repo keeps only the design constraints future feature work must not break, as a section of ducklake-materialization.md. Co-Authored-By: Claude Fable 5 * chore: regenerate system prompts after parse_s3_object docstring change Co-Authored-By: Claude Fable 5 * fix(sdk): reject empty-key s3 URIs; align asset parsers with the runtime rule Addresses CI review: s3:/// and s3://bucket/ now raise (an empty key would fall back to the auto-generated-key path the strict contract exists to prevent); the asset parsers' string branch applies the same valid-URI-with-non-empty-key rule so no R/W edge is recorded for a call that can only error (the generic URI-literal scan still records ambiguous access-None assets, by design); comments rephrased as current constraints per AGENTS.md. Co-Authored-By: Claude Fable 5 --------- Co-authored-by: Claude Fable 5 --- .../windmill-parser-py-asset/src/lib.rs | 50 +++++++++++++++- .../windmill-parser-ts-asset/src/lib.rs | 53 +++++++++++++++- cli/src/guidance/skills.gen.ts | 60 ++++++++++++------- docs/ducklake-materialization.md | 40 ++++++++++++- .../assets/AssetGraph/pipelineTemplates.ts | 16 +++-- python-client/tests/wmill_client_test.py | 44 ++++++++++++++ python-client/wmill/wmill/client.py | 23 +++++-- system_prompts/auto-generated/prompts.ts | 24 +++++--- system_prompts/auto-generated/script.md | 24 +++++--- system_prompts/auto-generated/sdks/python.md | 6 +- .../auto-generated/sdks/typescript.md | 18 +++--- .../skills/write-script-bun/SKILL.md | 18 +++--- .../skills/write-script-bunnative/SKILL.md | 18 +++--- .../skills/write-script-deno/SKILL.md | 18 +++--- .../skills/write-script-python3/SKILL.md | 6 +- typescript-client/client.ts | 13 +--- typescript-client/s3Types.d.ts | 1 + typescript-client/s3Types.ts | 26 +++++++- typescript-client/tests/s3Types.test.ts | 52 ++++++++++++++++ 19 files changed, 417 insertions(+), 93 deletions(-) create mode 100644 typescript-client/tests/s3Types.test.ts diff --git a/backend/parsers/windmill-parser-py-asset/src/lib.rs b/backend/parsers/windmill-parser-py-asset/src/lib.rs index 2282025092..4e39a077f8 100644 --- a/backend/parsers/windmill-parser-py-asset/src/lib.rs +++ b/backend/parsers/windmill-parser-py-asset/src/lib.rs @@ -319,12 +319,24 @@ fn dict_str_value(dict: &rustpython_ast::ExprDict, name: &str) -> Option /// `write_s3_file` to a canonical asset path, mirroring `windmill-parser-ts-asset`: /// `S3Object(s3="", storage=""?)` — or the equivalent dict literal — /// maps to the URI `s3:///` (empty bucket for default storage, i.e. -/// `s3:///`), and a bare `"s3://bucket/key"` string is passed through. +/// `s3:///`), and a `"s3://bucket/key"` URI string is passed through. +/// String args mirror the runtime `parse_s3_object` contract exactly: only a +/// `s3:///` URI with a non-empty key is valid — any other +/// string (bare key, `s3://x`, empty key) raises at run time, so recording an +/// edge for it would be a phantom node for a call that can only error. /// The resulting URI is fed through `parse_asset_syntax` so the stored path /// matches the TS object form and the `# on s3:///…` trigger form exactly. fn s3_object_arg_path(expr: &Expr) -> Option { let uri = match expr { - Expr::Constant(ExprConstant { value: Constant::Str(s), .. }) => s.clone(), + Expr::Constant(ExprConstant { value: Constant::Str(s), .. }) => { + match s + .strip_prefix("s3://") + .and_then(|rest| rest.split_once('/')) + { + Some((_, key)) if !key.is_empty() => s.clone(), + _ => return None, + } + } Expr::Call(call) => { // `S3Object(...)` imported directly or as `wmill.S3Object(...)` let func_name = call @@ -378,6 +390,40 @@ def main(): ); } + #[test] + fn test_py_asset_parser_bare_string_no_asset() { + // A plain (non-`s3://`) string is rejected by the runtime + // `parse_s3_object`, so the parser must not record a phantom asset + // for a call that can only error. + let input = r#" +import wmill +def main(): + wmill.write_s3_file("pipelines/etl/out.jsonl", b"") +"#; + let s = parse_assets(input).map(|o| o.assets); + assert_eq!(s.map_err(|e| e.to_string()), Ok(vec![])); + } + + #[test] + fn test_py_asset_parser_invalid_uri_no_write_edge() { + // `s3://x` (no key part) and `s3://bucket/` (empty key) are rejected + // by the runtime `parse_s3_object` — same rule for the SDK-arg path: + // no R/W edge. The generic URI-literal scan may still record them as + // ambiguous (`access_type: None`) assets, like any `s3://…` string + // constant anywhere in a script. + let input = r#" +import wmill +def main(): + wmill.write_s3_file("s3://broken", b"") + wmill.write_s3_file("s3://bucket/", b"") +"#; + let assets = parse_assets(input).expect("parse").assets; + assert!( + assets.iter().all(|a| a.access_type.is_none()), + "invalid URIs must not produce R/W edges: {assets:?}" + ); + } + #[test] fn test_py_asset_parser_write_s3_object_constructor() { // The SDK signature is `write_s3_file(s3object: S3Object | str, ...)` and diff --git a/backend/parsers/windmill-parser-ts-asset/src/lib.rs b/backend/parsers/windmill-parser-ts-asset/src/lib.rs index b7fe0fea5c..e38dc1e7f3 100644 --- a/backend/parsers/windmill-parser-ts-asset/src/lib.rs +++ b/backend/parsers/windmill-parser-ts-asset/src/lib.rs @@ -341,12 +341,25 @@ fn object_str_prop(obj: &ObjectLit, name: &str) -> Option { /// `writeS3File` to a canonical asset path, mirroring the runtime /// `parseS3Object`: an object `{ s3: "", storage?: "" }` maps to /// the URI `s3:///` (empty bucket for default storage, i.e. -/// `s3:///`), and a bare `"s3://bucket/key"` string is passed through. +/// `s3:///`), and a `"s3://bucket/key"` URI string is passed through. +/// String args mirror the runtime `parseS3Object` contract exactly: only a +/// `s3:///` URI with a non-empty key is valid — any other +/// string (bare key, `s3://x`, empty key) throws at run time, so recording an +/// edge for it would be a phantom node for a call that can only error. /// The resulting URI is fed through `parse_asset_syntax` so the stored path /// matches the `// on s3:///…` trigger form exactly. fn s3_object_arg_path(arg: &Expr) -> Option { let uri = match arg { - Expr::Lit(Lit::Str(s)) => s.value.to_string(), + Expr::Lit(Lit::Str(s)) => { + let v = s.value.to_string(); + match v + .strip_prefix("s3://") + .and_then(|rest| rest.split_once('/')) + { + Some((_, key)) if !key.is_empty() => v, + _ => return None, + } + } Expr::Object(obj) => { let key = object_str_prop(obj, "s3")?; let storage = object_str_prop(obj, "storage").unwrap_or_default(); @@ -477,6 +490,42 @@ mod tests { ); } + #[test] + fn test_ts_asset_parser_bare_string_no_asset() { + // A plain (non-`s3://`) string is rejected by the runtime + // `parseS3Object`, so the parser must not record a phantom asset for + // a call that can only error. + let input = r#" + import * as wmill from "windmill-client" + export async function main() { + await wmill.writeS3File("pipelines/etl/out.jsonl", "[]") + } + "#; + let s = parse_assets(input); + assert_eq!(s.map(|r| r.assets).map_err(|e| e.to_string()), Ok(vec![])); + } + + #[test] + fn test_ts_asset_parser_invalid_uri_no_write_edge() { + // `s3://x` (no key part) and `s3://bucket/` (empty key) are rejected + // by the runtime `parseS3Object` — same rule for the SDK-arg path: + // no R/W edge. The generic URI-literal scan may still record them as + // ambiguous (`access_type: None`) assets, like any `s3://…` string + // constant anywhere in a script. + let input = r#" + import * as wmill from "windmill-client" + export async function main() { + await wmill.writeS3File("s3://broken", "[]") + await wmill.writeS3File("s3://bucket/", "[]") + } + "#; + let assets = parse_assets(input).expect("parse").assets; + assert!( + assets.iter().all(|a| a.access_type.is_none()), + "invalid URIs must not produce R/W edges: {assets:?}" + ); + } + #[test] fn test_ts_asset_parser_multiple_s3_object_writes() { // Mirrors the f/km/r_seed shape: several direct object-form writes in diff --git a/cli/src/guidance/skills.gen.ts b/cli/src/guidance/skills.gen.ts index d0fa0e8aaf..b6d9394179 100644 --- a/cli/src/guidance/skills.gen.ts +++ b/cli/src/guidance/skills.gen.ts @@ -1000,13 +1000,6 @@ async requestInteractiveSlackApproval({ slackResourcePath, channelId, message, a */ async requestInteractiveTeamsApproval({ teamName, channelName, message, approver, defaultArgsJson, dynamicEnumsJson, }: TeamsApprovalOptions): Promise -/** - * Parse an S3 object from URI string or record format - * @param s3Object - S3 object as URI string (s3://storage/key) or record - * @returns S3 object record with storage and s3 key - */ -parseS3Object(s3Object: S3Object): S3ObjectRecord - setWorkflowCtx(ctx: WorkflowCtx | null): void async sleep(seconds: number): Promise @@ -1076,6 +1069,17 @@ async parallel(items: T[], fn: (item: T) => PromiseLike | R, options?: */ async commitKafkaOffsets(triggerPath: string, topic: string, partition: number, offset: number,): Promise +/** + * Parse an S3 object from URI string or record format + * @param s3Object - S3 object as URI string (\`s3://storage/key\`, \`s3:///key\` + * for the default storage) or record. Any other string throws rather than + * falling back to an auto-generated key: an auto key is requested by + * omitting the object, and a fallback would silently misplace the upload + * on any typo. + * @returns S3 object record with storage and s3 key + */ +parseS3Object(s3Object: S3Object): S3ObjectRecord + /** * Create a SQL template function for PostgreSQL/datatable queries * @param name - Database/datatable name (default: "main") @@ -1760,13 +1764,6 @@ async requestInteractiveSlackApproval({ slackResourcePath, channelId, message, a */ async requestInteractiveTeamsApproval({ teamName, channelName, message, approver, defaultArgsJson, dynamicEnumsJson, }: TeamsApprovalOptions): Promise -/** - * Parse an S3 object from URI string or record format - * @param s3Object - S3 object as URI string (s3://storage/key) or record - * @returns S3 object record with storage and s3 key - */ -parseS3Object(s3Object: S3Object): S3ObjectRecord - setWorkflowCtx(ctx: WorkflowCtx | null): void async sleep(seconds: number): Promise @@ -1836,6 +1833,17 @@ async parallel(items: T[], fn: (item: T) => PromiseLike | R, options?: */ async commitKafkaOffsets(triggerPath: string, topic: string, partition: number, offset: number,): Promise +/** + * Parse an S3 object from URI string or record format + * @param s3Object - S3 object as URI string (\`s3://storage/key\`, \`s3:///key\` + * for the default storage) or record. Any other string throws rather than + * falling back to an auto-generated key: an auto key is requested by + * omitting the object, and a fallback would silently misplace the upload + * on any typo. + * @returns S3 object record with storage and s3 key + */ +parseS3Object(s3Object: S3Object): S3ObjectRecord + /** * Create a SQL template function for PostgreSQL/datatable queries * @param name - Database/datatable name (default: "main") @@ -2612,13 +2620,6 @@ async requestInteractiveSlackApproval({ slackResourcePath, channelId, message, a */ async requestInteractiveTeamsApproval({ teamName, channelName, message, approver, defaultArgsJson, dynamicEnumsJson, }: TeamsApprovalOptions): Promise -/** - * Parse an S3 object from URI string or record format - * @param s3Object - S3 object as URI string (s3://storage/key) or record - * @returns S3 object record with storage and s3 key - */ -parseS3Object(s3Object: S3Object): S3ObjectRecord - setWorkflowCtx(ctx: WorkflowCtx | null): void async sleep(seconds: number): Promise @@ -2688,6 +2689,17 @@ async parallel(items: T[], fn: (item: T) => PromiseLike | R, options?: */ async commitKafkaOffsets(triggerPath: string, topic: string, partition: number, offset: number,): Promise +/** + * Parse an S3 object from URI string or record format + * @param s3Object - S3 object as URI string (\`s3://storage/key\`, \`s3:///key\` + * for the default storage) or record. Any other string throws rather than + * falling back to an auto-generated key: an auto key is requested by + * omitting the object, and a fallback would silently misplace the upload + * on any typo. + * @returns S3 object record with storage and s3 key + */ +parseS3Object(s3Object: S3Object): S3ObjectRecord + /** * Create a SQL template function for PostgreSQL/datatable queries * @param name - Database/datatable name (default: "main") @@ -4367,7 +4379,11 @@ def get_state_path() -> str # Parse resource syntax from string. def parse_resource_syntax(s: str) -> Optional[str] -# Parse S3 object from string or S3Object format. +# Parse S3 object from a \`s3:///\` URI string (\`s3:///\` +# for the default storage) or S3Object format. Any other string raises +# rather than falling back to an auto-generated key: an auto key is +# requested by omitting the object, and a fallback would silently misplace +# the upload on any typo. def parse_s3_object(s3_object: S3Object | str) -> S3Object # Parse variable syntax from string. diff --git a/docs/ducklake-materialization.md b/docs/ducklake-materialization.md index f0a8c38f45..8329958baa 100644 --- a/docs/ducklake-materialization.md +++ b/docs/ducklake-materialization.md @@ -2,7 +2,10 @@ Design sketch for "managed, versioned, incremental" assets built on the DuckLake substrate. This is a companion to [`pipelines-vs-dbt.md`](./pipelines-vs-dbt.md) -and extends its **Path C (hybrid, partition-first)** recommendation. The new +and extends its **Path C (hybrid, partition-first)** recommendation. For the +extract-load side that feeds these materializations, see §"Ingestion (EL)" +at the end of this doc (user-facing guide: windmilldocs +`core_concepts/63_pipelines` → "Ingestion (EL)"). The new contribution here is leveraging DuckLake's snapshot/time-travel layer, which the earlier doc's incremental deep-dive did not use. The annotation grammar is reconciled with that doc — `// partitioned` + `// unique_key` + `// append` @@ -465,3 +468,38 @@ forces"; DuckLake-specific: rejects anything else at deploy with a clear error pointing to `// materialize manual`. The classifier (`sql_materialize.rs`) is the single source of truth. + +## Ingestion (EL): the sanctioned entry-node shape + +Design constraints for how external data enters the lake — the user-facing +how-to (extract-engine choice, cursor recipes, schema-drift handling, worked +examples) lives in windmilldocs `core_concepts/63_pipelines` → "Ingestion +(EL)"; this section records only what future feature work must not break. + +- **`// materialize` is DuckDB-only** (deploy-rejected elsewhere, managed and + `manual` alike — `windmill-api-scripts/src/scripts.rs`), and the SDK + materialize helpers (`upsert_partition` / `upsertPartition`) build their SQL + inside the SDK, so the asset parsers cannot see the write. A polyglot node + that "writes the lake directly" therefore deploys with **no output edge** — + breaking lineage, cascade scheduling, and the backfill UI's producer lookup. +- **The sanctioned polyglot shape is two scripts**: an entry node that lands + the raw batch as an object in workspace storage (`write_s3_file` — a + parser-visible write), and a DuckDB loader (`-- on s3:///` + + `-- materialize`) that the asset dispatcher re-runs per batch. The landing + object is the seam; splitting E from L also keeps row work vectorized and + gives the load the full engine treatment (strategies, snapshots, partition + grid, `// data_test`). +- **One string spelling: `s3:///`.** SDK string params are strictly + `s3://…` URIs with a non-empty key; anything else raises (clients > + 1.746.0 — older clients silently upload such strings to an auto-generated + `windmill_uploads/…` name, so keep URIs in code that must run on them). + The same URI is what + `// on` annotations and DuckDB SQL take, and all forms (URI, `{s3}` object, + `S3Object(s3=…)`) canonicalize to path `/`. The asset parsers record + **no asset** for a bare-string SDK argument (the call can only error at + run time) — keep `parse_s3_object` (py client), `parseS3Object` + (ts client, `s3Types.ts`) and both `s3_object_arg_path` parsers in lockstep + when touching any of them. +- DuckDB workers load no ICU extension: bare `TIMESTAMPTZ - INTERVAL` + arithmetic binder-errors. Incremental-pull SQL casts both sides + (`updated_at::TIMESTAMP > now()::TIMESTAMP - INTERVAL 7 DAY`). diff --git a/frontend/src/lib/components/assets/AssetGraph/pipelineTemplates.ts b/frontend/src/lib/components/assets/AssetGraph/pipelineTemplates.ts index 63a90ef099..11077d3609 100644 --- a/frontend/src/lib/components/assets/AssetGraph/pipelineTemplates.ts +++ b/frontend/src/lib/components/assets/AssetGraph/pipelineTemplates.ts @@ -391,8 +391,11 @@ function bodyTs(ctx: TemplateContext): string { if (!input) return '' switch (input.kind) { case 's3object': + // `s3:///` URI — one spelling shared with the `// on + // s3:///…` annotation form (the object literal `{ s3: }` + // is equivalent). return [ - ` const buf = await wmill.loadS3File({ s3: ${JSON.stringify(s3Key(input.path))} })`, + ` const buf = await wmill.loadS3File(${JSON.stringify(`s3:///${s3Key(input.path)}`)})`, ` const rows = JSON.parse(new TextDecoder().decode(buf))`, `` ].join('\n') @@ -418,9 +421,10 @@ function bodyTs(ctx: TemplateContext): string { switch (outputKind) { case 's3_parquet': case 's3_object': + // `s3:///` URI — see the loadS3File note above. return [ ` const payload = new TextEncoder().encode(JSON.stringify(rows))`, - ` await wmill.writeS3File({ s3: ${JSON.stringify(s3Key(output.path))} }, payload)` + ` await wmill.writeS3File(${JSON.stringify(`s3:///${s3Key(output.path)}`)}, payload)` ].join('\n') case 'datatable': { const dbName = output.path.split('/')[0] ?? 'main' @@ -474,8 +478,11 @@ function bodyPython(ctx: TemplateContext): string { if (!input) return '' switch (input.kind) { case 's3object': + // `s3:///` URI — SDK string params must be s3:// URIs + // (bare keys are rejected), and this form matches the + // `# on s3:///…` annotation spelling. return [ - ` buf = wmill.load_s3_file(${JSON.stringify(s3Key(input.path))})`, + ` buf = wmill.load_s3_file(${JSON.stringify(`s3:///${s3Key(input.path)}`)})`, ` import json; rows = json.loads(buf.decode("utf-8"))` ].join('\n') case 'datatable': @@ -498,9 +505,10 @@ function bodyPython(ctx: TemplateContext): string { switch (outputKind) { case 's3_parquet': case 's3_object': + // `s3:///` URI — see the load_s3_file note above. return [ ` import json`, - ` wmill.write_s3_file(${JSON.stringify(s3Key(output.path))}, json.dumps(rows).encode("utf-8"))` + ` wmill.write_s3_file(${JSON.stringify(`s3:///${s3Key(output.path)}`)}, json.dumps(rows).encode("utf-8"))` ].join('\n') case 'datatable': { const dbName = output.path.split('/')[0] ?? 'main' diff --git a/python-client/tests/wmill_client_test.py b/python-client/tests/wmill_client_test.py index 3309e1ab5f..6df80919cf 100644 --- a/python-client/tests/wmill_client_test.py +++ b/python-client/tests/wmill_client_test.py @@ -124,5 +124,49 @@ SET s3_secret_access_key='80yMndIMcyXwEujxVNINQbf0tBlIzRaLPyM2m1n4'; wmill.load_s3_file(s3_obj) +class TestParseS3Object(unittest.TestCase): + """Pure-unit tests for parse_s3_object — no network/env needed.""" + + def test_bare_string_raises_with_uri_hint(self): + # A bare key is rejected rather than silently uploading under an + # auto-generated name; the error points at the s3:/// spelling. + with self.assertRaisesRegex(ValueError, "s3:///dir/file.json"): + wmill.parse_s3_object("dir/file.json") + + def test_triple_slash_uri_is_default_storage(self): + self.assertEqual( + wmill.parse_s3_object("s3:///dir/file.json"), + S3Object(s3="dir/file.json", storage=None), + ) + + def test_full_uri_splits_storage_and_key(self): + self.assertEqual( + wmill.parse_s3_object("s3://bucket/dir/f"), + S3Object(s3="dir/f", storage="bucket"), + ) + + def test_malformed_uri_raises(self): + # `s3://x` has no key part — fail loudly instead of silently + # misplacing the object. + with self.assertRaises(ValueError): + wmill.parse_s3_object("s3://broken") + + def test_empty_key_uri_raises(self): + # An empty key is never a valid target: it would fall back to an + # auto-generated key, which is requested by omitting the object. + with self.assertRaises(ValueError): + wmill.parse_s3_object("s3:///") + with self.assertRaises(ValueError): + wmill.parse_s3_object("s3://bucket/") + + def test_empty_string_raises(self): + # Auto-generated keys are requested by omitting the object (None), + # not by an empty string. + with self.assertRaises(ValueError): + wmill.parse_s3_object("") + + def test_s3object_passes_through(self): + self.assertEqual(wmill.parse_s3_object(S3Object(s3="x")), S3Object(s3="x")) + if __name__ == "__main__": unittest.main() diff --git a/python-client/wmill/wmill/client.py b/python-client/wmill/wmill/client.py index e1a59b312d..fc4c34737d 100644 --- a/python-client/wmill/wmill/client.py +++ b/python-client/wmill/wmill/client.py @@ -2212,12 +2212,27 @@ def parse_resource_syntax(s: str) -> Optional[str]: return None def parse_s3_object(s3_object: S3Object | str) -> S3Object: - """Parse S3 object from string or S3Object format.""" + """Parse S3 object from a `s3:///` URI string (`s3:///` + for the default storage) or S3Object format. Any other string raises + rather than falling back to an auto-generated key: an auto key is + requested by omitting the object, and a fallback would silently misplace + the upload on any typo. + """ if isinstance(s3_object, str): - match = re.match(r'^s3://([^/]*)/(.*)$', s3_object) + match = re.match(r'^s3://([^/]*)/(.+)$', s3_object) if match: - return S3Object(s3=match.group(2) or "", storage=match.group(1) or None) - return S3Object(s3="") + return S3Object(s3=match.group(2), storage=match.group(1) or None) + if s3_object.startswith("s3://"): + raise ValueError( + f"Invalid s3 object URI {s3_object!r}: expected " + "s3:/// with a non-empty key " + "(s3:/// for the default storage)" + ) + raise ValueError( + f"Invalid s3 object {s3_object!r}: expected an s3:/// " + f"URI (e.g. 's3:///{s3_object}' for key {s3_object!r} in the default " + "storage) or S3Object(s3=)" + ) else: return s3_object diff --git a/system_prompts/auto-generated/prompts.ts b/system_prompts/auto-generated/prompts.ts index 434a52da63..7b7e3fcf52 100644 --- a/system_prompts/auto-generated/prompts.ts +++ b/system_prompts/auto-generated/prompts.ts @@ -1447,13 +1447,6 @@ async requestInteractiveSlackApproval({ slackResourcePath, channelId, message, a */ async requestInteractiveTeamsApproval({ teamName, channelName, message, approver, defaultArgsJson, dynamicEnumsJson, }: TeamsApprovalOptions): Promise -/** - * Parse an S3 object from URI string or record format - * @param s3Object - S3 object as URI string (s3://storage/key) or record - * @returns S3 object record with storage and s3 key - */ -parseS3Object(s3Object: S3Object): S3ObjectRecord - setWorkflowCtx(ctx: WorkflowCtx | null): void async sleep(seconds: number): Promise @@ -1523,6 +1516,17 @@ async parallel(items: T[], fn: (item: T) => PromiseLike | R, options?: */ async commitKafkaOffsets(triggerPath: string, topic: string, partition: number, offset: number,): Promise +/** + * Parse an S3 object from URI string or record format + * @param s3Object - S3 object as URI string (\`s3://storage/key\`, \`s3:///key\` + * for the default storage) or record. Any other string throws rather than + * falling back to an auto-generated key: an auto key is requested by + * omitting the object, and a fallback would silently misplace the upload + * on any typo. + * @returns S3 object record with storage and s3 key + */ +parseS3Object(s3Object: S3Object): S3ObjectRecord + /** * Create a SQL template function for PostgreSQL/datatable queries * @param name - Database/datatable name (default: "main") @@ -2108,7 +2112,11 @@ def get_state_path() -> str # Parse resource syntax from string. def parse_resource_syntax(s: str) -> Optional[str] -# Parse S3 object from string or S3Object format. +# Parse S3 object from a \`s3:///\` URI string (\`s3:///\` +# for the default storage) or S3Object format. Any other string raises +# rather than falling back to an auto-generated key: an auto key is +# requested by omitting the object, and a fallback would silently misplace +# the upload on any typo. def parse_s3_object(s3_object: S3Object | str) -> S3Object # Parse variable syntax from string. diff --git a/system_prompts/auto-generated/script.md b/system_prompts/auto-generated/script.md index bdfec21036..408f1354b7 100644 --- a/system_prompts/auto-generated/script.md +++ b/system_prompts/auto-generated/script.md @@ -1888,13 +1888,6 @@ async requestInteractiveSlackApproval({ slackResourcePath, channelId, message, a */ async requestInteractiveTeamsApproval({ teamName, channelName, message, approver, defaultArgsJson, dynamicEnumsJson, }: TeamsApprovalOptions): Promise -/** - * Parse an S3 object from URI string or record format - * @param s3Object - S3 object as URI string (s3://storage/key) or record - * @returns S3 object record with storage and s3 key - */ -parseS3Object(s3Object: S3Object): S3ObjectRecord - setWorkflowCtx(ctx: WorkflowCtx | null): void async sleep(seconds: number): Promise @@ -1964,6 +1957,17 @@ async parallel(items: T[], fn: (item: T) => PromiseLike | R, options?: */ async commitKafkaOffsets(triggerPath: string, topic: string, partition: number, offset: number,): Promise +/** + * Parse an S3 object from URI string or record format + * @param s3Object - S3 object as URI string (`s3://storage/key`, `s3:///key` + * for the default storage) or record. Any other string throws rather than + * falling back to an auto-generated key: an auto key is requested by + * omitting the object, and a fallback would silently misplace the upload + * on any typo. + * @returns S3 object record with storage and s3 key + */ +parseS3Object(s3Object: S3Object): S3ObjectRecord + /** * Create a SQL template function for PostgreSQL/datatable queries * @param name - Database/datatable name (default: "main") @@ -2549,7 +2553,11 @@ def get_state_path() -> str # Parse resource syntax from string. def parse_resource_syntax(s: str) -> Optional[str] -# Parse S3 object from string or S3Object format. +# Parse S3 object from a `s3:///` URI string (`s3:///` +# for the default storage) or S3Object format. Any other string raises +# rather than falling back to an auto-generated key: an auto key is +# requested by omitting the object, and a fallback would silently misplace +# the upload on any typo. def parse_s3_object(s3_object: S3Object | str) -> S3Object # Parse variable syntax from string. diff --git a/system_prompts/auto-generated/sdks/python.md b/system_prompts/auto-generated/sdks/python.md index e6bb79d8d8..c750f03bcf 100644 --- a/system_prompts/auto-generated/sdks/python.md +++ b/system_prompts/auto-generated/sdks/python.md @@ -526,7 +526,11 @@ def get_state_path() -> str # Parse resource syntax from string. def parse_resource_syntax(s: str) -> Optional[str] -# Parse S3 object from string or S3Object format. +# Parse S3 object from a `s3:///` URI string (`s3:///` +# for the default storage) or S3Object format. Any other string raises +# rather than falling back to an auto-generated key: an auto key is +# requested by omitting the object, and a fallback would silently misplace +# the upload on any typo. def parse_s3_object(s3_object: S3Object | str) -> S3Object # Parse variable syntax from string. diff --git a/system_prompts/auto-generated/sdks/typescript.md b/system_prompts/auto-generated/sdks/typescript.md index 8557995d55..8100929776 100644 --- a/system_prompts/auto-generated/sdks/typescript.md +++ b/system_prompts/auto-generated/sdks/typescript.md @@ -455,13 +455,6 @@ async requestInteractiveSlackApproval({ slackResourcePath, channelId, message, a */ async requestInteractiveTeamsApproval({ teamName, channelName, message, approver, defaultArgsJson, dynamicEnumsJson, }: TeamsApprovalOptions): Promise -/** - * Parse an S3 object from URI string or record format - * @param s3Object - S3 object as URI string (s3://storage/key) or record - * @returns S3 object record with storage and s3 key - */ -parseS3Object(s3Object: S3Object): S3ObjectRecord - setWorkflowCtx(ctx: WorkflowCtx | null): void async sleep(seconds: number): Promise @@ -531,6 +524,17 @@ async parallel(items: T[], fn: (item: T) => PromiseLike | R, options?: */ async commitKafkaOffsets(triggerPath: string, topic: string, partition: number, offset: number,): Promise +/** + * Parse an S3 object from URI string or record format + * @param s3Object - S3 object as URI string (`s3://storage/key`, `s3:///key` + * for the default storage) or record. Any other string throws rather than + * falling back to an auto-generated key: an auto key is requested by + * omitting the object, and a fallback would silently misplace the upload + * on any typo. + * @returns S3 object record with storage and s3 key + */ +parseS3Object(s3Object: S3Object): S3ObjectRecord + /** * Create a SQL template function for PostgreSQL/datatable queries * @param name - Database/datatable name (default: "main") diff --git a/system_prompts/auto-generated/skills/write-script-bun/SKILL.md b/system_prompts/auto-generated/skills/write-script-bun/SKILL.md index e6b7e3273e..7dcaec30bc 100644 --- a/system_prompts/auto-generated/skills/write-script-bun/SKILL.md +++ b/system_prompts/auto-generated/skills/write-script-bun/SKILL.md @@ -626,13 +626,6 @@ async requestInteractiveSlackApproval({ slackResourcePath, channelId, message, a */ async requestInteractiveTeamsApproval({ teamName, channelName, message, approver, defaultArgsJson, dynamicEnumsJson, }: TeamsApprovalOptions): Promise -/** - * Parse an S3 object from URI string or record format - * @param s3Object - S3 object as URI string (s3://storage/key) or record - * @returns S3 object record with storage and s3 key - */ -parseS3Object(s3Object: S3Object): S3ObjectRecord - setWorkflowCtx(ctx: WorkflowCtx | null): void async sleep(seconds: number): Promise @@ -702,6 +695,17 @@ async parallel(items: T[], fn: (item: T) => PromiseLike | R, options?: */ async commitKafkaOffsets(triggerPath: string, topic: string, partition: number, offset: number,): Promise +/** + * Parse an S3 object from URI string or record format + * @param s3Object - S3 object as URI string (`s3://storage/key`, `s3:///key` + * for the default storage) or record. Any other string throws rather than + * falling back to an auto-generated key: an auto key is requested by + * omitting the object, and a fallback would silently misplace the upload + * on any typo. + * @returns S3 object record with storage and s3 key + */ +parseS3Object(s3Object: S3Object): S3ObjectRecord + /** * Create a SQL template function for PostgreSQL/datatable queries * @param name - Database/datatable name (default: "main") diff --git a/system_prompts/auto-generated/skills/write-script-bunnative/SKILL.md b/system_prompts/auto-generated/skills/write-script-bunnative/SKILL.md index 8ef23a996c..86c3bfd010 100644 --- a/system_prompts/auto-generated/skills/write-script-bunnative/SKILL.md +++ b/system_prompts/auto-generated/skills/write-script-bunnative/SKILL.md @@ -626,13 +626,6 @@ async requestInteractiveSlackApproval({ slackResourcePath, channelId, message, a */ async requestInteractiveTeamsApproval({ teamName, channelName, message, approver, defaultArgsJson, dynamicEnumsJson, }: TeamsApprovalOptions): Promise -/** - * Parse an S3 object from URI string or record format - * @param s3Object - S3 object as URI string (s3://storage/key) or record - * @returns S3 object record with storage and s3 key - */ -parseS3Object(s3Object: S3Object): S3ObjectRecord - setWorkflowCtx(ctx: WorkflowCtx | null): void async sleep(seconds: number): Promise @@ -702,6 +695,17 @@ async parallel(items: T[], fn: (item: T) => PromiseLike | R, options?: */ async commitKafkaOffsets(triggerPath: string, topic: string, partition: number, offset: number,): Promise +/** + * Parse an S3 object from URI string or record format + * @param s3Object - S3 object as URI string (`s3://storage/key`, `s3:///key` + * for the default storage) or record. Any other string throws rather than + * falling back to an auto-generated key: an auto key is requested by + * omitting the object, and a fallback would silently misplace the upload + * on any typo. + * @returns S3 object record with storage and s3 key + */ +parseS3Object(s3Object: S3Object): S3ObjectRecord + /** * Create a SQL template function for PostgreSQL/datatable queries * @param name - Database/datatable name (default: "main") diff --git a/system_prompts/auto-generated/skills/write-script-deno/SKILL.md b/system_prompts/auto-generated/skills/write-script-deno/SKILL.md index 9b1b04fa89..f7f3a517db 100644 --- a/system_prompts/auto-generated/skills/write-script-deno/SKILL.md +++ b/system_prompts/auto-generated/skills/write-script-deno/SKILL.md @@ -626,13 +626,6 @@ async requestInteractiveSlackApproval({ slackResourcePath, channelId, message, a */ async requestInteractiveTeamsApproval({ teamName, channelName, message, approver, defaultArgsJson, dynamicEnumsJson, }: TeamsApprovalOptions): Promise -/** - * Parse an S3 object from URI string or record format - * @param s3Object - S3 object as URI string (s3://storage/key) or record - * @returns S3 object record with storage and s3 key - */ -parseS3Object(s3Object: S3Object): S3ObjectRecord - setWorkflowCtx(ctx: WorkflowCtx | null): void async sleep(seconds: number): Promise @@ -702,6 +695,17 @@ async parallel(items: T[], fn: (item: T) => PromiseLike | R, options?: */ async commitKafkaOffsets(triggerPath: string, topic: string, partition: number, offset: number,): Promise +/** + * Parse an S3 object from URI string or record format + * @param s3Object - S3 object as URI string (`s3://storage/key`, `s3:///key` + * for the default storage) or record. Any other string throws rather than + * falling back to an auto-generated key: an auto key is requested by + * omitting the object, and a fallback would silently misplace the upload + * on any typo. + * @returns S3 object record with storage and s3 key + */ +parseS3Object(s3Object: S3Object): S3ObjectRecord + /** * Create a SQL template function for PostgreSQL/datatable queries * @param name - Database/datatable name (default: "main") diff --git a/system_prompts/auto-generated/skills/write-script-python3/SKILL.md b/system_prompts/auto-generated/skills/write-script-python3/SKILL.md index 4aee54e29a..49c0ab057f 100644 --- a/system_prompts/auto-generated/skills/write-script-python3/SKILL.md +++ b/system_prompts/auto-generated/skills/write-script-python3/SKILL.md @@ -711,7 +711,11 @@ def get_state_path() -> str # Parse resource syntax from string. def parse_resource_syntax(s: str) -> Optional[str] -# Parse S3 object from string or S3Object format. +# Parse S3 object from a `s3:///` URI string (`s3:///` +# for the default storage) or S3Object format. Any other string raises +# rather than falling back to an auto-generated key: an auto key is +# requested by omitting the object, and a fallback would silently misplace +# the upload on any typo. def parse_s3_object(s3_object: S3Object | str) -> S3Object # Parse variable syntax from string. diff --git a/typescript-client/client.ts b/typescript-client/client.ts index 8b6117fef1..5d2342471b 100644 --- a/typescript-client/client.ts +++ b/typescript-client/client.ts @@ -16,10 +16,12 @@ import { OpenAPI } from "./core/OpenAPI"; import { DenoS3LightClientSettings, S3ObjectRecord, + parseS3Object, type S3Object, } from "./s3Types"; export { + parseS3Object, type S3Object, type S3ObjectRecord, type S3ObjectURI, @@ -1505,17 +1507,6 @@ function parseResourceSyntax(s: string | undefined) { if (s?.startsWith("res://")) return s.substring(6); } -/** - * Parse an S3 object from URI string or record format - * @param s3Object - S3 object as URI string (s3://storage/key) or record - * @returns S3 object record with storage and s3 key - */ -export function parseS3Object(s3Object: S3Object): S3ObjectRecord { - if (typeof s3Object === "object") return s3Object; - const match = s3Object.match(/^s3:\/\/([^/]*)\/(.*)$/); - return { storage: match?.[1] || undefined, s3: match?.[2] ?? "" }; -} - function parseVariableSyntax(s: string) { if (s.startsWith("var://")) return s.substring(6); } diff --git a/typescript-client/s3Types.d.ts b/typescript-client/s3Types.d.ts index 4903edc8ff..57a4af2222 100644 --- a/typescript-client/s3Types.d.ts +++ b/typescript-client/s3Types.d.ts @@ -14,3 +14,4 @@ export type DenoS3LightClientSettings = { secretKey?: string; pathStyle?: boolean; }; +export declare function parseS3Object(s3Object: S3Object): S3ObjectRecord; diff --git a/typescript-client/s3Types.ts b/typescript-client/s3Types.ts index e9af9b2d30..d2049570f8 100644 --- a/typescript-client/s3Types.ts +++ b/typescript-client/s3Types.ts @@ -4,7 +4,8 @@ export type S3Object = S3ObjectURI | S3ObjectRecord; /** - * S3 object URI in the format `s3://storage/key` + * S3 object URI in the format `s3://storage/key` (`s3:///key` targets the + * workspace default storage) */ export type S3ObjectURI = `s3://${string}/${string}`; @@ -39,3 +40,26 @@ export type DenoS3LightClientSettings = { /** Use path-style URLs instead of virtual-hosted style */ pathStyle?: boolean; }; + +/** + * Parse an S3 object from URI string or record format + * @param s3Object - S3 object as URI string (`s3://storage/key`, `s3:///key` + * for the default storage) or record. Any other string throws rather than + * falling back to an auto-generated key: an auto key is requested by + * omitting the object, and a fallback would silently misplace the upload + * on any typo. + * @returns S3 object record with storage and s3 key + */ +export function parseS3Object(s3Object: S3Object): S3ObjectRecord { + if (typeof s3Object === "object") return s3Object; + const match = s3Object.match(/^s3:\/\/([^/]*)\/(.+)$/); + if (match) return { storage: match[1] || undefined, s3: match[2] }; + if (s3Object.startsWith("s3://")) { + throw new Error( + `Invalid s3 object URI ${JSON.stringify(s3Object)}: expected s3:/// with a non-empty key (s3:/// for the default storage)` + ); + } + throw new Error( + `Invalid s3 object ${JSON.stringify(s3Object)}: expected an s3:/// URI (e.g. "s3:///${s3Object}" for key "${s3Object}" in the default storage) or { s3: }` + ); +} diff --git a/typescript-client/tests/s3Types.test.ts b/typescript-client/tests/s3Types.test.ts new file mode 100644 index 0000000000..6f67a6493c --- /dev/null +++ b/typescript-client/tests/s3Types.test.ts @@ -0,0 +1,52 @@ +import { describe, expect, test } from "bun:test"; +import { parseS3Object } from "../s3Types"; + +describe("parseS3Object", () => { + test("bare string throws with the s3:/// hint", () => { + // A bare key is rejected rather than silently uploading under an + // auto-generated name; the error points at the s3:/// spelling. + expect(() => parseS3Object("dir/file.json" as any)).toThrow( + /s3:\/\/\/dir\/file\.json/ + ); + }); + + test("triple-slash URI targets the default storage", () => { + expect(parseS3Object("s3:///dir/file.json")).toEqual({ + storage: undefined, + s3: "dir/file.json", + }); + }); + + test("full URI splits storage and key", () => { + expect(parseS3Object("s3://bucket/dir/f")).toEqual({ + storage: "bucket", + s3: "dir/f", + }); + }); + + test("malformed s3:// URI throws", () => { + // `s3://x` has no key part — fail loudly instead of silently misplacing + // the object. + expect(() => parseS3Object("s3://broken" as any)).toThrow( + /Invalid s3 object/ + ); + }); + + test("empty-key URIs throw", () => { + // An empty key is never a valid target: it would fall back to an + // auto-generated key, which is requested by omitting the object. + expect(() => parseS3Object("s3:///" as any)).toThrow(/Invalid s3 object/); + expect(() => parseS3Object("s3://bucket/")).toThrow(/Invalid s3 object/); + }); + + test("empty string throws (omit the object for an auto-generated key)", () => { + expect(() => parseS3Object("" as any)).toThrow(/Invalid s3 object/); + }); + + test("record form passes through", () => { + expect(parseS3Object({ s3: "x", storage: "b" })).toEqual({ + s3: "x", + storage: "b", + }); + }); +});