Skip to content

Commit 95d2f9d

Browse files
committed
polish: address fresh-eyes review
Three parallel whole-PR reviews surfaced no blockers but several genuine resilience and clarity improvements. Applied: * Added a CLI regression test covering the realistic CI scenario: a config.yaml declaring a Postgres state connection whose credentials come from unset env vars. Proves end-to-end that no secrets are needed for format. Skipped when psycopg2 isn't importable. Non-vacuity verified: temporarily forcing load_state=True makes the test fail with the patched RuntimeError. * Hardened the lint test's string-replace anchors with explicit `assert anchor in read_file` checks. If sushi's config.py shape ever drifts, the test will now fail at the anchor with a useful message instead of silently no-op-ing and producing a confusing downstream symptom. * Documented the plan-state guard-rail test's mechanic in its docstring (we don't assert on exit_code because state is patched to raise; the spy records the constructor call regardless). * Added a code comment by `load_state = ...` in cli/main.py explaining why the assignment is outside the single-path conditional, so a future tidy-up doesn't re-introduce the multi-path regression that the previous fix commit just closed. * Tightened wording in the `load_state` docstring on GenericContext ("Consulted by load(); has no effect when load() is not called" rather than "Only meaningful when load=True", which was imprecise for manual post-construction `context.load()` calls). * Replaced `f.writelines(read_file)` with `f.write(read_file)` in the lint test. The original worked because strings are iterable, but read as a bug. * Documented the pre-existing Snowflake-default-auth limitation in the spec's Risks section: Postgres state configs validate fine with env_var-resolved None values, but Snowflake's `_validate_authenticator` raises ConfigError at config-load time if user and password are both missing under default auth. Not introduced by this PR (the validator predates it) but it's the one realistic CI setup where "zero secrets" doesn't hold. Coding-Agent: pi Model: anthropic/claude-opus-4-7 Signed-off-by: Joe Hartshorn <8881940+j-hartshorn@users.noreply.github.com>
1 parent ea59785 commit 95d2f9d

5 files changed

Lines changed: 75 additions & 6 deletions

File tree

docs/2026-05-21_local-only-format/spec.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@ Tests: add a regression test that constructs a `Context` against a project whose
6767
- **`update_model_schemas` cache state.** `load(update_schemas=True)` runs after the state-merge block. When the block is skipped, the local `MappingSchema` won't contain remote snapshot columns. For `format` and `lint` this is fine — neither needs resolved column types from remote projects — but if a linter rule is ever added later that relies on cross-project column resolution, it would silently produce different results under `load_state=False`. Note in the linter rule contract, but no code change needed today.
6868
- **CLI test isolation.** Asserting "no state connection was opened" is easier to assert by patching than by mocking the network. The cleanest seam is `EngineAdapterStateSync.get_versions` — patching it to raise should be enough; we then assert `format`/`lint` succeed and the patched method is never called. If that seam turns out to be wrong (e.g., a different method gets called first), we'll adjust during implementation.
6969
- **Multi-project monorepos.** The state-merge block's purpose is to import remote snapshots for *other* projects when the current load covers only a subset (`context.py:681-697`, gated on `any(self._projects)`). For `format`/`lint`, this means a model that depends on a model defined in a sibling project not currently being loaded won't have its upstream resolved. That's already the behavior for any model `format`/`lint` touches — they operate per-file, not on rendered downstream queries — so there should be no observable difference. Flagging in case a reviewer raises it.
70+
- **Snowflake default-auth state connections.** `PostgresConnectionConfig` accepts `password=None` from a missing `{{ env_var() }}` (Pydantic coercion), so a Postgres state config validates with no secrets and the gate then prevents the connection. `SnowflakeConnectionConfig._validate_authenticator` (`sqlmesh/core/config/connection.py:633-638`) is stricter: under default authentication it raises `ConfigError("User and password must be provided")` at config-load time if both are missing. That validation runs before our `load_state` gate can take effect, so a project whose state lives in Snowflake with default-auth still requires user/password env vars to be set (any non-empty values — they won't be used). This is pre-existing Snowflake validator behavior, not something this change introduces, but it's the one realistic CI configuration where "zero secrets" doesn't hold. Workarounds: use the Snowflake key-pair or OAuth authenticators (which have their own validators with different requirements), or set placeholder env vars in the CI job.
7071

7172
## Dependencies
7273

sqlmesh/cli/main.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,8 @@ def cli(
116116
configure_console(ignore_warnings=ignore_warnings)
117117

118118
load = True
119+
# Computed outside the single-path block: local-only behaviour must apply
120+
# regardless of how many --paths were provided, unlike SKIP_LOAD_COMMANDS.
119121
load_state = ctx.invoked_subcommand not in LOCAL_ONLY_COMMANDS
120122

121123
if len(paths) == 1:

sqlmesh/core/context.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -363,7 +363,7 @@ class GenericContext(BaseContext, t.Generic[C]):
363363
connection as it appears in configuration will be used.
364364
concurrent_tasks: The maximum number of tasks that can use the connection concurrently.
365365
load: Whether or not to automatically load all models and macros (default True).
366-
load_state: Whether to merge remote state into the local project during load (default True). Only meaningful when load=True.
366+
load_state: Whether to merge remote state into the local project during load (default True). Consulted by load(); has no effect when load() is not called.
367367
console: The rich instance used for printing out CLI command results.
368368
users: A list of users to make known to SQLMesh.
369369
"""

tests/cli/test_cli.py

Lines changed: 62 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2273,7 +2273,13 @@ def test_lint_runs_without_state(runner: CliRunner, tmp_path: Path, mocker):
22732273

22742274
def test_plan_still_loads_state(runner: CliRunner, tmp_path: Path, mocker):
22752275
"""Guard-rail: confirm `plan` is not in LOCAL_ONLY_COMMANDS by checking
2276-
the Context constructor received load_state=True for it."""
2276+
the Context constructor received load_state=True for it.
2277+
2278+
The `plan` invocation is expected to fail because state access is patched
2279+
to raise. We don't assert on exit_code; mocker.spy records the constructor
2280+
call before the wrapped __init__ runs, so the kwargs assertion holds
2281+
regardless of whether the call ultimately raised.
2282+
"""
22772283
_setup_local_only_project(tmp_path, mocker)
22782284
init_spy = mocker.spy(Context, "__init__")
22792285

@@ -2284,3 +2290,58 @@ def test_plan_still_loads_state(runner: CliRunner, tmp_path: Path, mocker):
22842290
assert all(load_state_values), (
22852291
f"Context was constructed with load_state=False for `plan`: {load_state_values}"
22862292
)
2293+
2294+
2295+
def test_format_runs_without_state_credentials(
2296+
runner: CliRunner, tmp_path: Path, mocker, monkeypatch
2297+
):
2298+
"""Realistic CI scenario: a config.yaml declaring a remote Postgres state
2299+
connection with credentials sourced from unset env vars. Format must still
2300+
succeed without opening the state connection.
2301+
2302+
Distinct from test_format_runs_without_state: that one proves the gate by
2303+
patching get_versions. This one proves the end-to-end CI use case where
2304+
no secrets are provisioned and YAML env_var() resolves to None.
2305+
"""
2306+
pytest.importorskip("psycopg2")
2307+
2308+
for var in ("PG_HOST", "PG_USER", "PG_PASSWORD", "PG_DATABASE"):
2309+
monkeypatch.delenv(var, raising=False)
2310+
2311+
create_example_project(tmp_path, template=ProjectTemplate.EMPTY)
2312+
(tmp_path / "config.yaml").write_text(
2313+
"""project: cli_test
2314+
2315+
gateways:
2316+
prod:
2317+
state_connection:
2318+
type: postgres
2319+
host: "{{ env_var('PG_HOST', 'postgres.internal.example.com') }}"
2320+
port: 5432
2321+
user: "{{ env_var('PG_USER') }}"
2322+
password: "{{ env_var('PG_PASSWORD') }}"
2323+
database: "{{ env_var('PG_DATABASE', 'sqlmesh_state') }}"
2324+
connection:
2325+
type: duckdb
2326+
database: "warehouse.db"
2327+
2328+
default_gateway: prod
2329+
2330+
model_defaults:
2331+
dialect: duckdb
2332+
""",
2333+
encoding="utf-8",
2334+
)
2335+
(tmp_path / "models" / "example.sql").write_text(
2336+
"MODEL(name local.example, dialect 'duckdb'); SELECT 1 AS col\n",
2337+
encoding="utf-8",
2338+
)
2339+
2340+
mock = mocker.patch(
2341+
"sqlmesh.core.state_sync.db.facade.EngineAdapterStateSync.get_versions",
2342+
side_effect=RuntimeError("state should not be accessed"),
2343+
)
2344+
2345+
result = runner.invoke(cli, ["--paths", str(tmp_path), "format"])
2346+
assert result.exit_code == 0, f"Format failed: {result.output}\nException: {result.exception}"
2347+
mock.assert_not_called()

tests/core/linter/test_builtin.py

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -244,11 +244,14 @@ def test_lint_without_state_load(tmp_path, copy_to_temp_path, mocker) -> None:
244244

245245
# Set a non-empty project name so `any(self._projects)` is truthy and the
246246
# state-merge guard in `Context.load()` actually exercises `self._load_state`.
247+
project_anchor = "config = Config(\n gateways="
248+
assert project_anchor in read_file, (
249+
"sushi config.py shape drifted; update project_anchor in test"
250+
)
247251
read_file = read_file.replace(
248-
"config = Config(\n gateways=",
252+
project_anchor,
249253
'config = Config(\n project="sushi",\n gateways=',
250254
)
251-
assert 'project="sushi"' in read_file
252255

253256
# Enable the linter with one built-in rule so `lint_models` actually executes
254257
# a rule under `load_state=False`, not just the empty-rule-set path.
@@ -264,11 +267,13 @@ def test_lint_without_state_load(tmp_path, copy_to_temp_path, mocker) -> None:
264267
],
265268
),"""
266269
after = 'linter=LinterConfig(enabled=True, rules=["nomissingexternalmodels"]),'
270+
assert before in read_file, (
271+
"sushi config.py LinterConfig block shape drifted; update `before` in test"
272+
)
267273
read_file = read_file.replace(before, after)
268-
assert after in read_file
269274

270275
with open(sushi_path / "config.py", "w") as f:
271-
f.writelines(read_file)
276+
f.write(read_file)
272277

273278
mock = mocker.patch(
274279
"sqlmesh.core.state_sync.db.facade.EngineAdapterStateSync.get_versions",

0 commit comments

Comments
 (0)