Skip to content

fix(sf,pg,ora,bq): insert every replacement literally when building SQL [sc-572077] - #650

Merged
vdelacruzb merged 6 commits into
mainfrom
fix/sc-572077/sf-library-inlining-replacement-patterns
Sep 8, 2026
Merged

vdelacruzb merged 6 commits into
mainfrom
fix/sc-572077/sf-library-inlining-replacement-patterns

Conversation

@vdelacruzb

@vdelacruzb vdelacruzb commented Sep 3, 2026 •

Copy link
Copy Markdown
Contributor

The bug

Snowflake, Postgres and Oracle all inline JavaScript libraries into their SQL with a call of this shape:

text = text.replace(new RegExp(library, 'g'), libraryContent);

String.prototype.replace treats a string replacement as a template, not as literal text: it scans it for $ directives and expands them. So $&, $`, $' and $$ inside a bundle are interpreted rather than inserted — and $& means "the whole match", which here is the placeholder itself.

Passing a replacer function removes the interpretation entirely: the return value is used verbatim.

What ships today

Snowflake — real corruption. h3_polyfill, in bignumber.js's number formatter (bundled via polyclip-ts):

// source, and what the source map says
f.replace(new RegExp('\\d{' + l + '}\\B', 'g'), '$&' + (t.fractionGroupSeparator || ''))

// what is inlined into modules.sql
f.replace(new RegExp("\\d{"+l+"}\\B","g"),"@@SF_LIBRARY_H3_POLYFILL@@"+(t.fractionGroupSeparator||""))

In the public H3_POLYFILL function. No known runtime impact — the branch needs toFormat with fractionGroupSize, and polyclip-ts never formats numbers for display, so it is unreachable in practice.

In production since 2025-10-23, commit cba3b123 "fix(bq,sf): update turf version (#564)" — turf 7 replaced turf-jsts with polyclip-ts, which brought bignumber.js in. Confirmed in a package built 2026-06-15. The mechanism itself dates to 8e7f813d (2022-08-31) but stayed latent until a bundled dependency happened to contain $&.

Postgres and Oracle — latent. Same call, no manifestation today: Postgres is on turf 6.3.0 with no bignumber.js in its tree and a clean h3.js; Oracle's h3.js and quadbin.js are clean too. They are included because Snowflake's instance became real through a routine dependency bump, not a code change — the same upgrade in either cloud would silently reintroduce it.

BigQuery — deliberately unchanged. Its substitution inserts a GCS bucket path rather than code, because BigQuery loads the JavaScript from a bucket instead of inlining it. No exposure, and no reason to re-test a cloud that has none.

Redshift and Databricks have no JavaScript libraries at all.

$1-$9 were always left literal because the pattern has no capture groups, which is why the $N sequence in the statistics bundle was never affected. Luck rather than design — adding a capture group to that regex would have started corrupting it too.

Verification

Snowflake (make build, premium package):

  • h3_polyfill as inlined in the generated modules.sql is now byte-identical to libraries/javascript/build/h3_polyfill.js (114,315 bytes both)
  • "$&" present in the inlined bundle; no @@SF_LIBRARY_H3_POLYFILL@@ inside any JS body
  • zero unresolved @@SF_LIBRARY_*@@ placeholders left in modules.sql

Postgres and Oracle — proof of no behavioural change: modules.sql built before and after the fix is byte-identical for both clouds (postgres 2,232,427 B, oracle 195,122 B, same sha256 either way). Expected, since neither has a $ sequence in any bundle, but worth demonstrating rather than assuming.

lint-common clean for all three clouds. AT is being probed against this branch in CartoDB/analytics-toolbox#1188, where the premium modules inline 14 further libraries.

How it was found

The source review index in #649 hashes each bundle; verifying those hashes against the bytes actually inlined in modules.sql failed for h3_polyfill and pointed straight at the expansion. (tiler_simple mismatched too, but legitimately — it embeds @@SF_PACKAGE_VERSION@@, substituted after inlining. #649 was updated to hash the shipped files instead.)

Beyond the corruption itself, this matters for #649: once source maps ship, a reviewer comparing a map's sourcesContent against modules.sql would find the readable source disagreeing with the shipped bundle — exactly the correspondence the maps exist to demonstrate to Snowflake's security review.

Story: sc-572077

🤖 Generated with Claude Code


Added in review

The first pass fixed only library inlining, in only three clouds. Jesús's review found the class was wider and one instance was live.

The live one. native_app/SETUP_SCRIPT.sql:38 substitutes @@SF_SCHEMA@@ for the caller-supplied DESTINATION_SCHEMA, in the INSTALL procedure that runs in the consumer's account. $ is legal in an unquoted Snowflake identifier, so $$ is reachable with a valid name and halves to $; $& puts the placeholder back unresolved into SQL that snowflake.execute then runs. Fixed here and, for the copy the premium package ships, in analytics-toolbox#1193.

The rest of the class. Env-var replacements in snowflake, postgres, oracle, bigquery, databricks and redshift build_modules, in build_share, and in both run-scripts — ten sites. The first pass fixed eight and missed databricks and redshift; corrected after review. BigQuery's library inlining too, which also carried real drift: apply_replacements(text) scanned content, an outer variable, instead of its own parameter.

Dead code removed. No .sql input carries a @@SF_LIBRARY_*@@ placeholder in either repo, so the loop in build_native_app_setup_script.js was unreachable. It is now a guard that fails loudly if one is ever added, and its --libs_build_dir argument is gone from the script and the Makefile.

Two new guards. Going literal means a $$ inside a bundle would now close an AS $$ … $$ body — Snowflake only, since Postgres dollar-quotes with $BODY$ and Oracle puts raw JS after LANGUAGE JAVASCRIPT AS. 0 occurrences across the 25 core bundles today, so it is forward-looking, and the build now rejects it. Every inlining also checks the inserted byte count, which catches a revert to a string replacement.

Bounded the placeholder patterns. @@SF_LIBRARY_.*@@ is greedy and collapses two placeholders on one line into a single bogus match; Oracle already used the bounded form. 0 such lines today — the same latent-risk argument this PR makes for PG and ORA.

Not done here: deduplicating apply_replacements. Six copies that have drifted is the root cause, but they are not interchangeable — BigQuery substitutes a bucket path and strips @@SKIP_DEP@@, Snowflake needs a $$ rejection that would be wrong elsewhere, Oracle resolves its own library directory. That is a refactor across four clouds' build paths, and it belongs in its own story rather than inside a correctness fix.

Verification

The CI evidence originally posted here was invalid and has been retracted: analytics-toolbox#1188 changed three READMEs and the submodule pointer, matched no path filter, and if (diffModulesFilter) is truthy on [], so zero functions were built — the runs took ~2m11s against ~17min for a real one. Replaced with a local premium build against this branch:

491 functions built            (the probe built 0)
0   unresolved placeholders
3   literal "$&" occurrences
h3_polyfill inlined as:  "$&"+(t.fractionGroupSeparator||"))
                previously:  "@@SF_LIBRARY_H3_POLYFILL@@"+(t.fractionGroupSeparator||"")

Both guards were tested by deliberately triggering them, Postgres and Oracle build clean, BigQuery's accessors.js warning reproduces identically on origin/main (a missing local bundle, not this change), and eslint is clean across all eight files.

Merges cleanly with #649 — they share only clouds/snowflake/modules/Makefile, which auto-merges.


Added in the second review round

Jesús checked 2a119db4 and found two gaps plus two forward-looking notes. All addressed:

  • databricks:167 and redshift:156 carried the same string replacement, so "eight sites" was eight of ten. Fixed; no occurrence of that pattern remains anywhere in clouds/.
  • SETUP_SCRIPT.sql:38 is the only runtime change here and nothing exercised it — it runs in Snowflake's JavaScript engine at install time, so the replacer form rested on the function overload of replaceAll behaving there, which replaceAll-with-a-string being in production does not prove. Switched to split(variable).join(value), which is literal by construction and needs no assumption about engine support. Verified identical output to the replacer for $$, $& and a plain name. Same change in analytics-toolbox#1193.
  • The bounded pattern narrowed the character class, not just the greed. @@SF_LIBRARY_H3-POLY@@ matches .*@@ but not [A-Z0-9_]+@@, and that miss is silent — a placeholder that never enters the libraries list gets no "does not exist" warning and ships unresolved. Now [^@]+ in every scanner: build_modules and list_libraries for Snowflake and Postgres, the setup-script guard, and Oracle, which already had the narrowed class and is where the pattern was copied from.
  • $$ at the installer site stays unguarded, as agreed — still strictly better than the previous silent $$ → $ halving, and a loud SQL error rather than a wrong schema.

Follow-up filed for the root cause, as requested: sc-572754 — deduplicate apply_replacements, including the insert_literally copies this PR adds, and the note that no CI path currently exercises the premium modules.

build_modules.js inlined each library with

    text.replace(new RegExp(library, 'g'), libraryContent)

and String.replace interprets $ sequences in the replacement string, so any
$&, $` or $' inside a bundle was expanded rather than inserted. $& means the
matched substring, which here is the placeholder itself.

One occurrence across the 39 Snowflake bundles: h3_polyfill, in bignumber.js's
number formatter (bundled via polyclip-ts), shipped as

    f.replace(new RegExp("\\d{"+l+"}\\B","g"),"@@SF_LIBRARY_H3_POLYFILL@@"+...)

instead of "$&"+... . The branch needs toFormat with fractionGroupSize, which
nothing in the toolbox calls, so there is no known runtime impact -- but it is
corrupted code in a shipped bundle, and it breaks the correspondence between
the source maps added in sc-571986 and the code they map to: the map's
sourcesContent has "$&" where modules.sql has the placeholder.

$1-$9 are left literal because the pattern has no capture groups, so the $N
sequence in the statistics bundle was never affected.

Fixed with a replacer function in both scripts that inline libraries.
build_share.js does not inline libraries and is unaffected.

Verified: h3_polyfill as inlined in the generated modules.sql is now
byte-identical to libraries/javascript/build/h3_polyfill.js, "$&" is present,
no placeholder leaked into a JS body, and no library placeholder is left
unresolved anywhere in modules.sql.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@vdelacruzb
vdelacruzb marked this pull request as draft September 3, 2026 13:44
@vdelacruzb

Copy link
Copy Markdown
Contributor Author

Parking this — the story (sc-572077) is in the backlog, so moving the PR to draft rather than leaving it looking like it awaits review. The fix and its verification stand as they are; mark it ready when the story is picked up.

Origin pinned since opening: the String.replace mechanism dates to 8e7f813d (2022-08-31) but stayed latent until cba3b123 "fix(bq,sf): update turf version (#564)" on 2025-10-23, when turf 7 replaced turf-jsts with polyclip-ts and pulled bignumber.js — whose formatter carries the $& — into the h3_polyfill bundle. Confirmed shipping in a package built 2026-06-15, so it has been in production for about ten months.

Affected function: H3_POLYFILL. Still no known path reaches the corrupted branch (bignumber.js toFormat with fractionGroupSize; polyclip-ts never formats numbers), which is why this is a correctness fix rather than an incident.

@vdelacruzb
vdelacruzb marked this pull request as ready for review September 3, 2026 13:51
@vdelacruzb

Copy link
Copy Markdown
Contributor Author

Un-parking: CI is green and the change is one line in each of two scripts, so taking it now rather than carrying a known corruption. Ready for review.

…572077]

Postgres and Oracle inline JavaScript libraries into their SQL with the same
String.replace(pattern, content) call as Snowflake, so they carry the same
defect: $&, $` and $' inside a bundle are expanded as replacement patterns
instead of being inserted literally.

Neither manifests today. Postgres is on turf 6.3.0 with no bignumber.js in its
tree and its only bundle (h3.js) has no $ sequences; Oracle's h3.js and
quadbin.js are equally clean. But Snowflake's instance became real through a
routine dependency bump, not a code change -- turf 7 pulled in polyclip-ts and
bignumber.js -- so the same upgrade here would silently reintroduce it.

Oracle's call was split across three lines, which is why it did not show up in
the first sweep; it now reads the bundle into a named constant first, matching
the other clouds.

BigQuery deliberately unchanged: its substitution inserts a GCS bucket path
rather than code, because BigQuery loads the JavaScript from a bucket instead
of inlining it.

Verified no behavioural change: modules.sql built before and after the fix is
byte-identical for both clouds (postgres 2,232,427 B, oracle 195,122 B, same
sha256 either way), and lint-common is clean for both.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@vdelacruzb vdelacruzb changed the title fix(sf): insert library bundles literally when inlining them [sc-572077] fix(sf,pg,ora): insert library bundles literally when inlining them [sc-572077] Sep 3, 2026
Matching the surrounding comment density.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@vdelacruzb

vdelacruzb commented Sep 4, 2026 •

Copy link
Copy Markdown
Contributor Author

Warning

Retracted — this evidence is invalid. Jesus is right. analytics-toolbox#1188 changed only three
READMEs and the core submodule pointer, which matches no build path filter, and if (diffModulesFilter)
is truthy on [], so the module filter came out empty and zero functions were built. The three runs
finished in 2m11s-2m18s, against ~17min for a real one. They tested nothing. Replaced with a local
premium build in the reply below.


AT CI is green against this branch as well.

Probe PR CartoDB/analytics-toolbox#1188 pinned the core submodule at e976988 (this PR's head) and ran the three clouds this change touches:

Cloud Run Result
Snowflake 33862730691 success
Postgres 33862730724 success
Oracle 33862730525 success

Each run is the full test job — lint, deploy to the CI warehouse, module test suite, schema teardown — so the premium modules build and run against the patched inlining, not just the core ones.

The probe PR is now closed; it was never a merge candidate. Last acceptance item on sc-572077 is ticked.

@vdelacruzb
vdelacruzb requested a review from Jesus89 September 4, 2026 14:23

@Jesus89 Jesus89 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes. The fix itself is right — () => value stops $&, $`, $' and $$ expanding on inline — but three things block it. It misses the one live instance of the same bug: replaceAll(variable, value) at native_app/SETUP_SCRIPT.sql:38, where the value is the caller-supplied DESTINATION_SCHEMA ($&/$$ are legal in unquoted Snowflake identifiers, and the result is unresolved @@SF_SCHEMA@@ in SQL that snowflake.execute runs) — and this PR already edits that file's generator. One of the four edits is dead code: no .sql input to build_native_app_setup_script.js has a @@SF_LIBRARY_*@@ placeholder, so the loop never runs. And going literal opens a new Snowflake-only hazard: $$ inside a bundle now lands verbatim in an AS $$ … $$ body and closes it early — 0 occurrences across all 25 bundles today, but that's the dependency-bump case this PR argues for, and the new comment omits $$. A build-time assert (no $$; inlined bytes == source bytes) would encode the h3_polyfill hash check you did by hand; no test is added currently. Also, the cited premium run doesn't cover those 14 libraries — analytics-toolbox#1188 changed only the core submodule pointer plus READMEs, matching no build pattern, and if (diffModulesFilter) is truthy on [], so it emitted zero functions (2m14s vs 17min here). Lastly, the same class stays unfixed for env-var replacements (snowflake:181, postgres:203, oracle:221, bigquery:195, build_share:187, run-script:69) and for BigQuery inlining (bigquery:184, which also reads content instead of its text param) — inconsistent with including PG/ORA on latent-risk grounds. Root cause: apply_replacements is copy-pasted across six files and already drifted, which is why one fix needed four edits.

Addresses the review on #650.

The live instance the first pass missed: SETUP_SCRIPT.sql line 38 replaces
@@SF_SCHEMA@@ with the caller-supplied DESTINATION_SCHEMA, in the INSTALL
procedure that runs in the consumer's account, so a $ directive there reaches
SQL that snowflake.execute runs.

The rest of the class, now consistent across clouds: the env-var replacements in
snowflake, postgres, oracle and bigquery build_modules, build_share, and both
run-scripts, plus bigquery's library inlining.

Dropped the dead library loop from build_native_app_setup_script.js -- no .sql
input in either repo carries a placeholder -- and replaced it with a guard, so
adding one fails loudly instead of shipping it unresolved. Its --libs_build_dir
flag went with it.

New guards: a bundle containing $$ is rejected, because going literal means it
would now close an AS $$ ... $$ body; and every inlining checks the inserted
bytes, which catches a future revert to a string replacement.

Bounded the snowflake and postgres placeholder patterns to match oracle's: the
greedy .*@@ over-matches if two placeholders ever share a line.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@vdelacruzb vdelacruzb left a comment •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All seven verified before fixing, and every one held. Fixed in 2a119db, plus a companion PR for the copy this repo does not own.

# Finding Resolution
1 Missed live instance: SETUP_SCRIPT.sql:38 Fixed here and in analytics-toolbox#1193
2 One of the four edits is dead code Loop removed, replaced with a guard
3 Going literal makes $$ close an AS $$ body Build now rejects a bundle containing $$
4 No test encoding the by-hand check Byte check on every inlining
5 The cited premium CI run covers nothing Retracted. Replaced with a local premium build
6 The class stays unfixed for env vars and BigQuery All eight sites fixed eight of ten — databricks and redshift missed, fixed in round two
7 Root cause: apply_replacements copy-pasted and drifted Two instances of drift fixed; dedupe argued below

1 — the one that mattered. modulesSql.replaceAll(variable, value) where value is the caller-supplied DESTINATION_SCHEMA, inside the INSTALL procedure that runs in the consumer's account. $ is legal in an unquoted Snowflake identifier, so $$ is reachable with a valid name and silently halves to $; $& puts @@SF_SCHEMA@@ back unresolved into SQL that snowflake.execute then runs. This repo's copy is fixed here; the premium package ships AT's copy (APP_DIR = $(MAKE_DIR)), so that needed its own PR.

2 — confirmed dead, and removed rather than fixed. No .sql input carries a placeholder in either repo — SETUP_SCRIPT.sql, ADDITIONAL_TABLES.sql and DROP_FUNCTIONS.sql are all 0, in core and in AT — and apply_replacements is called exactly once, on that content. So the loop was unreachable. Deleting it would silently lose the capability if someone later added a placeholder, so it is now a loud failure instead:

ERROR: the setup script cannot inline libraries, found @@SF_LIBRARY_H3@@

Its --libs_build_dir argument went with it, including where the Makefile passed it.

3 and 4 — both guards, with the $$ one being the regression this PR would otherwise introduce. Snowflake is the only cloud exposed: 94 of its SQL files use AS $$, while Postgres dollar-quotes with $BODY$ and Oracle puts raw JS after LANGUAGE JAVASCRIPT AS. Verified 0 occurrences across all 25 core bundles today, so this is purely forward-looking:

$$ injected into a bundle
  ERROR: library "@@SF_LIBRARY_QUADBIN@@" contains "$$", which would close the AS $$ ... $$ body

replacer reverted to a string replacement
  ERROR: "@@SF_LIBRARY_H3_POLYFILL@@" was not inserted literally

The second is a length identity — expected = before + count * (content.length - placeholder.length) — so it costs nothing and fails on any expansion. It is in all three inlining clouds. You are right that the comment omitted $$; it now names the hazard directly instead.

5 — you are right, and I have retracted the comment. #1188 changed three READMEs and the submodule pointer, matched no path filter, and [] being truthy left the module filter empty, so nothing was built. The runs were 2m11s-2m18s. Real evidence instead, building the premium modules against this branch:

491 functions built            (the probe built 0)
0   unresolved placeholders
3   literal "$&" occurrences
h3_polyfill inlined as:  "$&"+(t.fractionGroupSeparator||""))

That last line is the fix: it previously read "@@SF_LIBRARY_H3_POLYFILL@@"+(t.fractionGroupSeparator||""). Postgres and Oracle also build clean; BigQuery's accessors.js warning reproduces identically on origin/main, so it is a missing local bundle, not this change.

Separately, the path-filter behaviour you exposed is a real hole — a submodule bump can never exercise the premium modules in AT CI. Worth its own ticket.

6 — all of it, plus one more. Env-var replacements in snowflake:181, postgres:203, oracle:221, bigquery:195, build_share:187, run-script:69, and bigquery/common/run-script.js:25 which your list did not include. BigQuery's inlining too — and it had the drift you predicted: apply_replacements(text) scanned content, an outer variable, not its own parameter, while Snowflake's scanned text.

I also bounded the Snowflake and Postgres placeholder patterns to Oracle's shape. @@SF_LIBRARY_.*@@ is greedy, so two placeholders on one line collapse into a single bogus match:

greedy   -> ["@@SF_LIBRARY_A@@; y = @@SF_LIBRARY_B@@"]
bounded  -> ["@@SF_LIBRARY_A@@", "@@SF_LIBRARY_B@@"]

0 such lines today — the same latent-risk argument this PR already makes for PG and ORA.

7 — agreed on the diagnosis, pushing back on doing it here. Six copies that have already drifted is why one fix needed four edits, and the content/text bug is drift that a shared implementation would have prevented. But the copies are not interchangeable: BigQuery substitutes a bucket path rather than inlining code and strips @@SKIP_DEP@@, Snowflake needs the $$ rejection that would be wrong for the others, Oracle resolves its library directory internally. A shared helper would need those as options, which is a refactor across four clouds' build paths with no test coverage of its own — too much to smuggle into a correctness fix that is currently a one-line change per site. I would rather land this and take the dedupe as its own story, where the CI hole above also gets fixed so it can actually be verified. Say the word if you would rather it happened here and I will do it.

eslint clean across all eight files.

@vdelacruzb vdelacruzb changed the title fix(sf,pg,ora): insert library bundles literally when inlining them [sc-572077] fix(sf,pg,ora,bq): insert every replacement literally when building SQL [sc-572077] Sep 7, 2026
@vdelacruzb
vdelacruzb requested a review from Jesus89 September 7, 2026 16:07
@Jesus89

Jesus89 commented Sep 8, 2026

Copy link
Copy Markdown
Member

Checked 2a119db4. Five of the seven hold as described, and I verified the two I could get wrong myself: there is no @@SF_LIBRARY_*@@ in native_app/SETUP_SCRIPT.sql or ADDITIONAL_TABLES.sql in either repo, so the new hard guard cannot break the premium build; and the byte check does catch a revert to a string replacement. Two gaps left, both small.

1. Two sites of the same class are still unfixed, in clouds the title does not mention.

clouds/databricks/common/build_modules.js:167    text = text.replace(pattern, process.env[replacement]);
clouds/redshift/common/build_modules.js:156      text = text.replace(pattern, process.env[replacement]);

Byte-identical to the six you did fix. The table says "All eight sites fixed" and the commit message says "consistent across clouds" — it is eight of ten. Whatever latent-risk argument pulled Postgres and Oracle into this PR applies to these two unchanged, and leaving them means the next reader has to rediscover that two clouds were skipped. One line each.

2. SETUP_SCRIPT.sql:38 is the only runtime change in the PR, and none of the evidence exercises it.

Everything cited — 491 functions built, 0 unresolved placeholders, the h3_polyfill line, the byte and $$ guards — is build-time. Line 38 does not run at build time. It runs inside Snowflake's JavaScript engine, in the INSTALL procedure, in the consumer's account, and #1193 ships the same line to the premium package. So the one change with a blast radius outside our own CI is the one nothing tested.

The specific risk is the replacer form: replaceAll(variable, () => value) requires that engine to accept a function as the second argument. replaceAll with a string argument is already in production there, which tells us the method exists — not that the function overload behaves. If it does not, every native-app install fails, in both repos, and we find out from a customer.

Two ways out, either is fine:

modulesSql = modulesSql.split(variable).join(value);

That is literal by construction, needs no $-directive reasoning, and raises no question about engine support. Or keep the replacer and post the output of an actual native-app install against a patched setup script.

Worth noting on the way past, though neither blocks: going literal here is what makes a DESTINATION_SCHEMA containing $$ land verbatim in SQL that is full of AS $$ … $$ bodies and close one early. That is the hazard finding 3 is about, arriving through finding 1's fix, and it is unguarded at this site. It is still an improvement — a loud SQL error beats the old silent $$ → $ halving — but the $$ guard you added covers library inlining only, not this value and not the Snowflake env-var replacements.

Also: the bounded pattern narrows the character class, not just the greed. @@SF_LIBRARY_H3-POLY@@ matches @@SF_LIBRARY_.*@@ and matches [^@]+@@, but does not match [A-Z0-9_]+@@ — and a miss there is silent, since a placeholder that never enters the libraries list gets no "does not exist" warning and ships unresolved. Every name is safe today (core is all [A-Z0-9_]; AT's bundles are EXPORT, GATEWAY, LDS, RASTER, STATISTICS, DATA, TILER), so this is the same forward-looking argument the rest of the PR makes. [^@]+@@ fixes the two-placeholders-on-one-line case without the new silent-miss.

And I agree on deferring the dedupe — but insert_literally is now copy-pasted three times, which is the root cause growing while we wait. Fine if the follow-up story is real; please link it here when you file it.

vdelacruzb and others added 2 commits September 8, 2026 14:54
…r literal by construction

Jesus was right on all three points.

The two env-var sites the first pass missed:
databricks/common/build_modules.js:167 and redshift/common/build_modules.js:156
carry the same string replacement as the six already fixed, so "all eight
sites" was eight of ten.

SETUP_SCRIPT.sql line 38 is the only runtime change in this PR and nothing
exercised it: it runs in Snowflake's JavaScript engine at install time, not at
build time. replaceAll with a string is already in production there, which
proves the method exists but not that the function overload behaves, and a
failure would surface as a broken install in a consumer account. split/join is
literal by construction, needs no $-directive reasoning and no assumption about
engine support. Verified identical output to the replacer for $$, $& and a
plain name.

The bounded placeholder pattern narrowed the character class as well as the
greed: @@SF_LIBRARY_H3-POLY@@ matches .*@@ but not [A-Z0-9_]+@@, and a miss
there is silent -- a placeholder that never enters the libraries list gets no
"does not exist" warning and ships unresolved. [^@]+ fixes the
two-on-one-line case without that, and is applied to all five scanners so
build_modules, list_libraries and the setup-script guard agree.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Oracle already had [A-Z0-9_]+, which is where the snowflake and postgres
patterns were copied from. The same silent miss applies: a placeholder with a
character outside that class is not matched, never enters the libraries list,
gets no "does not exist" warning, and ships unresolved.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@vdelacruzb vdelacruzb left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both gaps fixed, both notes taken, and the root-cause story is filed. You also found one I had introduced without noticing.

1. databricks:167 and redshift:156 — right, and my claim was wrong. Both carried the identical string replacement. Fixed; no occurrence of replace(pattern, process.env[replacement]) remains anywhere under clouds/, and ten sites now use a replacer. I have corrected "all eight sites" in the PR description and struck the row in my earlier table rather than leave it reading as done.

2. SETUP_SCRIPT.sql:38 — taking your split/join. The point stands as you put it: everything I cited was build-time, and that line is the only thing in the PR that runs anywhere else. replaceAll with a string being in production proves the method exists, not that the function overload behaves, and the failure mode is a broken install in a consumer account.

modulesSql = modulesSql.split(variable).join(value);

Literal by construction, no $-directive reasoning, no engine assumption. Verified byte-identical to the replacer for MY$$SCHEMA, A$&B and a plain name, and both generated procedures still pass node --check. Same change in analytics-toolbox#1193, which ships the copy the premium package uses.

3. The character class — you are right, and Oracle is where I got it from. Confirmed:

                        two-on-one-line          @@SF_LIBRARY_H3-POLY@@
greedy  .*@@            one bogus match          matched
mine    [A-Z0-9_]+@@    correct                  null          <- silent
yours   [^@]+@@         correct                  matched

And the miss really is silent: nothing downstream flags a leftover @@SF_LIBRARY_*@@. Worse, my change had made build_modules stricter than list_libraries, so the two could disagree about which libraries exist — the drift class you are complaining about, introduced by the fix for it.

Now [^@]+ in every scanner: build_modules and list_libraries for Snowflake and Postgres, the setup-script guard, and Oracle's build_modules, which already had [A-Z0-9_]+ and is where I copied it from. BigQuery keeps [^@]*?_BUCKET@@, which is a different shape and already avoids the problem.

4. $$ at the installer site stays unguarded, as you suggested. Still an improvement — a loud SQL error beats the old silent $$ → $ halving — and guarding it would mean validating a caller-supplied identifier inside installed code, which I would rather not add here.

Root cause: sc-572754. Filed with the drift evidence this PR turned up — BigQuery scanning content instead of its parameter, the three disagreeing patterns, the $-expansion bug living in eight copies, and the three insert_literally copies this PR adds. It also records the per-cloud differences a shared implementation has to express as configuration rather than duplication, and that no CI path currently exercises the premium modules, so any refactor there is a blind change across six clouds until that is fixed.

CI on f3943e0a: BigQuery, Databricks, Oracle and Postgres green; Redshift and Snowflake still running.

@vdelacruzb

Copy link
Copy Markdown
Contributor Author

CI green on f3943e0a — all six clouds: BigQuery, Databricks, Oracle, Postgres, Redshift, Snowflake.

@vdelacruzb
vdelacruzb merged commit f9e501e into main Sep 8, 2026
29 checks passed
@vdelacruzb
vdelacruzb deleted the fix/sc-572077/sf-library-inlining-replacement-patterns branch September 8, 2026 14:22
@vdelacruzb vdelacruzb mentioned this pull request Sep 24, 2026
vdelacruzb added a commit that referenced this pull request Sep 24, 2026
### Changelog

- chore(sf): remove the broken h3_fromlonglat JS library entry [sc-569993] (#647)
- chore(sf): connect lazily in the Snowflake test harness [sc-570150] (#648)
- chore(sf): ship JavaScript source maps in the native app package [sc-571986] (#649)
- fix(sf,pg,ora,bq): insert every replacement literally when building SQL [sc-572077] (#650)
- fix(db): renew the Databricks token instead of minting it once per job (#652)
- chore(deps): bump sqlparse from 0.5.4 to 0.6.0 (#642, #643, #644)
- refactor: migrate gsutil to gcloud storage [sc-574714] (#651)
- chore(ci): migrate GitHub Actions auth to WIF (#641)

### Version

No package changes. Every commit this cycle is build, CI or test-harness work, so no
provider version moves and nothing publishes; this release syncs stable and records
the changelog.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
vdelacruzb added a commit that referenced this pull request Sep 24, 2026
### Changelog

- chore(sf): remove the broken h3_fromlonglat JS library entry (#647)
- chore(sf): connect lazily in the Snowflake test harness (#648)
- chore(sf): ship JavaScript source maps in the native app package (#649)
- fix(sf,pg,ora,bq): insert every replacement literally when building SQL (#650)
- fix(db): renew the Databricks token instead of minting it once per job (#652)
- chore(deps): bump sqlparse from 0.5.4 to 0.6.0 (#642, #643, #644)
- refactor: migrate gsutil to gcloud storage (#651)
- chore(ci): migrate GitHub Actions auth to WIF (#641)

### Version

No package changes. Every commit this cycle is build, CI or test-harness work, so no
provider version moves and nothing publishes; this release syncs stable and records
the changelog.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants