Repository navigation
UN-4232 [FEAT] Agent-KV API serving the table extractor only - #2317
vishnuszipstack wants to merge 59 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>
[Critical impact] The latest documentation correction appears safe to merge and fixes the remaining numbered finding.
|
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
left a comment
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>
…n fixes opened P1. `_never_dispatched` lost a case when I dropped its `status == PENDING` arm. Dropping it was right for the reason given -- the cancel has already rewritten `status`, so that test could only ever be answered from the stale object -- but `dispatched_at` alone is not enough. `dispatch_job` wraps `_record_dispatch` in a try/except on purpose: the task is already queued, so a bookkeeping failure must not be reported as a failed dispatch. A job can therefore be RUNNING, with an executor spending money, while `dispatched_at` is still NULL, and the executor's first stage report is the only thing that recorded it. Cancelling there released a live executor's slot and the next submit could take it, exceeding the per-org cap. DELETE had the same path. The old `status == PENDING` arm had been catching this by accident, off the stale row. Now consults BOTH durable markers, read fresh: `dispatched_at`, and `stage` (written by `StageReportView` on the first progress report, and never touched by the cancel). Five DB-backed cases in `test_adapter_scoping.py` -- including the RUNNING-with-NULL-dispatched_at row and the original stale-snapshot interleaving -- plus a view test asserting the query consults both fields, so neither can be dropped again. Mutation-checked: removing the `stage` marker fails three. P1. The migration fix could not reach the environments that needed it. Django records a migration as applied and never re-runs it, so rewriting 0004's forward operation to DELETE the seeded rows did nothing at all in every dev namespace that had already applied the seeding version -- while changing behaviour only for fresh installs. Its own docstring claimed to clean up environments it could not touch. 0004 is restored to seeding (with a note that 0007 supersedes it) and the removal is now `0007_remove_agent_kv_periodic_tasks`. A fresh install seeds then deletes; an already-seeded one just deletes; both land on zero rows. Verified against a real Postgres in all three states -- fresh, pre-seeded, and reverse-then-forward -- with the five dashboard_metrics periodics untouched each time. 0007 reads the row names out of 0004 rather than re-declaring them, and a test asserts 0004 still seeds, so folding the removal back into it fails loudly. P2. My `Enum:`/`REGEX:` fix over-rejected valid free text. Comparing the pre-colon segment against every name in `_KNOWN_FORMATS` meant `format: "number: of items"` -- and `"date: of issue"`, `"currency: USD"`, `"string: free text"` -- raised SchemaError, forcing authors to reword hints that are a SUPPORTED way to describe a key. That is a worse failure than the one being fixed: the original bug silently disabled validation for one key, this refused the whole schema. Scoped to `_ARGUMENT_FORMATS` (`enum`, `regex`) -- the only two that take an argument, so the only two where `name:arg` can be a mis-cased format. There is no such thing as `number:`. Verified across a 14-case matrix; mutation-checked. P2. The e2e module docstring still told operators to set the removed `AGENT_KV_E2E_BAD_KEY_JOB=1` and run `-k bad_llm_key`, a selector that matches nothing since the rename. Rewritten for `AGENT_KV_E2E_BAD_LLM_ADAPTER`, with the three adapter ids the accepted-submit scenarios need, and noting the new arrangement needs no special stack -- the bad credential is swapped in for the `llm` role on one submit, so the rest of the module still completes. Same correction in `docs/agent-kv-api.md` §11. Verification: agent_kv 302 passed, workers 74 passed 1 skipped, agent-kv-schema 143 passed. Migrations 0004/0007 exercised forward, reverse and re-forward against a real Postgres from both a fresh and a pre-seeded database; `makemigrations --check` clean. ruff 0.3.4 check + format clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ts full OCR A submit for pages 1-5 of a 400-page PDF under a 100-page cap was accepted with a 202, took a concurrency slot and staged the upload -- and the worker then failed it with "Document exceeds the 100-page limit", because the cloud side has always refused on `pages_total` while this side had moved to `pages_selected`. The premise behind that move was wrong, and the comment stating it was wrong in the file. "Pages 1-5 is five pages of OCR" is not how the engine works: it renders EVERY page (`convert_pdf_to_images(file_content, dir)`) and OCRs the whole file -- `X2Text.process` is handed `input_file_path` and no range at all -- and `load_pages(starting_page=, ending_page=)` filters afterwards, choosing only which rendered pages the agents read. A range selects what is EXTRACTED, never what is scanned or billed. Capping the range would have let a caller buy 400 pages of OCR under a 100-page limit. So the cap goes back to `pages_total`, which also puts the two repos back in agreement. `pages_selected` is still computed -- it is the honest count of what will be extracted, and the refusal message uses it to explain why a small range was refused anyway. Two tests asserted the old behaviour and have been rewritten rather than deleted: one now pins that a one-page range of a two-page document is still refused under a one-page cap, the other that this side compares the same quantity the worker does, so the two cannot drift apart again. Docs section 6.1 and the `AGENT_KV_MAX_PAGES` row said the same wrong thing and now carry the billing reason, plus a note that this is the line to revisit if OCR ever honours the range. Reported by @athul-rs on #2317 and unstract-cloud#1828 (blocking, both sides). 367 agent_kv tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`AGENT_KV_STUCK_JOB_GRACE_SECONDS` and `rate_limiter.SLOT_TTL_SECONDS` are both 21600, and phase 3 used the first as its floor and the second as its ceiling. The window was `completed_at < now-21600 AND completed_at > now-21600` -- empty, and empty for ANY grace >= TTL. So a cancelled job whose executor died held its org's concurrency slot for the full six hours exactly as it would have with no phase 3 at all, and `released` was always 0. The ceiling was wrong in a second way. It asked whether Redis still holds the slot but read `completed_at`, the CANCEL time; slots are scored by acquisition at submit (`check_and_acquire` zadds `now` and trims below `now - SLOT_TTL`). A job submitted seven hours ago and cancelled a minute ago therefore looked eligible with nothing left to release, and would have been re-selected every sweep. Both bounds now read `dispatched_at`, with a dedicated `AGENT_KV_ABANDONED_SLOT_GRACE_SECONDS` (default 3600) that must stay below the slot TTL. One hour rather than six because the job is ALREADY cancelled: a live executor learns of that at its next stage report and stops, so an hour of silence after dispatch is already strong evidence. Waiting six hours is indistinguishable from waiting for Redis to expire the slot, which is the behaviour this phase exists to improve on. The existing tests mock `AgentKVJob.objects` and assert filter kwargs, which is structurally blind to an empty window -- they stayed green throughout. Added `TestPhaseThreeActuallySelectsSomething`, DB-backed: a slot IS released, a just-dispatched job is left alone, a job past the TTL is left alone, the window is measured from dispatch rather than from the cancel, and the grace-below-TTL invariant is pinned so the two cannot meet again. The mocked test was updated to the corrected predicate and now also asserts that invariant. Mutation-checked: restoring the old bounds fails the DB-backed release test and the kwargs test together. Reported by @athul-rs on #2317. 372 agent_kv tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ntication
The execution API authenticates in its views, so its prefix goes into
`WHITELISTED_PATHS` -- and `CustomAuthMiddleware` matches that list with
`startswith`, which makes the value security-relevant in a way an env var
normally is not. Verified both failure modes against the real settings:
AGENT_KV_PATH_PREFIX="" -> entry "/" -> every path exempt
AGENT_KV_PATH_PREFIX="api" -> entry "/api" -> /api/v1/... exempt
AGENT_KV_PATH_PREFIX="api/v1" -> entry "/api/v1" -> the tenant API exempt
The second case takes key management with it, so the endpoint that mints
Agent-KV keys would stop requiring a session. Both are operator mistakes
rather than attacks, and both are silent: the server starts, every request
succeeds, and nothing anywhere says the service is now open.
Now normalised and validated at import, raising `ImproperlyConfigured` --
Django's own signal, which aborts the boot, the only outcome that cannot be
missed. Refused: empty, whitespace-only, bare slashes, and any value equal to
or an ancestor of `PATH_PREFIX` or `API_DEPLOYMENT_PATH_PREFIX`. A DESCENDANT
is still allowed (`api/v1/unstract/agent-kv` exempts only itself), as is a
value that merely shares leading characters (`deploy` vs `deployment`).
Extracted as `validate_agent_kv_path_prefix` rather than left inline so the
bad values can be enumerated in a test instead of only the good one being
checked -- 17 cases, including a guard-the-guard assertion that the shipped
default is itself accepted and present in the whitelist.
Deliberately NOT changed: the entry stays `/{prefix}` rather than
`/{prefix}/`. The trailing slash would stop `/agent-kv-anything` matching,
but nothing routes there, so the only effect I could demonstrate was turning
the documented `POST /agent-kv` -> 301 -> `/agent-kv/` redirect into a 401,
since CommonMiddleware only appends the slash after the view 404s, by which
time auth has already run. Happy to add it if the reviewer still wants it.
Mutation-checked: dropping either half of the validation fails 5 of 17.
Reported by @athul-rs on #2317. 389 agent_kv tests pass.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… retrying it
`URLField` accepts `http://`, `localhost` and private address literals. The
sink accepts none of them: `send_webhook` is called without `allow_http`, so
it permits https to public hosts only. The caller therefore got a 202 and a
webhook that never arrived, with nothing in the response contradicting the
"https-only" line in the docs.
The cancel path made it worse. `send_webhook` returns False for three
different things -- a non-2xx response, a connection failure, and a URL its
SSRF guard refuses -- and `agent_kv_cancelled` treated all three as a
delivery failure worth retrying. A refusal is a property of the URL, not of
the network, so it burned the full ~20-attempt budget and ended in a poison
drop for a caller who simply gave an http:// hook.
Both halves fixed:
* `SubmitSerializer.validate_webhook_url` applies the sink's own rule with
`webhook_url_refusal(allowed_schemes=("https",), resolve=False)`, the same
shape `notification_v2.serializers` uses. `resolve=False` keeps DNS off the
request thread -- `getaddrinfo` honours no timeout -- so a hostname pointing
inward is still caught at the sink, while scheme, credentials-in-URL, parser
disagreement and address literals are all refused up front.
* `agent_kv_cancelled` asks `is_retryable_refusal` before raising. A
permanently undeliverable URL is logged at ERROR and the message
acknowledged; only a genuinely retryable failure still raises. Keys created
before this change, and resolvers that start answering differently, can both
still reach that path.
Tests cover five refused shapes and the two accepted ones (including blank,
which means "do not notify" and must not become an error), plus a guard that
the submit check and the sink stay on the same scheme list.
Docs section 8 and the submit-field table now say the rule is enforced at
submit, not only at delivery.
Reported by @athul-rs on #2317. 397 agent_kv and 29 ide_callback tests pass.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e_callback slot `timeout` in `requests` is per socket operation, not total, and `send_webhook` read the response BODY before returning. Delivery runs inline on the `worker-pg-ide-callback` consumer -- concurrency 2 by default, and shared with Prompt Studio's own `ide_callback` queue -- so a receiver that answers 200 and then trickles one byte every nine seconds, or returns a very large body, held one of those two slots for as long as it liked, resetting the read budget on every chunk. Cheap for any key holder to arrange: submit, cancel immediately. That spends no LLM budget and still enqueues `agent_kv_cancelled`, which delivers a webhook. The docs already claimed "the response body is never read". It was aspirational -- nothing asked `requests` not to. Now `stream=True` with the response closed in a `finally`, so only the status line and headers are consumed and the connection is released without draining. The timeout is an explicit `(5, 10)` connect/read pair rather than a scalar, because the scalar read as one total budget and was not. What this does NOT fix, said plainly in the code and in docs section 8: a peer that trickles HEADERS can still extend a single delivery, since each read resets its own budget. Removing that needs delivery to leave this consumer for `notifications_webhook`, the way pipeline notifications already work -- a queue change rather than a client one, so it is recommended on the PR rather than done here. Reported by @athul-rs on #2317. 49 shared + ide_callback tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
All three found by Greptile reviewing the fixes themselves. **The sweep could free a LIVE slot.** Phase 3's floor moved to `dispatched_at`, but that column records ENQUEUEING, not pickup. Under a queue backlog longer than the grace, a job can be an hour past dispatch with an executor that has only just started -- cancel it and the next sweep frees a slot still in use, letting another submit exceed AGENT_KV_CONCURRENT_LIMIT while the executor bills on. The floor is time since the CANCEL again (`completed_at`), because cancelling is what makes a live executor stop; the ceiling stays on `dispatched_at`, which is the question it actually answers (does Redis still hold the slot, scored from acquisition). Two different columns, so the window still cannot collapse the way it did when both were `completed_at` with equal constants. The first fix got one half right and replaced the other with a different mistake. **The submit-side webhook check broke the e2e webhook scenario.** `test_webhook_delivered_on_completion` posts to `http://host.docker.internal` under `AGENT_KV_WEBHOOK_INSECURE_ALLOW_HTTP_PRIVATE`, which only the WORKER read -- so the new validation 400'd it before a job existed. The backend now honours the same waiver, and the e2e compose override sets it on the backend service as well. Verified both ways: with the waiver set the http URL is accepted, with it unset (production) it is still refused. **A docs claim that was not true.** Section 6.1 said `pages_selected` "is still computed and reported, and the refusal message uses it". Neither: nothing returns it, and the message reports `pages_total`. Now says it is internal only. New test: a job cancelled moments ago keeps its slot, which is the live-slot case. Mutation-checked -- putting the floor back on `dispatched_at` fails it and the kwargs test together. 397 agent_kv tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ose block
I added the backend's `AGENT_KV_WEBHOOK_INSECURE_ALLOW_HTTP_PRIVATE` as a
SECOND `backend:` key, in a file that already defines one at line 5. Docker
Compose's loader rejects a duplicate mapping key outright, so the e2e tier
died at `compose up` in 0.4s, before a single container started:
line 93: mapping key "backend" already defined at line 5
`check-yaml` passed it, both when I broke it and when I fixed it:
pre-commit's hook parses with `yaml.safe_load`, which resolves a duplicate
key to the last one silently rather than rejecting it. So the two loaders
disagree, and the strict one is the one that runs in CI.
The variable is now a line in the original block, which also carries the
`image:` pin and MAX_PARALLEL_FILE_BATCHES, AGENT_KV_MAX_PAGES,
AGENT_KV_STORAGE_DIR_PREFIX and AGENT_KV_CALCULATIONS_ENABLED. Verified by
loading the file with a duplicate-key-strict loader: 8 services, backend
keeps all 5 env vars and the waiver.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
f5ae5a5 to
afbb199
Compare
SonarCloud's last open issue on this PR (MINOR code smell): `waitFor` + `getByText` where Testing Library offers `findByText`, which is the same retry loop with the query built in. `waitFor` is kept in the other test, which waits on a mock call rather than on the DOM, so the import stays. Checked the assertion still bites: mutating the expected title fails the test, so this is not a query that silently finds nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both are skipped in CI (no adapter env), so neither has ever run. Both
would have gone red the first time someone configured the lane properly --
blaming the code for behaviour that is right.
**Excel scenario asserted `success` in the wrong place.** It read it off
`table_result_body()`, which is `extractors.table` -- the engine's own
record. The engine writes `tables`, `page_count`, `table_pages`, `headers`,
`row_count`, `detected` and `reason` there; the only nested `success`
belongs to `code_execution` and describes that step. So `.get("success")`
was None and a SUCCESSFUL extraction failed the assert. Now read from the
top level, like the three other `success` asserts in the file, including
the sync-wait scenario whose comment already states the rule.
**Page-cap scenario ran on a false premise at the boundary.** The
serializer refuses `pages_total > cap`, so a cap EQUAL to the page count
accepts the document -- correctly. The guard skipped only on `cap >
fixture_pages`, leaving `cap == 2` running a two-page fixture and expecting
a 400 that should never come. Now skips on `>=`; checked across caps 0-199
that the new guard never runs the scenario when the serializer would
accept. The comment also cited `invoice.pdf` while the body submits
`rent_roll.pdf` (both 2 pages, so the number was right by luck).
Also corrected the comment in `execution_views_result.py` that claimed
"each extractor keeps its own `success` inside its own block". No extractor
does. That sentence read as a contract, is what the Excel scenario was
written against, and nearly got this finding dismissed as invalid. The
change is comment-only -- verified by comparing the parsed AST before and
after, which is identical.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…se cannot lose it Review finding on #2317: the terminal webhook could be lost on a finalize replay. Finalize commits, the HTTP response is lost (timeout or 5xx), `_make_request` retries or the PG consumer redelivers, the replay finds an already-terminal row and returns `finalized: False`, and `_maybe_webhook` returns silently. The caller is never notified and nothing records the miss. It could not be fixed where it was. From inside the callback worker the "already notified" and "never notified" cases are INDISTINGUISHABLE -- nothing records whether the webhook fired -- so the declined send was the correct call from there, and a log line would have fired on the ordinary duplicate-callback path too, which is how real signals get ignored. So the notification moves to the backend, queued by whichever request won the terminal guard. That is not a new mechanism: it is exactly what the cancel path already does via `dispatch_cancelled_webhook`, and the two are mutually exclusive, so a job still gets exactly one terminal notification. Tying the enqueue to the guard rather than to an HTTP reply is the fix -- losing the response no longer loses the notification, and the queued task brings a retry budget and a visible failure, which also closes the durable- delivery gap tracked as 2.10. I had argued this belonged in a follow-up because it "crosses the repo boundary". That was wrong: `backend/` and `workers/` are both in this repo. And because `backend/agent_kv/` does not exist on main at all, the whole feature ships as one unit -- there is no deployed old worker to stay compatible with, so no flag or shim is needed. - `_enqueue_webhook` is shared by both paths so they cannot drift; the status rides on the message, so the worker announces what the backend actually terminalized the row to instead of guessing. - `agent_kv_finalized` joins `agent_kv_cancelled`, sharing one delivery helper -- the hard part, telling a retryable failure from a permanently undeliverable URL, is identical and belongs in one place. - Registered in the task-route registry. A task name missing there routes to a queue with no consumer, which accepts work and drains nothing. - `_maybe_webhook` keeps only the `result_available` follow-up, which is not a terminal transition and so has no guard to hang on. Tests: the three new view-level tests were mutation-checked -- removing the enqueue fails all three. The fourth (a replay queues nothing) passes either way by design: it guards against trading the lost webhook for a duplicate one. Four existing tests asserted the old inline send and were rewritten rather than deleted; one of them, the "delivered webhook is quiet" case, would have passed VACUOUSLY afterwards (no send means no error log), so it is now a real assertion against the task. 342 backend and 82 worker tests pass. The 64 DB-backed errors in this worktree are a local Postgres credential mismatch, confirmed identical with these changes stashed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… worker assuming Greptile on the previous commit: a NEW callback worker talking to an OLD backend drops the terminal webhook entirely. The worker has stopped sending inline, the backend does not yet enqueue, and the callback is acknowledged with nothing delivered. My argument that this cannot happen -- the feature ships as one unit, because `backend/agent_kv/` does not exist on main -- holds for a release rollout but not for a dev namespace, where mixed builds of this branch do run against each other. The three combinations: new worker + new backend -> backend queues, worker quiet -> 1 new worker + OLD backend -> nobody sends -> 0 LOST old worker + new backend -> backend queues, old worker sends -> 2 dup So the finalize response now carries `webhook_enqueued`, and the worker's inline send is kept as a fallback gated on it. Absent or false means delivery is still the worker's -- which is exactly how an older backend's response reads, so no version check is involved. That turns the middle row into one notification. The last row stays a duplicate and cannot be fixed from the new code; a duplicate is the benign direction, and receivers of an at-least-once notification have to tolerate it anyway. Writing the test for this found a flaw in my own first cut of the flag. I had set it to `bool(job.webhook_url)` -- whether the job HAS a webhook, not whether one was queued. The enqueue is best-effort and swallows its exception, so a queue that is down would have reported True and told the worker to stand down with nothing queued: the same silent loss, reached a different way. `_enqueue_webhook` now returns what it actually did and the flag is that value, so a failed enqueue falls back to the inline path. Both semantics are pinned by their own tests rather than left to the three exact-dict contract assertions, which assert False only incidentally (there is no queue in a unit test). The fallback gate is mutation-checked: force it closed and the two rollout tests fail. The fallback is transitional and marked as such in both files and in docs §8 -- it carries the original replay defect by construction, because it is the old code path. A notification that can be lost on a replay still beats one that is never sent. 344 backend and 84 worker tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
athul-rs
left a comment
There was a problem hiding this comment.
Approving. The page-cap blocker and the other findings are fixed, and the two deferrals (lost webhook on a finalize replay, and the header-trickle residual) are reasonable follow-ups. Please open tickets for both: enqueue the terminal webhook in the same transaction as the terminal write, and move delivery to notifications_webhook.
Two small things, neither blocking:
- Prefix guard gap.
CustomAuthMiddlewarematches withrequest.path.startswith(path)(custom_auth_middleware.py:63), butvalidate_agent_kv_path_prefixonly refusesprefix == otherorother.startswith(f"{prefix}/"). SoAGENT_KV_PATH_PREFIX=appasses, becomes/ap, and exempts all of/api/v1/.... Refusing whenother.startswith(prefix)closes it without changing the whitelist entry, so thePOST /agent-kv→ 301 redirect you wanted to keep still works. It does rejectdeployalongsidedeployment, which is correct understartswithmatching. - Docstring nit: the
AGENT_KV_ABANDONED_SLOT_GRACE_SECONDSdocstring insettings/base.pyopens with "How long after DISPATCH" and then says the grace is measured from the cancel.
…om ATOMIC_REQUESTS
Two Greptile findings on the main-merge, both valid.
**1. A swept job notified nobody.** `run_sweep` is the THIRD route to a
terminal state, after finalize and cancel, and docs §8 promises a POST on
every one. Phases 1 and 2 mark a job FAILED -- dispatch lost, or executor
dead -- release its slot and send nothing. No later callback can cover for
it: one arriving afterwards finds an already-terminal row, is told
`finalized: False`, and correctly declines to send, because from the worker
a non-fresh finalize is indistinguishable from the ordinary duplicate. So a
caller whose job timed out was told it was accepted and then never heard
again.
This predates today's change rather than being caused by it -- the sweep
never sent a webhook -- but the change makes it a two-line fix and raises
the bar: the backend now owns terminal notifications, and the sweep was the
only terminalizing path left not doing it. Both phases now queue
`dispatch_terminal_webhook(job, "failed")`, inside the `if won` that already
gates the slot release, so the guard decides who notifies, exactly as on the
other two paths.
Phase 3 deliberately still sends nothing, and that asymmetry is now written
down: it terminalizes nothing, it only frees a slot on a row that is already
CANCELLED and whose notification went out when the cancel won.
**2. Sync-wait would always time out under ATOMIC_REQUESTS.** A sync-wait
submit polls for the job to terminalize while still inside the request. With
`DJANGO_ATOMIC_REQUESTS=True` neither the job row nor the queue insert is
committed during that poll, so no worker can see the work, the job cannot
start, and every positive `timeout` burns its full duration and returns 202
-- silently; the caller just never gets the inline result.
The flag defaults to False and ships False, so nothing hits this today. The
same precondition is already documented for the RPC path in
`pg_queue/executor_rpc.py` ("dispatch must NOT be called inside an open
transaction"), so this is a known platform-wide hazard rather than an
agent-kv one -- but the view costs one wrapper to make correct either way,
and it needs nothing from an implicit transaction: the job insert is one
statement and every terminal write is its own guarded UPDATE. Applied in the
URLconf, which is where Django reads the marker from; applying it to the
class would silently not take.
Tests: four for the sweep (both phases notify; phase 3 does not; a job whose
guard was LOST notifies nobody and releases nothing). Mock-backed, not
`TestCase`, so they assert the calls `run_sweep` makes and run without a
database. Mutation-checked -- drop the two enqueues and both positive tests
fail. The two negatives pass either way by design: they guard against the
notification widening, not against its absence. Two more for the exemption,
asserted through the resolved URLconf rather than the imported class, since
the class-level assertion would pass while the routed view stayed atomic;
also mutation-checked.
350 backend and 84 worker tests pass.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…her API @ritwik-g on unstract-cloud#1828: "why did we deviate from the /api/v1? Any specific reason? Else we should consider moving this under that prefix and removing these rules." There was no specific reason, so it no longer deviates. `AGENT_KV_PATH_PREFIX` now defaults to `api/v1/agent-kv` instead of `agent-kv`, and the routing rules that existed only to reach a top-level mount come out with it: - the cloud ingress entry, and its repeat in `dev.base.values.yaml` - the traefik terms in `docker/docker-compose.yaml` -- BOTH of them, the backend's `PathPrefix` and the frontend's negation of it That pair is why the move is worth making rather than just tidy. A top-level mount had to be added to the backend rule AND removed from the frontend's negation, in every file that redefines the list; miss either and the SPA answers the API with a 200 and an HTML page, which is not a 404 and shows up in no log. It had already happened twice: once in compose (branch review 2.2) and once on-prem, where all three values files replace `ingress.hosts` and none carried `/agent-kv`. It is NOT under `TENANT_SUBFOLDER_PREFIX` (`api/v1/unstract`): the API resolves its organisation from the Bearer key, not a URL segment. The whitelist entry is still needed -- the views own their auth -- and the startup guard still refuses a prefix that is empty or collides with `PATH_PREFIX` / `API_DEPLOYMENT_PATH_PREFIX`; a descendant like this one was always allowed. Verified by resolving the real URLconf rather than reading it: all four routes resolve under `/api/v1/agent-kv/` and the whitelist entry is present. That mattered, because `api/v1/` is `include`d BEFORE the Agent-KV mount -- Django falls through when no sub-pattern matches, but this proves it does. The traefik test is rewritten to assert the coupling is GONE (no `/agent-kv` term, `/api/v1` present) instead of asserting the term exists, and its companion `test_traefik_excludes_agent_kv_from_the_frontend` is deleted -- the general complement invariant below it already covers the negation list. Docs, both sample.envs and the e2e URL helpers follow. The internal worker API (`/internal/v1/agent-kv/...`) is untouched; so is key management, which was always tenant-scoped. 350 backend and 12 worker-wiring tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…the docs Greptile on #2317: the prefix change also rewrote the key-management paths, which did not move. An admin following those examples would get a 404. My own regex did it. The pattern required the character before `/agent-kv/` to be neither a word character nor a slash, to avoid touching `/internal/v1/agent-kv/...` -- but the docs write the management routes as `.../agent-kv/keys/`, and `.` satisfies that, so all three code-block lines and the authentication table were rewritten too. The two surfaces are genuinely different mounts, which is the thing to hold on to: execution base_urls.py {AGENT_KV_PATH_PREFIX}/ -> /api/v1/agent-kv/ key management urls_v2.py agent-kv/ under the -> /api/v1/unstract/ tenant prefix {org}/agent-kv/keys/ Only the first moved. Restored the four management paths and confirmed the split by resolving the real URLconf rather than re-reading the diff. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Frontend Lint Report (Biome)✅ All checks passed! No linting or formatting issues found. |
|
Unstract test resultsPer-group results
Critical paths
|



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.tableextractor runs on the CALLER's platform adapters, not operator env credentials. The submit namesllm/lite_llm/x2textadapter ids and the API validates each against the Bearer key's organization, itsAdapterTypes, and whether it is usable. This reverses spec D6 and is the largest functional change on the branch — see Why.kvtotable, and inverts two guards rather than deleting them.ApiKeyManagerscreen that already backs Platform API keys and Global API deployment keys. Until now the only way to obtain a key was to POST to the management endpoint by hand, so the honest answer to "how does a user get one?" was "open devtools".llm/lite_llm/x2textby instance id, and that id was in no screen — the card shows the adapter name, so the first call answeredMust be a valid UUIDthree times with no way forward but the network tab.ResourceTablegains an opt-inhandleCopyId, wired byToolSettings.detected/reason, cloud side). A run that matches no page is a successful run and it bills;tables: []withrow_count: 0previously looked identical whether detection found nothing or extraction returned nothing, and the only way to tell was which keys were missing fromtiming.listnow masks to****-<last 4>and addscreated_by_email;retrieve/create/rotatecarry the full value, each being a deliberate request for one key. Mirrorsplatform_api, which settled this first.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.Why adapters replaced env credentials (spec D6 reversed). D6 specified the blind API would have "no platform adapters by design", with models and OCR credentials env-configured and "no end-user model control". That was right while the consumer was hypothetical. It is wrong now that the consumers are existing customers with accounts: they already own adapters, and there was no operator-level LLM credential anywhere in the platform to reuse. Confirmed empirically rather than assumed — the only LLMWhisperer credential in a dev namespace is
LLMWHISPERER_PORTAL_KEY, anX-PORTAL-KEYadmin provisioning key (wrong type and over-privileged), and there is no operator LLM key in env or in DB adapters at all.docs/agent-kv-api.md§3a documents the contract and the reversal.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.is_usableandis_availableneed both a DB and the Bearer key, so they live in the view — and run before the job row, the concurrency slot and the staging write, so a bad submit is a 400 that costs nothing.is_usableis the one with billing teeth: it is how billing cuts off a frictionlessly onboarded org's operator-funded trial, and the platform service does not check it, so without this gate an exhausted trial that the IDE, workflows and Prompt Studio all refuse would still run here on credentials the operator pays for.platform-servicere-scopes every adapter lookup asWHERE id=%s and organization_id=%s, against the org of the platform keydispatch._platform_api_key(job)mints from the job's own org — so org A cannot spend org B's credential even with this gate removed. Correction to an earlier version of this description and to four code comments: they claimed the submit-time check was the only one. What the gate actually buys is a clean 400 naming the role instead of a mid-runSdkError, plus the two refusals the platform service does not make (is_usable,is_available).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:
A
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.The generated-code trust boundary is unchanged but more reachable.
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.Quota admission is not wired. The entitlement gate ships (402 on expired trial or inactive subscription); an org inside a valid subscription but over its allowance can still run and bill. Tracked as UN-4216 and consciously deferred for this release.
adaptersinexecutor_params; an old worker has noParamKeys.ADAPTERS, ignores the key, and callsExtractionConfig.from_env(). Where the sixAGENT_KV_*values are set it runs on operator credentials, bills theagent_kv:{provider}/{model}sentinel, and returns a 202 with a correct-looking result — while the adapters the caller named, and is paying for, were never used. Nothing in either repo's logs reveals it. The reverse order is safe by construction: the new worker hard-fails "Missing required adapters". Mitigation beyond ordering: leaveglobal.sharedConfigs.agentKvunset in every table-serving namespace, so an old worker fails loudly instead. Documented in §12 of the API doc and at the chart group.The sweep and TTL cleanup have exactly one owner, and it is the cloud CronJob. Migration
0004seeded twoPgPeriodicTaskrows on the premise that nothing fired these commands; that premise was false about its own branch — the cloud chart already ran them. The rows were seeded inert (pg_owned: False), and0007scoped its delete to that flag so a self-hosted operator who had adopted them was not stranded — on the premise that only a human ever sets it. That premise is false and the collision was live. The cloud chart'sbackend.pgScheduleMirrorhook runsconverge_pg_scheduler --periodicson every install, upgrade and rollback; that callsmirror_pg_periodic_tasks --adoptwith no names, which isPgPeriodicTask.objects.all(); andvalidate-pg-worker-fleet.yamlrefuses to render with the hook off. Deployingsnapshot.415foundagent_kv_sweepatpg_owned=t,last_run 11:00:02 / next_run 11:10, firing every 10 minutes beside the CronJob's*/15— meeting at:30, with ttl-cleanup daily on one owner and hourly on the other.0007's guard spares precisely the rows that double-fire, so0009repeats the delete by name alone; it has to be a new migration because0007is recorded as applied wherever the problem exists. Self-hosted OSS operators must schedulemanage.py agent_kv_sweepandmanage.py agent_kv_ttl_cleanupthemselves — see §12. Without them a stranded job holds its concurrency slot until the 6 h Redis TTL andAGENT_KV_RESULT_TTL_DAYSis advisory, which is a retention failure rather than a disk-usage one. (Note a pure-OSS install cannot dispatch an Agent-KV job at all, sincetableneeds the cloud plugin, so there is nothing to reap until an engine plugin ships.)The page cap now bounds
pages_selected, not the document's length. Checked afterpage_start/page_end, so pages 1–50 of a 400-page PDF is accepted under a 100-page cap while the same document with no range is refused. Intended — the cap bounds paid work per job — but it is a behaviour change that was previously undocumented. Now in §6.1 and the env table.Not yet validated outside compose with real adapters. The branch is deployed to
vishnu-unstract-devatsnapshot.382(Synced/Healthy, migrations applied), but no job has run end to end on real adapters — that needs an LLM adapter, a cheaper LLM adapter and an LLMWhisperer X2TEXT adapter created in the dev org.Key-management response shapes changed (
listmasks the key and gainscreated_by_email;createrequires a sanitiseddescription; a duplicate name per org is now a 400 rather than the model's unique constraint surfacing as a 500). No external breakage: these endpoints are new in this PR and had no consumer before the page landed.The table result gains
detectedandreason(always present;reasonis""on a successful extraction). Additive, and the companion change is inunstract-cloud#1828— the backend serves the stored engine output verbatim, so no OSS code change was needed. Three whole-dict assertions in the engine's own suite needed the new keys and were updated rather than loosened.Database Migrations
Nine, all for the new
agent_kvapp — no existing table is touched, with one exception noted below: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_at0004_pg_periodic_tasks— superseded by0007/0009; left seeding deliberately. Django records a migration as applied and never re-runs it, so rewriting this one would have cleaned nothing in the environments that actually have the rows while silently changing what a fresh install does.0005_agentkvjob_extractor_choices— addschoicesand drops the default, so an unknown extractor name cannot reach the column0006_agentkvjob_adapters— addsadapters(JSONB,{}default). Additive; existing rows predate caller-supplied adapters and genuinely ran on env credentials, which{}honestly represents0007_remove_agent_kv_periodic_tasks— deletes0004's rows, scoped by name and bypg_owned=False. Incomplete; see0009. Left exactly as applied: it is recorded in every environment that has the problem, so widening its predicate here would reach none of them.0008_agentkvjob_files_purged— addsfiles_purged(bool,default=False). Records purge intent directly instead of inferring it frominput_ref/result_ref, which mean "never set", "purged" and "purge attempted but the delete failed" at different times. A plainAddFieldwith a non-volatile default is metadata-only on PG 11+ — no rewrite, no backfill scan0009_drop_agent_kv_periodic_tasks— repeats0007's delete without the ownership scope, because the chart's schedule-mirror hook adopts every row inpg_periodic_taskon every sync, so0007spared exactly the rows that were double-firing. Touchespg_queue's sharedpg_periodic_tasktable; the name scope is the load-bearing one now and the fivedashboard_metricsperiodics are untouched. Reverse is a documented no-op —0007's reverse already restores the pair for a rollback that goes back past it, and restoring them here would put the second owner back. The self-hosted operator0007's guard protected cannot exist at the point either migration runs:0004and its removals ship in one release, and an operator who creates their own rows afterwards is untouched because0009is already recorded appliedEnv 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_URL— none of these six is read on thetablepath, and they must be left UNSET in any namespace serving it. They are the operator's credentials, read only by the executor'sExtractionConfig.from_env();tableresolves the caller's adapters instead. Leaving them set is what makes the deploy-order skew above silent rather than loud. They remain the credential model for the dormantkvextractor.AGENT_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_ENABLEDTwo operator knobs are read on the table path, independently of the credential group:
AGENT_KV_STORAGE_DIR_PREFIX(must match the backend's — the executor derives the job's expected directory from it to enforce the tenant boundary on the staged input) andAGENT_KV_PARALLEL_PAGES.The e2e lane gains three required and one optional variable, none of them product config:
AGENT_KV_E2E_LLM_ADAPTER,AGENT_KV_E2E_LITE_LLM_ADAPTER,AGENT_KV_E2E_X2TEXT_ADAPTER(adapter ids in the key's org; without them every accepted-submit scenario skips rather than fails) andAGENT_KV_E2E_BAD_LLM_ADAPTER(an adapter carrying a deliberately invalid credential, for the one scenario that needs a dispatched job to end FAILED). This replacesAGENT_KV_E2E_BAD_KEY_JOB, which the table path no longer reads — an operator following the old instructions got a job that ran on a valid credential, reachedcompleted, and failed the test's own assertion.No
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. Added since: §3a (the adapter contract — the three roles, the four400cases, and the D6 reversal stated as a reversal), theadaptersfield in the §7.2 status document, the deploy-order warning and the self-hosted scheduling requirement in §12, the corrected page-cap semantics (pages_selected, not document length), and the rewritten e2e instructions in §11.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.Note this is the merge order; the DEPLOY order is the opposite — cloud worker first (see the risks section). Merging this one first is safe because the new worker refuses a submit with no adapters rather than falling back to env.
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 (118 / 162 / 698), 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→ 366 passed, 1 skippedfrontend—vitest→ 627 passed (40 files), biome clean on the new filesworkers—ide_callback/tests,shared/infrastructure/config/tests,shared/tests/test_webhook_notify.py,shared/tests/test_agent_kv_client.py,tests/test_agent_kv_scheduler_tasks.py,tests/test_queue_consumer_wiring.py,tests/test_llm_bearing_ops.py→ 74 passed, 1 skippedunstract/agent-kv-schema→ 143 passed;unstract/sdk1→ 766 passed0001–0009applied against a real Postgres from a fresh database and from one pre-seeded with0004's rows; reversed and re-applied;makemigrations --checkclean0006,UPDATE pg_periodic_task SET pg_owned = trueas the hook does, migrate to0007(both rows survive — the bug), then to0009(both gone, the fivedashboard_metricsperiodics intact). The in-process replay test fails if the ownership scope comes back, and nothing else doesruff0.3.4 (the pinned pre-commit version) clean oncheckandformat; the full pre-commit suite passesMutation-checked, because each is silent when broken. Every fix in the round-2 review pass was verified by reverting it and confirming the new test fails:
_base_manager, not.objects.AdapterInstance's default manager auto-filters byUserContext.get_organization(), a request-local thread-local — and/agent-kv/is deliberately whitelisted past the tenant middleware, so that value is unset and the ambient filter becameorganization=None. Every valid submit would have 400'd. Thirteen passing view tests could not see it because they all mocked the manager.tests/test_adapter_scoping.pyis DB-backed with the thread-local unset, which is the only arrangement that reproduces it._never_dispatchednow reads two durable markers from the database (dispatched_atandstage, written by the first stage report) rather than a snapshot taken beforemark_terminal. Both a stale-row read and a RUNNING-with-NULL-dispatched_atrow are covered.is_usable/is_availablerefuse at submit — an exhausted trial or a deprecated adapter 400s before the slot and the staging write.EXTRACTOR_ROUTES, so re-enablingkvflips the expectation automatically instead of requiring a guard to be deleted. Also coversrun-worker-docker.sh, which was guarded by nothing — droppingagent_kv_callbackthere passed every test in the tree while a docker-launched fleet drained no terminal callbacks._LLM_BEARING_OPS— derived, because the two hand-maintained literals could only fail when they disagreed, never when both were wrong.format: "Enum:paid,unpaid"is refused._parse_formatmatches the prefix case-sensitively, so the mis-cased form slipped the typo guard and validation was silently off for that key. Scoped to the two argument-taking formats, so"number: of items"stays valid free text.Worth verifying by hand on review:
Settings → Agent-KV API Keys lists keys with a masked value, creates one (full key copied to the clipboard), rotates, disables and deletes
A non-admin cannot reach that page (
RequireAdminon the route,IsOrganizationAdminon the viewset)Settings → LLMs → a row's Copy ID button puts the adapter's instance UUID on the clipboard, and that value is accepted by a submit
A
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 outsideA
kvsubmit returns 400, not202— the single most important subtraction in this PRA
tablesubmit naming adapters from another organization is refused, and refused identically to one naming an id that does not existA submit with no
adaptersblock is a 400 namingextractors, andGET /agent-kv/{job}echoes the adapter ids the job ran onCancel mid-run stops the run and releases the concurrency slot
An
.xlsxsubmit extracts and is capped post-OCRPage usage and token/cost land on the standard
Audit()pathNo queue is advertised without a consumer — check against the PG fleet guard, not by eyeballing compose
Run end to end on real adapters.
snapshot.415onvishnu-unstract-dev(Synced/Healthy, all migrations applied): atablesubmit returned202, completed in ~44 s with 6 rows off a 2-page rent roll, and billed33,386in /1,338out tokens at$0.144095across 2 pages. Also verified there, none of it reachable from a test: an unauthenticatedPOST /agent-kv/returns 403 rather than the frontend's HTML shell; the three adapter-gate refusals (missing roles, unknown id,X2TEXTsupplied in thellmslot) each 400 with the documented message; the status document echoesadaptersand carries a non-emptystages; thePageUsagerow carriesfile_name='input.pdf'; flippingis_usable=Falseon the LLM adapter refuses the submit with the trial-exhausted message before any spend (flag restored afterwards); and cancel returns the documented{"success": false, "status": "cancelled"}.That deploy is also what found the double-fire above — no unit test could, since it only exists once the chart's hook has run against a database.
Re-deployed since as
snapshot.428.snapshot.426added the keys page and themasking split. Verified on that deployment: both page modules compile and are
served by Vite, all three key routes resolve to
AgentKVKeyViewSet, and a realkey created in the dev org serialises as
****-b1a8inlist(the real valueabsent from the whole payload) and in full from
retrieve. The page itself waschecked in the browser by the author.
The same deploy confirmed migration
0009: the twopg_periodic_taskrows thatwere live at
pg_owned=truebefore it are gone, with the fivedashboard_metricsrows untouched, so the CronJobs are the only owner again.A Postman collection covering submit / status / result / cancel / delete and
the six documented refusals is attached to the ticket.
Still unverified by hand: an
.xlsxsubmit, and akvsubmit returning 400(asserted in tests, and refused live during the snapshot.425 run).
Two commits here are not this branch's subject, called out so a reviewer can skip them.
[MISC] Remove the dead ListOfItems adapter carddeletes a card component that nothing imports — every adapter screen rendersToolSettings, which usesResourceTable. It was briefly its own PR (#2337, now closed) and folded in here on request. It matters to this branch because the Copy ID action was first added to that dead file and shipped unreachable; the file reads exactly like the adapter list and will mislead the next person too. Verified by building:vite buildresolves every import and succeeds.And:
test_wfe_status_created_idx_planner.py(UN-3883 / #2276, untouched by this feature) backdates rows with(random() * n || ' days')::interval, which round-trips the float through its own text form — and Postgres renders a small enough double in scientific notation, which::intervalrejects:invalid input syntax for type interval: "7.418860519248227e-05 days". It failed CI twice on this PR and was the only thing red, so it is fixed here rather than left to make the PR look broken:random() * n * INTERVAL '1 day'never formats anything. Verified against a real Postgres rather than by re-running, since at ~1-in-10k a green run proves nothing — the exact value from the CI log raises with the old form and gives00:03:12.296865with the new one, and the midpoint and maximum still give 15 and 30 days. One SQL string in one fixture; happy to pull it onto its own branch if you would rather.Screenshots
N/A
Checklist
I have read and understood the Contribution Guidelines.
🤖 Generated with Claude Code