Repository navigation
UN-4232 [FEAT] Agent-KV API serving the table extractor only - #2317
Open
vishnuszipstack wants to merge 16 commits into
Open
vishnuszipstack wants to merge 16 commits into
vishnuszipstack wants to merge 16 commits into
Conversation
Tasks 2 and 3 of docs/superpowers/plans/2026-10-06-table-extractor-api-carveout.md. Stands the Agent-KV API up on a branch cut from main, routing `table` and nothing else, so table extraction reaches customers without waiting for the KV engine, the schema codegen path or the hardened sandbox to stabilise. WHAT CAME ACROSS The `agent_kv` Django app and its URL/settings mounts, the AGENT_KV storage type, the `agent_kv_callback` queue and its ide_callback tasks, the scheduler's sweep/TTL beat tasks, the internal API client, the webhook notifier, and the compose/worker wiring for all of it. FOUR SUBTRACTIONS, EACH REVERTIBLE IN ONE LINE 1. `kv` is out of `EXTRACTOR_ROUTES`. `SUPPORTED_EXTRACTORS` is derived from that table, so a `kv` submit now returns 400 at the serializer. This is the load-bearing line: leave `kv` routable with no `agentic_kv` plugin deployed and the submit returns 202, dispatches to `celery_executor_agentic_kv`, and sits in DISPATCHED forever -- no error at the producer, nothing in any log. `V1_EXTRACTOR_NAME`, `STAGE_NAMES` and `KVOptionsSerializer` all stay in the tree, dormant and still tested. 2. `celery_executor_agentic_kv` is out of both compose fleets and run-worker.sh, for the same reason. 3. `/validate` is unregistered. It compiles `kv` key schemas and nothing else; publishing it for an extractor this deployment refuses is an incoherent contract. The view class and `unstract/agent-kv-schema` stay. 4. No sandbox: not the worker, not its compose service, not PG_ROLE_SANDBOX, not its SANDBOX_* env block, not its registry/enum entries, not its coverage target. `agentic_table` keeps running generated code in the executor pod exactly as the IDE table path does in production today; UN-4215 is the fast-follow. worker-unified.Dockerfile is reverted to main: its UID pin exists so the sandbox pod can assert runAsNonRoot with a numeric UID, and its comments cite a chart template this PR does not ship. TESTS Two guards were INVERTED rather than deleted, which is the point of them: * `test_queue_consumer_wiring.py` used to assert the KV queue HAS a consumer; it now asserts no fleet advertises it. Mutation-checked -- re-adding the queue to compose fails the guard. * `test_registry.py` was entirely sandbox assertions; it now asserts the ide_callback worker subscribes to `agent_kv_callback` and that both terminal callbacks route there. A job whose callback is unconsumed runs to SUCCESS and then sits in RUNNING forever, result unpersisted and slot unreleased. Backend suite re-pointed from `kv` to `table`. `KVOptionsSerializer` is now tested directly rather than through a submit -- `kv` is refused at `validate_name` before any options validator runs, so driving those rules through `SubmitSerializer` would have asserted nothing while still passing. E2E: the generic scenarios (auth, cancel, concurrency slot release, delete, page cap, sync-wait, webhook, 429, unreadable PDF, Excel) re-pointed at the table extractor; the five kv-engine scenarios dropped (happy path -- the table twin already covers it, /validate, calculations, hostile calculations, and the KV document cache, which the table path does not have until spec step 5). Green: 191 backend agent_kv, 65 workers, 42 agent-kv-schema, 1 filesystem. ruff 0.3.4 (the pinned pre-commit version) clean on check and format. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Task 5a and Task 6. THE STAGE NAME `TABLE_STAGE_NAMES[0]` (here) is the stage name the status endpoint will SHOW. `STAGE_TABLE_EXTRACTION` (cloud `agentic_table/src/api_binding.py`) is the name the executor actually SENDS. Nothing enforced their equality: StageReportView persists whatever arrives, and `_status_document` then filters a job's recorded stages through the list here. On drift the job completes normally, bills normally, and every status response returns an empty `stages` array. No test on either side could catch it -- each repo is internally consistent on its own -- and from a client's seat it reads as "the API is broken". Until now the only thing tying the two was a pair of comments. Both halves now assert the literal and name the other's file. A literal, not an import: the repos are separate checkouts that meet only in the merged tree, so an import would pass vacuously exactly where drift gets introduced. DOCS `docs/agent-kv-api.md` said the `name` field takes "`kv` or `table`". It now says `table` is the only supported value, with a note explaining why a `kv` submit returns 400 -- otherwise that refusal reads as a bug -- and that the wire format does not change when `kv` ships. The `/validate` section is removed along with its route, and every reference to it, and sections 7-13 are renumbered to 6-12 with their anchors and cross-links. Section 2 (the `kv` schema language) is kept but now opens by saying the extractor it describes is not routable here: the compiler ships and the language is frozen, so an integration can still be written against it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
All six were `python:S3776` on code this PR introduces to `main`. Every one is a pure extraction -- no behaviour change, and the existing suites are the proof (193 backend, 42 schema, unchanged and green throughout). execution_views.py SubmitView.post 16 -> 8 internal_views.py FinalizeView.post 23 -> ~7 kv_schema.py _walk 25 -> ~9 compile.py compile_schema 20 -> ~4 constraints.py _aggregate 18 -> ~9 constraints.py _truth 18 -> ~6 The extractions follow the shape each function already had: * `SubmitView.post` -- `_subscription_denial`, `_dispatch_or_fail` (which folds the two identical except-branches into one exit) and `_sync_wait_response`. * `FinalizeView.post` -- `_complete`, `_fail` and `_drop_staged_input`, which flattens a four-deep nest into a dispatch and one guard. * `_walk` -- `_walk_array` and `_append_leaf`, one per node kind; the function now only does the depth/shape guards and dispatches. * `compile_schema` -- `_enforce_shape_caps`, `_enforce_leaf_caps` and `_validated_constraints`, split along the three things it was checking. * `_aggregate` -- `_parse_agg_call` takes the fail-closed call validation and the path split. * `_truth` -- `_compare_chain` takes the chained-comparison loop. Verified under flake8-cognitive-complexity at `--max-cognitive-complexity=15`: clean across `backend/agent_kv/` and `unstract/agent-kv-schema/src/`. The two functions that still measure 14-15 on that checker were analysed by Sonar in this same PR and not flagged, so they score at or under the threshold on its algorithm; they are untouched rather than churned for no gain. Also drops an orphan missed in the e2e re-point: `_CALC_ENV_GATE` and `_skip_unless_calculations_declared` outlived the two calculation scenarios they gated, and referenced a sandbox worker this PR does not ship. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ult_ref Greptile P1 on PR #2317. Real, and it is data loss. The race: DELETE reads a RUNNING job whose `result_ref` is still "", the finalize callback wins the guarded UPDATE in between and writes COMPLETED plus a real `result_ref`, and `mark_terminal` here returns False. Execution then continued against the STALE in-memory job. `delete_job_files` reports an already-empty ref as "cleared" -- correctly, there was nothing to delete -- so the stale `result_ref: ""` came back in `cleared`, and `job.save(update_fields=cleared)` wrote "" over the ref the callback had just persisted. The outcome: a job that reports COMPLETED, a result that 404s (`JobResultView` reads COMPLETED-without-a-ref as swept), and the result object orphaned in the bucket with nothing pointing at it -- TTL cleanup selects on `result_ref > ""`, so a blanked row never comes back as a candidate. Fix: re-read the row when the terminal race is lost, before touching files. Deleting is still the caller's intent; it just has to act on the refs that actually exist rather than on a copy that predates the winner's write. Only on the lost-race path -- winning means nothing else wrote to the row, so the in-memory copy is current and the extra query would be waste on the common path. Both directions are asserted. Mutation-checked: replacing the refresh with `pass` fails `test_delete_refreshes_the_job_when_it_loses_the_terminal_race`. One existing test needed a stub: this suite touches no database, so the new read is mocked in `test_delete_does_not_release_the_slot_when_it_loses_the_terminal_race`. 195 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
vishnuszipstack
commented
Oct 6, 2026
vishnuszipstack
left a comment
vishnuszipstack
left a comment
Contributor
Author
There was a problem hiding this comment.
Multi-agent review — unstract#2317
Reviewed with the PR-review toolkit (code quality, silent failures, test coverage, type design, comments), then every load-bearing claim re-verified by hand against the PR branch. Inline comments below; CONFIRMED = traced in code, PLAUSIBLE = suspected, not fully traced.
Cross-repo defects — green in both PRs until merged together
1. Stage status "failed" is rejected and the rejection is swallowed. OSS internal_views.py:25 accepts only {"running","done"} (400 otherwise). Cloud api_binding.py:392 posts "failed". StageReporter.report catches the 400 into a logger.warning, so every over-cap Excel job finalizes failed beside a stage permanently reading running. The cloud PR's own progress.py:42-44 documents this constraint. One-line fix on the cloud side.
2. Token metering has no backstop for the new operation. workers/executor/tasks.py:45-52 lists four _LLM_BEARING_OPS; the new op is table_extract_api, which is absent. So the elif at :161 — the guard whose purpose is logging "LLM-bearing op emitted no usage" — never fires for table extraction. Combined with flush() returning [] on a silent hasattr skip, total loss of billing rows produces zero log lines. tasks.py is not in this diff, so flagging here: add "table_extract_api" to that set.
Verified good
Multi-tenancy is clean on every path traced — _get_job filters on organization_id, mark_terminal carries it in the WHERE, and both internal views match a body-supplied org_id against the row. The concurrency acquire is genuinely atomic (one Lua script, not check-then-act). write_result's per-attempt nonce correctly fixes the duplicate-finalize race. storage.py's confirmed-clear delete contract is a well-reasoned inversion. The jsonb || stage merge avoids the read-modify-write clobber. Both limiters now fail closed, and the test pins the inversion in both directions. test_cross_repo_stage_name.py is a model cross-repo lock — literal, not import, with a named twin on the cloud side; I verified both sides match.
Test counts are real: backend agent_kv/tests is exactly 193 test functions, and the worker numbers all check out. Both deliberately-inverted guards hold mechanically — test_queue_consumer_wiring.py parses the real compose and shell files, and re-adding the queue does fail it.
Note on CI
e2e was still pending when I reviewed. One e2e assertion will fail when it runs — see the comment on test_agent_kv_e2e.py.
…s, page cap, cancel webhook All four traced and confirmed before changing anything. 1. SUBSCRIPTION GATE FAILED OPEN (`execution_views.py`) A plugin without `service_class` skipped the check and continued to dispatch. Reachable only on a cloud image predating the gate -- but the admitted request spends money, and this route's URL carries no org segment, so `SubscriptionMiddleware` cannot catch it downstream either: a mixed deploy ran unmetered paid work with nothing anywhere enforcing entitlement. Now refuses, via a new `SubscriptionGateUnavailable` (503). Deliberately not a 402 -- the subscription was never evaluated, and reporting it as denied would send an operator to the billing system for what is an image-pairing problem. `test_plugin_without_a_service_class_still_proceeds` is inverted to `..._is_refused`; it pinned the old behaviour as a contract, so it had to change rather than be deleted. 13 other submit tests now carry an admitting gate. 2. NOTHING SCHEDULED THE SWEEP OR THE TTL CLEANUP (migration 0004) Both tasks registered, both endpoints live, no `PgPeriodicTask` row anywhere -- the module docstring said an operator registers them by hand, which means they never ran. So an OOM-killed executor left its row non-terminal forever (`link_error` never fires for a killed process) holding a concurrency slot until the 6h Redis TTL, and `AGENT_KV_RESULT_TTL_DAYS` was advisory: every customer document retained indefinitely. Data migration following the dashboard_metrics precedent (0004-0006): same table, `update_or_create` so a re-run is idempotent, `pg_owned: False` so applying it does not itself start firing them. Sweep every 10 minutes (a stuck job holds a slot); TTL cleanup daily at 03:17, off the hour and off the metrics cleanups so two bulk deletes do not share a tick. The new test pins the migration's `task_name`s against the worker's registered wire names as literals -- an import would pass vacuously in the backend venv, which is exactly where the two sides drift. 3. PAGE CAP COUNTED THE DOCUMENT, NOT THE REQUEST (`execution_serializers.py`) Asking for pages 1-5 of a 400-page PDF was refused against a 100-page cap, despite requesting five pages of work. The cap now bounds the selected range. `pages_total` is unchanged -- it is what metering and the status document report -- and a new `pages_selected` carries what the cap compares. Also rejects a `page_start` past the end of the document, which previously selected nothing and billed for a job over zero pages. 4. CANCELLATION NEVER SENT THE DOCUMENTED WEBHOOK (`dispatch.py`, both cancel paths) Cancellation does not reach finalize, so no webhook fired; and a late executor callback could not send one either -- it loses the terminal guard, and `_maybe_webhook` correctly declines a non-fresh finalize. A caller who supplied `webhook_url` was simply never told, against docs §8. New `agent_kv_cancelled` worker task, routed on the existing callback queue, and enqueued by the backend from both `JobCancelView` and the DELETE-side cancel. Guarded on `won`, which is what makes the two paths mutually exclusive: a cancel that lost means a finalize won and will send. Exactly one path owns the notification. Best-effort enqueue -- the job is already cancelled and the caller already has their 200, so a lost notification logs rather than failing their request. The SSRF waiver moved into one `_send_webhook` helper so the cancel path cannot drift from the finalize path on what it permits. 211 backend tests pass (up from 195), plus ide_callback 18, registry 3, scheduler 7, task-imports 4, queue wiring 9. ruff clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`execute_extraction` logs when an op in `_LLM_BEARING_OPS` finishes successfully having emitted no usage records. That log line is the ONLY signal that a run's billing produced nothing: the cloud `flush()` returns an empty list rather than raising, so total loss of a job's usage rows is otherwise indistinguishable from a job that legitimately made no LLM calls. `table_extract_api` was absent from the set, so the entire Agent-KV table path shipped with no backstop at all — every table job drives two LLMs, and a run that lost all of its billing rows would have produced zero log lines. Adds the op, and a test file pinning the set against a declared list of paid operations in BOTH directions: a new paid op missing from the set fails, and an op added to the set without being declared fails too. One-directional would let the pair drift into a superset and quietly stop meaning anything. Also documents on the set itself that keeping a paid op out of it is a missing alarm rather than a missing log line — the next person adding an operation is the one who needs to know that. Note `kv_extract` is deliberately NOT added: `kv` is not routable on this deployment, so declaring it here would assert a backstop for a path that cannot run. It belongs with the branch that makes the extractor routable. Companion: the cloud half of 1.2 hardens `flush()` itself. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…API, a swallowed finalize
2.1 — EXTRACTOR IDENTITY WAS DECIDED TWICE, UNDER TWO DIFFERENT VALUES
`validate_keys` branched on raw `initial_data["name"]`; `validate_name` saw the
value DRF had already trimmed (`CharField.trim_whitespace` defaults True). So
`{"name": " table "}` passed the name check as `table` and then took the **kv**
branch for keys, never running `TableKeysSerializer`.
That is not merely untidy. A kv-shaped payload like
`{"target_table": {"description": "x"}}` compiles cleanly as a KV schema, so the
submit returned **202** and dispatched to `agentic_table` with a `target_table`
that is a dict rather than the string the binding requires -- the document
staged, the job billed, and it failed at the executor.
Keys validation moves into `validate()`, where `data["name"]` is trimmed and
already checked against `SUPPORTED_EXTRACTORS` exactly once, behind a
`_KEYS_SERIALIZERS` table mirroring `_OPTIONS_SERIALIZERS`. An extractor absent
from it falls through to the schema compiler, which is `kv`'s actual contract.
Mutation-checked by restoring the raw-name lookup.
2.2 — /agent-kv/ WAS NEVER ROUTED THROUGH THE COMPOSE STACK
traefik's backend rule matched `/api/v1`, `/deployment` and `/public`; the
frontend rule is the negation of exactly those three. `base_urls.py` mounts the
API at `/agent-kv/`, so every request through the stack was served by the SPA's
nginx and never reached Django.
The e2e lane could not catch it: `conftest.py` talks to `UNSTRACT_BACKEND_URL`
(port 8000) directly, bypassing traefik. So the API was unreachable exactly as a
customer would reach it, and green.
Both rules updated. Three guards added to `test_queue_consumer_wiring.py` --
the established home for "configured-looking but unreachable" wiring. The third
asserts the RELATIONSHIP rather than a hardcoded prefix: every prefix routed to
the backend must be excluded from the frontend, so the next mount point cannot
be added to one side only. Mutation-checked.
2.4 — A FAILED FINALIZE WAS REPORTED AS A SUCCESSFUL CALLBACK
`agent_kv_error` caught, logged and returned None. The consumer records that as
SUCCESS, deletes the message and moves on: no retry, no dead letter, no
failed-task record. Since this task is the SOLE terminalizer of a failed job, a
momentary backend blip during finalize stranded the job in RUNNING until the
sweep terminalized it with "Job timed out" -- overwriting the real executor
error, which existed only in the log line above.
Now `bind=True` with backoff retries, raising on exhaustion, matching its
sibling `agent_kv_complete` and the `process_batch_callback_api` precedent in
`workers/callback/tasks.py`. Verified first that the PG transport honours
task-level autoretry -- `queue_backend/pg_queue/consumer.py` relies on it
explicitly for the executor task.
`test_finalize_raising_is_swallowed_and_logged` asserted `result is None`,
pinning the silence as a contract. Inverted rather than deleted: that assertion
is exactly what a regression would restore. Mutation-checked.
214 backend tests (up from 211), ide_callback 20, queue wiring 12.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ap and three silent failures
2.5 — CANCEL RELEASED THE SLOT WITHOUT STOPPING THE EXECUTOR
`mark_terminal` excludes only TERMINAL, so cancelling a DISPATCHED/RUNNING job
returned won=True and the slot was released immediately. Nothing revokes a
running executor -- `job.task_id` is written at dispatch and never read again --
so the engine kept running, kept calling LLMs and kept billing while its slot
was handed to the next submit.
Submit-then-cancel in a loop therefore ran arbitrarily many concurrent
extractions against a ceiling of AGENT_KV_CONCURRENT_LIMIT, all paid for. This
is a cost bug, not just a bookkeeping one.
Release is now narrowed to jobs that never reached an executor, via a
`_never_dispatched` helper applied at both cancel sites. Narrowing loses
nothing: a mid-run cancel's slot is released by FinalizeView's `finally` when
the callback lands, and release() is idempotent.
`test_delete_on_running_job_releases_the_concurrency_slot` asserted the release
-- it pinned the bug. Inverted rather than deleted, with the positive case
(never-dispatched DOES release) added alongside, plus a unit test for the
PENDING-but-dispatched edge: post-enqueue bookkeeping can fail after the task
is queued, leaving a row that reads PENDING while an executor runs, and
releasing that would over-subscribe exactly as before.
2.9 — maintenance.py HAD NO LOGGER AT ALL
Not "logged too little": no `import logging`, no logger, in the module that can
terminalize a thousand jobs as FAILED in one call. A backlog of stranded jobs
was indistinguishable from a quiet, healthy system.
Both entry points now report what they did, at WARNING when the counts are
non-zero (jobs WERE stranded and slots WERE held) and INFO when there was
nothing to do. The scheduler proxies log their result too -- counts alone
cannot distinguish "ran and found nothing" from "never ran", which is precisely
the state this task pair shipped in.
Also pinned `retained` in the TTL return shape. The test asserted
`{"cleaned": 2}` while the endpoint returns `{"cleaned": N, "retained": M}`, so
the field that counts files it could NOT delete was outside the contract.
2.11 — `except Exception: pass` LOST THE REAL EXECUTOR ERROR
An empty catch with no log, around the result-backend lookup. The common case
is a kombu deserialization failure: the executor raised a cloud-plugin
exception class not importable in the OSS ide_callback image, so the error
exists and this process cannot read it. The job was then finalized with
"Executor failed without an error message" -- the exact useless error the lookup
exists to avoid -- and the cause was unrecoverable even from logs.
2.12 — AN UNKNOWN JOB AND A DUPLICATE RETURNED THE SAME 200
FinalizeView and StageReportView both answered a byte-identical 200 no-op
whether the job was already terminal (ordinary) or no row matched the job/org
pair at all (never ordinary). The module imported `logger` and never called it.
That second case is what an org-slug-vs-FK-pk mix-up looks like, and dispatch.py
documents that exact confusion shipping once already. If it recurs, every
finalize is a silent no-op, every job stays non-terminal and is reaped as "Job
timed out": a 100% failure rate presenting as timeouts, with nothing anywhere.
Both views now log the unknown-job case and return a `reason` field so the
worker can tell them apart. Three finalize tests asserted the exact response
dict and gained the new key -- an additive contract change, not a loosened
assertion.
221 backend tests (from 218), ide_callback 20, scheduler 7, queue wiring 12.
Every fix mutation-checked by reintroducing the original behaviour.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2.14 — THE SWEEP'S NULL BACKSTOP COULD NEVER FIRE UNDER BACKLOG
Phase 2's second Q arm exists to recover rows whose post-enqueue bookkeeping was
lost -- they have `dispatched_at IS NULL`. But the batch was
`.order_by("dispatched_at")[:500]`, and Postgres sorts ascending NULLS LAST, so
with 500 non-NULL stuck rows ahead of them those rows were never selected. The
backstop could not fire in exactly the situation it exists for.
Now orders by `Coalesce("dispatched_at", "created_at")`. The test asserted
`order_by("dispatched_at")` -- it pinned the bug -- so it is inverted with the
reasoning recorded rather than deleted.
2.15 — TTL CLEANUP DELETED THE INPUT OF A RUNNING JOB
The candidate query filtered on `expires_at` and a non-blank ref, with no
status filter, so a job still RUNNING past its TTL had its staged input deleted
out from under the executor. The TTL is a retention policy for finished work,
not a kill switch for running work. Reachable whenever a job is stuck
non-terminal longer than AGENT_KV_RESULT_TTL_DAYS -- which is what the sweep's
phase 2 exists to catch, and which until this branch was never scheduled at all.
TERMINAL-only now. The sweep terminalizes stuck jobs first; then this cleans
them. The mocks follow the new three-filter chain and the status predicate is
asserted, so it cannot be dropped silently.
2.13 — UNTRUSTED COUNTERS COULD REWRITE THE STATUS DOCUMENT
`_status_document` builds each stage entry as `{"name": name, **entry}` with the
spread LAST, so a persisted `name` counter overrode it -- renaming the stage in
every later status response and breaking the `if name in stages_json` filter
that decides which stages are shown. One counter could make a job's progress
disappear. `name` is now reserved alongside `status`/`seconds`.
Two more on the same endpoint: `seconds` was taken verbatim, so a dict or list
was persisted and echoed back to the caller (now type-checked, with `bool`
excluded since it subclasses `int`); and `stage` is varchar(32), so a longer
name was an unhandled 500 and `job.stages` could grow unbounded distinct keys.
2.7 — THE `extractors` FILE PART WAS MATERIALISED BEFORE ANY SIZE CHECK
The cap lives in `validate_extractors`, i.e. after the read, and
`DATA_UPLOAD_MAX_MEMORY_SIZE` excludes file-typed parts -- so a 500 MB part was
read in full (plus up to 4x again for the `str`) before being rejected at
256 KiB. One request per worker process OOMs the pod: a cheap denial of service.
Now reads `limit + 1` bytes. Also `errors="strict"`: a latin-1 key name used to
decode to U+FFFD and then compile cleanly, so a malformed payload became a job
running against a schema the caller never wrote.
2.8 — A DB FAILURE AFTER STAGING ORPHANED THE UPLOAD PERMANENTLY
If `stage_input` succeeded and `job.save()` raised, the upload existed with no
row to carry its ref -- and `run_ttl_cleanup` selects candidates from AgentKVJob
rows, so an object with no row is structurally unreachable by every cleanup path
there is. Customer data in the bucket that nobody can find or delete. Now
deleted on that path, with its own failure logged rather than swallowed.
2.6 — AN E2E ASSERTION AND THE DOCS BOTH ASSERTED THE WRONG CASE
Both claimed the cancel 409 body carries the RAW uppercase enum, and the docs
pre-empted correction with "not a typo in this document", citing a unit test as
proof. `JobCancelView` returns `job.status.lower()`, and the cited test asserts
`{"status": "completed"}`. The e2e lane had never run, so the wrong assertion
was never executed. Both corrected, and the docs now record that they were
wrong rather than quietly flipping.
228 backend tests (from 221). 2.8, 2.14 and 2.15 mutation-checked.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
All six land on code added earlier in this branch, and all six are real. Two of them are consequences of narrowing the slot release, which is worth stating plainly: fixing the over-subscription opened the opposite hole at both ends. 1. CANCELLED BEFORE ENQUEUE -> PAID WORK WITH NO SLOT HELD (P1) A cancel can land between the submit's `job.save()` and `dispatch_job`'s enqueue. The cancel sees a PENDING, never-dispatched row, so it terminalizes it AND releases the slot -- correctly, nothing had been dispatched. But dispatch then enqueued anyway: paid work for a job the caller already cancelled, with its slot already handed to the next submit, so the concurrency ceiling is bypassed too. `dispatch_job` now re-reads the row immediately before enqueueing. Re-read, not `job.status`: the in-memory row predates the cancel by construction. 2. CANCELLED AFTER DISPATCH -> SLOT HELD FOR SIX HOURS (P1) The other end of the same narrowing. Cancelling a dispatched job deliberately leaves the slot to the finalize callback, because the executor is still running and still billing. If that executor dies, no callback arrives -- and neither sweep phase selects a CANCELLED row (phase 1 wants PENDING, phase 2 wants DISPATCHED/RUNNING), so the slot sat occupied until Redis expired it. New sweep phase 3, bounded on BOTH sides: older than the stuck grace, so a live executor is not cut short, and newer than the slot TTL, past which Redis has already dropped the entry and there is nothing left to release. Without that second bound it would rescan every cancelled job ever, forever. 3. A FAILED CANCELLATION WEBHOOK WAS ACKNOWLEDGED (P1) `agent_kv_cancelled` ignored `send_webhook`'s return, so a non-2xx or a connection failure was reported as success, the queue message acknowledged, and the caller simply never received the terminal notification the task exists to deliver. `_send_webhook` now returns the result and the task raises on a failed delivery, under the same retry budget `agent_kv_error` got. 4. THE ORPHAN CLEANUP DID NOT CHECK WHETHER IT WORKED (P1) The 2.8 fix called `delete_job_files` and ignored the result. That helper reports which refs are confirmed gone and swallows the rest -- normally the ref survives as the retry handle, but here there is no row to hold it. A failed delete therefore still orphaned the object, silently. Now checks, and logs the ref itself at ERROR: it is the only way anyone can clean it up by hand. 5. A NUMERIC `stage` RETURNED 500 (P2) A truthy non-string passed the presence check and then raised TypeError at `len(stage)` -- my own bounds check from 2.13, turning a malformed report into a server error. Now a 400. 6. CANCELLATION WEBHOOK CAN REPEAT ON REDELIVERY (P2) Acknowledged, not fixed here: it is inherent to at-least-once delivery and the finalize webhook has the same property. Deduplicating needs delivery state on the job row, which is the design question raised under 2.10 and not something to decide inside this commit. Test-mock changes worth flagging: `test_dispatch.py`'s positional `call_args_list[N]` assertions broke on every added query, so they are now content-based (`_filtered_with`). The two bookkeeping-failure tests had a blanket `filter.side_effect` that now hits the new terminal check -- a different, earlier failure -- so the failure is scoped to the bookkeeping call and their original meaning is preserved. 232 backend tests (from 228), ide_callback 20. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ed the opposite of the code WEBHOOK DELIVERY RESULT (2.10, partial) `send_webhook` returns False for a non-2xx or a connection failure, and both call sites discarded it -- so a terminal notification could fail to land with no retry, no record, and for a non-2xx not even a log line. The cancellation task raises (its retry budget then applies). The finalize path logs at ERROR instead: it runs AFTER finalize has already terminalized the job, so raising would re-run a finalize that is no longer free. Three tests. Durable delivery -- attempt counts and a `webhook_delivered_at` the status document can expose -- is the open design question in 2.10 and is deliberately not decided here. TWO COMMENTS THAT SAID THE OPPOSITE OF WHAT THE CODE DOES `constants.py` line 1 -- the first line of the file that defines the routing table -- said "v1 accepts exactly one extractor (`kv`), so the job row does not carry which one it ran". Both halves are now false: `kv` is the one extractor this deployment does NOT accept, and `AgentKVJob.extractor` has carried the name since migration 0002. `test_queue_consumer_wiring.py`'s docstring described only "every queue we dispatch to must have a consumer" and then narrated how `celery_executor_agentic_kv` ought to be wired in -- while the file's actual assertion is that it must NOT be advertised. That is the one thing a guard with an inverted assertion must not say: a reader checking the file against its own description would have concluded the test was wrong. It now states both directions and why each exists. Both were mine, and both are the kind of stale comment that is worse than no comment -- a reader trusts them over the code. 232 backend tests, ide_callback 23, queue wiring 12. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Nine open PR threads, in four groups.
**Silent-skip bugs that reported success (2.19)**
`constraints.py` called itself fail-closed in its module docstring and on the
`except Exception` line, but `evaluate_constraints` records only `is False` as
a violation -- so a constraint that raised was reported as "no violation", a
positive assurance that nothing was checked. The path is reachable, not
theoretical: `_BIN` has `truediv` and `_ALLOWED_NODES` has `ast.Div`, so a row
with `quantity == 0` raises `ZeroDivisionError`. Two senses of "fail-closed"
were conflated: the grammar IS closed (nothing outside the allowlist runs),
the outcome is not. Relabelled, and every drop is now logged -- `exception`
level for a raise, `debug` for the ordinary missing-operand skip, which is
normal on a document with optional keys.
`_aggregate` returned a PARTIAL sum/avg when some cells failed `coerce_number`,
which is then compared against a scalar key covering the whole column and
reports a violation on a CORRECT document. One blank cell in a 40-row invoice
was enough. sum/avg now require every row to contribute; min/max/count are
left total over a subset, which is what they mean.
**Schemas accepted that could never validate (2.18)**
`format: "Number"` is not a known format, so it became a free-text LLM hint,
so `validate_format` returned True for every value forever and `/validate`
reported `{"valid": true}` -- validation configured, none delivered.
`format: "enum:"` and `format: "regex:"` are worse: they do not disable
validation, they invert it, and no value can ever conform. An array `_key`
naming a non-existent column silently falls back to positional identity,
costing accuracy on exactly the arrays the author cared enough to key.
All four are refused at `compile_schema` now, with the intended spelling in
the message. Free-text format stays accepted -- that is deliberate. `KeySpec`
and `ArraySpec` are frozen: they are compile output, the one derived variant
already goes through `dataclasses.replace`.
**Docs and docstrings that documented the opposite of the code**
- docs §12 described both limiters as failing OPEN on Redis errors. They fail
CLOSED, and have for the life of this branch. An operator sizing a Redis
outage from that row would expect "requests get through" and get "every
submit 429s" -- the opposite incident. Corrected, `AGENT_KV_LIMITER_FAIL_OPEN`
documented with its real default, and added to both sample.env files.
- `internal_client.agent_kv_stage_report`'s examples (`"extract"`,
`"started"`, `"completed"`) are all rejected: `StageReportView` accepts only
`running`/`done`, and an off-list stage name is stored then filtered out of
every status response by `_status_document`.
- `ValidateView` had no docstring. A reader of `execution_views.py` sees a
complete, decorated, live-looking endpoint; the only thing that decided
otherwise was `execution_urls.py`, a file they may never open.
- The schema package's pyproject still called itself the "single source of
truth for API validation and the extraction engine", which `compile.py:7-15`
explicitly retracts.
**Latent (2.16)**
`mark_terminal`'s guarded UPDATE excludes terminal ROWS, not non-terminal
ARGUMENTS, so `mark_terminal(..., RUNNING)` would stamp `completed_at=now()`:
a row that reads as finished to the TTL filter and the sweep's cancelled-job
phase while staying invisible to the terminal guard, so nothing could ever
terminalize it again. No caller does this; the method is named for the
invariant, so it enforces it.
**Verification**
248 backend tests (was 233) and 128 schema-package tests, all green. Every new
guard mutation-tested: reintroducing each bug fails the test that covers it --
the format/`_key` checks (14 tests), the partial-sum skip (3), the drop
logging (1), the freeze (1), the `mark_terminal` guard (3), the ValidateView
docstring (1) and the two docs rows (2).
`unstract/agent-kv-schema/tests/test_constraints.py` was 0 bytes against a
258-line module -- an empty file with that name reads as coverage. It now
carries the operator, arithmetic, boolean and aggregate matrix, plus a test
that `compile._ALLOWED_NODES`/`_ALLOWED_CALLS` and
`constraints._CMP`/`_BIN`/`_AGG` agree: two hand-maintained allowlists in two
files, where a node accepted at submit but absent from the evaluator is a
constraint silently skipped for every document (2.20).
Not done here, and why: surfacing a skipped-with-reason third state in the QA
output is a public contract change on the `kv` extractor, which this
deployment does not ship.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Main moved two commits ahead (UN-4223 PG-consumer supervisor, UN-4224 GPT-6 temperature), which left the PR CONFLICTING -- so pre-commit.ci could not compute a mergeable state and reported "error during mergeable check" instead of running the hooks at all. The only real conflict was `workers/uv.lock` (53 hunks). Resolved by taking main's lockfile and re-running `uv lock`, not by hand-merging: the branch's only dependency change is the `unstract-agent-kv-schema` editable source, and all four locks (root, backend, workers, workflow-execution) now differ from main by exactly that one package and nothing else. The branch's own lock had drifted ~1400 lines from main's resolution; this removes that drift. Nothing in main's two commits touches `agent_kv` or any file this branch modifies, so every other path auto-merged. Verified after the merge: 128 schema-package tests, 248 backend `agent_kv` tests, 1459 worker tests (166 skipped) -- all green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review finding 2.17. `extractor` was the one stringly-typed field on the model without `choices=` while `status` has had them since 0001, so nothing but a reader's memory connected the column to `EXTRACTOR_ROUTES`. The sharper half is the default. Migration 0002 added the column with `default="kv"`, which was the historical truth then -- the API accepted exactly one extractor and it was always `kv` -- and became a mis-filing trap the moment `table` existed. A creation path omitting `extractor=` files a TABLE job as `kv`, and `kv` IS a valid key in `STAGE_NAMES_BY_EXTRACTOR`, so `_status_document` hands back the KV stage list and silently drops `table_extraction` from every status response. The job runs, the caller is billed, the stages array comes back empty, and `_status_document`'s unknown-extractor warning does NOT fire, because nothing is wrong as far as the filter can tell. Only the one production creation path exists today and it passes `extractor=` explicitly, so this is latent rather than live. Removed rather than re-pointed at `table`: which extractor ran is a fact about the job, not something with a sensible default. With no default an omission is loud -- `""` matches no route, so `dispatch_job` raises, the job terminalizes FAILED with an error the caller can see, and the status warning does fire. `JobExtractor` lists KV even though this deployment refuses it. The column records which extractor RAN, and the rows 0002 back-filled legitimately say `kv`; routability is `EXTRACTOR_ROUTES`' job. The two sets are deliberately different, so neither is derived from the other -- deriving would either quietly re-enable the extractor or make old rows unreadable. `test_recordable_extractors_are_a_superset_of_routable_ones` pins that relationship as a strict subset. Migration 0005 is a schema no-op on Postgres: Django keeps `choices` and a field `default` in Python only -- no CHECK constraint, no column DEFAULT -- so the AlterField changes model state and emits no DDL touching data. `makemigrations --check --dry-run` reports no changes, and the leaf check shows a single leaf per app. Two existing tests failed on this and were right to: - `test_the_extractor_column_defaults_to_kv` asserted `== "kv"`, pinning the defect as the contract. Renamed, inverted, and it now also asserts `kv` remains a legal value to READ back, which is what the retired-extractor status test below it depends on. - Two `test_job_views` tests built jobs with KV stage names and `kv`-namespaced result assertions while relying on the column default to supply the extractor. Both now pass it explicitly: the filtering is load-bearing for what they assert, and riding on a default made that invisible in the test that depends on it. 251 backend tests (was 248). Mutation-checked: restoring `default=V1_EXTRACTOR_NAME` fails 2, dropping `choices` fails 3. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…bute
Review finding 2.20. `ExtractorSerializer.compiled` was populated during
validation and collected by `SubmitSerializer.validate_extractors` into a
`{extractor_name: compiled schema}` dict that no non-test code read.
`dispatch_job` sends `schema=entry["keys"]` -- the raw dict -- and the engine
compiles it itself.
Plumbing the `CompiledSchema` through instead was the other option the finding
offered, and the queue does not allow it: the compiled form would have to
survive a JSON round-trip to the executor, which is precisely why the engine
recompiles (`compile.py`'s docstring already says the caps are a submit-time
gate, not an invariant the engine re-checks). An attribute that is written and
never read reads as plumbing that exists, so it is gone.
On this deployment the dict was always `{"table": None}` anyway: `table` has a
dedicated keys serializer, so the branch that set `compiled` never ran.
`compile_schema` is still called in the no-keys-serializer fall-through, for
what it REFUSES -- the 400 is the only reason it is there, and the comment now
says so, so a later reader does not delete the call as unused.
That branch is DORMANT here: `kv` is the only extractor without a keys
serializer and `validate_name` refuses it first, so nothing in the request path
reaches it. It is still the contract for the next extractor added without one,
and nothing covered it, so three tests call it directly -- the bad-schema 400,
the raw spec passing through identically, and that neither serializer carries
a `compiled` attribute any more.
Also fixed the now-wrong comment in `test_submit_view.py`, which said the
options dict "must ... carry the compiled schema".
254 backend tests (was 251). Mutation-checked: deleting the `compile_schema`
call fails the 400 test, reintroducing the attribute fails the third.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- **S3776** `SubmitView.post` at cognitive complexity 20 against a limit of 15. Extracted the two self-contained blocks as module-level helpers next to the ones already there (`_subscription_denial`, `_dispatch_or_fail`, `_sync_wait_response`): `_request_data_with_extractors_inlined` (the bounded file-part read) and `_discard_orphaned_input` (the staged-object cleanup for a submit that failed before its row landed). Behaviour unchanged; the rationale comments moved with the code they explain rather than being left behind in `post`. - **S5778 x3** in `test_compile.py`: `pytest.raises` blocks containing two calls, because the spec was built inside them. Hoisted `_one_key(fmt)` out, so what the block asserts about is `compile_schema` alone. - **S5799 x2**: implicitly concatenated string literals in `dispatch.py` and `agent_kv_tasks.py` -- ruff-format artefacts that landed as `"... not " "enqueueing"` on one line, which is exactly the shape of a forgotten comma. Merged; both stay inside 90 columns. - **S117 x2** in migration 0004: `PgPeriodicTask = apps.get_model(...)` is the standard Django idiom but is a local variable, so S117 reads the CamelCase as a naming violation. Renamed to `periodic_task` with a comment saying why it departs from the idiom, so the next person does not "fix" it back. Cloud #1828 has zero SonarCloud findings. 254 backend tests, 128 schema-package tests, 23 ide_callback tests, all green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Contributor
Unstract test resultsPer-group results
Critical paths
|
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
1 participant
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.
What
POST /agent-kv/) up on a branch cut frommain, routing thetableextractor and nothing else.agent_kvDjango app, its URL/settings mounts, theAGENT_KVstorage type, theagent_kv_callbackqueue and itside_callbacktasks, the scheduler's sweep/TTL beat tasks, the internal API client, the webhook notifier, and the compose/worker wiring for all of it.kvis out ofEXTRACTOR_ROUTES, so akvsubmit returns 400 at the serializer.celery_executor_agentic_kvis out of both compose fleets andrun-worker.sh./validateis unregistered (the view class andunstract/agent-kv-schemaboth stay).PG_ROLE_SANDBOX, itsSANDBOX_*env block, its registry/enum entries, or its coverage target.kvtotable, and inverts two guards rather than deleting them.docs/agent-kv-api.md.Why
Table extraction on the API is ready, but it currently rides unstract#2309 / unstract-cloud#1816 — a 338-file change set across two repos that also carries the KV extraction engine, the schema codegen path and a hardened sandbox worker. Those need time to stabilise. This PR carves out the subset customers actually need now so it can ship on its own.
The
kvextractor is left in the tree, dormant and still tested, so the larger PRs re-enable it by restoring one dict entry rather than merging content back into the files they rewrite most.How
mainfile sweep.EXTRACTOR_ROUTES.SUPPORTED_EXTRACTORSis derived from its keys, so droppingkvis what turns akvsubmit into a 400. Leaving it routable with noagentic_kvplugin deployed would return202, dispatch tocelery_executor_agentic_kv, and leave the job inDISPATCHEDforever — no error at the producer, nothing in any log.test_queue_consumer_wiring.pyused to assert the KV queue has a consumer; it now asserts no fleet advertises it. Mutation-checked — re-adding the queue to compose fails the guard.shared/infrastructure/config/tests/test_registry.pywas entirely sandbox assertions; it now asserts theide_callbackworker subscribes toagent_kv_callbackand that both terminal callbacks route there.KVOptionsSerializeris now tested directly rather than through a submit. Withkvrefused atvalidate_name, driving those rules throughSubmitSerializerwould have asserted nothing while still passing.TABLE_STAGE_NAMES[0]here must equalSTAGE_TABLE_EXTRACTIONin the cloud repo'sapi_binding.py;StageReportViewpersists whatever name arrives and_status_documentfilters through the list here, so drift yields jobs that complete and bill normally while every status response returns an emptystagesarray. Asserted as a literal, not an import — the repos are separate checkouts that meet only in the merged tree.worker-unified.Dockerfileis reverted tomain: its UID pin exists so the sandbox pod can assertrunAsNonRootwith a numeric UID, and its comments cite a chart template this PR does not ship.Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
No existing feature is affected. Everything here is additive to OSS
main, which has noagent_kvapp today — there is no prior Agent-KV deployment to regress.Risks that do exist, stated plainly:
kvsubmit now 400s. Intentional and the central point of the PR, but it is a visible difference from #2309. It cannot be a regression for anyone, becausekvhas never been available onmain./validateis not routed. Same reasoning — never shipped onmain.agentic_tableexecutes LLM-generated Python in the executor pod on every run, exactly as the IDE table path does in production today. This PR does not add that risk, but it does widen reach from Prompt Studio users in an org to any holder of an API key. UN-4215 is the agreed fast-follow.Database Migrations
Three, all for the new
agent_kvapp — no existing table is touched:0001_initial—AgentKVKey,AgentKVJob0002_agentkvjob_extractor— addsextractor, defaulting existing rows tokv(there are none on a fresh deploy; new jobs always set it explicitly from the serializer)0003_agentkvjob_cleanup_failed_atEnv Config
New, all
AGENT_KV_*and all documented indocs/agent-kv-api.md§12 plusbackend/sample.env,workers/sample.envanddocker/sample.env:AGENT_KV_LLM_PROVIDER,AGENT_KV_LITE_MODEL,AGENT_KV_ADVANCED_MODEL,AGENT_KV_LLM_API_KEY,AGENT_KV_LLMWHISPERER_API_KEY,AGENT_KV_LLMWHISPERER_BASE_URLAGENT_KV_MAX_TOKENS,AGENT_KV_PARALLEL_PAGES,AGENT_KV_MAX_JOB_TOKENS,AGENT_KV_MAX_PAGES,AGENT_KV_MAX_FILE_SIZE_MB,AGENT_KV_MAX_SCHEMA_BYTES,AGENT_KV_MAX_TIMEOUT_SECONDSAGENT_KV_STORAGE_DIR_PREFIX,AGENT_KV_FILE_STORAGE_CREDENTIALS,AGENT_KV_RESULT_TTL_DAYSAGENT_KV_CALCULATIONS_ENABLED,AGENT_KV_STRUCTURED_OUTPUT_ENABLEDNo
SANDBOX_*variables — this PR ships no sandbox worker.Relevant Docs
docs/agent-kv-api.md— updated in this PR:namenow documentstableas the only supported value, with a note explaining whykvreturns 400; the/validatesection and all references removed; sections 7–13 renumbered to 6–12 with anchors and cross-links.docs/superpowers/plans/2026-10-06-table-extractor-api-carveout.md— the plan this PR implements, included here.Related Issues or PRs
executor_paramscontract the cloud operation is dispatched through.Correction to an earlier version of this description: it said the cloud test groups would be red by design until this lands. They are not — all three pass on the cloud PR today (69 / 676 / 66), because the shared-seams move left those plugins with no OSS dependency. Treat any red in them as a real failure.
Dependencies Versions
unstract-agent-kv-schema(new, in-repo, zero third-party dependencies) and wires it intoworkers/pyproject.toml.backend/pyproject.toml,workers/pyproject.tomland the matchinguv.lockfiles updated accordingly.Notes on Testing
Automated, all green on this branch:
backend—agent_kv/tests→ 193 passedworkers—ide_callback/tests(18),shared/infrastructure/config/tests(3),shared/tests/test_webhook_notify.py(19),shared/tests/test_agent_kv_client.py(5),tests/test_agent_kv_scheduler_tasks.py(7),tests/test_queue_consumer_wiring.py(9)unstract/agent-kv-schema→ 42 passed;unstract/filesystem→ 1 passedruff0.3.4 (the pinned pre-commit version) clean oncheckandformat; the full pre-commit suite passesWorth verifying by hand on review:
tablesubmit returns202, polls tocompleted, and the result is filed underextractors.tableGET /agent-kv/{job}returns a non-emptystages: [{"name": "table_extraction", …}]— this is the assertion that catches cross-repo constant drift from the outsidekvsubmit returns 400, not202— the single most important behaviour in this PR.xlsxsubmit extracts and is capped post-OCRAudit()pathNot yet done, and the one gap worth naming: none of this has run outside compose. A dev-namespace deploy exercising the PG consumer wiring and the agent-kv CronJobs should happen before production.
Screenshots
N/A
Checklist
I have read and understood the Contribution Guidelines.
🤖 Generated with Claude Code