Repository navigation
feat(subagents): add acceptance checks to durable batch items - #5289
Conversation
Carry optional per-item criteria into native subagents, reuse the deterministic checker, and expose separate verdicts through item queries and exports. Preserve execution and retry semantics, renew leases during checks, and migrate existing batch rows with nullable acceptance fields.
There was a problem hiding this comment.
🟡 Changes recommended
acceptance_criteria is persisted/exported without bounding/normalization, which can bloat durable rows and exported payloads (and can store semantically-empty criteria as non-null) despite later checker/executor hygiene.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds deterministic acceptance-check plumbing to durable batch_task items, bringing them closer to ordinary task delegations by persisting per-item criteria, running the existing checklist against owner-scoped thread files / recorded bash evidence, and exposing a validated verdict in read APIs and exports.
Changes:
- Persist optional per-item
acceptance_criteriaand nullableacceptance_verdictfor durable batch items (including migration + bootstrap/head assertions). - Run acceptance checks for completed items off-loop with sandbox authorization/lease holding and durable item-lease renewal while waiting; lost leases block publication.
- Expand API/export surfaces and add targeted unit + blocking-IO coverage for durability, authz scoping, and lease-loss behavior.
File summaries
| File | Description |
|---|---|
| backend/tests/test_persistence_forward_revision_compat.py | Updates persistence head constant to 0019_batch_acceptance. |
| backend/tests/test_persistence_bootstrap.py | Updates persistence head constant to 0019_batch_acceptance. |
| backend/tests/test_persistence_bootstrap_regression.py | Updates regression assertions for new schema head. |
| backend/tests/test_persistence_bootstrap_concurrency.py | Updates concurrency test head constant to 0019_batch_acceptance. |
| backend/tests/test_migration_0019_batch_acceptance.py | New migration tests for upgrade/downgrade preservation and forward-compat column requirements. |
| backend/tests/test_migration_0015_scheduled_task_enqueue.py | Updates expected final version after bootstrap to new head. |
| backend/tests/test_migration_0007_scheduled_run_active_dedupe.py | Updates expected final version after bootstrap to new head. |
| backend/tests/test_migration_0004_run_ownership_dedupe.py | Updates expected final version after bootstrap to new head. |
| backend/tests/test_batch_acceptance.py | New end-to-end-ish tests for submission, persistence, verdict behavior, owner scoping, and lease-loss handling. |
| backend/tests/blocking_io/test_batch_acceptance.py | New blocking-IO anchor tests ensuring checklist file reads are offloaded and cancellation-safe w.r.t. sandbox lease release. |
| backend/packages/harness/deerflow/tools/builtins/batch_task_tool.py | Adds per-item acceptance_criteria to the batch tool schema and passes it through submission. |
| backend/packages/harness/deerflow/tools/AGENTS.md | Documents batch-item acceptance criteria semantics and the separation of execution vs acceptance. |
| backend/packages/harness/deerflow/subagents/batch_service.py | Wires criteria into executor inputs and records acceptance verdicts for completed items with lease-renewal gating. |
| backend/packages/harness/deerflow/subagents/batch_runtime.py | Tightens batch item typing with BatchItemInput TypedDict including optional criteria. |
| backend/packages/harness/deerflow/subagents/batch_acceptance.py | New helper to run the existing acceptance checklist for durable batches with owner-scoped sandbox authorization/lease. |
| backend/packages/harness/deerflow/subagents/AGENTS.md | Documents durable batch acceptance architecture and guarantees (leases, off-loop reads, advisory verdict). |
| backend/packages/harness/deerflow/persistence/subagent_batches/sql.py | Persists/returns criteria and validates persisted verdict shape on read; stores verdict on finalize. |
| backend/packages/harness/deerflow/persistence/subagent_batches/model.py | Adds nullable JSON columns for acceptance_criteria and acceptance_verdict. |
| backend/packages/harness/deerflow/persistence/migrations/versions/0019_batch_acceptance.py | New migration adding the nullable acceptance columns to subagent_batch_items. |
| backend/packages/harness/deerflow/persistence/migrations/AGENTS.md | Updates migration forward-compatibility documentation to require batch-acceptance columns under the incarnation-only revision. |
| backend/packages/harness/deerflow/persistence/bootstrap.py | Enforces required batch-acceptance columns when encountering the forward-compatible 0019_thread_incarnations revision (including SQLite race recovery path). |
Review details
- Files reviewed: 21/21 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
@AnnaSuSu Found one new reproducible issue in the sandbox authorization/lease boundary, separate from the existing criteria-normalization comment. Validation: 80 related tests passed with one optional PostgreSQL test skipped; all 119 blocking-I/O tests and targeted lint/format checks passed. Additional regression cases reproduce the issue below.
| if any(isinstance(criterion, str) and criterion.strip().lower().startswith(("file:", "file_written:")) for criterion in criteria): | ||
| await authorize_sandbox_execution_async(context=context, app_config=app_config) | ||
| provider = await run_sync_lifecycle_operation(get_sandbox_provider) | ||
| lease = await acquire_sandbox_client_lease(provider, thread_id, user_id=user_id, owner_prefix="batch-acceptance") | ||
| runtime.state["sandbox"] = {"sandbox_id": lease.sandbox_id} | ||
| context[SANDBOX_LEASE_OWNER_CONTEXT_KEY] = lease.owner_id |
There was a problem hiding this comment.
[P2] Match the checker's criterion syntax before sandbox access
The lower().startswith() gate disagrees with the checker's Unicode-aware re.IGNORECASE patterns: fıle: (dotless i, U+0131) and FİLE: (U+0130) are accepted as file checks but skip authorization and lease acquisition here. I reproduced this with a caller-owned configuration denying sandbox access and no global authorization policy: ASCII file:../outputs/report.md exists is denied with no acquisition, while both Unicode variants skip authorize_sandbox_execution_async and call provider.acquire('thread-1', user_id='user-1') through the checker's ensure_sandbox_initialized fallback, which consults global configuration instead. Since lease remains None, that acquisition also bypasses holder cleanup. Please share the same criterion classification between this gate and the checker, or reject these spellings consistently, and add coverage for the Unicode case-folding variants.
There was a problem hiding this comment.
Confirmed and fixed in 4b6a7b4. Sandbox admission and the checklist now call the same parse_file_criterion on criteria from the shared normalizer. The gate therefore recognizes the checker's Unicode spellings and cannot fall through to lazy acquisition without the caller's authorization and a managed holder. Conditions dropped by the count/length cap, and malformed file conditions, do not request a sandbox.
Added deny/allow controls for ASCII plus fıle, FİLE, fıle_written, and FİLE_WRİTTEN. The deny cases use the real RBAC policy with no global policy and assert zero provider acquisition; allowed cases read an actual file and assert holder acquisition/release.
Integrated upstream Projects in 66e0487: the migration is now 0021_batch_acceptance, following 0020_threads_meta_project_id, and uses the upstream schema-floor guard. Final validation: 468 related tests passed (11 skipped), 49 route/project permission tests passed, all 119 blocking-I/O tests passed, and both native replays passed. Full backend: 14,363 passed / 15 environment failures. Clean upstream 05dc8f4 had 19 failures; the four stale project-permission expectations were aligned, and the remaining 15 failure IDs match exactly.
|
@AnnaSuSu please resolve the conflict with the main branch. |
Chain batch acceptance after the project migrations and retain the upstream schema guard. Align auth/me test expectations with project route permissions.
There was a problem hiding this comment.
@AnnaSuSu Reviewed at e4238f4. Both earlier findings are fixed at head: repository insertion now goes through the shared normalize_acceptance_criteria (I re-verified idempotency with a 20k-case fuzz over tag/boundary/Unicode inputs, including expansion past the 500-char cap), and sandbox admission shares parse_file_criterion with the checker on the same effective list, so every recognized file spelling is authorized and leased before any read (the deny path is pinned for all seven spellings). Re-ran at head: the new suites (39 passed, including the blocking-IO drain cases), the pre-existing acceptance/batch/contract suites (267 passed, 10 skipped), and the migration/bootstrap suites (37 passed); ruff check and ruff format --check are clean. One new non-blocking observation below about capacity accounting while a check is in flight.
| cleanup_background_task(execution_id) | ||
|
|
||
| async def _check_acceptance_with_lease(self, item, result, app_config): | ||
| """Keep a completed execution leased until its advisory check drains.""" |
There was a problem hiding this comment.
[suggestion] Holding the lease here is required, but the item's worker slot is also held for the whole check, so checker IO can throttle execution throughput. run_once claims max_running - len(self._executions) items and claim_items sizes batch_available from leased + running, while this _execute_item task stays in _executions and the item stays leased/running until the verdict is in — so with default_max_running_items = 3, three items that finish together leave zero slots for new executions until their sandbox reads drain. The lease must survive the check; the concurrency accounting does not have to, since the checker is bounded sandbox IO rather than model work. Fine for small batches, but worth tracking checks outside the max_running/max_running_items budget (or at least measuring the gap) before large file:-criteria batches run against a remote sandbox provider.
There was a problem hiding this comment.
Confirmed at e4238f4 with a controlled checker and real SQLite scheduling. With six items and both limits set to three, all three completed executions remain tracked during the check, and neither another local poll nor another worker can claim the remaining items. Separate process-limit and batch-limit controls reproduce each gate independently; without criteria, the next poll starts the remaining three items. All four probe cases passed. This establishes the scheduling effect, not remote-provider latency or a production throughput estimate.
I suggest keeping the current bounded behavior for this PR and documenting the tradeoff. A follow-up can separate execution and verification accounting at both the process and durable batch levels, with a bounded verification backlog and preserved lease renewal, cancellation/drain ownership, and crash recovery. Simply removing checking tasks from _executions would leave the SQL limit in place and also remove those tasks from shutdown tracking.
Why
Ordinary
taskdelegations can supply deterministic acceptance criteria, but durablebatch_taskitems currently retain only a key and prompt. A batch worker can claim that a missing deliverable exists and still appear as a succeeded item with no recorded acceptance check.This brings the existing checklist to durable batches, following #4998 and #5109. Related background: #4651.
What changed
acceptance_criteria, normalizes them before persistence (20 usable entries, 500 characters after neutralization; empty becomes null), preserves them across lease recovery, and passes them to the native executor's existing untrusted task input.acceptance_criteriaand a separately validatedacceptance_verdict. A completed item with an existing report, a missing CSV, and an unsupported quality claim remainssucceeded, with respectivelyholds,does not hold, andUNVERIFIEDleaves. It is not automatically retried.0021_batch_acceptanceafter0020_threads_meta_project_id. Update schema-head assertions and use the upstream metadata-derived schema guard to require the batch columns on both0019_thread_incarnationsforward-compatibility paths.The migration follows the Projects migrations merged in #5265. Upgrade/downgrade tests cover both the legacy 0018 schema and the project-enabled 0020 schema, preserving existing batch results and project rows. This does not implement the separate child-teardown changes in #5221/#5253. Lead/ordinary-task follow-up guidance was merged separately in #5287.
The normalization boundary has four consumers: repository insertion, the native executor's report renderer, the batch-check entry point, and the common checker. All use
normalize_acceptance_criteria; normalization is stable across persistence and rechecking, including tag expansion at the character cap. File classification has two consumers, sandbox admission and common checking, both usingparse_file_criterion. Query/export projections reuse the stored criteria.Surface area
Bug fix verification
Four batch regressions fail against unmodified
48bbea6dand pass with this change: submission propagation, mixed durable verdicts, lease-recovery persistence, and owner scope. The native replay also completes on base but has noacceptance_verdict; on the branch it records the expected three different outcomes despite a false success claim in the report.Review regressions cover Unicode file/file_written spellings under a real deny policy with no global policy, allowed reads and holder release, effective-list-only sandbox admission, oversized/empty input, and tag expansion/truncation. Fourteen cases failed on the original PR head, and an additional truncation case failed before its follow-up fix; ASCII controls passed.
Additional coverage checks legacy schema upgrade/downgrade with preserved rows, missing columns on both forward-compatibility paths, checker failure isolation, failed/no-criteria execution, authorization for case-insensitive criterion names, lost leases, real blocking file reads, and repeated cancellation before holder release.
The upstream merge also introduced project route permissions alongside the new
/auth/metests. The backend/auth/metests and frontend auth-disabled contract now include the three project permissions. The four backend failures reproduced on clean upstream, and the frontend contract failed in CI before these test-only alignments.Validation
make formatandmake lint: passed.pnpm format,pnpm check(lint + types), and production build: passed; frontend unit tests: 1,209 passed. Full-stack Playwright in CI mode: 3 passed. The first local run passed the auth contract and DOM assertions but lacked a macOS screenshot baseline; CI mode uses the repository's DOM gate and captures screenshots as artifacts./auth/metests: 49 passed.make test-blocking-io: 119 passed.make test: 14,363 passed / 15 failed / 119 skipped. Clean upstream05dc8f41baseline: 14,323 passed / 19 failed / 119 skipped. The four stale/auth/mepermission expectations were aligned with the merged project routes; the remaining 15 failure IDs exactly match the previously established browser/web-fetch DNS/SSRF environment failures. No new failures.SubagentExecutor, agent graph, four model turns, real file tools, SQLite persistence, and the production checker. The report was readable, the missing CSV was rejected, and the semantic claim remained unverified. A second native replay ran a real unittest command and confirmedtests_passedfrom executor-harvested evidence. These validate the plumbing; they are not LLM quality benchmarks. No additional paid calls were used.AI assistance
Tool(s) used: Codex.
How you used it: Traced the existing task and batch contracts, implemented the integration, reviewed the changes, and ran regressions plus deterministic native replay.