Repository navigation
fix(KEN-4431): keep agent assignments from choosing skill revisions - #5576
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Held plans lose assigned skills’ recorded revisions, and targeted skill updates release unrelated ancestor holds.
2 open issues
What changed in this PR
This PR changes kendex dependency planning so agent assignments do not choose skill revisions.
Changes:
- Separate assignment revisions from agent pins and holds.
- Replay hook-pin checks from declaration and set seeds.
- Add regression coverage, authoring guidance, and a release note.
| File | Description |
|---|---|
| docs/authoring/README.md | Documents assignment revision behavior. |
| crates/core/tests/package_pins/mod.rs | Registers tests and updates revision expectations. |
| crates/core/tests/package_pins/assigned.rs | Covers assignment overlap, traversal order, and hook-pin replay. |
| crates/core/src/engine/expansion.rs | Stops assignments inheriting agent holds. |
| crates/core/src/engine/deps.rs | Uses edge-aware revisions and replays hook-pin checks. |
| changelog.d/fixed/agent-assignment-revisions.md | Adds the consumer-facing release note. |
Review effort: Balanced
Orchestration stopped
|
KEN-4431 follow-up brief: held readings and attestationPR kendex#5576 is cut from this release, draft and unmerged. Preserve branch Done when: unchangedAn agent's skill assignment never chooses a skill revision or prevents installation; the plan is the same in every agent order. The three order-dependent cases must plan the same declarations and revisions in both traversal/name orders:
The three earlier regression cases remain green: nested lead/scout/x/helper/y/x cycle; removing s at A does not drop helper's hard t at B; a no-auto-skills helper reached through an assignment brings its hard skills. Authoring Skill dependencies and Agent skills state the rule without depending on agent order; one fixed changelog item states the outcome. The required control revision remains exactly Defect class and source contractRule every site must satisfy: a held reading the planner consumes is reported through the one held-evidence channel attestation reads. Eligible but unused reservations remain non-seeding and absent from that proof. The correction separated own-record readings from manifest-injected pins inside The failed assumption was that extending held planning and its direct read consumers completed the ownership change. The producer/consumer inventory stopped before the report-to-attestation contract. The next design review must trace that full contract instead of adding an isolated attestation exception. Current failing evidenceThe validated correctness artifact is attached:
This is a new correction-introduced report gap; the four original recorded findings are fixed. No new baseline or mutation control was run for this newly reachable probe. Do not turn this current-head reproduction into an unsupported baseline claim. What b063b09 already provesAll six original issue cases pass, including all three earlier regressions. The original three regression controls pass at exact 41c01d2 and at the corrected head; the three original order-dependent controls fail at that exact baseline as required. Traversal order is exercised directly in addition to agent-name swaps. Transitive neutral versus independent hard ownership, locked/at-record own reads, installed native lookup, targeted direct/descendant/mixed hard/set owners, agent-target ownership, planned held-by projection and repeated-hook cache evidence also pass their recorded checks. Final affected gate: package_pins 70/70, model_classes 11/11, agent_skills 4/4, hold units 12/12 and cache/expansion units 2/2, plus core Clippy/fmt and guards. The configured selector passed once in 654 seconds and commit chain once in 86 seconds. The committed tree exactly matches the selector receipt. The accepted receipt and evidence archive are attached. Preserve the baseline caveats: expanded retention tests additionally demand exact unknown-selector preservation that the baseline refills; the expanded add test checks exact existing/requested roles although the baseline already refuses AddRevisionConflict. The raw nonzero sweep remains retained, not reported as green. No merge-group proof exists for this unmerged head. Open design question before any next implementationRun one one-shot reviewer-arch review first: which existing owner supplies the complete authoritative inventory of held readings actually consumed by a plan, so Expansion, EngineReport.held and attestation share that contract while unused reservations remain non-seeding? Trace every consumer of The next overseer approves that reviewed brief before implementation and sets its bounds. Scope and Done-when stay unchanged; KEN-4442 per-tool widening remains separate. The overseer will relaunch after PR #5558 (KEN-4428) and other engine-adjacent work allow it. No next round is authorized by this brief itself. Design verdicts and handoff artifactsAttached to this comment:
The original design brief is also retained in the earlier KEN-4431 comments; both verdicts remain in the PR body. The lane keeps its full status at |
Assignments bring missing skills at the source default without inheriting their agent's recorded hold. Hard requirements retain their revision ownership. Replay hook-pin counterfactuals from declaration and set seeds so an existing assignment subtree cannot affect the hypothetical answer. The three overlap tests and the hook-pin test fail behaviorally on 41c01d2. Cycle, removed-requirement and no-auto-skills regressions pass on that baseline and after this change. Controls used a disposable source tree with its own target directory. Evidence: tmp/waiter.JabmwA; initial fixture diagnostics: tmp/waiter.FDJJNR. Validation: 63 package_pins tests and four existing agent_skills tests pass; core clippy all-targets, cargo fmt, preflight and doc-limits pass. Configured CI-mode validation passes its scoped suites and records merge-group CI pending. Evidence: tmp/waiter.F5BdER, tmp/waiter.vAsqB1 and tmp/dev-validate-20261019T121017Z-69042.
Address PR review Keep neutral assignment lineage separate from independent revision demands. Retain each installed item's recorded reading for scoped writes and native lookups, stop carrier ownership at assignments, and hydrate source evidence on every cached catalog read. Keep the held-owner projection consistent. Cover transitive hard overlap, locked and at-record retention, targeted and mixed owners, installed native lookup, add retention, and repeated hook counterfactuals alongside the six original issue cases.
Expansion already records in Planned.held_by which hold each item read through. It now reports those (source, hold) pairs, DesiredState keeps them from the expansion the plan renders, and HeldPins::read_by turns them into the one held list: every injected pin, then only the own-record reservations an item read through. desired_pass exports that list, so EngineReport.held, attestation's source-history and base-floor checks and record_readings all see a consumed reservation, while an eligible reservation no item read stays out of the proof. Controls on b063b09 with the new assigned.rs: the floor, history, reached-inventory and redeclared-source rows fail; the 14 existing rows pass. With report.held asserted last, the floor and history rows fail because attest::record returns no problems. The history row moves the source record forward with a targeted update of another package. Without that step the stale source record is already refused, so the row would not be a must-fail control. Over-inclusion mutant, run once in a disposable copy: read_by returning every reservation turns the unreached row red, since report.held then carries y at B. No changelog entry: the omitted reservations came from this branch's earlier commits and never shipped. The branch's existing fixed fragment still states the outcome.
b063b09 to
da80e97
Compare
Address PR comments owners_of now takes the item identity (kind, name, source), the key every consumer of its answer uses, and walks it once with one visited set, reading the union of every harness copy's reasons. reserve_readings decided on the first copy, so a targeted update reserved a dependency that another harness's copy tied to the target, depending on harness name order. The per-parent cloned guard made the walk cost harnesses^depth; reserve_readings also skips the walk when nothing is exempt (locked, at-record and installed reads). Control: targeted_updates_read_every_harness_copys_owners fails on da80e97 in the Split(Claude, Copilot) row (q's reservation is read and q stays at its recorded commit) and passes there in the Split(Copilot, Claude) and Nested rows; every row passes on this head. Measured (release, scratch held_manifest bench, 17 derived skills with two parents each): locked 12-35us and targeted 44-168us per call at 1/3/5/8 harnesses; reviewer-perf measured 479ms at 3 and 32.3s at 5 harnesses on da80e97.
Address PR comments The Nested row of targeted_updates_read_every_harness_copys_owners, which 0e505e4's message cites, is deleted. It passed before the fix: at three harnesses the old walk never reaches its blowup, its copies carry identical reasons on every harness, and the rest of what it asserted is the single-harness Descendant row of targeted_updates_stop_at_assignment_boundaries_and_keep_independent_owners. The two split rows remain the test, and the harness-copy walk's cost carries no regression test. Control: with da80e97's hold.rs in a disposable copy, the q-assigned-on-Claude row still fails (q's reservation is read); the test passes on this head.
Orchestration stopped
|
Address PR review A lock entry is reserved at its own record only where no owner the lock records for it is still declared in the manifest being planned, matched by the source identity the targeted-update exemptions use. A member of a live set and a dependency of a live declaration follow their owner again; an assigned skill, and an item whose declaration or set was removed by hand, keep their own record. A locked write fetches for pins as before. A reservation is checked against the local cache only; one refused in a repository nobody fetched is fetched for only when one probe walk of the planning manifest reads it, through a planned item or an attempted catalog open. An offline removal no longer refuses over reservations nothing reads. A disagreement side read through a reservation resolves to the reservation's recorded commit, so an added set carrying a reserved agent at that same commit is no conflict. Compiled must-fail controls, run once in mktemp copies: - 3b28a7d with these rows: offline removal fails with and without the assignment subtree (SourcePending); the set add fails (AddRevisionConflict); the harness-copy row holds aa and ab. - Eligibility only (every reservation fetched as before): the subtree removal fails with SourcePending. - Reservations selected by held_readings alone: the unreachable leg drops y with a warning instead of refusing with SourcePending. - Any recorded Requested or MemberOf reason excludes the entry: the hand-removed assigned skill moves off its record. - Reservation sides resolved through state.pinned only: the set add fails with AddRevisionConflict.
Address PR review reserve_readings and exempted_by build one lookup of every item's reasons, keyed by (kind, name, source) and unioned across harness copies, and owners_of walks against it instead of scanning the whole lock at each ancestor it visits. Each walk keeps its own visited set, the root's carrier choice, and the all-copy reason union, so owner sets, exemptions and reservations are unchanged; the hold unit tests and the package_pins rows pin them. The reviewer's numbers locate the cost: from a 100- to a 300-deep chain at 8 harnesses the base (held_at and selector scans, items x entries) grew 7.4x against a predicted 9x, and the head grew 22x against 27x for items x depth x entries, which only the per-ancestor scan has. Delete a_tracking_sets_unwalked_member_reads_the_sets_revision. Its source stays at a tag on the commit every member was installed from, so the set's reading equals each member's record and no case can tell a reserved member from an owned one; it passed at 3b28a7d and with the live-owner check removed. No valid manifest separates the two readings: an item rev must be a full commit id.
Address PR comments A skill the person keeps removed installs from nothing, so it takes no own-record reservation; reserving it made an offline removal fetch the skill's recorded commit and refuse. An undeclared item's recorded item revision was copied from the declaration it came in under, so a write that keeps records releases its reservation only on the source revision. Controls at 761414e: the Assigned row of an_offline_removal_of_the_last_package_fetches_nothing failed with SourcePending { name: "cat" }, and passed with the is_held_back skip alone; locked_writes_keep_each_assignment_items_own_record failed in the Inherited leg only (y moved to the source head) until the item-rev comparison was removed.
Completed Issues
Recommendations ProcessedFixed in PR
Skipped
Fix rounds: 7 | 15 fixes, 0 escalated, 0 declined, 4 comment-triage passes; internal verification, Copilot (review 5581454041, approved) and required CI pass at 95f5332. |
Summary
[suppressed]) gets no reservation, so removing an assigned skill reads and fetches nothing for it. A write that keeps records releases a reservation only when the source's recorded revision changed; an item revision the old planner copied from a parent declaration is not read as an edit, so locks written before this change keep their assigned skills at their recorded commits. A member of a live set and a dependency of a live declaration follow their owner. Locked writes fetch for pins as before; a reservation is checked against the local cache, and its repository is fetched only when one probe walk of the planning manifest reads it. A disagreement side read through a reservation resolves to its recorded commit. Every reservation an item actually reads through reachesEngineReport.held, soverify --at-recordchecks it against the source's history and the base-record floor; a reservation no item read stays out of the proof.owners_of) is answered per item (kind, name, source), unioning every harness copy's reasons in one walk, so the result no longer depends on harness name order and its cost is linear in lock entries. Catalog cache hits carry source resolution and failure evidence to every consuming state; hook counterfactuals replay from declaration and set seeds. No schema or public API change.Merge danger
verify --at-record, native agent model lookup and hook pin notices.Completed Issues
Review history
reviewer-archreview before implementation, then an overseer-approved correction design (own-record reservations also feed installed-only expansion) and a held-reading inventory design for the attestation gap. Briefs and verdicts are on KEN-4431.04e3fa563(assignment authority),2b1b13821(four review regressions: transitive neutral edges, own recorded reads, cache evidence, assignment-aware carriers),da80e97e5(consumed reservations reach the report and attestation),0e505e410(per-itemowners_of, Copilot threads on harness copies and walk cost),3b28a7dc8(drops a test row that guarded nothing),fd8e27b6d(reservation eligibility, availability and evidence, after the stop below),761414ef2(owner walk against one reason lookup; drops the set-member row),95f5332f8(no reservation for a held-back item; reservations release on the source revision only).da80e97e5: architecture, test, correctness and external all pass. Verification ofda80e97e5...3b28a7dc8: correctness, architecture, performance and external pass; the test reviewer's one blocker (the Nested row passed on the old code) is fixed by deleting that row.reviewer-archdesign judgment on the overseer's brief (action_required, three corrections), approved, built infd8e27b6d. Verification of3b28a7dc8...fd8e27b6d: correctness, architecture, test, performance, error and external pass with two fix suggestions, both fixed in761414ef2under the overseer's cleanup approval. Verification offd8e27b6d...761414ef2: correctness, performance, test and external pass.761414ef2: two blocking threads, both reproduced byreviewer-correctnessand introduced by this branch (PRRT_kwDORvss6s6uMzZd, a held-back assigned skill reserved and fetched on an offline removal; PRRT_kwDORvss6s6uMzZs, the old planner's inherited item revision read as an edit). The overseer approved one structural close of the eligibility rule atreserve_readings; a one-shotreviewer-archread of the rule passed. Built in95f5332f8. Verification of761414ef2...95f5332f8: correctness, test and external pass.Test Plan
Each must-fail control was run on the named baseline in a disposable copy with its own target directory, then on this branch.
41c01d2c9: Incomplete, old revision, linear uninstalled41c01d2c9in unpinned-agent-first traversal41c01d2c9scout-first: linear uninstalled41c01d2c9: typedPin::LeavesOutrow absent382bc34f5382bc34f5reservation_reads_enforce_the_base_record_floorb063b0987:attest::record(.., Floor::Record(C))returns no problems,held=[]held, the C floor refusedreservation_reads_stay_on_their_sources_historyb063b0987:Floor::Openreturns no problemsonly_reservations_a_plan_reads_through_enter_the_proof, reached rowb063b0987:held=[]where y@B is expectedread_byreturns every reservation turns it reda_consumed_reservation_keeps_its_redeclared_sources_recordb063b0987: the record readscatafresh atrev=mainReading::Kept, y stays at Btargeted_updates_read_every_harness_copys_owners, q assigned on Claudeda80e97e5: q's reservation is read, q stays at its recorded commit while t movesda80e97e5(order guard)3b28a7dc8:SourcePending3b28a7dc8:SourcePending; also fails with every reservation fetched (eligibility-only mutant)3b28a7dc8(eager fetch); the unreachable leg fails under a held_readings-only selection mutant (y dropped with a warning)SourcePendingwhen unreachable3b28a7dc8; fails under the literal Requested/MemberOf exclusion mutant3b28a7dc8:AddRevisionConflict; fails with reservation sides resolved throughstate.pinnedonlya_reservation_is_fetched_for_only_where_the_walk_reads_it(hold units)an_offline_removal_of_the_last_package_fetches_nothing, Assigned case: remove assigned y offline, lead cached at A, y's B not in the mirror761414ef2:SourcePending { name: "cat" }locked_writes_keep_each_assignment_items_own_record, Inherited selector leg (item rev equal to each entry's recorded commit, as the old planner wrote it)761414ef2: y moves to the source headHistory control fixture. The design's literal history control (rewrite
main, then refresh the source) is not must-fail: onb063b0987the lock'ssources.catcommit B is already refused as off-history bysource_problem, so the row would pass at baseline for another reason. The committed row adds one production step,update_one(solo), which moves the source record to D while y keeps its reservation at B. With that stepattest::recordonb063b0987returns no problems, so the row fails there for the reservation gap alone.No regression test for the owners walk's cost. The previous walk cloned its guard per parent harness copy and cost harnesses^depth; reviewer-perf measured locked
held_manifeston the Jarvis closure (17 derived skills) at 479 ms with 3 harnesses and 32.3 s with 5 onda80e97e5. On0e505e410the same call takes 10-27 us at 1 to 8 harnesses. A row that fails on the old walk needs a wall-clock deadline over a many-harness fixture, which would add CI time and a timing-dependent failure; the one-visit walk is stated inowners_of's doc comment instead, and the order-dependence defect the same change fixed has the must-fail row above.No row for a set member at a tracking reading (Copilot thread PRRT_kwDORvss6s6uGexT). The thread's input, a set at
rev = "release", is refused by the manifest: an item revision must be a full commit id. With the source at thereleasetag instead, the set's reading equals each member's record, so no case separates a reserved member from an owned one. The row written for it passed on3b28a7dc8and with theOwner::Bundlearm ofdeclared_indisabled, so761414ef2deletes it. The live-owner rule itself is guarded by the harness-copy row, which fails with the live-owner check removed.Held-read cost. reviewer-perf measured
held_manifest(locked)p50 (21 calls, release builds of each revision, 8 harnesses),fd8e27b6dto761414ef2: 320-skill depth-2 scope 4.7-5.0 ms to 1.1-1.2 ms with roots declared and 9.7-9.9 ms to 5.8-5.9 ms with roots orphaned; 100-deep chain 16.0-16.9 ms to 2.1 ms; 300-deep chain 360-365 ms to 21.6 ms. The3b28a7dc8figures (2.9 ms wide, 2.9 ms chain300) come from another session, so they are not a matched baseline. No project budget applies.Validation:
Round
fd8e27b6d:dev-validate-runci mode (scoped suites) pass in 631 s; package_pins::assigned 24/24, hold units 13/13.Round
761414ef2:dev-validate-runci mode (scoped suites) pass in 120 s; package_pins 79/79,engine::desiredunits 14/14.Round
95f5332f8:dev-validate-runci mode (scoped suites) pass in 93 s; package_pins 79/79, package_pins::assigned 23/23.Fix round
0e505e410: package_pins 75/75,engine::desired::holdunits 12/12;dev-validate-runci mode pass in 119 s.Round
3b28a7dc8:dev-validate-runci mode pass in 101 s.Round
da80e97e5: core integration 89/89, core units 14/14, CLIverify_at_record9/9;dev-validate-runci mode pass in 687 s.cargo clippy --all-targetsastools/guardruns it exits 0 (warnings are advisory there and in CI).Every kendex change is queue-only under D027: the merge-group run proves the combined head.
Merge decision
Head
95f5332f847e5df0d8bd6b9b27a47cfe7fb14521: queued (pr-merge --expected-head, exit 75), routemerge-route: queue cause=no-bypass ruleset=20569265 bypass=never, after Copilot review5581454041approved it, every thread was resolved and required CI passed. The merge-group run proves the combined head. Auto-merge on761414ef2was disarmed when Copilot's two blockers on that head were confirmed. Copilot review5577661218's two blockers are addressed infd8e27b6d: PRRT_kwDORvss6s6uGewZ (offline removal no longer fetches unread reservations; N1, N2) and PRRT_kwDORvss6s6uGexT (a live set's members follow the set; its literal input is refused by the manifest, see the Test Plan note). The two blockers on761414ef2are addressed in95f5332f8: PRRT_kwDORvss6s6uMzZd (N7) and PRRT_kwDORvss6s6uMzZs (N8).