Skip to content

fix: address 15 open issues — security, data integrity, tests, CI - #202

Merged
amar-python merged 19 commits into
mainfrom
claude/csv-loading-postgres-migration-qvkl4l
Nov 9, 2026
Merged

fix: address 15 open issues — security, data integrity, tests, CI#202
amar-python merged 19 commits into
mainfrom
claude/csv-loading-postgres-migration-qvkl4l

Conversation

@amar-python

@amar-python amar-python commented Nov 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

Summary

Fixes fifteen open issues across security, data integrity, test reliability and CI:

Issue Title Category
#183 Rate limiter used API key prefix as identity (bypass via rotation) Security
#178 Distinct CSV rows silently discarded — ambiguous hash serialization Data integrity
#175 Upload overwrite/mode-switch bypassed delete-permission gate Security
#170 All-failed T&E import destroyed prior dataset via cleanup Data integrity
#189 Row errors reported survivor-list position, not original CSV row Bug
#186 Confirmed header mappings silently ignored (API model had no headerMapping) Bug
#185 Preserved original was a UTF-8 re-encoding of decoded text, not the uploaded bytes Data integrity
#171 setup.sh truncated config.local.env on heredoc failure Bug
#180 Tier I eval swallowed SQL errors (missing ON_ERROR_STOP) Test reliability
#181 run_tests.sh exited 0 despite SQL assertion failures (see note below) Test reliability
#192 Ragged rows: accounting identity regression test Test reliability
#193 Formula injection in XLSX exports (guard + tests) Security
#191 Registry race: concurrent upload regression test Test reliability
#194 api/services coverage gate with per-loader floors CI
#169 Test catalogue generator + CI freshness check CI

Correction: an earlier revision of this description also listed #195 (type provenance). That commit was pushed after this PR merged and is not in main; it is PR #213.

Not in this PR: the docs issues #164, #165 and #166. PR #206 rewrote the same text; my earlier docs edits had two defects (they linked /api/docs, a 404 — the live path is /docs — and removed a duplicate-detection step that is shipped for dynamic mode) and were backed out in ac89545.

Changes

Security & data integrity

  • api/ratelimit.py — _client_id() uses peer IP only; API key removed from identity
  • api/services/destructive_ops.py (new) — centralised auth gate for table drops
  • api/services/dynamic_loader.py — row dedup switched to JSON serialization (v2: prefix)
  • api/services/te_loader.py — tracks original CSV row numbers; rolls back on 100% failure; reupload detection moved before inserts
  • csv-table-hub-main/src/lib/export.ts — XLSX export now sanitizes formula-starting cells (same guard as CSV)

Header mapping (#186)

  • UploadRequest accepts a validated headerMapping; apply_header_mapping() renames raw headers before sanitisation. Unknown or repeated originals, and renames that would collide with another column, are rejected (invalid_header_mapping) rather than bent to name_2. T&E mode returns 422 for a mapping instead of ignoring it. tests/test_header_mapping.py maps foo → bar and asserts the database column name.

Preserved original bytes (#185)

  • UploadRequest accepts contentBase64; record_upload(..., original=) hashes and stores those bytes. The loaders and source_rows still use the decoded text. The uploader sends the file's bytes (file-bytes.ts). tests/test_original_bytes.py: BOM-prefixed and lone-0x80 CSVs round-trip byte-identical with a matching hash.
  • Not done: a separate encoding-metadata column. The bytes are not checked against the text, because readAsText legitimately decodes UTF-16.

Test reliability, CI and bug fixes

  • tests/test_api_coverage.py, tests/test_ragged_rows.py, tests/test_registry_race.py (new), XLSX formula tests in export.test.ts
  • .github/workflows/quality-gate.yml — coverage gate and test catalogue freshness check; scripts/check_coverage_floors.py and scripts/test_catalogue.py (new); docs/TEST_CASES.md (generated)
  • build/setup.sh atomic config write; evals/runner.py ON_ERROR_STOP=1; tests/run_tests.sh parses output for FAIL lines

Notes

  • RV-S7: SQL test wrapper exits 0 on failed assertions (run_tests.sh) #181 has a second, unmerged fix. Branch fix/181-sql-suite-exit-code (fd305d7, no PR) makes tests/run_all_tests.sql raise at the end of a failing run and adds ON_ERROR_STOP=1 to the evals runner's suite call. That is the more robust root-cause fix; this PR's run_tests.sh change greps psql output. Mine was checked on a real database (exit 0 on a clean dev DB, 142/142; exit 1 and the failing assertion named on a polluted one). Suggest taking the branch's and dropping this PR's run_tests.sh hunk.
  • The test catalogue check goes stale whenever main gains tests that an open PR's docs/TEST_CASES.md lacks (it hit this PR when RV-F13: T&E review displays invalid rows without validation errors (empty type list) #190 merged); the fix is to regenerate with python3 scripts/test_catalogue.py.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VBrxqChRJtxdvSpFhiUWUy

claude added 4 commits November 8, 2026 23:06
@claude
fix: address 8 open issues — security, data integrity, test reliability
3ffd878 
#183 — Rate limiter uses peer IP instead of API key as identity, preventing
       bucket-rotation bypass via arbitrary X-API-Key values.

#178 — CSV row dedup uses JSON serialization (v2: prefix) instead of U+001F
       join, preventing distinct rows with embedded separators from hashing
       identically (silent data loss).

#175 — Upload overwrite and T&E mode-switch now check delete permission via
       a centralised destructive_ops module; previously bypassed the gate.

#170 — All-failed T&E import rolls back the transaction and preserves the
       prior dataset instead of running cleanup that destroys it.

#189 — T&E row errors now report the original CSV row number instead of the
       position in the survivor list (post-validation subset).

#171 — setup.sh writes config.local.env atomically (temp file + mv), so a
       failed heredoc expansion no longer truncates the prior config.

#180 — Tier I idempotency eval adds ON_ERROR_STOP=1 to psql so SQL errors
       actually fail the eval instead of being silently swallowed.

#181 — run_tests.sh parses psql output for FAIL/assertion lines, catching
       cases where psql exits 0 despite SQL-level test failures.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VBrxqChRJtxdvSpFhiUWUy
@claude
Fix CI: move reupload cleanup before inserts, update all-failed tests
59d9016 
The all-failed rollback (#170) was triggering prematurely for T&E reuploads:
the duplicate-key violations caused all rows to fail, which rolled back the
transaction before the registry cleanup could run. Move reupload detection
and cleanup before the insert phase so the rollback does not undo it, and
skip the all-failed error for reuploads where the data already exists.

Also update 3 tests that send a single bad row to expect status='error'
(correct behaviour when every row fails) instead of 'ok'.

Fixes: integration CI failures on PR #202
  - test_enum_constraint_violation_reported_per_row
  - test_null_in_not_null_column_reported_per_row
  - test_row_level_error_reported
  - test_te_reupload_same_file_replaces_registry
  - test_reupload_cleans_prior_registry_entry

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VBrxqChRJtxdvSpFhiUWUy
@claude
docs: supersede API_INTEGRATION.md, fix starter/quickstart refs (#164 #…
6a29cc2 
…165 #166)

- Add SUPERSEDED banner to API_INTEGRATION.md redirecting to OpenAPI docs
- Update STARTER_GUIDE.md: add sign-in step (#166), remove dedup step (#165),
  point API reference to OpenAPI docs (#164)
- Update QUICKSTART.md: replace API_INTEGRATION.md references with OpenAPI docs
- Update README.md tree entry with "(historical)" annotation
- Add accounting identity assertions and multi-ragged-row test (#192)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VBrxqChRJtxdvSpFhiUWUy
@claude
fix(review): T&E review shows why the database rejected a row (#190)
3382641 
T&E uploads stored an empty column-type list, so review fell back to
text and returned errors: [] for rows a CHECK/FK/UNIQUE rejected.

- te_loader returns the target's mapped column types, so review
  revalidates T&E values against them (unmapped types stay text).
- review attaches the stored upload-time database reason as a
  "(database)" error on unedited T&E rows.
- review table shows any error not tied to a column beside the row.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011T6Wqi8yaiaYWUVH8C4QAY
@amar-python amar-python mentioned this pull request Nov 8, 2026
Merged
4 of 5 tasks
claude added 3 commits November 8, 2026 23:26
@claude
fix: guard XLSX exports against formula injection, add registry race …
92a240e 
…test

#193 — XLSX export path now sanitizes cell values through the same
formula guard as CSV (OWASP CSV injection). Cells starting with =, +,
-, @, tab or CR are prefixed with a single quote unless they are plain
numbers. Added 6 vitest assertions verifying XLSX formula neutralization
and that plain numbers pass through unchanged.

#191 — New integration test (test_registry_race.py) exercises concurrent
uploads of the same file and same-name-different-content uploads,
asserting that the UNIQUE constraints on csv_files settle the race with
exactly one registry row and no partial state.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VBrxqChRJtxdvSpFhiUWUy
@claude
spike(#154): M4 review screen in Streamlit
1593c31 
Throwaway, one-file Streamlit rebuild of the review screen against the
existing API: paging, edit and exclude/include with required reason,
history, role gating, red/blue cell tint. README records what was
comfortable, where it strained, and what each #154 outcome means for
the parked issues. Not wired into CI or the build.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011T6Wqi8yaiaYWUVH8C4QAY
@claude
ci: add api/services coverage gate and test catalogue (#194, #169)
fdc2eb1 
#194 — Integration CI job now measures coverage for api/services and
enforces per-file floors: csv_parse.py ≥ 79%, dynamic_loader.py ≥ 84%,
te_loader.py ≥ 96%. A new script (scripts/check_coverage_floors.py)
reads the JSON coverage report and fails the build if any loader drops
below its floor.

#169 — New script (scripts/test_catalogue.py) walks the test tree,
extracts test names, markers and docstrings, and generates
docs/TEST_CASES.md. The free-tier CI job runs --check mode to fail
the build when the catalogue is stale.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VBrxqChRJtxdvSpFhiUWUy
@amar-python amar-python changed the title fix: address 8 open issues — security, data integrity, test reliability fix: address 14 open issues — security, data integrity, tests, docs, CI Nov 8, 2026
claude added 4 commits November 8, 2026 23:30
@claude
docs: fix STARTER_GUIDE walkthrough accuracy (#165, #166)
88fe6fa 
The guided walkthrough was drafted before ART Milestone 2 landed and
described duplicate detection inaccurately.

- Add a sign-in step: the app has required authentication since M2, so
  localhost:8080 opens on the sign-in page, not the upload area. Points at
  QUICKSTART's "Create your sign-in (first run only)" section rather than
  duplicating the create_tenant_admin.py command (#166).
- Fix the duplicate-detection step. Content-hash dedup IS shipped for
  dynamic mode (api/services/dynamic_loader.py checks filename then content
  hash and returns status "duplicate_file"), but T&E mode replaces the prior
  registry entry instead of refusing (api/services/te_loader.py
  _cleanup_prior_registry_entries). The step now names the dynamic-mode file
  and scopes the claim to that mode, noting per-tenant/per-record dedup is
  still M5 scope (#165).
- Renumber the sequence 1-6 and correct "all five" to "all six".
- Soften the API_INTEGRATION.md pointer: it documents 8 of the 21 live
  routes, so "every frontend<->backend API call" overclaimed. Points at the
  OpenAPI schema for the complete list.
- Drop the stale "API_INTEGRATION.md describes a superseded API" line from
  docs/prompts/ART_FRAMEWORK.md: the file documents current endpoints
  including M2 auth and was last updated 4 Nov 2026.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011T6Wqm1jLdq3ZEfuKdp9aM
@claude
fix: resolve flake8 lint failures in test_catalogue.py
176a84b 
Remove unused datetime/timezone imports (F401) and replace empty
f-strings with plain strings (F541).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VBrxqChRJtxdvSpFhiUWUy
@claude
test(#190): T&E response now carries one type per column
99a63c5 
The contract test still expected types: [] for T&E uploads. Update it,
and the M4 test-case note that described the old "reviewed as text" limit.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011T6Wqi8yaiaYWUVH8C4QAY
@claude
fix: apply confirmed header mappings to dynamic-mode column names (#186)
813e0ec 
The frontend sent headerMapping but UploadRequest had no such field, so
Pydantic dropped it and the imported table kept the CSV's original
headers with no error.

- UploadRequest accepts a validated headerMapping (bounded lengths/count).
- apply_header_mapping renames raw headers before sanitisation; a blank
  rename keeps the original. Unknown or repeated originals, and renames
  that would collide with another column, return a structured
  invalid_header_mapping error and load nothing, rather than bending the
  name to name_2.
- T&E mode rejects headerMapping with 422: its columns must match the
  fixed schema, so a rename could only be silently ignored.
- Tests assert the resulting database column names (foo -> bar), not just
  the request object.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VBrxqChRJtxdvSpFhiUWUy
@amar-python amar-python changed the title fix: address 14 open issues — security, data integrity, tests, docs, CI fix: address 17 open issues — security, data integrity, tests, docs, CI Nov 9, 2026
amar-python and others added 4 commits November 9, 2026 15:12
@amar-python
Merge pull request #203 from amar-python/claude/issue-190-te-review-e…
7c2d645 
…rrors

fix(review): T&E review shows why the database rejected a row (#190)
@amar-python
Merge pull request #205 from amar-python/claude/issue-154-streamlit-s…
78ca648 
…pike

spike(#154): M4 review screen in Streamlit
@claude
docs: replace API_INTEGRATION.md route table with pointer to /docs (#207
295c450 
)

The hand-kept endpoint table documented 8 of the 22 live routes. Per the
product owner's choice (option b on #207), drop it and point at the
OpenAPI schema FastAPI generates from the code, so it cannot drift. The
auth model, known-gaps findings and migrations sections are kept.

The docs path is /docs (Swagger UI) and /openapi.json, verified against
the real app with TestClient: /docs 200, /openapi.json 200 with 22
operations, /api/docs 404. No docs_url is set in api/main.py, and auth is
per-router, so neither route needs a key.

- Correct the earlier-introduced "/api/docs" in STARTER_GUIDE.md and
  ART_FRAMEWORK.md to "/docs" (the path was copied from #164 unchecked).
- Re-label the API_INTEGRATION.md entries in README.md and QUICKSTART.md
  from "endpoint mapping" to what the file now is.
- Add a STARTER_GUIDE.md row pointing at Swagger UI for routes.
- Drop a stale "endpoint table" reference to README in the doc's header.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011T6Wqm1jLdq3ZEfuKdp9aM
@claude
fix: preserve the uploaded bytes, not a UTF-8 re-encoding of decoded …
f42be8c 
…text (#185)

The browser decodes the CSV to text (dropping a BOM, replacing invalid
bytes) and the API re-encoded that text before hashing and storing it, so
the preserved "original" and its sha256 could not match the uploader's file.

- UploadRequest accepts contentBase64, the file's raw bytes. record_upload
  hashes and stores those as the original; the loaders and source_rows
  still use the decoded text, so the decoded view is unchanged. Bad base64
  is a 422, oversize a 413. Without contentBase64 the previous behaviour
  (UTF-8 encoding of content) is kept for API clients.
- The uploader reads the file's bytes (readFileBase64) and sends them with
  the text.
- Tests: BOM-prefixed and 0x80-byte CSVs round-trip byte-identical with a
  matching hash, for both successful and failed uploads. A negative control
  with the fix stashed fails those tests.

Not done here: a separate encoding-metadata column for the decoded view.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VBrxqChRJtxdvSpFhiUWUy
@amar-python
amar-python marked this pull request as ready for review November 9, 2026 04:15
@claude
docs: drop STARTER_GUIDE/QUICKSTART/API_INTEGRATION/README edits; #206 …
ac89545 
…owns them

My earlier docs commit duplicated PR #206 and had two defects, both
verified against the app with TestClient:

- It pointed readers at /api/docs, which returns 404. The live OpenAPI UI
  is /docs.
- It removed the duplicate-detection step from STARTER_GUIDE.md on the
  strength of #165, but content-hash and filename duplicate detection are
  shipped for dynamic mode (dynamic_loader.py returns duplicate_file).

Restore these four files to their base versions so this PR no longer
touches them and cannot conflict with #206, which rewrites the same text
with the correct path and scoping.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VBrxqChRJtxdvSpFhiUWUy
@amar-python amar-python changed the title fix: address 17 open issues — security, data integrity, tests, docs, CI fix: address 15 open issues — security, data integrity, tests, CI Nov 9, 2026
amar-python and others added 3 commits November 9, 2026 15:19
@amar-python
Merge pull request #206 from amar-python/docs/starter-guide-walkthrou…
016329e 
…gh-accuracy

docs: fix STARTER_GUIDE walkthrough accuracy (#165, #166)
@claude
Merge remote-tracking branch 'origin/claude/csv-loading-postgres-migr…
e6e391c 
…ation-qvkl4l' into scratch-merge-202
@claude
docs: add #190's review test to the test catalogue
fc8240d 
#203 added test_te_review_shows_the_database_reason_a_row_was_rejected
after this branch's catalogue was generated, so the new catalogue check
failed once the two met. Regenerated with scripts/test_catalogue.py.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011T6Wqi8yaiaYWUVH8C4QAY
@amar-python
amar-python merged commit 203c8ae into main Nov 9, 2026
3 checks passed
This was referenced Nov 9, 2026
Closed
Closed
Closed
Closed
Closed
This was referenced Nov 9, 2026
Closed
Closed
Closed
@amar-python amar-python changed the title fix: address 15 open issues — security, data integrity, tests, CI fix: address 16 open issues — security, data integrity, tests, CI Nov 9, 2026
@amar-python amar-python mentioned this pull request Nov 9, 2026
Merged
1 task done
@amar-python amar-python changed the title fix: address 16 open issues — security, data integrity, tests, CI fix: address 15 open issues — security, data integrity, tests, CI Nov 9, 2026
This was referenced Nov 9, 2026
Draft
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Draft
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@amar-python @claude