Harden AI and ELT pipeline execution for v0.4.0 - #14
Conversation
📝 WalkthroughWalkthroughThis PR adds benchmark and AI-transform example infrastructure, centralizes provider model defaults, extends incremental and sandbox execution, hardens PostgreSQL SQL composition and file publication, and updates associated tests, documentation, and release metadata. ChangesImplementation and validation
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
loafer/agents/load_raw.py (1)
79-87: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
split_qualified_namefor schema extraction.Naive
split(".", maxsplit=1)diverges from the parser used byPostgresTargetConfig.tablevalidation /qualified_identifier(e.g. quoted identifiers containing dots would yield a bogus schema). Deriving the schema throughloafer.core.identifiers.split_qualified_namekeeps staging names consistent with how the target name is validated and quoted.♻️ Suggested change
target_table = getattr(target_config, "table", None) - if isinstance(target_table, str) and "." in target_table: - schema = target_table.split(".", maxsplit=1)[0] - return f"{schema}.{object_name}" + if isinstance(target_table, str): + from loafer.core.identifiers import split_qualified_name + + schema, _ = split_qualified_name(target_table) + if schema: + return f"{schema}.{object_name}" return object_name🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@loafer/agents/load_raw.py` around lines 79 - 87, Update the staging table name generation logic around the unique-name helper to derive the schema with loafer.core.identifiers.split_qualified_name instead of target_table.split. Preserve the existing fallback for unqualified or unavailable table names, and use the parser’s validated schema component when constructing the qualified staging name.loafer/core/sandbox.py (1)
105-116: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueOptional hardening: give the worker a minimal environment.
The worker inherits the parent's full environment, including provider API keys. Passing a trimmed
env(justPATH/PYTHONPATH/PYTHONHASHSEEDand whatever import resolution needs) shrinks the blast radius if code ever reachesos.environ.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@loafer/core/sandbox.py` around lines 105 - 116, Harden the worker startup in the subprocess.Popen call by supplying a minimal environment instead of inheriting the parent environment. Preserve only the variables required for executable and module resolution, such as PATH, PYTHONPATH, and PYTHONHASHSEED, while excluding provider credentials and unrelated secrets; keep the existing worker command and OSError-to-TransformError handling unchanged.loafer/llm/claude.py (1)
89-105: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBroaden the thinking-disable guard. The exact-equality check only disables thinking for
claude-sonnet-5; the Claude pipelines pinned toclaude-opus-5fall through to the branch that omitsthinking={"type":"disabled"}, so adaptive thinking can still consume output budget. Match the Claude 5 family here, or try disabling thinking where supported and fall back on the API error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@loafer/llm/claude.py` around lines 89 - 105, Update the model guard in _call so thinking is disabled for all Claude 5 models, including claude-opus-5, rather than only DEFAULT_CLAUDE_MODEL. Preserve the existing request parameters and non-Claude-5 behavior.loafer/adapters/targets/postgres.py (1)
83-101: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNarrow
except psycopg2.Errordoesn't cover identifier-composition failures.
qualified_identifier(self._table)/column_list(self._key)(which can raiseValueErrorviasplit_qualified_name) run inside the sametryblock that only catchespsycopg2.Error. If an invalid table/key ever reaches this connector (it performs no validation itself, unlikePostgresTargetConfig), the resultingValueErrorbypasses the intendedLoadErrorwrapping.🛡️ Widen the except clause
- except psycopg2.Error as exc: + except (psycopg2.Error, ValueError) as exc: self._conn.rollback()Also applies to: 209-230
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@loafer/adapters/targets/postgres.py` around lines 83 - 101, Widen the exception handling in _apply_write_mode and the corresponding write path around column_list(self._key) to catch identifier-composition ValueError alongside psycopg2.Error. Preserve rollback behavior and wrap either failure in LoadError, while retaining the existing error context and exception chaining.loafer/config.py (1)
39-58: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winScope relative-path resolution to known path fields, not any string.
_walk_and_resolverewrites any string in the parsed YAML that starts with./,../, or.into an absolute path — regardless of which config key it lives under. Aname:, SQLquery:, orinstruction:value that happens to start with./would be silently mangled into a filesystem path.♻️ Scope resolution to path-bearing keys
-def _walk_and_resolve(obj: Any, base_dir: Path | None = None) -> Any: +_PATH_KEYS = {"path", "custom_path"} + + +def _walk_and_resolve(obj: Any, base_dir: Path | None = None, key: str | None = None) -> Any: if isinstance(obj, str): value = _resolve_env_vars(obj) - if base_dir is not None: + if base_dir is not None and key in _PATH_KEYS: raw = value.lstrip() if raw.startswith("./") or raw.startswith("../") or raw == ".": resolved = (base_dir / raw).resolve() return str(resolved) return value if isinstance(obj, dict): - return {k: _walk_and_resolve(v, base_dir) for k, v in obj.items()} + return {k: _walk_and_resolve(v, base_dir, key=k) for k, v in obj.items()} if isinstance(obj, list): - return [_walk_and_resolve(item, base_dir) for item in obj] + return [_walk_and_resolve(item, base_dir, key=key) for item in obj] return obj🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@loafer/config.py` around lines 39 - 58, Restrict relative-path conversion in _walk_and_resolve to values under known path-bearing configuration keys instead of applying it to every string. Propagate the current key or path-field context while traversing dictionaries, and only resolve ./, ../, or . values when that context identifies a supported path field; leave names, queries, instructions, and other strings unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/AI_TRANSFORM_TEST_REPORT.md`:
- Line 117: Update the fenced output blocks at the affected locations in
AI_TRANSFORM_TEST_REPORT.md to include a language tag on each opening fence,
using text or another appropriate language consistently.
In `@examples/ai_transform_tests.py`:
- Around line 223-245: Update the docstrings in examples/ai_transform_tests.py
at lines 223-245 and 314-341: revise
schema_sample_statistics_describe_the_whole_dataset to state that schema
statistics cover the full dataset while sample_values remain bounded, and revise
the generate_transform_function test docstring to describe verifying that
custom_code reaches the provider, removing the obsolete claim that the parameter
is absent.
- Around line 1207-1215: Update validate_accepts_every_example_pipeline to
restore the original LOAFER_PG_URL environment value after loading
07_ai_elt_postgres.yaml when it was initially unset. Remove the placeholder
afterward, while preserving any pre-existing value and the current pipeline
validation flow.
- Around line 1018-1047: Protect the destructive reset in _reset_pg by requiring
an explicit safety condition before executing DROP SCHEMA, such as a
test-database name pattern or LOAFER_ALLOW_DESTRUCTIVE_TESTS=1. Validate the
parsed connection target or override in _reset_pg (and fail clearly when it is
unsafe), while preserving normal test behavior for approved databases.
In `@examples/README.md`:
- Line 7: Update the fenced directory-listing block in the examples README to
specify the text language tag, changing the opening fence to ```text while
preserving the listing contents and closing fence.
In `@loafer/adapters/targets/atomic_file.py`:
- Around line 13-32: Update open_temporary_text to set the temporary file’s
permissions according to the process umask before returning it, using the open
handle’s file-descriptor permission operation; preserve the existing
temporary-file creation and atomic publishing behavior.
In `@loafer/cli.py`:
- Around line 337-339: Normalize each token before checking provider prefixes in
the model-detection logic near the existing model assignment: strip surrounding
punctuation or quotes first, then apply the gemini/claude/gpt/qwen prefix test
and retain the normalized value. Add a regression case in test_llm_models.py
covering a quoted model such as 'gpt-retired' and verifying the
provider-specific recommendation is produced.
In `@loafer/core/sandbox.py`:
- Around line 56-79: Update the worker response protocol in _worker_main so
stdout never contains pickle data that the parent must unpickle, including
values returned by _run_transform and exception text. Replace the pickle-based
response with a data-only serialization format already supported by the parent,
or coordinate a restricted unpickler there if arbitrary row values must be
preserved; ensure both success and error responses remain distinguishable and
compatible.
In `@PRODUCTION_READINESS.md`:
- Around line 56-72: Update both occurrences of the unit-test count in
PRODUCTION_READINESS.md—the “Verified strengths” entry and the repeated
validation summary—to the confirmed final count of 626 passing tests, preserving
the reported skip count if still accurate.
---
Nitpick comments:
In `@loafer/adapters/targets/postgres.py`:
- Around line 83-101: Widen the exception handling in _apply_write_mode and the
corresponding write path around column_list(self._key) to catch
identifier-composition ValueError alongside psycopg2.Error. Preserve rollback
behavior and wrap either failure in LoadError, while retaining the existing
error context and exception chaining.
In `@loafer/agents/load_raw.py`:
- Around line 79-87: Update the staging table name generation logic around the
unique-name helper to derive the schema with
loafer.core.identifiers.split_qualified_name instead of target_table.split.
Preserve the existing fallback for unqualified or unavailable table names, and
use the parser’s validated schema component when constructing the qualified
staging name.
In `@loafer/config.py`:
- Around line 39-58: Restrict relative-path conversion in _walk_and_resolve to
values under known path-bearing configuration keys instead of applying it to
every string. Propagate the current key or path-field context while traversing
dictionaries, and only resolve ./, ../, or . values when that context identifies
a supported path field; leave names, queries, instructions, and other strings
unchanged.
In `@loafer/core/sandbox.py`:
- Around line 105-116: Harden the worker startup in the subprocess.Popen call by
supplying a minimal environment instead of inheriting the parent environment.
Preserve only the variables required for executable and module resolution, such
as PATH, PYTHONPATH, and PYTHONHASHSEED, while excluding provider credentials
and unrelated secrets; keep the existing worker command and
OSError-to-TransformError handling unchanged.
In `@loafer/llm/claude.py`:
- Around line 89-105: Update the model guard in _call so thinking is disabled
for all Claude 5 models, including claude-opus-5, rather than only
DEFAULT_CLAUDE_MODEL. Preserve the existing request parameters and non-Claude-5
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ac186d7f-9c93-41f3-95a6-a0a68ec4d3db
⛔ Files ignored due to path filters (3)
examples/data/orders.csvis excluded by!**/*.csvexamples/output/inventory_flat.csvis excluded by!**/*.csvexamples/output/orders_clean.csvis excluded by!**/*.csv
📒 Files selected for processing (93)
.env.example.gitignoreCHANGELOG.mdPRODUCTION_READINESS.mdREADME.mdbenchmarks/__init__.pybenchmarks/full_pipeline.pyexamples/AI_TRANSFORM_TEST_REPORT.mdexamples/README.mdexamples/ai_transform_tests.pyexamples/elt_api_to_postgres.yamlexamples/etl_postgres_to_csv.yamlexamples/output/output.jsonexamples/output/users_clean.jsonexamples/pipeline.example.yamlexamples/pipeline.quickstart.yamlexamples/pipelines/01_ai_basic.yamlexamples/pipelines/02_ai_after_custom.yamlexamples/pipelines/03_ai_before_custom.yamlexamples/pipelines/04_bypass_ai.yamlexamples/pipelines/05_ai_review.yamlexamples/pipelines/06_multi_step_ai.yamlexamples/pipelines/07_ai_elt_postgres.yamlexamples/pipelines/08_ai_incremental.yamlexamples/pipelines/09_ai_sandbox_limits.yamlexamples/pipelines/csv_to_postgres.yamlexamples/pipelines/excel_to_csv.yamlexamples/pipelines/incremental_sqlite.yamlexamples/pipelines/mongo_to_csv.yamlexamples/pipelines/multi_step_transform.yamlexamples/pipelines/mysql_to_json.yamlexamples/pipelines/postgres_orders_to_csv.yamlexamples/pipelines/postgres_users_to_json.yamlexamples/pipelines/upsert_postgres.yamlexamples/scripted_llm.pyexamples/transforms/csv_transform.pyexamples/transforms/excel_transform.pyexamples/transforms/mongo_transform.pyexamples/transforms/mysql_transform.pyexamples/transforms/postgres_orders_transform.pyexamples/transforms/postgres_users_transform.pyexamples/transforms/sample_transform.pyexamples/transforms/tag_active.pyexamples/transforms/tag_region.pyloafer/adapters/postgres_sql.pyloafer/adapters/targets/atomic_file.pyloafer/adapters/targets/csv_target.pyloafer/adapters/targets/json_target.pyloafer/adapters/targets/postgres.pyloafer/agents/extract.pyloafer/agents/load_raw.pyloafer/agents/transform_in_target.pyloafer/cli.pyloafer/config.pyloafer/core/identifiers.pyloafer/core/incremental.pyloafer/core/sandbox.pyloafer/llm/claude.pyloafer/llm/gemini.pyloafer/llm/models.pyloafer/llm/openai.pyloafer/llm/qwen.pyloafer/llm/registry.pyloafer/ports/connector.pyloafer/ports/llm.pyloafer/runner.pyloafer/transform/__init__.pyloafer/transform/ai_runner.pyloafer/transform/sql_runner.pyskills/loafer-engineering/references/architecture.mdtests/postgres_sql.pytests/unit/agents/test_extract_agent.pytests/unit/agents/test_load_raw.pytests/unit/agents/test_transform_agent.pytests/unit/agents/test_transform_in_target.pytests/unit/connectors/test_csv_target.pytests/unit/connectors/test_json_target.pytests/unit/connectors/test_postgres_target.pytests/unit/test_config.pytests/unit/test_full_pipeline_benchmark.pytests/unit/test_gemini.pytests/unit/test_incremental.pytests/unit/test_llm_models.pytests/unit/test_llm_providers.pytests/unit/test_multi_llm_providers.pytests/unit/test_runner_streaming.pytests/unit/test_sandbox.pytests/unit/test_upsert.pyweb/public/search-index.jsonweb/src/components/home/DemoVideo.tsxweb/src/content/docs/llms.mdxweb/src/content/docs/transform.mdxweb/src/screens/Changelog.tsx
💤 Files with no reviewable changes (21)
- examples/pipeline.quickstart.yaml
- examples/pipelines/csv_to_postgres.yaml
- examples/transforms/tag_active.py
- examples/pipelines/incremental_sqlite.yaml
- examples/transforms/mysql_transform.py
- examples/pipeline.example.yaml
- examples/pipelines/multi_step_transform.yaml
- examples/pipelines/mysql_to_json.yaml
- examples/transforms/csv_transform.py
- examples/transforms/mongo_transform.py
- examples/pipelines/upsert_postgres.yaml
- examples/pipelines/excel_to_csv.yaml
- examples/transforms/postgres_users_transform.py
- examples/elt_api_to_postgres.yaml
- examples/output/output.json
- examples/pipelines/mongo_to_csv.yaml
- examples/pipelines/postgres_orders_to_csv.yaml
- examples/transforms/excel_transform.py
- examples/pipelines/postgres_users_to_json.yaml
- examples/transforms/postgres_orders_transform.py
- examples/etl_postgres_to_csv.yaml
|
|
||
| The first full pytest run reported **2 failures**: | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add language tags to the fenced output blocks.
markdownlint reports MD040 at Lines 117, 143, and 160. Use text (or another appropriate language) on each opening fence.
Proposed fix
-```
+```textAlso applies to: 143-143, 160-160
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 117-117: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/AI_TRANSFORM_TEST_REPORT.md` at line 117, Update the fenced output
blocks at the affected locations in AI_TRANSFORM_TEST_REPORT.md to include a
language tag on each opening fence, using text or another appropriate language
consistently.
Source: Linters/SAST tools
| def schema_sample_statistics_describe_the_whole_dataset() -> str: | ||
| """total_count/nullable are presented to the model as dataset-wide facts. | ||
|
|
||
| They are computed from ``raw_data[:5]``, so a column that is only ever null | ||
| later in the file is advertised as non-nullable and the model writes code | ||
| with no None guard — which then blows up on the real rows. | ||
| """ | ||
| from loafer.llm.schema import build_schema_sample | ||
|
|
||
| rows = [{"a": 1, "b": "x"} for _ in range(8)] | ||
| rows.append({"a": 9, "b": None}) # 9th row: the only null | ||
|
|
||
| # Metadata is computed across all rows; only sample_values are bounded. | ||
| sent = build_schema_sample(rows, max_sample_rows=5) | ||
| expect( | ||
| sent["b"]["total_count"] == len(rows), | ||
| f"model is told total_count={sent['b']['total_count']} for a {len(rows)}-row dataset", | ||
| ) | ||
| expect( | ||
| sent["b"]["nullable"] is True, | ||
| "column 'b' contains a null but is advertised to the model as non-nullable", | ||
| ) | ||
| return "schema statistics reflect the full dataset" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Two test docstrings still describe defects this PR fixed. Both were written while the bug was live and assert the opposite of what the test body now checks, so a reader can no longer tell what is being guarded.
examples/ai_transform_tests.py#L223-L245: drop the "computed fromraw_data[:5]… blows up on the real rows" narrative and state that schema statistics must reflect the whole dataset while onlysample_valuesis bounded.examples/ai_transform_tests.py#L314-L341: remove the claim thatgenerate_transform_functionhas nocustom_codeparameter —loafer/ports/llm.pyLine 53 now defines it — and describe the test as guarding that the custom code reaches the provider.
📍 Affects 1 file
examples/ai_transform_tests.py#L223-L245(this comment)examples/ai_transform_tests.py#L314-L341
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/ai_transform_tests.py` around lines 223 - 245, Update the docstrings
in examples/ai_transform_tests.py at lines 223-245 and 314-341: revise
schema_sample_statistics_describe_the_whole_dataset to state that schema
statistics cover the full dataset while sample_values remain bounded, and revise
the generate_transform_function test docstring to describe verifying that
custom_code reaches the provider, removing the obsolete claim that the parameter
is absent.
| def _require_postgres() -> str: | ||
| url = os.environ.get(PG_URL_ENV) | ||
| if not url: | ||
| raise Skip(f"set {PG_URL_ENV} (and pass --with-postgres) to run ELT tests") | ||
| try: | ||
| import psycopg2 | ||
|
|
||
| psycopg2.connect(url).close() | ||
| except Exception as exc: | ||
| raise Skip(f"cannot reach Postgres at {PG_URL_ENV}: {exc}") from exc | ||
| return url | ||
|
|
||
|
|
||
| def _perfect_elt_sql(call: Any) -> str: | ||
| """A model that does exactly what Loafer's prompt asked for. | ||
|
|
||
| ``build_elt_sql_prompt`` names the staging table, so a correct model | ||
| selects from ``call.raw_table_name``. Any failure from here on is | ||
| Loafer's, not the model's. | ||
| """ | ||
| return scripted_llm.ELT_SELECT_PAID.format(table=call.raw_table_name).strip() | ||
|
|
||
|
|
||
| def _reset_pg(url: str) -> None: | ||
| import psycopg2 | ||
|
|
||
| with psycopg2.connect(url) as conn: | ||
| conn.autocommit = True | ||
| with conn.cursor() as cur: | ||
| cur.execute("DROP SCHEMA public CASCADE; CREATE SCHEMA public;") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
_reset_pg will drop the public schema of whatever LOAFER_PG_URL points at.
_require_postgres only verifies connectivity, so a developer who has a staging/production URL exported loses that schema on the first ELT test. Add a guard (e.g. require the database name to match a test pattern, or an explicit LOAFER_ALLOW_DESTRUCTIVE_TESTS=1).
🛡️ Proposed guard
def _require_postgres() -> str:
url = os.environ.get(PG_URL_ENV)
if not url:
raise Skip(f"set {PG_URL_ENV} (and pass --with-postgres) to run ELT tests")
+ dbname = url.rsplit("/", 1)[-1].split("?")[0]
+ if "test" not in dbname and os.environ.get("LOAFER_ALLOW_DESTRUCTIVE_TESTS") != "1":
+ raise Skip(
+ f"refusing to reset schema in database {dbname!r}; point {PG_URL_ENV} at a "
+ "throwaway *test* database or set LOAFER_ALLOW_DESTRUCTIVE_TESTS=1"
+ )
try:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _require_postgres() -> str: | |
| url = os.environ.get(PG_URL_ENV) | |
| if not url: | |
| raise Skip(f"set {PG_URL_ENV} (and pass --with-postgres) to run ELT tests") | |
| try: | |
| import psycopg2 | |
| psycopg2.connect(url).close() | |
| except Exception as exc: | |
| raise Skip(f"cannot reach Postgres at {PG_URL_ENV}: {exc}") from exc | |
| return url | |
| def _perfect_elt_sql(call: Any) -> str: | |
| """A model that does exactly what Loafer's prompt asked for. | |
| ``build_elt_sql_prompt`` names the staging table, so a correct model | |
| selects from ``call.raw_table_name``. Any failure from here on is | |
| Loafer's, not the model's. | |
| """ | |
| return scripted_llm.ELT_SELECT_PAID.format(table=call.raw_table_name).strip() | |
| def _reset_pg(url: str) -> None: | |
| import psycopg2 | |
| with psycopg2.connect(url) as conn: | |
| conn.autocommit = True | |
| with conn.cursor() as cur: | |
| cur.execute("DROP SCHEMA public CASCADE; CREATE SCHEMA public;") | |
| def _require_postgres() -> str: | |
| url = os.environ.get(PG_URL_ENV) | |
| if not url: | |
| raise Skip(f"set {PG_URL_ENV} (and pass --with-postgres) to run ELT tests") | |
| dbname = url.rsplit("/", 1)[-1].split("?")[0] | |
| if "test" not in dbname and os.environ.get("LOAFER_ALLOW_DESTRUCTIVE_TESTS") != "1": | |
| raise Skip( | |
| f"refusing to reset schema in database {dbname!r}; point {PG_URL_ENV} at a " | |
| "throwaway *test* database or set LOAFER_ALLOW_DESTRUCTIVE_TESTS=1" | |
| ) | |
| try: | |
| import psycopg2 | |
| psycopg2.connect(url).close() | |
| except Exception as exc: | |
| raise Skip(f"cannot reach Postgres at {PG_URL_ENV}: {exc}") from exc | |
| return url | |
| def _perfect_elt_sql(call: Any) -> str: | |
| """A model that does exactly what Loafer's prompt asked for. | |
| ``build_elt_sql_prompt`` names the staging table, so a correct model | |
| selects from ``call.raw_table_name``. Any failure from here on is | |
| Loafer's, not the model's. | |
| """ | |
| return scripted_llm.ELT_SELECT_PAID.format(table=call.raw_table_name).strip() | |
| def _reset_pg(url: str) -> None: | |
| import psycopg2 | |
| with psycopg2.connect(url) as conn: | |
| conn.autocommit = True | |
| with conn.cursor() as cur: | |
| cur.execute("DROP SCHEMA public CASCADE; CREATE SCHEMA public;") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/ai_transform_tests.py` around lines 1018 - 1047, Protect the
destructive reset in _reset_pg by requiring an explicit safety condition before
executing DROP SCHEMA, such as a test-database name pattern or
LOAFER_ALLOW_DESTRUCTIVE_TESTS=1. Validate the parsed connection target or
override in _reset_pg (and fail clearly when it is unsafe), while preserving
normal test behavior for approved databases.
| def validate_accepts_every_example_pipeline() -> str: | ||
| """`loafer validate` must parse every YAML we ship.""" | ||
| checked = [] | ||
| for path in sorted(PIPELINES.glob("*.yaml")): | ||
| if path.name == "07_ai_elt_postgres.yaml" and not os.environ.get(PG_URL_ENV): | ||
| os.environ[PG_URL_ENV] = "postgresql://u:p@localhost:5432/db" | ||
| load_config(path) | ||
| checked.append(path.name) | ||
| return f"{len(checked)} pipelines parsed" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Restore LOAFER_PG_URL instead of leaking a fake value into the process env.
The placeholder is never removed, so any subsequent ELT test sees a bogus URL and skips with a connection error instead of the intended "not set" message.
🧹 Proposed fix
checked = []
- for path in sorted(PIPELINES.glob("*.yaml")):
- if path.name == "07_ai_elt_postgres.yaml" and not os.environ.get(PG_URL_ENV):
- os.environ[PG_URL_ENV] = "postgresql://u:p@localhost:5432/db"
- load_config(path)
- checked.append(path.name)
+ saved = os.environ.get(PG_URL_ENV)
+ try:
+ for path in sorted(PIPELINES.glob("*.yaml")):
+ if not os.environ.get(PG_URL_ENV):
+ os.environ[PG_URL_ENV] = "postgresql://u:p@localhost:5432/db"
+ load_config(path)
+ checked.append(path.name)
+ finally:
+ if saved is None:
+ os.environ.pop(PG_URL_ENV, None)
+ else:
+ os.environ[PG_URL_ENV] = saved
return f"{len(checked)} pipelines parsed"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def validate_accepts_every_example_pipeline() -> str: | |
| """`loafer validate` must parse every YAML we ship.""" | |
| checked = [] | |
| for path in sorted(PIPELINES.glob("*.yaml")): | |
| if path.name == "07_ai_elt_postgres.yaml" and not os.environ.get(PG_URL_ENV): | |
| os.environ[PG_URL_ENV] = "postgresql://u:p@localhost:5432/db" | |
| load_config(path) | |
| checked.append(path.name) | |
| return f"{len(checked)} pipelines parsed" | |
| def validate_accepts_every_example_pipeline() -> str: | |
| """`loafer validate` must parse every YAML we ship.""" | |
| checked = [] | |
| saved = os.environ.get(PG_URL_ENV) | |
| try: | |
| for path in sorted(PIPELINES.glob("*.yaml")): | |
| if not os.environ.get(PG_URL_ENV): | |
| os.environ[PG_URL_ENV] = "postgresql://u:p@localhost:5432/db" | |
| load_config(path) | |
| checked.append(path.name) | |
| finally: | |
| if saved is None: | |
| os.environ.pop(PG_URL_ENV, None) | |
| else: | |
| os.environ[PG_URL_ENV] = saved | |
| return f"{len(checked)} pipelines parsed" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/ai_transform_tests.py` around lines 1207 - 1215, Update
validate_accepts_every_example_pipeline to restore the original LOAFER_PG_URL
environment value after loading 07_ai_elt_postgres.yaml when it was initially
unset. Remove the placeholder afterward, while preserving any pre-existing value
and the current pipeline validation flow.
| transforms. Everything here drives Loafer the way a user does: real YAML, real | ||
| CSV input, real files on disk, real Postgres. | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language tag to the directory-listing fence.
markdownlint-cli2 reports MD040 here; use text for this non-executable tree listing.
Proposed fix
-```
+```text📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/README.md` at line 7, Update the fenced directory-listing block in
the examples README to specify the text language tag, changing the opening fence
to ```text while preserving the listing contents and closing fence.
Source: Linters/SAST tools
| def open_temporary_text( | ||
| destination: Path, | ||
| *, | ||
| newline: str | None = None, | ||
| ) -> tuple[TextIO, Path]: | ||
| """Open a hidden temporary file beside *destination*. | ||
|
|
||
| Keeping both paths on the same filesystem makes the final rename atomic. | ||
| """ | ||
| destination.parent.mkdir(parents=True, exist_ok=True) | ||
| handle = tempfile.NamedTemporaryFile( | ||
| mode="w", | ||
| encoding="utf-8", | ||
| newline=newline, | ||
| prefix=f".{destination.name}.", | ||
| suffix=".tmp", | ||
| dir=destination.parent, | ||
| delete=False, | ||
| ) | ||
| return handle, Path(handle.name) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant files and inspect the helper plus callers.
git ls-files | rg 'loafer/adapters/targets/(atomic_file|.*target.*|.*csv.*|.*json.*)\.py|loafer/adapters/targets/.*\.py$'
printf '\n--- atomic_file.py ---\n'
cat -n loafer/adapters/targets/atomic_file.py
printf '\n--- callers of open_temporary_text / publish_temporary_file ---\n'
rg -n "open_temporary_text|publish_temporary_file|NamedTemporaryFile|os\.link|os\.replace" loafer/adapters/targets -S
printf '\n--- surrounding target files (first matches) ---\n'
for f in $(rg -l "open_temporary_text|publish_temporary_file" loafer/adapters/targets -S); do
printf '\n### %s ###\n' "$f"
cat -n "$f"
doneRepository: lupppig/loafer
Length of output: 14773
🏁 Script executed:
python3 - <<'PY'
import os
import stat
import tempfile
from pathlib import Path
def mode(path):
return stat.S_IMODE(os.stat(path).st_mode)
base = Path(tempfile.mkdtemp(prefix="perm-check-"))
try:
for umask in (0o022, 0o077):
old = os.umask(umask)
try:
temp = tempfile.NamedTemporaryFile(mode="w", delete=False, dir=base)
temp_path = Path(temp.name)
temp.write("x")
temp.close()
dest_replace = base / f"replace-{umask:o}.txt"
os.replace(temp_path, dest_replace)
temp2 = tempfile.NamedTemporaryFile(mode="w", delete=False, dir=base)
temp2_path = Path(temp2.name)
temp2.write("x")
temp2.close()
dest_link = base / f"link-{umask:o}.txt"
os.link(temp2_path, dest_link)
print(f"umask {umask:o}: temp mode={mode(dest_replace):o} after replace, {mode(dest_link):o} after link")
finally:
os.umask(old)
finally:
for p in base.iterdir():
p.unlink()
base.rmdir()
PYRepository: lupppig/loafer
Length of output: 260
Published files stay 0600.
NamedTemporaryFile creates the staging file owner-only, and os.replace/os.link carry that mode onto the final CSV/JSON file. If these outputs should follow the process umask, set the permissions before publishing (for example with fchmod on the temp handle).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@loafer/adapters/targets/atomic_file.py` around lines 13 - 32, Update
open_temporary_text to set the temporary file’s permissions according to the
process umask before returning it, using the open handle’s file-descriptor
permission operation; preserve the existing temporary-file creation and atomic
publishing behavior.
| if part.startswith(("gemini", "claude", "gpt", "qwen")): | ||
| model = part.strip("',.") | ||
| break |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Normalize the model token before prefix detection.
For input such as 404 model 'gpt-retired' was not found, the leading quote prevents startswith() from matching, so the provider-specific recommendation is skipped.
Proposed fix
model = "unknown"
for part in raw.split():
- if part.startswith(("gemini", "claude", "gpt", "qwen")):
- model = part.strip("',.")
+ candidate = part.split("=", 1)[-1].strip("'\".,:()[]{}")
+ if candidate.lower().startswith(("gemini", "claude", "gpt", "qwen")):
+ model = candidate
breakAdd a quoted-model regression case to tests/unit/test_llm_models.py.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if part.startswith(("gemini", "claude", "gpt", "qwen")): | |
| model = part.strip("',.") | |
| break | |
| candidate = part.split("=", 1)[-1].strip("'\".,:()[]{}") | |
| if candidate.lower().startswith(("gemini", "claude", "gpt", "qwen")): | |
| model = candidate | |
| break |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@loafer/cli.py` around lines 337 - 339, Normalize each token before checking
provider prefixes in the model-detection logic near the existing model
assignment: strip surrounding punctuation or quotes first, then apply the
gemini/claude/gpt/qwen prefix test and retain the normalized value. Add a
regression case in test_llm_models.py covering a quoted model such as
'gpt-retired' and verifying the provider-specific recommendation is produced.
|
|
||
| def _worker_main() -> None: | ||
| """Read one request from stdin and write one result to stdout. | ||
|
|
||
| This module entrypoint is intentionally independent of the caller's | ||
| ``__main__`` module. Unlike ``multiprocessing`` with the spawn start method, | ||
| it never re-imports and re-executes an unguarded user script. | ||
| """ | ||
| try: | ||
| result = _run_transform(code, data) | ||
| out.put(("ok", result)) | ||
| request = pickle.loads(sys.stdin.buffer.read()) | ||
| code, data, timeout, max_memory_mb = request | ||
| _apply_resource_limits(timeout, max_memory_mb) | ||
| # Keep transform print() calls away from the binary result protocol. | ||
| with contextlib.redirect_stdout(sys.stderr): | ||
| result = _run_transform(code, data) | ||
| response: tuple[str, Any] = ("ok", result) | ||
| except BaseException as exc: | ||
| out.put(("err", f"{type(exc).__name__}: {exc}")) | ||
| response = ("err", f"{type(exc).__name__}: {exc}") | ||
|
|
||
| try: | ||
| sys.stdout.buffer.write(pickle.dumps(response, protocol=pickle.HIGHEST_PROTOCOL)) | ||
| sys.stdout.buffer.flush() | ||
| except BaseException: | ||
| raise SystemExit(2) from None |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# What types actually flow out of transforms/sandbox into targets?
rg -nP --type=py -C3 '\brun_sandboxed\s*\('
rg -nP --type=py -C3 'json\.dump|default=str|Decimal|datetime' loafer/adapters/targetsRepository: lupppig/loafer
Length of output: 152
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Files ==\n'
git ls-files | rg '^(loafer/core/sandbox\.py|loafer/adapters/targets|loafer/.*sandbox|loafer/.*transform|loafer/.*target)'
printf '\n== sandbox.py outline ==\n'
ast-grep outline loafer/core/sandbox.py --view expanded
printf '\n== sandbox.py relevant lines ==\n'
nl -ba loafer/core/sandbox.py | sed -n '1,220p'
printf '\n== search for sandbox/transform output handling ==\n'
rg -n --hidden --glob '!**/.git/**' '\b(run_sandboxed|pickle\.loads|pickle\.dumps|json\.loads|json\.dumps|default=str|TransformError|datetime|Decimal)\b' loafer
printf '\n== target adapter files ==\n'
find loafer/adapters/targets -maxdepth 2 -type f | sort | while read -r f; do
echo "--- $f"
wc -l "$f"
doneRepository: lupppig/loafer
Length of output: 1154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== sandbox.py ==\n'
sed -n '1,220p' loafer/core/sandbox.py
printf '\n== safe exec / validator ==\n'
sed -n '1,260p' loafer/transform/_safe_exec.py
printf '\n--- code_validator.py ---\n'
sed -n '1,260p' loafer/transform/code_validator.py
printf '\n== target adapters ==\n'
for f in loafer/adapters/targets/*.py; do
echo "--- $f"
sed -n '1,240p' "$f"
doneRepository: lupppig/loafer
Length of output: 31282
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== sandbox.py (first 220 lines) ==\n'
sed -n '1,220p' loafer/core/sandbox.py
printf '\n== _safe_exec.py ==\n'
sed -n '1,260p' loafer/transform/_safe_exec.py
printf '\n== code_validator.py ==\n'
sed -n '1,260p' loafer/transform/code_validator.py
printf '\n== target adapter summaries ==\n'
for f in loafer/adapters/targets/*.py; do
echo "--- $f"
sed -n '1,260p' "$f"
doneRepository: lupppig/loafer
Length of output: 31882
🏁 Script executed:
python3 - <<'PY'
import pickle
SAFE_BUILTINS = {
"len": len, "str": str, "int": int, "float": float, "bool": bool,
"list": list, "dict": dict, "set": set, "tuple": tuple, "range": range,
"enumerate": enumerate, "zip": zip, "map": map, "filter": filter,
"sorted": sorted, "reversed": reversed, "sum": sum, "min": min,
"max": max, "abs": abs, "round": round, "isinstance": isinstance,
"type": type, "None": None, "True": True, "False": False, "print": print,
}
g = {"__builtins__": SAFE_BUILTINS}
# emulate allowed module injections
import re, json, datetime, math, decimal, uuid, itertools
g.update({"re": re, "json": json, "datetime": datetime, "math": math, "decimal": decimal, "uuid": uuid, "itertools": itertools})
code = r'''
def transform(data):
class X:
def __reduce__(self):
obj = type(1).__mro__[1]
subs = obj.__subclasses__()
cw = [c for c in subs if c.__name__ == "catch_warnings"][0]
builtins = cw.__init__.__globals__["__builtins__"]
return (builtins["eval"], ("40 + 2",))
return [{"x": X()}]
'''
exec(code, g)
result = g["transform"]([])
payload = pickle.dumps(("ok", result), protocol=pickle.HIGHEST_PROTOCOL)
print("pickled bytes:", len(payload))
print("unpickled:", pickle.loads(payload))
PYRepository: lupppig/loafer
Length of output: 303
🏁 Script executed:
python3 - <<'PY'
import pickle
SAFE_BUILTINS = {
"len": len, "str": str, "int": int, "float": float, "bool": bool,
"list": list, "dict": dict, "set": set, "tuple": tuple, "range": range,
"enumerate": enumerate, "zip": zip, "map": map, "filter": filter,
"sorted": sorted, "reversed": reversed, "sum": sum, "min": min,
"max": max, "abs": abs, "round": round, "isinstance": isinstance,
"type": type, "None": None, "True": True, "False": False, "print": print,
}
g = {"__builtins__": SAFE_BUILTINS}
import re, json, datetime, math, decimal, uuid, itertools
g.update({"re": re, "json": json, "datetime": datetime, "math": math, "decimal": decimal, "uuid": uuid, "itertools": itertools})
code = r'''
def transform(data):
class X:
def __reduce__(self):
obj = type(1).__mro__[1]
subs = obj.__subclasses__()
cw = [c for c in subs if c.__name__ == "catch_warnings"][0]
builtins = cw.__init__.__globals__["__builtins__"]
return (builtins["eval"], ("40 + 2",))
return [{"x": X()}]
'''
exec(code, g)
result = g["transform"]([])
payload = pickle.dumps(("ok", result), protocol=pickle.HIGHEST_PROTOCOL)
print("pickled bytes:", len(payload))
print("unpickled:", pickle.loads(payload))
PYRepository: lupppig/loafer
Length of output: 303
🏁 Script executed:
python3 - <<'PY'
import pickle, builtins, re, json, datetime, math, decimal, uuid, itertools
SAFE_BUILTINS = {
"len": len, "str": str, "int": int, "float": float, "bool": bool,
"list": list, "dict": dict, "set": set, "tuple": tuple, "range": range,
"enumerate": enumerate, "zip": zip, "map": map, "filter": filter,
"sorted": sorted, "reversed": reversed, "sum": sum, "min": min,
"max": max, "abs": abs, "round": round, "isinstance": isinstance,
"type": type, "None": None, "True": True, "False": False, "print": print,
}
g = {"__builtins__": SAFE_BUILTINS, "re": re, "json": json, "datetime": datetime,
"math": math, "decimal": decimal, "uuid": uuid, "itertools": itertools}
code = r'''
def transform(data):
O = type(1).__mro__[1]
X = type("X", (O,), {
"__reduce__": lambda self: (int, ("7",)),
})
return [{"x": X()}]
'''
exec(code, g)
result = g["transform"]([])
print("result type:", type(result[0]["x"]).__name__)
payload = pickle.dumps(("ok", result), protocol=pickle.HIGHEST_PROTOCOL)
print("pickle ok, bytes:", len(payload))
print("unpickled:", pickle.loads(payload))
PYRepository: lupppig/loafer
Length of output: 218
Avoid unpickling worker output
The worker runs untrusted transform code, and the parent still pickle.loads(stdout). Since type() is allowed, a transform can build a runtime class with a custom __reduce__ and turn the response into a pickle gadget that executes in the parent during deserialization. Return a data-only format instead, or use a restricted unpickler if arbitrary row values must be preserved.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 64-64: pickle.load/loads executes arbitrary code when the data is untrusted (a model file, cache, or request payload). Use a safe format like JSON, or only unpickle data from a trusted, integrity-checked source.
Context: pickle.loads(sys.stdin.buffer.read())
Note: [CWE-502] Deserialization of Untrusted Data.
(pickle-deserialization-python)
🪛 OpenGrep (1.26.0)
[ERROR] 65-65: pickle.load/loads deserializes arbitrary Python objects and can execute arbitrary code. Use a safe format like JSON instead.
(coderabbit.deserialization.python-pickle)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@loafer/core/sandbox.py` around lines 56 - 79, Update the worker response
protocol in _worker_main so stdout never contains pickle data that the parent
must unpickle, including values returned by _run_transform and exception text.
Replace the pickle-based response with a data-only serialization format already
supported by the parent, or coordinate a restricted unpickler there if arbitrary
row values must be preserved; ensure both success and error responses remain
distinguishable and compatible.
Source: Linters/SAST tools
| ## Verified strengths | ||
|
|
||
| - The unit suite contains 578 passing tests with 8 skips in the current working tree. | ||
| - The unit suite contains 614 passing tests with 8 skips in the current working tree. | ||
| - Clean-room wheel and production Docker-image smoke tests execute version/help, connector | ||
| discovery, configuration validation, and a real pipeline successfully. | ||
| - ETL and ELT are modeled as separate graphs. | ||
| - Source adapters expose a chunk iterator and database sources use streaming cursors. | ||
| - LLM prompts receive a schema sample rather than a full dataset. | ||
| - Multiple LLM providers implement a common interface. | ||
| - Custom and generated Python run in a separate process with CPU/memory/time limits on POSIX. | ||
| - Incremental extraction advances its cursor after a completed run. | ||
| - PostgreSQL and MongoDB support upsert modes. | ||
| - CSV and JSON targets publish same-directory temporary files atomically after successful | ||
| finalization and preserve the previous output on failure. | ||
| - PostgreSQL target and ELT identifiers use driver-native composition; incremental cursor | ||
| identifiers escape their engine-specific quote character. | ||
| - The CLI exposes stage-aware Rich output and human-readable errors. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Test count likely stale vs. reported validation numbers.
This section states "The unit suite contains 614 passing tests with 8 skips in the current working tree." The PR's validation summary reports 626 unit tests passed. The same "614 unit tests pass" figure is repeated at Line 310. Please confirm the final count and update both occurrences before this doc is treated as the release baseline.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@PRODUCTION_READINESS.md` around lines 56 - 72, Update both occurrences of the
unit-test count in PRODUCTION_READINESS.md—the “Verified strengths” entry and
the repeated validation summary—to the confirmed final count of 626 passing
tests, preserving the reported skip count if still accurate.
Summary
run_pipeline()callsWhy
The previous runtime could write ELT raw rows into the final table, report exhausted SQL retries as success, omit providers for nested AI steps, discard custom transform context, retry permanent provider failures, and recursively re-import unguarded programmatic callers through multiprocessing spawn. File targets could also expose partial output on failed runs.
This release makes these failure modes explicit, isolated, recoverable, and regression-tested.
User impact
if __name__ == "__main__"guard for sandboxed transformsValidation
uv run ruff check .uv run ruff format --check .uv run pytest tests/unit/ -q— 626 passed, 8 skippedRelease
The changelog is finalized for
v0.4.0. Publishing the GitHub release after merge will trigger the existing PyPI and GHCR workflows.Summary by CodeRabbit
New Features
Bug Fixes
Documentation