Repository navigation
Bound recipient and activity projections to requested threads - #965
Merged
Merged
Conversation
…ty-20261016 # Conflicts: # src/agent_comms/attempt_recovery.py # src/agent_comms/owner_lifecycle.py # tests/test_private_nk_entrypoint.py # tools/cutover/thread_retirement_installed_pilot.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Owner and consumers
RecordedThreadIncarnation.lookupnow owns the single production SHA preimage rule, cached per positive-birth member.ThreadIncarnationRecord.decodeselects recorded, deleted (−1), or unrecorded (0) at the wire boundary; every ordinary constructor now names its intended member. Deleted/unrecorded members have nolookupattribute.ThreadProvenanceretains a declared recorded incarnation; its unrecorded and deleted archive members retain their own birth semantics. Rename/replacement of a recorded thread keeps the same hash. The liveRegistrySnapshotderives its immutableby_lookupprojection once and now rejects duplicate or unrecorded births at construction, so publisher and recipient observation no longer repeat a collision check. ItsrecipientreturnsRegisteredRecipient(thread)orRetiredRecipient(lookup); an original committed send must name a missing recipient to prove retirement, otherwise it raises. No broadKeyErrorcatch or invented deleted hash remains. An archived source has a different owner, the existingRegistryProvenancetype:HistoryArchive.attachcaptures it directly from the original registry document under the store read, without constructing a live snapshot. It retains duplicate/zero births without rewriting the original; the historical paint ledger validates original name+birth evidence. There is no shared fallback method.AgentActivity.observe_recipientsand read-ledger selection invoke recipient-member behavior, not consumer-side type checks: registered owners keep their original behavior, a proved retired recipient becomes the existingUnavailableRecipientActivityand has no human paint witness, and an archived member cannot claim live activity. Neither consumer scans/re-hashes unrelated registered threads. The live registry has 884 deleted-name entries without birth hashes, so a lookup-only retirement guess would be wrong; the original send supplies the proof without a second store or bus scan. The birth-only persistedRecordedGoalWaitretains its original stored birth, while reply checks receive the actual owner'sRecordedThreadIncarnation, compare that birth, then read.lookup; there is no second hash helper, fabricated name or nullable lookup cache.GoalWaitTargetrequires a recorded member.Conversationpersists the declared(name, created_at)pair instead ofdataclasses.astuple, which otherwise would persist implementation fields.The free
bus_publication.stable_thread_lookupis deleted, not re-exported, wrapped or cached at module scope. Every production, test and cutover-tool import/call and old incarnation property was migrated. The stored lookup is unchanged: three positive-birth samples, including this thread's live registry birth, exactly match the installed Core 50f2b0e hash. The identity wire codec and archived registry codec preserve member class at −1, 0 and positive births. A live thread constructed or replaced with zero birth is rejected rather than silently changing type.Structural evidence
AST scan of
origin/mainacross source, tests and tools: 53 production free-function name references + 37 imports + 1 declaration; 170 test calls + 51 imports; 1 cutover-tool call + 1 import. After this branch: zero old-symbol declarations, imports, names or attributes. No AST parse omissions in either tree.git grep stable_thread_lookupfinds no tracked source/test/tool reference. SHA preimage rule occurs exactly once in production:src/agent_comms/thread_identity.py. The current branch before migration also had 46 typed incarnation property reads (29 source files); they now use.lookup. Static AST and grep cannot prove arbitrarily computed dynamic attribute names, but no literal old spelling remains. Review census against the prior pushed head (source/tests/tools, no parse omissions): base__new__1→0,DELETED_BIRTHdeclaration 1→0, direct base constructor calls 22→0 production, 193→0 tests. The sharedThreadIncarnationRecordis the one number decoder for archived registry and persisted conversation evidence; explicit positive-birth call sites constructRecordedThreadIncarnation. Base-classisinstance(self, ...)decisions 3→0.RegistrySnapshot.by_lookupfrozenobject.__setattr__1→0 and classmethod capture 0→1; its constructor validates supplied index/threads together, includingdataclasses.replace, without a second collision authority.isinstance(selected, …)at activity/read consumers 2→0; recipient members own their activity and paint effects. ProductionGoalWait | Noneannotations 21→0; declaredNoGoalWaitand persistedRecordedGoalWaiteach 0→1. Historical ledgersnapshot.threads.get1→0; archive typed name+birth selector 0→1, distinct from strict live lookup selection. The installed live registry was read-only censused at 220 registered: 0 same-birth collision groups, 0 unrecorded/deleted births (live-collisions-965.json). No_ambiguous_lookupsfield, live name fallback or repeated ownership check remains. Three formerrequire_unambiguous_ownership()consumers and the method were deleted; the live snapshot constructor owns refusal. The archived two-names-one-birth and zero-birth cases now constructRegistryProvenancein the focused test, and the existing attached-source history test passes.ActivityLog.all_current()now means all events;current_for(threads)is the selected projection. AST found 28 pre-changeall_currentcall sites, including 3 scoped calls; all 3 scoped callers migrated and none remain (zero parse omissions). TheGoalWaitfamily puts absent/persisted behavior onNoGoalWait/RecordedGoalWait;OrdinaryContextCheckcarries the actual owner to dependency matching. The stored birth-only row keeps its wire format. There are zero_owner_lookupfields,_lookup_for_birthhelpers or alternate production SHA preimages. New owner index:src/agent_comms/registry_document.py; consumers:agent_activity.py,read_ledger.py; dependency consumer:goal_waits.py.Source results and remaining boundary
After the member split, complementary focused suites: 58 passed/1 deselected (identity, index, read-ledger, archives) and 41 passed/1 skipped/4 deselected (goal waits, wake, zero-birth UI refusal); after requiring a recorded target, 11 passed/1 skipped on that boundary. After the strict live/archived type separation, 69 focused activity/index/read-ledger/archived notification tests passed. After the registered/retired family change, 36 focused index/read-ledger/notification tests passed, followed by 4 focused index tests exercising a real certified scratch send, deletion, unavailable activity, paint projection and refusal of a lookup absent from that send. After deriving wait lookup from the actual owner, 24 focused goal-wait, ordinary reservation and locked-store tests passed; the reservation fixture's obsolete
keyskwarg/empty batch was corrected to the currentInputBatchcontract. Two excluded failure classes were reproduced on installed unchanged Core 50f2b0e:FieldCodecnow throwsTypeErrorwheretest_thread_identity.pyexpectsValueError, andNativeArguments.defaultno longer exists fortest_goal_standby.py. After the six structural repairs at aca3177, independent bounded source checks with one reused installed-dependency Python and no coverage gate: 43 recipient/read/route/identity, 2 archived schema/provenance, and 56 goal-wait/locked-store passed (101 distinct), 5 skipped and 6 deselected (known FieldCodec exception mismatch, four missingNativeArguments.defaultcases, one optional native fixture without package configuration). AST parsed every source, test and tool file without omission;git diff --checkis clean. Resource check is critical (15.6 GiB RAM available, 17.8 GiB swap used, 6.6 GiB/free); no broad suite or paid fork was started.Fresh detached copy of the actual live registry after the six repairs (219 registered, 10 running at this sample), with its activity files: one recipient observation, 40 warmed calls, prior whole-registry SHA scan 0.4508 ms median versus owned index 0.0800 ms median. The earlier pre-review source sample was 219/9, 0.3865→0.0658 ms. The sampled recipient came from a separate certified one-message scratch bus with the same recorded birth; the live registry itself was only copied. These are branch/source-path timings, not installed UI or message→reply latency. Receipt:
/home/ts/.cache/agent-scratch/core-recipient-activity-20261016/benchmark-lookup-965.json; disposable copied files were removed. Earlier first-checkpoint measurements (219/27) remain inlive-registry-copy/result.jsonandbenchmark-final.jsonbut are not this refactor's installed acceptance.Draft, unmerged, not installed; this checkpoint was 2979497 and is superseded below. The earlier main integration was at 0da6331; recheck main at merge. Codex confirmed the underlying claim cutover installed, bridges restarted, and ten live indexed claim changes; #950's separate hold remains in force. Using that installed Core 50f2b0e binary on a new, isolated private root, a real CLI send returned
e70607592c92and inbox of the recipient returned that exact message. This proves the installed command transport on the switched runtime in isolation; it does not exercise #965 code, live owner claims, or message→reply latency. The first two scratch setup attempts failed before any send (missing private marker, then inherited live root ID); no input was replayed, and the third used a fresh root and sanitized environment. All disposable roots were removed; CLI JSON receipts remain in the owned scratch directory. Codex owns runtime publication, and installed #965 behavior still awaits it.Second review correction — 0b94c62
The incarnation and archive provenance classes now share their owner module,
thread_identity.py; the formerthread_provenance.pyis deleted, not aliased. All 15 imports of the deleted module in changed source/tests/cutover tools are gone. This removes the identity↔provenance import cycle.RegistryProvenancenow owns the complete five-member recipient-selection family, including live, archived and retired behavior;registry_document.pyimports its live selection, rather than making the archive import it from inside a method. The retired activity implementation is imported normally afteragent_activity.pyandnative_input_owner.pymoved annotation-only imports intoTYPE_CHECKING; no runtime cycle remains.FieldCodecis imported once at the identity owner, rather than repeatedly in its methods. The zero-birth/archive codec, original-send retirement proof and lookup SHA are unchanged.GoalWaits.for_goalnow accepts a realGoal; its nullableexecutionwrapper is deleted. Callers of the optional storedThread.goalexplicitly returnNoGoalWaitor omit the execution projection when the goal is absent, so no goal identity is fabricated.GoalWait.matches_idtakes a real wait ID;OwnedTurnhandles absence before calling it, preserving original-owner input permission. A new source regression test checks that a thread without a goal hasNoGoalWait.AST census of all changed source, tests and tools at aca3177 → 0b94c62: function-local imports in the three reviewed modules 10→0 (including all seven imports identified by review); old provenance-module imports 15→0; the four nullable reviewed arguments (
matches_id×2,for_goal,execution) 4→0;GoalWaits.executioncall sites 3→0; provenance declarations move 4→4 under one owner, recipient-selection declarations move 5→5 under one owner. No parse omissions. The import graph had three direct/indirectagent_activity→registry import paths; all annotation-only edges are removed. AST cannot resolve arbitrary computed import names, but repository source/test/tool searches find no literal old module or deleted static helper. Evidence:review-seven-census.jsonand its script in the owned scratch directory.Serial bounded checks after the repair: 64 passed, 5 skipped, 4 deliberately excluded (two known FieldCodec exception mismatches, one old guard unrelated to this PR, one native fixture missing an explicit package); the new no-goal case passed separately. A complementary run had 31 passes but 17 failures: 16 selected-owner tests fail on the untouched aca3177 parent with exactly the same 16 test IDs (3 pass, 1 skip on both heads), and one ordinary-owner test lacks
PI_COMPACTION_TEST_PACKAGE. This is not claimed as a clean suite. The source family import smoke andgit diff --checkpass. No #965 installed CLI/UI result is claimed; Codex publication and review remain outstanding.Third review correction — 2979497
GoalWaitTargetnow declares exactly one field, aRecordedThreadIncarnation; its name and birth are properties of that member. All production/test constructors and the only alias-projecting consumer useincarnation.renamed, not a copied name/birth pair orreplace(target, name=...). Its existingWireValuecapability preserves the same durable{"name", "created_at"}wait-target representation and schema; decoding uses the recorded incarnation owner, which rejects an unrecorded birth or unexpected wire field.GoalExecution.to_wiredelegates to the existingFieldCodec; all five public ACP/tool/history/presentation consumers stopped callingasdicton the internal target layout. The matchingfrom_wiredelegates to that codec, without a legacy second reader. Existing persisted wait rows are not rewritten.ThreadProvenance.incarnationis acached_propertycomputed once from its declared name/birth and member factory; the duplicate_incarnationfield and__post_init__assignment are gone. The owner drops slots to permit its normal__dict__cache (including frozen, slotted Thread subclasses), retaining the same per-thread hash reuse that #965's indexed lookup needs. The recorded, unrecorded and deleted incarnation members supplybirth_description:created <original birth>,birth unrecorded, anddeleted declaration. This lets the #986 historical viewer show retained evidence without displaying a zero or deleted marker as a real date; it does not manufacture a birth or lookup.AST across 777 source/test/cutover Python files: zero parse omissions;
_incarnationattribute reads 3→0, the two review-addedobject.__setattr__mirror assignments 2→0, directasdictprojections of goal execution in presentation/history/runtime 4→0. The complete GoalWaitTarget constructor family migrated, including the alias case; no remaining two-argument constructor. After finding and fixing that alias consumer, bounded checks 31 passed, 1 skipped (wait/store/recipient/block-reason); a separate archived/strict-wire set 27 passed. Four real ACP goal-snapshot tests passed when provided the installed native package fixture; these use a real owner socket, not a paid provider turn. Earlier wider first-pass run had 55 passes, 3 alias failures now repaired, 4 missing-fixture errors now repaired by explicit package, and one skip; do not count that failing run as a clean suite.git diff --checkis clean. New branch head is still draft, not merged or installed; installed #981 and its remaining Toad acceptance are separate.Fourth review correction — 0702f09
ThreadIncarnationowns birth semantics by member. Its base no longer stores acreated_at; onlyRecordedThreadIncarnationdoes. Deleted and unrecorded members have name-only instance fields; each owns its externalWIRE_BIRTHspelling (−1 or 0). The sole boundaryThreadIncarnationRecordis decoded byFieldCodecand chooses the member using those member-declared wire constants.ThreadIncarnation.schemauses that same record. Positive recorded birth validation usesFieldCodec.decode(float, ...), a finite/positive check, and no manual bool/int/type lattice. SHA is still calculated only byRecordedThreadIncarnation.lookup. The archive's originalThreadProvenance.created_atretains its declared historical wire evidence; it is not a deleted-member state field.The two
ClassVarbirth/deletion flags and all consumer-side flag branches are deleted. Recorded members own route lookup, archived recipient lookup selection, assigned source creation, transcript source selection, and same-publication-birth matching; other members return unavailable or the original absence callback.DeletedThreadIncarnation.currentowns the absent-registry case.GoalWaitTargethas nonameorcreated_atforwarding property; goal actions, presentations, reviews and alias-safe wait matching operate ontarget.incarnation, including identity comparisons rather than birth-number equality. The wait wire stays exactly{"name", "created_at"}. ACP cursor scope checks a recorded incarnation once and no longer repeats its positive-birth check.AST census before this correction against 2979497 (production), then after it (all source/tests/cutover): birth/deletion flag attribute reads 9→0, flag declarations 4→0, deleted member's
created_atfield 1→0,GoalWaitTargetname/birth forwarding properties 2→0. 777 Python files parsed, zero omissions. Grep confirms no remaining flag references ortarget.created_atin production. AST is structural evidence, not an acceptance run.Bounded source confirmations after implementation: 32 passed recipient/index/goal-owner/block-reason/archived-history tests; a complementary identity/ACP/route/snapshot/selected-transcript group 28 passed, 2 intentionally deselected, 1 failed. That failure (
test_acp_extension.py::test_declared_family_roundtrip_and_strict_boundary, missingTranscriptDraftUpdatein the test's sample set) is reproduced with the same failure on the untouched 2979497 head. A wider run exposed unrelated transcript-native assertions; the exacttest_outbound_transcript_source.py::test_original_read_contention_is_unavailable_then_same_receipts_returnandtest_native_transcript.py::test_native_ancestry_is_acquired_before_coordinator_readfailures were also reproduced on 2979497. The optionaltest_goal_wait_certified_reply.py::test_nativewith an explicitly pinned canonical installed native package still ends before its second mock-provider post; that exact failure was likewise reproduced on 2979497 and reported to core-retry. The remaining wider failures were not all individually baseline-proven. The initially misconfigured package-root fixture failed preflight before any native request; that run is not a behavioral result.git diff --checkpassed; #965 is draft, unmerged, uninstalled, so source results do not claim installed send/inbox, Toad or message→reply latency.Fifth review correction — 276c9fe
Claude caught a real inversion: three consumer-shaped hooks on
ThreadIncarnationmade identity know routing, ACP and transcript types. Replaced all three (route_lookup,assigned_source_identity,transcript_source) with one genericrecorded_or(make, otherwise)member operation. Recorded supplies itself; deleted/unrecorded invoke the caller's absence behavior. The original consumers inrouting.py,acp_extension.py,transcript_receipts.pyeach construct their own values, so identity no longer accepts a consumer type or importstranscript_receiptseven in TYPE_CHECKING. The two actual identity questions (selects_lookup,same_publication_birth) remain on members. No nullable return, type switch, new store or compatibility alias was added.Exact requested source grep:
rg -c 'route_lookup|assigned_source_identity|transcript_source\(' src/agent_comms/thread_identity.py6→0 (after exits 1 with no output); consumer-shaped identity TYPE_CHECKING import 1→0. Focused selected-source/recipient/route/ACP wire tests 11 passed;git diff --checkpassed. The previous fourth-head source results and baseline-proven unrelated failures remain documented above. New head is draft, unmerged, uninstalled, pending review; do not infer installed acceptance from this source result.Sixth review correction — 0668349
Removed
CoordinationChangedUpdate.source_identity, its nullable return and its last local transcript import. The coordination event still carries its declaredthreadandwire_root, and its caller decides what to construct fromthread.recorded_or. A cross-root AST census of 1,375 Core+Toad source/test/cutover Python files parsed with zero omissions: exactly one Toad consumer ofcoordination.source_identity,src/toad/acp/tool_calls.py:38(SessionToolCalls.scope); no Core consumer. This Core head cannot be installed alone with the current Toad: the Toad caller must migrate tocoordination.thread.recorded_or(lambda recorded: AssignedSourceIdentity(coordination.wire_root, recorded), lambda: None). @toad-ui-2 has been asked to own the paired Toad change while fixing its separately reproducedsession_navigationcrash. The 11 focused Core tests passed after deletion;git diff --checkpassed. Draft, unmerged and uninstalled until exact-head review and paired Toad publication. AST cannot resolve arbitrary dynamically computed attribute names; literal source/test/tool search found no other coordination-event consumer.