Repository navigation
Gap remediation pass O-01..O-12 and ADR-0001 - #44
Merged
Merged
Conversation
added 6 commits
September 10, 2026 13:39
…ad audit An unset API_KEY previously left every router open behind nothing but a startup warning, which is easy to miss in a container log. api/main.py now calls assert_auth_configured() and refuses to boot when API_ENV is anything other than 'dev' and no key is set. The guard is a pure function so it can be unit-tested without opening a connection pool. Rate limiting is a hand-rolled sliding window rather than slowapi: the API runs as a single instance behind an external_enabled=false boundary, so a Redis-backed limiter would add a dependency and a network hop for no extra protection, and avoiding the package keeps the air-gapped install path simple. Replace it if this ever scales past one instance -- per-instance counters stop being a global limit. Writes get a tighter budget than reads because an upload costs far more server work than a SELECT. X-Forwarded-For is deliberately ignored, since trusting a client-supplied header would let anyone mint a fresh bucket on demand. csv_uploads.audit_log had exactly one writer (delete_file), so it recorded destructions but never creations -- an auditor could see that a table had been dropped and never see who loaded it. Uploads now write through a shared writer in api/services/audit.py. Known limitation, documented in that module: the loaders own their own transactions, so an upload's audit row lands after the load has already committed. A failed audit write therefore logs at ERROR rather than failing a load that actually succeeded; reporting failure for committed data would be worse than a missing log line. tests/conftest.py disables both budgets for the suite, mirroring how it already bypasses require_api_key. Left enabled, the write budget would start returning 429 partway through upload-heavy modules and fail tests for reasons unrelated to what they assert. api/auth.py's docstring claimed it was applied globally in api/main.py. Enforcement was real but per-router; only the documentation was wrong. Corrected. 24 tests in tests/test_api_hardening.py cover both concerns, which is why they land in one commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1v7ow7NxH9y1ELUyGgACn
…not regress test_sanitize_columns_reserved_stems_get_suffix asserted only that no output name appeared in the reserved set. That also passed under the bug the test is named for: when the reserved check ran after underscores were stripped, '_id' became 'id', never matched _RESERVED, and lost its '_col' suffix -- and 'id' is not in the reserved set either, so the loop stayed green. The test could not detect its own bug. Pins the exact output instead, and adds the case that makes the bug bite: ['_id', 'id'] both sanitised to 'id' under the old ordering, so the de-duplicator renamed one to 'id_1' -- two distinct source columns silently became near-identical names, with the winner decided by header order. Verified against a reproduction of the old ordering: the new assertions fail on it, the old loop still passes. The fix itself is already in main for both csv_parse.py and csv-preview.ts; the TypeScript test had a proper assertion and the Python one did not. Recovered from uncommitted work in .claude/worktrees/velvety-chasing-badger before that worktree was removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1v7ow7NxH9y1ELUyGgACn
…ructure Tier E closes VCRM BR-01. Only Dev was ever structurally verified; Test, Staging and Prod were deployable but nothing asserted they matched. An edit to env_staging.sql adding a column would deploy with exit 0 and stay invisible until production data met a schema no test had compared. Scenario 01 deploys all four environments and reduces each to a table -> ordered column:type:nullability fingerprint, excluding the schema name so the one difference BR-01 permits compares equal. Reordered columns count as drift, because ordinal position is part of the deployed contract. Tier X partially addresses BR-02. It executes build/schema/sqlite/te_core_schema.sql in an in-memory database via stdlib sqlite3, so no sqlite3 binary is needed. It targets the DDL rather than adapter_sqlite.sh because the DDL is what drifts; the adapter is a thin sed wrapper around it. That leaves the adapter scripts unexercised -- a smaller, separate gap, recorded rather than glossed over. Tier X measured a shortfall nobody had counted: PostgreSQL defines 12 tables, SQLite and MariaDB 7, Teradata 1, while InfluxDB and Redis ship seed data with no DDL. The scenario pins SQLite's current 7-table baseline rather than asserting 12, so CI is never permanently red for known-outstanding work, and a job that is always red stops being read. It still fails on drift in either direction. Per ADR-0001 (next commit) Tier X is NOT wired into CI -- multi-engine support is deferred, so the pipeline stays PostgreSQL only. Run it with 'make evals-engines'. Tier E does run in the integration-postgres job. 19 unit tests in tests/test_evals_tier_e_x.py cover both tiers' comparison logic, with negative controls for added column, changed type, changed nullability, reordered columns, and missing or extra tables -- a parity check that silently returns 'no differences' for two different schemas is worse than no check. scripts/lint.sh linted a hardcoded file list, so new modules escaped flake8 and bandit entirely. The four new files are now inside the gate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1v7ow7NxH9y1ELUyGgACn
ADR-0001 records the decision to reach a working PostgreSQL milestone before extending to other engines, with the per-engine measurements that made it possible, the three alternatives rejected, the accepted risk, and four revisit triggers. It exists mainly to answer the question a future reader will have: why do six adapter scripts and six schema directories sit in the tree when the docs describe one engine? BR-02's wording in VCRM.md is retained unchanged and annotated as deferred -- it states the eventual target, not the current milestone. Its status moves from 'not verified' to 'deferred by decision', which is a different thing: an unverified requirement is a gap, a deferred one is a choice with a trigger to revisit. VCRM.md was independently stale: it still reported 17 of 22 (77%) with BR-01 and BR-15 listed as unverified, though BR-15 had been fixed by assertion S10 in SQL suite 05 and BR-01 is closed by Tier E. Corrected to 19 of 22 (86%), with zero actionable requirement gaps -- the three remaining are all deferred by recorded decision. RECONCILED_GAPS.md carries a correction worth keeping: an earlier register was built from a partial 7z snapshot rather than this repository and wrongly concluded the routers/services API had been superseded by a thin translator. Six of the twelve gaps it listed as open were already closed. The lesson is recorded because the failure mode -- reviewing the wrong artifact confidently -- is cheap to repeat. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1v7ow7NxH9y1ELUyGgACn
…a wrong test name Three eval docs still described Tiers E and X as deferred or future after both were implemented. Annotated rather than rewritten: each gets a dated status banner and the affected lines are struck through with the current position beside them, so the record of what was planned -- including HANDOFF.md's quote of the original 'ignore them for now' instruction -- stays intact while the docs stop misleading. Tier D remains genuinely deferred and is left alone. docs/TEST_CONDITIONS.md gains Tier E and X rows in its at-a-glance table. Its counts for the pre-existing categories were not re-measured in this pass, and the intro now says so rather than implying they were checked. One row in TEST_CONDITIONS.md was actively wrong rather than merely stale: it documented test_tier_i_skips_when_postgresql_is_unavailable as returning skipped=True. That test no longer exists -- fix F5 inverted the contract and renamed it to test_tier_i_fails_when_postgresql_is_unavailable, which asserts skipped is False and passed is False. Corrected, and the correction is noted inline. Other pre-existing staleness in these files (the phase table, an 85-assertion reference, file counts, the unit-test total) was found but deliberately left, to keep this diff reviewable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1v7ow7NxH9y1ELUyGgACn
The upload audit added in e0339cb had only unit coverage against a mock cursor -- nothing asserted a row actually lands in csv_uploads.audit_log. Six tests in a new AuditLogTrail class close that: a successful upload writes exactly one 'upload' row; its mode, tableName and fileId match the upload response; upload-then-delete leaves both rows; a refused duplicate adds no second row; a failed upload adds none; and the endpoint honours its total/limit contract. Deliberately a separate class rather than additions to CsvPipelineWithDatabase, because that class's 'BR-15 audit trail' tests read GET /api/csv/files -- the upload registry (csv_files), not audit_log. Two different tables with a confusingly similar name. The new class docstring spells out the distinction so the next reader does not have to rediscover it. setUpClass enables settings.allow_destructive and restores it after, matching the pattern already used in test_api_coverage.py and test_issue_05_import_summary.py. Without it the delete half of the trail could not be exercised at all, and tearDown's cleanup would silently 403 and leave rows for the next run to trip over. audit_routes.py's docstring said the feed existed so 'the deletion trail' is consultable, which stopped being accurate once uploads started writing to the table. Updated. Not yet executed: these are integration tests and no PostgreSQL was available in this session. They collect cleanly, lint clean, and the DB-free suite is unaffected at 115 passing. Their first real run will be 'pytest -m integration'. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1v7ow7NxH9y1ELUyGgACn
This was referenced Oct 25, 2026
…ght place (#45) Two independent causes, both reproduced locally against PostgreSQL 16 with the job's exact steps: 1. Tier E bound the schema name with `psql --set ... -c`. psql never interpolates :'var' inside -c, so every query was a syntax error, which _column_signature turned into an empty result: "0 tables". The query now goes on stdin, and a failed query is reported as a query error instead of an empty schema. Three negative controls; two of them fail against the old code. 2. The job sets PGDATABASE=postgres, and api/config.py reads PGDATABASE, so the API looked for te_dev in the wrong database. The full-suite step now sets PGDATABASE=te_mgmt_dev. With Tier E working it found one real difference: dev has test_run_results, created by the Tier S SQL test harness. It is listed in the Tier E expectation file as ignored, with the reason, rather than filtered silently. Local replay: evals P/I/S/E 26/26, test_report --strict 239/239, 0 skipped; make lint clean. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AYM3RR8Fmw2pwqFfnJhAo6
This was referenced Oct 25, 2026
Replaces the ignore_tables exception from the previous commit, per review: Tier E should need no exceptions. run_all_tests.sql now creates schema te_test_harness and puts it first on search_path, so test_framework.sql's unqualified CREATEs (test_run_results, assert_*, report_*) land there instead of in te_dev. T&E tables still resolve from the env schema, and the suites already address it through te.schema_name. The four report calls are qualified with te_test_harness. Tier S caught the first attempt, which left the report calls qualified with the env schema. Local replay on PostgreSQL 16: te_dev holds exactly 12 tables; evals P/I/S/E 26/26, 0 skipped; test_report --strict 238/238, 0 skipped; make lint clean. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AYM3RR8Fmw2pwqFfnJhAo6
fix(ci): integration job queries the right place (#45)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Gap remediation pass (O-01..O-12), committed locally 2026-09-10 and not pushed until now.
Commits:
Verification: the local DB-free suite was recorded as passing in September. CI on this PR is the first independent run, and integration-postgres is the first end-to-end run of Tier E.
Merge with "Create a merge commit" so local main fast-forwards.