test(coverage): close audit gaps — token_classification, semseg characterization, _refresh_token#282
Merged
Merged
Conversation
Structural refactor phase P5 (backend#796), god-class decomposition slice 2. The file-based table lock (#772 P2) — compute path under STORAGE_PATH, atomic O_EXCL acquire with stale-reclaim + corrupt-lock mtime fallback, release — was three methods + a constant on BaseIngestor. It's a cohesive, stateful responsibility, so it moves verbatim (every error message unchanged) into a `TableLock` class (ingestors/table_lock.py) that BaseIngestor composes via a `_table_lock` property. _ingest_with_lock now does `lock = self._table_lock; lock.acquire()` ... `lock.release(path)`. STORAGE_PATH is a Config class constant (not env/override driven), so TableLock reads it from a plain Config() exactly as before — the ~10 lock tests that patch Config.STORAGE_PATH keep working; they now exercise the lock via `ing._table_lock.acquire()` etc. Removed the now-unused `os` and `..config.Config` imports from base.py (their last users moved out in P5a/P5b). Behaviour-preserving: full unit suite 1080 passed, 96.8% coverage; e2e (real MySQL) 23 passed, 1 xpassed — #247 characterization goldens unchanged. Stacked on #274 (P5a). Next: P5c (RecordProcessor) and P5d (the batch / DB / API write path), each behaviour-preserving; the deferred bug fixes (mask_id leak, per-row tolerance #235, atomicity #227 / #260) ride later focused slices. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Structural refactor phase P5 (backend#796), god-class decomposition slice 3. process_record + _map_unique_id — turning a raw source row into the cleaned, DB-ready dict (schema-filtered + NA-normalised columns, resolved data_id, the label after the configured label policy, data_intent, annotation, framework columns) — move verbatim into a RecordProcessor class (ingestors/record_processor.py). The attribute names match the ingestor's so the bodies are byte-for-byte unchanged. BaseIngestor composes it via a `_record_processor` property (built from the run's column / label / intent config). Its public `process_record` stays as a one-line delegate, since the ingest loop and ~26 test call-sites use `self.process_record` / `ing.process_record`; _map_unique_id (no direct test callers) moves fully inside RecordProcessor. Removed the now-unused pandas / Intent / TaskCategory imports from base.py. Behaviour-preserving: full unit suite 1080 passed, 96.8% coverage; e2e (real MySQL) 23 passed, 1 xpassed — #247 characterization goldens unchanged. Two tests that reached into the old BaseIngestor._map_unique_id now target RecordProcessor (the label-policy hook lives there). The deferred mask_id cross-layer leak is preserved exactly (process still writes mask_id for semantic_segmentation) — untangling it is a follow-up, not this slice. base.py: 1180 -> 774 lines across P5a+P5b+P5c. Stacked on #275 (P5b). Next: P5d (the batch / DB / API write path). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Structural refactor phase P5 (backend#796), god-class decomposition slice 4 — the final extraction. _flush_batch + _process_batch — insert a batch to MySQL, publish the inserted rows to the backend, and fold the outcome (inserted / api-sent / failed) into the run's stats + failed_records — move verbatim into a BatchWriter class (ingestors/batch_writer.py). The subtle mid-batch-DB-failure accounting (match inserted-but-unsent records by data_id, not position) and the #99 / api_send_failed surfacing are byte-for-byte unchanged. Attribute names match the ingestor's so the bodies are identical. BaseIngestor composes it via a `_batch_writer` property; _ingest_with_lock's two flush sites call self._batch_writer.flush(...). The unused `session` parameter is threaded through unchanged for parity (a later cleanup can drop it). Tests that drove _process_batch directly now call ing._batch_writer._process(...). Behaviour-preserving: full unit suite 1080 passed, 96.9% coverage; e2e (real MySQL) 23 passed, 1 xpassed — #247 characterization goldens unchanged. Completes the P5 decomposition: BaseIngestor (now 643 lines, was 1180) is a thin orchestrator composing Preflight (P5a) + TableLock (P5b) + RecordProcessor (P5c) + BatchWriter (P5d), keeping only ingest()/_ingest_with_lock orchestration + validate_data + create_table/dataset registration. The deferred correctness fixes (mask_id leak, per-row tolerance #235, atomicity #227/#260) now have clean seams to land on as separate reviewed PRs. Stacked on #276 (P5c). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Completes the P5 decomposition's last cross-layer loose end. semantic_ segmentation's mask_id is a per-row pointer to a mask FILE, not a table column (there's no mask_id column on the standard table — #212). It used to ride the cleaned, DB-bound record across three layers: RecordProcessor SET it, map_file_transfer READ it, and BatchWriter POPPED it before insert — a fragile indirection where forgetting the pop broke inserts on non-semseg tables. Now the sidecar pointer stays on the RAW source record and map_file_transfer owns its lifecycle: it LENDS mask_id from the raw record to the transfer for the duration of the copy, then STRIPS it before return (in finally — even on a failed / None transfer). So: - RecordProcessor no longer writes mask_id onto the cleaned record (and no longer needs `category` at all — dropped). - BatchWriter no longer pops it (the cleaned record never carries it). - BaseIngestor.ingest passes the raw record as map_file_transfer(..., source_record=record). The cleaned record now holds ONLY table + framework columns — a runtime-only pointer can't reach the DB insert by construction (the #212 class of bug is now unrepresentable, not just guarded). Behaviour-preserving: full unit suite 1080 passed, 96.9% coverage; e2e (real MySQL) 23 passed, 1 xpassed — the semantic_segmentation template still ingests (its mask transfers via the lent pointer; #247 goldens unchanged). Tests that asserted mask_id on the cleaned record / BatchWriter's pop are updated; a new test pins map_file_transfer's lend-then-strip. Stacked on #278 (P5d). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…en in schema
Audit follow-up — self-review caught a behaviour change the first cut smuggled
in. It assumed the cleaned record never carries mask_id once RecordProcessor
stops adding it, but the shipped semantic_segmentation TEMPLATE lists mask_id in
its schema (schema={"mask_id": "VARCHAR(255)"}). So RecordProcessor's schema
comprehension KEPT it, map_file_transfer's `key not in record` guard then
skipped the strip, and mask_id reached insert_batch — flipping that DB column
from NULL (develop: unconditional pop) to populated. The e2e missed it because
the harness's semseg case is schema-less.
Fix (restores develop's behaviour AND genuinely removes the leak):
- SIDECAR_KEYS moves to utils/constants (shared). RecordProcessor excludes it
from the cleaned record EVEN when listed in schema, so mask_id is never
DB-bound regardless of how a template declares it.
- map_file_transfer strips the FULL SIDECAR_KEYS set in `finally` (not only what
it lent) — defense-in-depth so a sidecar pointer can never bind as a column.
- BatchWriter / RecordProcessor docstrings corrected (the old comment asserted a
now-false invariant).
Tests: + test_process_record_excludes_mask_id_even_when_in_schema (the leak path)
and + test_map_file_transfer_strips_preexisting_sidecar_key (the strip-all
defense). Full suite 1082 passed, 96.9%; e2e 23 passed, 1 xpassed (schema-less
semseg still transfers via the lend).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… even when in schema" This reverts commit cc50bc3.
… client contract Audit conclusion (backend#816): the prior "B1 fix" (reverted in the parent commit) was itself the regression, and so was develop's blanket pop. The training client SELECTs the dataset row from cluster MySQL and does str(row["mask_id"]) to locate each mask file, raising FileNotFoundError if mask_id is missing/NULL (tracebloc-client segmentation_dataset_pytorch.py:98). So semseg REQUIRES mask_id stored — develop NULLed it; excluding it (B1) kept it NULL; both break semseg training. Correct behaviour (restored by the revert): mask_id is KEPT and stored when the template DECLARES it in schema (a real column the client reads); it is lent from the raw record + stripped for the transfer ONLY when not declared (no column). This commit: - Corrects the now-stale docstrings (record_processor / file_transfer map_file_transfer / transfer / batch_writer) that wrongly claimed mask_id is "never on the cleaned record" — the framing that caused the misdiagnosis. - Adds test_process_record_stores_mask_id_when_declared_in_schema (the contract guard that was missing) and reframes the schema-less complement test. Full suite 1081 passed, 96.9%; e2e 23 passed, 1 xpassed. The end-to-end ingestor->client contract is tracked for DS sign-off in backend#816. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…haracterization, _refresh_token, README Closes the test-coverage gaps the audit surfaced: - token_classification: was in ZERO e2e tests despite a bundled template — now ingests cleanly in test_ingest_e2e (its CSV quotes the extension as '.txt', exercising that path too). - semantic_segmentation: added to the characterization harness — pins the full contract (masks land in DEST_PATH + mask_id stored for the client). It PASSES: the P5 mask_id work wires masks through the declarative path, so #136 is fixed and its stale test_ingest_e2e xfail(strict=False) is removed (the silent-fix gap the audit flagged). The cross-repo client contract is tracked in backend#816. - _refresh_token: the #772 mid-run BACKEND_TOKEN rotation body was only ever exercised with the method STUBBED; added direct tests for the rotation path (env rotates -> True / unchanged -> False) and the creds re-auth path (authenticate -> True / raises -> False). - e2e/README.md: corrected the stale "Known gaps" table (OD #135, MLM #137, semseg #136 all landed) and switched guidance to xfail(strict=True). e2e: 26 passed (was 23 + 1 xpass). Unit: 1085 passed, 97.1% (+4 token tests; api/client _refresh_token now covered). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
👋 Heads-up — Code review queue is at 23 / 8 Above the WIP limit. The team convention is to review existing PRs before opening new work. Open PRs currently in Code review (oldest first):
Pull from review before opening new work. (This is a nudge from the kanban WIP check, not a block.) |
This was referenced Jun 16, 2026
divyasinghds
approved these changes
Jun 17, 2026
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
2 participants
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.
Summary
Closes the test-coverage gaps the audit surfaced (the deep-dive you asked for). Stacked on #280 (the P5 mask_id fix) — semseg coverage depends on it.
Gaps closed
token_classification— zero e2e coverage. A fully-supported modality with a bundled template, in neither harness. Added totest_ingest_e2e(passes). Its CSV quotes the extension as'.txt', so this also exercises that parse path.semantic_segmentation— never characterized. Added to the characterization harness, pinning the full contract: masks land inDEST_PATHandmask_idis stored (the value the training client reads). It passes — the P5 mask_id work wires masks through the declarative path, so feat: wire segmentation mask sidecars through the declarative path #136 is effectively fixed, and its staletest_ingest_e2exfail(strict=False)(which let a fix land silently) is removed. The cross-repo client contract is tracked for DS sign-off in backend#816._refresh_tokenbody untested. The #772 mid-run token-rotation logic — the code preventing "rows committed but never registered" on long runs — was only ever exercised with the method stubbed. Added direct tests: BACKEND_TOKEN env rotation (rotated →True, unchanged →False) and the CLIENT_ID/PASSWORD re-auth path (authenticate→True, raises →False).e2e/README.mdstale gaps table. Corrected (OD fix: validator/UX gaps (reserved id, OD difficult=2, TS leading-NaN, image defaults) #135, MLM fix: MLM template missing tokenizer.json (can't ingest itself) #137, semseg feat: wire segmentation mask sidecars through the declarative path #136 all landed) and switched guidance toxfail(strict=True)so future fixes surface loudly instead of silently.Verification
_refresh_tokentests;api/clienttoken-rotation body now covered).Not addressed here (deliberately)
The remaining audit items are pre-existing / lower-priority (dead
max_retriesattr,csv_ingestor__all__/unused-e,send_batchpayload-shape assertion, image_validator path-arg foot-gun). Flagged in the audit; not bundled into this PR.🤖 Generated with Claude Code