Skip to content

Harden AI and ELT pipeline execution for v0.4.0 - #14

Merged
lupppig merged 1 commit into
mainfrom
fix/ai-etl-runtime-hardening
Jul 29, 2026
Merged

Harden AI and ELT pipeline execution for v0.4.0#14
lupppig merged 1 commit into
mainfrom
fix/ai-etl-runtime-hardening

Conversation

@lupppig

@lupppig lupppig commented Jul 29, 2026

Copy link
Copy Markdown
Owner

Summary

  • isolate ELT raw loads in schema-local staging tables and propagate terminal failures correctly
  • harden AI transforms with custom-code prompt context, execution retries, permanent-error classification, current provider defaults, and nested pipeline provider setup
  • add client-side incremental filtering for non-query sources and full-dataset schema statistics
  • publish CSV/JSON output atomically and safely compose PostgreSQL identifiers
  • replace the POSIX multiprocessing spawn sandbox with an internal worker that supports unguarded programmatic run_pipeline() calls
  • replace generated example artifacts with a deterministic 45-case AI/ELT suite, benchmark harness, production-readiness guidance, and concise release documentation

Why

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

  • failed ELT jobs now exit non-zero and do not leave raw data in the final target
  • AI transforms self-correct runtime failures and fail fast on permanent 4xx responses
  • CSV, Excel, MongoDB, and PDF incremental runs honor their cursor with an explicit full-scan warning
  • CSV and JSON targets publish complete files atomically
  • programmatic callers no longer require an if __name__ == "__main__" guard for sandboxed transforms
  • provider defaults are updated for the v0.4.0 release

Validation

  • uv run ruff check .
  • uv run ruff format --check .
  • uv run pytest tests/unit/ -q — 626 passed, 8 skipped
  • full repository suite in isolated writable HOME — 673 passed, 50 skipped
  • deterministic AI/ELT suite — 45 passed with PostgreSQL
  • live OpenAI acceptance — 30 passed, 15 structural skips

Release

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

    • Added AI transform examples, including multi-step workflows, review gates, sandbox limits, incremental processing, and PostgreSQL output.
    • Added support for provider-specific default models across Gemini, Claude, OpenAI, and Qwen.
    • Added offline AI transform testing and full-pipeline benchmarking tools.
  • Bug Fixes

    • Improved incremental filtering and cursor advancement across more source types.
    • Prevented partial CSV/JSON files and protected existing outputs during failures or write races.
    • Strengthened PostgreSQL identifier handling, ELT transactions, cleanup, and error reporting.
  • Documentation

    • Updated setup guidance, examples, release notes, benchmarks, and production-readiness documentation.

@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This 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.

Changes

Implementation and validation

Layer / File(s) Summary
Benchmark harness and readiness evidence
benchmarks/*, README.md, PRODUCTION_READINESS.md, CHANGELOG.md
Adds a monitored CSV-to-Loafer benchmark with output verification, resource limits, reports, and readiness documentation.
AI-transform examples and test suite
examples/*, tests/unit/test_sandbox.py
Adds nine example pipelines, a scripted provider, custom transforms, and broad offline/live coverage for safety, retries, ELT, review, and CLI behavior.
Provider contracts and configuration
loafer/config.py, loafer/llm/*, loafer/cli.py
Centralizes provider defaults, supports environment interpolation, propagates custom transform code, updates provider requests, and generates provider-aware model guidance.
Incremental, sandbox, and runner execution
loafer/core/*, loafer/agents/*, loafer/transform/*, loafer/runner.py
Adds client-side incremental filtering and cursor advancement, subprocess sandbox isolation, retry classification, nested AI-provider detection, and ELT cleanup/failure signaling.
PostgreSQL safety and ELT transactions
loafer/adapters/postgres_sql.py, loafer/adapters/targets/postgres.py, loafer/agents/transform_in_target.py
Uses composed PostgreSQL identifiers and explicit transactions for target, upsert, and ELT SQL operations, including schema-qualified staging tables.
Atomic local-file publication
loafer/adapters/targets/atomic_file.py, csv_target.py, json_target.py
Writes CSV and JSON outputs to synchronized same-directory temporary files and publishes or discards them according to pipeline completion and write mode.

Estimated code review effort: 5 (Critical) | ~120 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 58.29% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately captures the main theme: hardening AI and ELT execution for the v0.4.0 release.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/ai-etl-runtime-hardening

Comment @coderabbitai help to get the list of available commands.

@lupppig
lupppig merged commit 911f8bd into main Jul 29, 2026
3 of 4 checks passed
@lupppig
lupppig deleted the fix/ai-etl-runtime-hardening branch July 29, 2026 07:09

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 9

🧹 Nitpick comments (5)
loafer/agents/load_raw.py (1)

79-87: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Reuse split_qualified_name for schema extraction.

Naive split(".", maxsplit=1) diverges from the parser used by PostgresTargetConfig.table validation / qualified_identifier (e.g. quoted identifiers containing dots would yield a bogus schema). Deriving the schema through loafer.core.identifiers.split_qualified_name keeps 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 value

Optional hardening: give the worker a minimal environment.

The worker inherits the parent's full environment, including provider API keys. Passing a trimmed env (just PATH/PYTHONPATH/PYTHONHASHSEED and whatever import resolution needs) shrinks the blast radius if code ever reaches os.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 win

Broaden the thinking-disable guard. The exact-equality check only disables thinking for claude-sonnet-5; the Claude pipelines pinned to claude-opus-5 fall through to the branch that omits thinking={"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 win

Narrow except psycopg2.Error doesn't cover identifier-composition failures.

qualified_identifier(self._table) / column_list(self._key) (which can raise ValueError via split_qualified_name) run inside the same try block that only catches psycopg2.Error. If an invalid table/key ever reaches this connector (it performs no validation itself, unlike PostgresTargetConfig), the resulting ValueError bypasses the intended LoadError wrapping.

🛡️ 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 win

Scope relative-path resolution to known path fields, not any string.

_walk_and_resolve rewrites any string in the parsed YAML that starts with ./, ../, or . into an absolute path — regardless of which config key it lives under. A name:, SQL query:, or instruction: 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

📥 Commits

Reviewing files that changed from the base of the PR and between 4021c66 and b300747.

⛔ Files ignored due to path filters (3)
  • examples/data/orders.csv is excluded by !**/*.csv
  • examples/output/inventory_flat.csv is excluded by !**/*.csv
  • examples/output/orders_clean.csv is excluded by !**/*.csv
📒 Files selected for processing (93)
  • .env.example
  • .gitignore
  • CHANGELOG.md
  • PRODUCTION_READINESS.md
  • README.md
  • benchmarks/__init__.py
  • benchmarks/full_pipeline.py
  • examples/AI_TRANSFORM_TEST_REPORT.md
  • examples/README.md
  • examples/ai_transform_tests.py
  • examples/elt_api_to_postgres.yaml
  • examples/etl_postgres_to_csv.yaml
  • examples/output/output.json
  • examples/output/users_clean.json
  • examples/pipeline.example.yaml
  • examples/pipeline.quickstart.yaml
  • examples/pipelines/01_ai_basic.yaml
  • examples/pipelines/02_ai_after_custom.yaml
  • examples/pipelines/03_ai_before_custom.yaml
  • examples/pipelines/04_bypass_ai.yaml
  • examples/pipelines/05_ai_review.yaml
  • examples/pipelines/06_multi_step_ai.yaml
  • examples/pipelines/07_ai_elt_postgres.yaml
  • examples/pipelines/08_ai_incremental.yaml
  • examples/pipelines/09_ai_sandbox_limits.yaml
  • examples/pipelines/csv_to_postgres.yaml
  • examples/pipelines/excel_to_csv.yaml
  • examples/pipelines/incremental_sqlite.yaml
  • examples/pipelines/mongo_to_csv.yaml
  • examples/pipelines/multi_step_transform.yaml
  • examples/pipelines/mysql_to_json.yaml
  • examples/pipelines/postgres_orders_to_csv.yaml
  • examples/pipelines/postgres_users_to_json.yaml
  • examples/pipelines/upsert_postgres.yaml
  • examples/scripted_llm.py
  • examples/transforms/csv_transform.py
  • examples/transforms/excel_transform.py
  • examples/transforms/mongo_transform.py
  • examples/transforms/mysql_transform.py
  • examples/transforms/postgres_orders_transform.py
  • examples/transforms/postgres_users_transform.py
  • examples/transforms/sample_transform.py
  • examples/transforms/tag_active.py
  • examples/transforms/tag_region.py
  • loafer/adapters/postgres_sql.py
  • loafer/adapters/targets/atomic_file.py
  • loafer/adapters/targets/csv_target.py
  • loafer/adapters/targets/json_target.py
  • loafer/adapters/targets/postgres.py
  • loafer/agents/extract.py
  • loafer/agents/load_raw.py
  • loafer/agents/transform_in_target.py
  • loafer/cli.py
  • loafer/config.py
  • loafer/core/identifiers.py
  • loafer/core/incremental.py
  • loafer/core/sandbox.py
  • loafer/llm/claude.py
  • loafer/llm/gemini.py
  • loafer/llm/models.py
  • loafer/llm/openai.py
  • loafer/llm/qwen.py
  • loafer/llm/registry.py
  • loafer/ports/connector.py
  • loafer/ports/llm.py
  • loafer/runner.py
  • loafer/transform/__init__.py
  • loafer/transform/ai_runner.py
  • loafer/transform/sql_runner.py
  • skills/loafer-engineering/references/architecture.md
  • tests/postgres_sql.py
  • tests/unit/agents/test_extract_agent.py
  • tests/unit/agents/test_load_raw.py
  • tests/unit/agents/test_transform_agent.py
  • tests/unit/agents/test_transform_in_target.py
  • tests/unit/connectors/test_csv_target.py
  • tests/unit/connectors/test_json_target.py
  • tests/unit/connectors/test_postgres_target.py
  • tests/unit/test_config.py
  • tests/unit/test_full_pipeline_benchmark.py
  • tests/unit/test_gemini.py
  • tests/unit/test_incremental.py
  • tests/unit/test_llm_models.py
  • tests/unit/test_llm_providers.py
  • tests/unit/test_multi_llm_providers.py
  • tests/unit/test_runner_streaming.py
  • tests/unit/test_sandbox.py
  • tests/unit/test_upsert.py
  • web/public/search-index.json
  • web/src/components/home/DemoVideo.tsx
  • web/src/content/docs/llms.mdx
  • web/src/content/docs/transform.mdx
  • web/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**:

```

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 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
-```
+```text

Also 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

Comment on lines +223 to +245
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"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 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 from raw_data[:5] … blows up on the real rows" narrative and state that schema statistics must reflect the whole dataset while only sample_values is bounded.
  • examples/ai_transform_tests.py#L314-L341: remove the claim that generate_transform_function has no custom_code parameter — loafer/ports/llm.py Line 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.

Comment on lines +1018 to +1047
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;")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🗄️ 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.

Suggested change
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.

Comment on lines +1207 to +1215
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"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 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.

Suggested change
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.

Comment thread examples/README.md
transforms. Everything here drives Loafer the way a user does: real YAML, real
CSV input, real files on disk, real Postgres.

```

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 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.

Suggested change
```
🧰 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

Comment on lines +13 to +32
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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🗄️ 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"
done

Repository: 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()
PY

Repository: 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.

Comment thread loafer/cli.py
Comment on lines +337 to 339
if part.startswith(("gemini", "claude", "gpt", "qwen")):
model = part.strip("',.")
break

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 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
             break

Add 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.

Suggested change
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.

Comment thread loafer/core/sandbox.py
Comment on lines +56 to +79

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔒 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/targets

Repository: 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"
done

Repository: 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"
done

Repository: 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"
done

Repository: 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))
PY

Repository: 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))
PY

Repository: 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))
PY

Repository: 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

Comment thread PRODUCTION_READINESS.md
Comment on lines 56 to 72
## 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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 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.

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.

1 participant