Repository navigation
UN-4232 [FEAT] Agent-KV API serving the table extractor only - #2317
Open
vishnuszipstack wants to merge 19 commits into
Open
vishnuszipstack wants to merge 19 commits into
vishnuszipstack wants to merge 19 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>
… operator env vars
Reverses spec D6 for the `table` extractor. D6 specified "no platform adapters
by design" -- models and OCR credentials in operator env vars, "no end-user
model control". That was right while the consumer was hypothetical and wrong
once the consumers are existing customers with accounts:
* There was NO operator-level LLM credential anywhere in the platform to
reuse. LLM credentials live in per-org `AdapterInstance` rows, encrypted;
the only LLMWhisperer key in a namespace is `LLMWHISPERER_PORTAL_KEY`, which
is the portal ADMIN key that mints per-org extraction keys -- wrong
credential, and over-privileged for executor pods. So the six required env
vars had no source, and a deployment could not be completed.
* Env configuration put every customer's model choice and every customer's LLM
spend on one shared operator key.
* The IDE table path in the SAME plugin already resolves `llm` / `lite_llm` /
`x2text` from adapter instances. This makes the two paths agree.
Wire format follows the platform's own conventions rather than inventing one:
* `adapters: {llm, lite_llm, x2text}` -- a SIBLING of `keys`/`options`, named
by ROLE. `prompt_profile_manager_v2.ProfileManager` declares `llm` /
`x2text` / `embedding_model` / `vector_store`, as fields on the owning
object rather than inside a knobs blob, so neither `*_adapter_id` naming nor
burying them in `options` would have matched. Keeping them out of `options`
also leaves that member optional.
* Types validated against `AdapterTypes` (`unstract/sdk1/constants.py`).
* Errors are DRF `ValidationError`s -> 400, like the rest of the submit caps.
**Validation is split, and the split is the point.** Shape
(`_validated_adapter_shape`) is in the serializer and touches no database --
`test_submit_serializer.py` is a unit suite, and my first attempt put an
`AdapterInstance` lookup there and reddened 22 tests. Ownership and type
(`_resolved_adapters`) are in the VIEW, where the Bearer key is, for the same
reason `_subscription_denial` lives there. Both run before the job row, the
concurrency slot and the staging write -- the first version of the view gate
sat AFTER staging while its own comment claimed otherwise.
**The org filter is the tenant boundary and nothing downstream repeats it.**
The executor resolves these ids through the platform service and does not
re-check whose they are, so without
`AdapterInstance.objects.filter(id=..., organization_id=key.organization_id)`
a caller names any adapter UUID and the run spends ANOTHER TENANT'S LLM
credential. Same class as the `organization_id` filter in `_get_job`, higher
consequence. No platform helper fitted:
`AdapterProcessor.get_adapters_by_type` filters `.for_user(user)` and this
path has no user (a key resolves to an ORGANIZATION), and
`get_adapter_by_name_and_type` is unscoped.
Three deliberate choices in that gate, each documented at the code:
* "No such adapter" and "not yours" return the IDENTICAL message, with a test
asserting the bodies are byte-equal -- distinguishing them makes the
endpoint an oracle for which adapter ids exist in other organizations.
* Type is checked because an `X2TEXT` id in the `llm` slot resolves fine and
then fails deep in the engine as a provider error, which reads like a broken
model rather than two swapped UUIDs.
* Per-user adapter visibility is NOT honoured. `AdapterInstance` is
`HasMembersMixin` with `ResourceMembership` VIEWER rows, and an org-scoped
key has no user to evaluate them against -- so a key reaches any adapter in
its own org. A key is already an organization-level credential, so this is a
decision, not an oversight.
`kv` is untouched and stays env-configured: one credential model per
extractor, never both for one extractor. `ExtractionConfig` keeps its
credential fields for it.
Docs: new §3a covering the three roles, what each is for (lite_llm runs once
per page, so it is the cost-critical one), the four 400 cases, and the D6
reversal with its consequences stated -- the API is no longer callable by
someone who has never signed in, and a table-only deployment can leave
`global.sharedConfigs.agentKv.enabled: false` entirely. Added a `table` curl
example; the existing `kv` one is now labelled as wire-format-only, since this
build 400s it.
259 backend tests (was 254). Mutation-checked: removing the `organization_id`
filter fails 4.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…hem broke every submit **P1: the adapter lookup hid every valid adapter.** `_resolved_adapters` used `AdapterInstance.objects`, which is an `AdapterInstanceModelManager` and inherits `DefaultOrganizationManagerMixin.get_queryset()` -- that filters EVERY query by `UserContext.get_organization()`, a request-local thread-local the tenant middleware sets. `/agent-kv/` is deliberately whitelisted past that middleware (the org comes from the Bearer key, not the URL), so the thread-local is unset and the ambient filter became `organization=None`. Result: the gate refused EVERY submit, including a correctly-configured caller naming their own adapter, with "no such adapter in this organization". The API would not have worked at all. I wrote it by analogy to the `organization_id` filter in `_get_job`, and the analogy is false: `AgentKVJob` uses `BaseModelManager`, which does not auto-filter, so its explicit filter is the only one. `AdapterInstance` sets a different default manager. Verified empirically rather than assumed -- `type(AdapterInstance.objects)` is the filtering manager and `type(AdapterInstance._base_manager)` is a plain `Manager`. Fixed with `_base_manager`, Django's unfiltered manager, which exists for exactly this: internal lookups that must not inherit a custom default manager's filtering. That makes the explicit `organization_id` the ONLY org filter, which is why it stays required rather than optional. NOT fixed by setting the thread-local instead: that would silently re-scope every other org-managed query in the request, and `UserContext` keys on the org SLUG while the key carries the FK pk -- the confusion `dispatch._platform_api_key` documents shipping once already. Extracted as `_lookup_adapter`, because `_base_manager` is a read-only property that cannot be patched, and because mocking `.objects` is what hid this: every view test replaced the manager wholesale, so the manager's own behaviour never ran. **The test that was missing.** `tests/test_adapter_scoping.py` is DB-backed (`TestCase`, so the integration tier picks it up by inference) and exercises the real manager with the thread-local unset -- the only arrangement that reproduces the production path. Four cases: the org's own adapter resolves with no request context (the regression), another org's does not (bypassing the AMBIENT filter must not bypass the explicit one, or the fix would have opened the hole it exists to close), an unknown id is None, and `.objects` really does hide it -- so the explanation on `_lookup_adapter` stays truthful if that manager ever changes. Run against a real Postgres: 4 passed. Mutation-checked by reverting `_base_manager` to `.objects`, which fails the regression case. The unit suite could not have caught this at all. **P1: the e2e lane's submits carried no adapters**, so its shared helper got 400 where it asserted 202 and the lane could no longer validate table extraction on a deployed stack. `submit_raw`/`submit` now take `adapters`, defaulting to ids read from `AGENT_KV_E2E_LLM_ADAPTER` / `_LITE_LLM_ADAPTER` / `_X2TEXT_ADAPTER`. These have to come from the environment: they are rows in the deployed stack's database carrying real provider credentials, and an e2e client cannot mint them. Nine scenarios whose submit must be ACCEPTED now take a `require_adapters` fixture, which skips naming the unset variables -- following the lane's existing gate convention (`AGENT_KV_E2E`, `require_llm`, `AGENT_KV_E2E_BAD_KEY_JOB`) rather than inventing one. The seven scenarios that expect a rejection BEFORE the adapter gate (bad key 403, rate limit 429, absent `extractors`, bad schema 400) deliberately do not take it, and keep working unchanged. Also corrected the module docstring, which still described `AGENT_KV_LLM_API_KEY` as the gate for real extraction -- on the table path that credential now lives on the adapter, so a real-adapter run satisfies it whether or not the variable is exported. 259 unit + 4 integration backend tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… dead guards, and one false premise
A1 (CONFIRMED, the one with teeth). The adapter gate checked organization and
adapter_type and nothing else. `is_usable` is how billing cuts off a
frictionlessly onboarded org's operator-funded trial, and the platform service
does NOT check it -- it hands the credentials back regardless. So the IDE,
workflows and Prompt Studio all refuse an exhausted trial
(tool_instance_helper.py:533, prompt_studio_helper.py:149/203/3292) while an
Agent-KV submit naming the SAME adapter id got a 202 and ran on credentials the
operator pays for and has already cut off. Added `is_usable` and `is_available`
(deprecated -> `InValidAdapterId` deep in the engine, now a 400 at submit),
reusing the platform's own message.
A3. Migration 0004 seeded two PgPeriodicTask rows on the stated premise that
there was "no PgPeriodicTask, no beat entry, no CronJob". That premise was
false about its own tree: the cloud chart already ran the same two management
commands, added four commits earlier on this branch. The rows were
`pg_owned: False` so nothing double-fired yet -- but that flag exists to be
flipped, and a flip would have put a */10 sweep against the chart's */15,
colliding at :30 on the same rows, with TTL cadences disagreeing 24x. Only the
CronJob side has `concurrencyPolicy: Forbid` and a deadline. 0004 now DELETES
the rows (idempotent, name-scoped, reverse restores them inert) and the CronJob
is the sole owner. Verified forward/reverse/re-apply against a real DB,
including that the five dashboard_metrics periodics are untouched. Self-hosted
OSS operators are told to schedule the two commands themselves -- docs §12.
C1. `_never_dispatched` read the in-memory row, fetched BEFORE `mark_terminal`.
A dispatch completing in that gap made the cancel release a slot whose executor
was running and billing -- a smaller instance of the over-release bug the
narrowing was written to fix. Now re-reads `dispatched_at` from the DB. The
residual enqueue-to-bookkeeping window is documented rather than papered over:
bounded at one slot, self-correcting via the idempotent finalize release.
C2. Sweep phase 3 counted slot releases, logged them, and returned only
`{swept, timed_out}` -- so neither `SweepView` nor the scheduler log could ever
show a slot was recovered. Now returns `released` too.
E9. Nothing recorded which adapters a run spent. Adapter choice is now
per-request, caller-controlled and cost-bearing, so "which model did job X
use?" needed a usage_v2 join the API cannot do and the customer cannot see.
New `AgentKVJob.adapters` (migration 0006), populated at submit and reported in
the status document. Ids only, never adapter_metadata -- that column is
returned to the caller.
B1. The queue guard was literal-pinned, so re-enabling `kv` would have required
DELETING its inverse assertion to go green. Added
`tests/test_queue_wiring_is_derived.py`, which derives both directions from
`EXTRACTOR_ROUTES` (and so flips on its own), covers `run-worker-docker.sh`
(guarded by nothing -- dropping `agent_kv_callback` there passed every test
while a docker-launched fleet drained no terminal callbacks), and asserts the
derivation is non-vacuous. Also derives the billing backstop: every routed
operation must be in `_LLM_BEARING_OPS`, which the two hand-maintained literals
could not catch when both were wrong together. The workers-side suite keeps its
literals as a second opinion, now cross-referenced. The dead-Celery-var test is
NOT parametrized -- review asked for that and dev compose legitimately sets the
variable; generalized to "never advertises an unserved queue" instead, over
both files.
B2. The format-typo guard missed the two formats that take an argument:
`_parse_format` matches the `enum:`/`regex:` prefix case-sensitively, so
`format: "Enum:paid,unpaid"` never became kind `enum`, slipped the guard as
free text, and validation was silently off for that key -- the exact failure the
round-1 fix closed for bare names. Now splits on the colon. Rejected rather than
auto-corrected, matching the bare case.
B7 / C3. Pinned two round-1 fixes that no test could detect: `maintenance.py`'s
logging (nothing used caplog on it; all five lines were deletable) and the
`except Exception: pass` in `ide_callback/agent_kv_tasks.py` (its test was
byte-identical before and after). Fixed the same bare `pass` in the sibling
`ide_callback/tasks.py`. Replaced
`test_agent_kv_error_matches_its_siblings_failure_posture`, which asserted
`agent_kv_complete is not None`, with one that actually drives both callbacks
through a failing finalize. Corrected the `retry_backoff` docstring: the PG
consumer runs tasks via `task.apply(throw=True)` -- eager -- so eager retry
recurses synchronously and ignores countdown; the three attempts are
back-to-back and ride out no blip. The real delay comes from vt-redelivery
after the raise.
C4. `require_llm` gated the only two scenarios that poll a job to a genuine
COMPLETED on `AGENT_KV_LLM_API_KEY` -- a variable the table path does not read,
since the credential is on the adapter. A correctly configured lane SKIPPED
them. Removed; `require_adapters` is the whole gate. `test_bad_llm_key_ends_failed`
had lost its trigger entirely (`AGENT_KV_E2E_BAD_KEY_JOB` is unread, so an
operator following the docs got a job that reached `completed` and failed the
test's own assertion) -- re-pointed at `AGENT_KV_E2E_BAD_LLM_ADAPTER`, where the
credential now lives.
Part D. `test_submit_unreadable_pdf_is_400` and
`test_page_cap_rejects_oversized_document` went RED, not skipped, without
adapter env: the adapter shape check runs in a FIELD validator, so DRF raises it
before `validate()` and the blamed attr is `extractors`, not `file`. Both now
take `require_adapters`. Replaced the identical-refusal test that ran the same
configuration twice and compared the body to itself -- the oracle property is
now asserted from two genuinely different DB states in
`test_adapter_scoping.py`, which also gained the `adapter_type` comparison
against a real row (previously only ever against a mock that chose its own
value) and explicit `is_usable`/`is_available` on every stand-in, since a bare
`Mock` satisfies any attribute the gate reads.
E1. Four places across both repos asserted the submit gate was the only tenancy
check and were cited as the justification for its design. It is not:
platform-service resolves adapters with `WHERE id=%s and organization_id=%s`
against the org of the key `dispatch._platform_api_key(job)` mints from the
job's org, so org A cannot spend org B's credential even with this gate removed.
Corrected to defence-in-depth, with what the gate actually buys: a clean 400,
plus the two refusals the platform service does not make.
E7. Documented that the page cap bounds `pages_selected` (after
page_start/page_end), not the document's length -- an unlisted behaviour change.
A4. Deploy-order skew: a new backend against an OLD worker sends validated
`adapters` to a worker with no `ParamKeys.ADAPTERS`, which ignores them and
calls `from_env()`. Where the six AGENT_KV_* values are set it runs on OPERATOR
credentials and returns a correct-looking 202. Documented: deploy cloud first,
and leave that group unset.
Also: the stale "see the backend migration that schedules them" in
workers/scheduler, and `_resolved_adapters`' discarded return value (now
assigned back, so future normalization reaches dispatch).
New: `unstract/sdk1/tests/test_page_usage_metering.py` pins that non-PDF inputs
meter as ONE page while PDFs meter per real page. That under-counts Excel, which
the table engine splits into virtual pages -- product-wide, rooted in the SDK's
own TODO, and affecting the IDE path identically. Pinned rather than changed:
correcting it re-prices every spreadsheet extraction and needs sign-off.
Verification: agent_kv 294 passed; workers (ide_callback, config, webhook,
client, scheduler, queue-wiring, llm-bearing-ops) 74 passed 1 skipped;
agent-kv-schema 138 passed; sdk1 766 passed. Migrations 0004 forward/reverse
and 0006 applied against a real Postgres; `makemigrations --check` clean.
ruff 0.3.4 check + format clean. Every fix above was mutation-verified by
reverting it and confirming the new test fails.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
| `dispatched_at` means to both sweep phases -- a larger change than the | ||
| defect warrants. | ||
| """ | ||
| return not AgentKVJob.objects.filter(id=job.id, dispatched_at__isnull=False).exists() |
Contributor
There was a problem hiding this comment.
If dispatch bookkeeping fails after a task is queued, a stage report can mark the job RUNNING while dispatched_at remains null. Cancelling it then treats it as never dispatched and releases its concurrency slot, although the executor can keep running and billing. Another submit can take that slot and exceed the configured limit. DELETE has the same path.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/agent_kv/execution_views.py
Line: 132
Comment:
**Running job releases its slot**
If dispatch bookkeeping fails after a task is queued, a stage report can mark the job RUNNING while `dispatched_at` remains null. Cancelling it then treats it as never dispatched and releases its concurrency slot, although the executor can keep running and billing. Another submit can take that slot and exceed the configured limit. DELETE has the same path.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| ] | ||
|
|
||
| operations = [ | ||
| migrations.RunPython(remove_pg_periodic_tasks, create_pg_periodic_tasks), |
Contributor
There was a problem hiding this comment.
In environments that already applied the earlier version of migration 0004, changing its forward operation does not run the new deletion. The migration notes that dev namespaces applied that version, so their periodic-task rows remain. If PostgreSQL ownership is later enabled, those rows can run alongside the CronJobs as a second maintenance schedule.
Knowledge Base Used: Backend API platform
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/agent_kv/migrations/0004_pg_periodic_tasks.py
Line: 115
Comment:
**Applied migration leaves schedules**
In environments that already applied the earlier version of migration `0004`, changing its forward operation does not run the new deletion. The migration notes that dev namespaces applied that version, so their periodic-task rows remain. If PostgreSQL ownership is later enabled, those rows can run alongside the CronJobs as a second maintenance schedule.
**Knowledge Base Used:** [Backend API platform](https://app.greptile.com/zipstack/-/custom-context/knowledge-base/zipstack/unstract/-/docs/backend-api-platform.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+268
to
+271
| kind_only = lowered.split(":", 1)[0] | ||
| if fmt not in _KNOWN_FORMATS and ( | ||
| lowered in _KNOWN_FORMATS or kind_only in _KNOWN_FORMATS | ||
| ): |
Contributor
There was a problem hiding this comment.
The new prefix check rejects a free-text hint such as "number: of items": splitting at the colon produces the known format name number, so compilation raises SchemaError. Free-text formats are otherwise permitted, making authors reword valid hints to get their schemas accepted.
Prompt To Fix With AI
This is a comment left during a code review.
Path: unstract/agent-kv-schema/src/unstract/agent_kv_schema/compile.py
Line: 268-271
Comment:
**Valid format hints rejected**
The new prefix check rejects a free-text hint such as `"number: of items"`: splitting at the colon produces the known format name `number`, so compilation raises `SchemaError`. Free-text formats are otherwise permitted, making authors reword valid hints to get their schemas accepted.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+32
to
+38
| One scenario needs the opposite -- a job that dispatches and then FAILS inside | ||
| the engine -- and takes ``bad_llm_adapter``, the id of an adapter deliberately | ||
| carrying an invalid credential. That also moved: it was gated on | ||
| ``AGENT_KV_E2E_BAD_KEY_JOB=1`` plus an invalid ``AGENT_KV_LLM_API_KEY``, a | ||
| variable this path does not read, so an operator who set the stack up exactly | ||
| as documented got a job that ran on the valid adapter and reached | ||
| ``completed``. |
Contributor
There was a problem hiding this comment.
The failure scenario now requires AGENT_KV_E2E_BAD_LLM_ADAPTER, but the module still tells operators to set the removed AGENT_KV_E2E_BAD_KEY_JOB gate and run -k bad_llm_key. That selector no longer matches the renamed test, so following the instructions does not exercise the scenario.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/e2e/agent_kv/test_agent_kv_e2e.py
Line: 32-38
Comment:
**Failure test instructions outdated**
The failure scenario now requires `AGENT_KV_E2E_BAD_LLM_ADAPTER`, but the module still tells operators to set the removed `AGENT_KV_E2E_BAD_KEY_JOB` gate and run `-k bad_llm_key`. That selector no longer matches the renamed test, so following the instructions does not exercise the scenario.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
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