Repository navigation
Conversation
AnnaSuSu
left a comment
There was a problem hiding this comment.
Reviewed 0396672f. I found two recovery/isolation issues below that I recommend fixing before merging.
Validation: the combined relevant local test run finished with 882 passed, 1 failed, and 1 skipped. The failure was test_sigkill_recovers_without_replay_or_double_version[file_synced-ABORTED] (APPLIED rather than ABORTED); its isolated rerun passed. I have not established the intermittent failure's cause, so I am recording it as a validation observation rather than a third confirmed implementation defect. The skipped test requires live PostgreSQL; I did not run the full backend suite or live PostgreSQL concurrency locally.
Separate reproduction probes confirmed both findings, with passing controls. Hosted CI has no failed checks on this head.
willem-bd
left a comment
There was a problem hiding this comment.
Reviewed 0396672f (full 89-file diff). I re-ran the new suites at head with PYTHONPATH pinned to the head tree: test_skill_mutations, guard, validation, scanner, workers, completed_run_evidence, evidence admission, asset versions, host lifecycle, host_access, admin, topology, async reads, package cost, evolution integration, and migration 0028 all pass (179 passed, 1 skipped, 1 PostgreSQL-opt-in); the only failure was the file_synced-ABORTED SIGKILL case already recorded as a validation observation in the existing review. The two significant findings I could reproduce (terminal-operation legacy writes leaving owners stuck in NEEDS_REPAIR, and public-skill toggles acquiring every enrolled owner's lock) are already raised in the existing thread, so I am not duplicating them. Below are three smaller new items, none blocking.
willem-bd
left a comment
There was a problem hiding this comment.
Re-reviewed 3c4eb55d (delta since 0396672f, 9 files). The follow-up commit resolves both open review findings — recover_locked now re-observes interrupted legacy writes tied to terminal operations, with the requested publication → failed manual edit → recovery regression, and the global toggle now takes a per-name fence and only reserves owners that actually have the affected asset, with both requested concurrency tests — plus all three smaller items from the prior round (created_at keyset ordering for list_operations, scope_digest/revision_for_run accessors replacing the private-state reach-through, and INVALID_ASSESSMENT fail-closed wrapping with a true JSON-size bound). I re-ran the mutation/evidence/host suites at this head with PYTHONPATH pinned to the head tree: 415 passed, including the previously-flaky SIGKILL case (the crash fixture now parks after signal delivery). One new smaller item below; nothing else in the delta.
willem-bd
left a comment
There was a problem hiding this comment.
Delta review of 99bb9a80 (since 3c4eb55d). The b26a0e34 fence-ordering fix is correct for managed_global_state_write in isolation, and the ordering regression in test_skill_asset_versions.py is valid (the late function-local imports pick up both monkeypatches). The main merges are clean: the services.py resolution keeps normalize_input unconditional before the resume branch and preserves the list-only wire check on raw messages; the merged-in test_gateway_services.py helpers (_make_start_run_request, _capture_start_run_graph_input, _stub_app_config) all resolve at this head; the created_at keyset ordering in list_operations survived the merges; and the main-side changes riding along (external system-message boundary, memory-store change_seq) are not PR-authored. One new item below about the composite lock order at the toggle call site; nothing else in the delta.
|
@RykerFeng, please resolve the conflicts and review the PR comments. |
|
@RykerFeng a heads-up on migration numbers, since you approved #4843. I brought #4843 (scheduled-run IM notifications) up to date with Would it work for you if this PR keeps 0027/0028 and I move the outbox revisions to 0029/0030 once it lands? On my side that is one commit. If #4843 happens to land first, the change here would be small: the two revision ids, the |
|
@RykerFeng follow-up: the #4843 carry is now PR #6135, still at |
|
@RykerFeng please resolve the conflicts with the main branch. |
Reject persisted skill enrollment without a durable mutation runtime in standalone SDK processes. Cover fresh-process access, late enrollment and unavailable databases. Merge main, move the additive migrations to 0034/0035 and advance the extension API to 0.2.6.
Finding 1 — Non-enrolled users with
|
Finding 2 —
|
Finding 3 — EvidenceLimits constructed and validated, then immediately discardedFile: backend/packages/harness/deerflow/extensions/completed_run_evidence.py:793 The call EvidenceLimits(max_events=limit) runs post_init validation but the resulting object is immediately discarded. The raw limit integer is passed directly to .limit(limit + 1). Failure scenario: If future validation logic were added (e.g., a cap tighter than the caller's limit), it would never fire — the object is not even bound to a name. The intent was clearly to validate and then apply the cap, but only the validation runs. |
Finding 4 — Hardcoded evidence_origin literals duplicated across two files instead of a StrEnumFile: backend/app/gateway/services.py:210 and backend/packages/harness/deerflow/runtime/runs/run_record.py (via _validate_evidence_origin) The set {'interactive', 'scheduled', 'extension_evaluation', 'unknown'} appears inline in services.py and is duplicated in run_record.py via _validate_evidence_origin. Adding a new origin requires updating both copies atomically or they diverge. Failure scenario: The existing run_origin.py already defines ORIGIN_KINDS as a frozenset for a related concept, proving the codebase prefers named constants for this pattern. Adding 'extension_evaluation' required no such extraction, but future additions will need coordinated changes in two places. |
Related: #5646, #1865, #5539
Why
The Skill Evolution Host APIs RFC (#5646) proposes a bounded host contract for trusted learning extensions. Existing extensions can read broad run history and call legacy Skill writers, but they cannot safely coordinate with human edits, recover an accepted publication after a process crash, or keep recovery available after the originating plugin is removed.
This PR implements only DeerFlow's P0 host responsibilities. It does not ship a learning plugin, candidate-generation strategy, evaluator, background queue, or frontend review UI.
What changed
completed_run_evidenceandskill_mutationsdependencies indeerflow-extension-api0.2.7. Existing extensions retain their previous defaults and the older run-evidence reader remains compatible.app_config. Owned custom-skill YAML diagnostics use the same readiness/enrollment guard, recovering pending publications before scanning and refusing standalone enrolled access.Change size
Compared with the merged
mainrevision (ec07de961):P0 can only update the body and description of an existing enabled user-custom
SKILL.md. It cannot create, rename, delete, toggle, or modify support files.Automatic publication is limited to POSIX single-host local storage: one Gateway process with SQLite, or same-host PostgreSQL workers. Multi-node/NFS/CSI deployments and direct out-of-band writes are unsupported. Python extensions remain trusted in-process code; grants constrain the supported API, not hostile plugins.
Extension integration
The capability is exposed through the existing extension service lifecycle; no new plugin registration mechanism is introduced. A plugin registers an
ExtensionServiceand reads two optional, source-bound dependencies fromstart():Both dependencies remain
Noneunless an operator grants the plugin's exact registerednameanduseentry point inconfig.yamland restarts the Gateway:The intended plugin flow is:
completed_run_evidenceto discover a completed run, obtain a sealed snapshot, and page through bounded evidence.skill_mutations.read_skill()to obtain the target reference and current asset revision.stage()with that revision, inspect the detached proposal bundle, and run the plugin's own evaluation if needed.check()and requiredecision == "allow"beforecommit().revert()only while the committed revision is still the asset head.The host derives plugin identity from the registry source rather than caller-supplied IDs, and rechecks grants, source evidence, owner state, asset revision, and current scan policy at publication time. The complete operator and plugin integration guide is in
docs/skill-evolution-host.md.Surface area
frontend/docker/or sandboxed executionScreenshots / Recording
Not applicable; this PR has no frontend UI.
Validation
Latest local validation on 2026-10-16, after integrating main (
ec07de961):make test, four shards): 32,283 passed, 607 skipped, 0 failed.make test-blocking-io: 257 passed, 1 skipped.make format,make lint,uv lock --check, and diff whitespace checks passed.2adc667fe) has no conflicts. Its complete inherited guidance audit has 0 errors, with 20 soft-limit warnings.Earlier validation on 2026-10-15:
0043_runtime_im_noticesas 0044/0045. Projection readers and mutation writers retain the shared reentrant owner lock and contention-only advisory-lock retry helper.On 2026-09-23, live PostgreSQL validation completed locally on PostgreSQL 17.11 (aarch64,
READ COMMITTED) using the PR's mock-data opt-in regression: one pytest host process with two concurrent actors (a publication-fence worker thread and an async run-deletion worker) sharing the same database and test schema. The regression passed 6/6 runs (five consecutive repetitions plus the complete related suite); the related mutation, evidence, host, migration, crash-recovery, and rollback suite passed 209 tests. The PostgreSQL run specifically validates the source share-lock/deletion interleaving without deadlock; idempotency, recovery, scan-budget, and lease behavior remain covered by deterministic suites rather than a multi-process PostgreSQL soak. Every random test schema, the temporary container and image, the PostgreSQL extra, and the local Colima runtime were removed afterward.AI assistance
Tool(s) used: Codex
How you used it: Implemented the approved P0 host design with test-first development and independent specification and code-quality review passes. Human maintainer review is still required.