Repository navigation
P895: qualify edit ownership by source workplace, not latest session order - #3571
Merged
Merged
Conversation
|
debt-src at 99363d7, every finding: |
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
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.
Summary
P895: 7 commits, 10 files, +532/-64. Delete the recency fallback and derive edit membership from qualified source workplaces. Seven new real-record control functions (nine declared native/external/finding cases) are written. Root recorded SourceOK on exact
83e23ccf73d0a4c49a3204a69729927e84ecb5f4; actual split execution is recorded below. Executor ran no tests. Root message35478 removes the earlier P883/P872 integration hold: P883 failed its focused controls and is excluded from this landing; no dependency requires P895 to wait. Root reports all required hosted checks successful on exact83e and forge MERGEABLE/CLEAN. Authorized to mark ready and request normalgh pr merge 3571 --auto --merge, without bypassing required checks or changing source/locks. Not installed; P895 not closed.Actual root qualification — split heads, not one rerun
Root message35417 records SourceOK at exact83e and the following actual results:
83e23ccf73d0a4c49a3204a69729927e84ecb5f4test_noncanonical_worktree_needs_qualified_link_not_matching_origin, including alias/cache identity, actual independent-clone membership and same-clone wrong-branch refusal6704236f6df1e5fe984131bc6618d68f6ddc2d4053a816d5bf1d86663392ac48a7a40c2702e88a08These are separate executions at their actual revisions, not a claimed all-controls rerun or combined runtime at83e. SourceOK is not an installation result. No author tests/checkers or source changes followed this qualification. Root's earlier draft/unarmed batch hold is lifted by message35478; P895 proceeds independently through the normal required-check merge path. Root owns installation and P895 application closure.
Before / after: ownership
Before:
EditWorkplaces.latestselectedmax(order time);StorageRule.checkout_problemreturned that arbitrary fallback for every unrelated path. Unit/session identity plus recency manufactured Core source provenance. IDEN-6; fact: which source order owns an edit; at baselineturn_digests.py:272/storage_rules.py:245; derive from actual allocation/repository and explicit or linked source boundaries; byEditWorkplacesthroughCheckoutAllocation,Repo, checked order declarations andPullRequests.After: existing
EditWorkplacescaptures ephemeral qualified root-to-problem sets. Canonical allocations override generic roots, including released, unknown and missing allocations. Actual origin must match the problem's primary/declared companion repository and registered project. Invalid allocations reserve their boundary without supplying an owner. Explicit pre-PR source roots remain valid. Noncanonical worktrees require an existing qualified PR's repository and exact branch; matching origin alone supplies nothing. Multiple candidates select nobody, never latest. New/deleted filenames beneath a proved source root need no filename roster. Lexical and resolved membership reject aliases/escapes.Deleted: the
latestfield,of/max(order time)selection,order_of, the 8-lineStorageRule.checkout_problemfallback, and per-order reconstruction of an alleged latest workplace. No compatibility path, persistent attribution registry, synthetic verdict or source-event rewrite replaces them. A provisionalEditWorkplace(Checkout)wrapper from the first checkpoint is also gone; the existing owner carries matching directly.Test-only correction: construct the real lexical alias through its repository owner
Request35395: root's actual failed-only selection on670 reported 21 passed, 1 failed in60.73s. Both hosted debt checks succeeded at670; root's focused type check no longer reported the source/path assignment error. The remaining clone control stopped at my
dataclasses.replace(independent, checkout=...):SourceRepositoryinherits PydanticRepo, not a dataclass. Those21 passes and hosted source results belong to670; they are not an execution result for this new head.Only
tests/test_commander_forks.py:541–542changes: useSourceRepository.of(separate / ".." / separate.name)to acquire the real origin while retaining the lexical checkout spelling, and assertalias.checkout != independent.checkout. The existingpulls.worktrees(alias) is independent_trees, independent-clone membership and SAME-clone wrong-branch refusals remain unchanged. No production src/tools byte changes, model_copy dict, fake cache, wrapper or weakened assertion. Delta 1 test file, +2/-1.Whole-tree AST:1,188 files, no omissions, latest0. Changed test file py_compile passed without importing product/test code; disposable bytecode removed. No executor tests/checkers/runtime probes. At author delivery this case was unrun; root subsequently ran the one affected case on83e:1 passed in15.04s, and recorded SourceOK. Before/after source evidence:
p895-alias-source-before.txt,p895-alias-source-after.txt.Additive capture correction: acquire registered source proof once
Request35363 continues the completed finding-family and local-cache corrections rather than redoing them. Before:
actualincluded any canonical Git root, then allocation and generic-source consumers each rechecked registered identity. The linked-source loop also reusedpathfor a raw str after Path acquisitions; root's actual focused mypy on370 reported that assignment at turn_digests.py:380. No executor checker was run.After:
EditWorkplaces.capturederives the registered repository identities from its captured project before acquiringactual: dict[Path, Repo]. Each canonical absolute present root is read withRepo.ofonce; only registered actual origins enter this one positive-proof map. Allocation, explicit pre-PR and linked-PR consumers share it. No consumer repeats registered membership, no helper/flag/wrapper/new selection cache is introduced, and canonical local-Path caching inPullRequests.worktreesis unchanged.Negative reservations remain separate: unfiltered
present = Checkout.git_roots(*paths)feeds captured(Thread, CheckoutAllocation)tuples for every recognized allocation. Every reserved path starts with an empty frozenset in the existingallocatedresult. Only then do qualified actual roots prove candidates through existing Todo membership, allocation-declared primary/companion repositories andowns_declared. Foreign/unregistered/wrong-origin present allocations cannot fall through to an ancestor generic source. Missing/unknown/released lexical boundaries, ambiguity and symlink handling remain unchanged. The temporary allocation tuple map is captured declaration input, not a second derived membership result.The external worktree spelling is now
worktree_path: str, converted once toroot: Path; no cast, ignore or Any. Scope/finding producer/consumer inventory still derives qualified identities viaFileLine.repository.named_repositories. Both existing located/unlocated companion variants, native/external defaults, independent clone and same-clone wrong-branch counterexamples are unchanged.Delta relative370: 1 file, +28/-22. Whole-tree AST: 1,188 files, zero omissions, latest0, stale finding.located0;
py_compileof the changed production file passed without product imports, disposable bytecode removed. Existing source evidence:p895-capture-source-before.txt,p895-capture-source-after.txt. Static source evidence is not mypy or behavioral acceptance. At the670 author-delivery checkpoint these controls were unrun. Root subsequently ran the actual failed-only selection on670: 21 passed, 1 failed in60.73s, including the added companion parameter case; the clone fixture failure was corrected at83e and its one affected case passed separately. The original passing refusal remains at its actual tested53a head. All original raw records/verdicts untouched; draft/unarmed.Review correction: finding capability belonged on the finding family
Root reports the actual 22-case run on
53a816d5bf1d86663392ac48a7a40c2702e88a08: 21 failed, 1 passed, 16.44 seconds. The 21 failures all stopped at my nonexistentProblemFinding.located()call, before source membership. The one passing verdict-refusal belongs only to that tested head; it is not current-head acceptance. Executor did not run or repeat this selection.IMPL-4; fact: whether a finding contributes source locations; at
todos.py:2514; derive byProblemFinding.source_locations, default(), andLocatedProblemFindingoverride(self.where,), through the existing family.CheckedProblemOrder.source_repositoriesnow consumes this method beside owner and scope locations.FileLine.repository.named_repositoriesstill derives qualified repository identity. NoSourceReferenceimport/inheritance, wrapper, type/attribute probe or text parsing.The existing meaningful companion membership/released-source control is parameterized in place:
scope-unlocated-findingsupplies the companion through scope and keeps its finding unlocated;located-findingsupplies that same qualified location through the actual finding only. The undeclared-companion negative removes both scope and finding evidence. Native/external unlocated fixtures, cache correction, independent clone/wrong-branch and all other controls remain unchanged. This adds one declared parameter case, not a mirror test. Root chooses the failed-only rerun with the updated parameter IDs.Correction delta: 2 files, +21/-6. Before/after whole-tree AST producer/consumer census and changed-file compile are in
p895-finding-source.txt: 1,188 files, no parse omissions, zero stalefinding.located()calls, declaredlatestcount 0. Dynamic bindings remain outside AST proof. At that historical370 delivery checkpoint there was no behavioral or installed acceptance; subsequent split root qualification is recorded above. Installation remains unproved.Review correction: local Git facts were keyed by the wrong identity
Root rejected head
21fadab0cd6c3dab111c18ab9eea79226859abbcbefore running tests: my added explicit-clone caller still reached_worktrees[RepositoryName]. Two distinct clones of the same forge repository therefore received the first clone's local listing. The separate-clone control was a real counterexample, not a passing result.IDEN-6; fact: one local checkout's branch/worktree listing; at
todos.py:990,1088; derive the canonical checkout Path throughCheckout.resolved, by the existingPullRequestscache._worktreesnow usesPath -> Mapping[str, tuple[str, ...]];worktrees(repository)computes the canonical checkout once, readsWorkBranches(checkout)on a miss and retains that same mapping for repeat calls._read,_openand_todosretain their forge-owned keys.working_onandEditWorkplaces.capturestill share this same method. No second cache, refresh, common-dir framework or per-edit discovery.The existing independent-clone/wrong-branch test is preserved and extended: one actual
PullRequestsinstance reads two real sources with identical origins but different local paths, retains each mapping on repeats, and normalizes a..alias to the same local key. No mocks or preloaded answers. Those controls were UNRUN at author delivery; root's subsequent 53a run is recorded above. This correction adds only 2 files, +22/-7 relative to the rejected head; no earlier source/tests were removed.Before / after: shared owners and consumers
CheckoutAllocationremains the allocation grammar owner; base defaults and primary/companion overrides declare repository eligibility. No consumer copies its P-number/suffix decoder.CitedRepository/NamedRepositoryandOrderScope/FixScopeexpose their existing qualified declarations.CheckedProblemOrder.source_repositoriesderives companion identities from those declarations and PR links.PullRequests.worktreesexposes its existing frame cache with the corrected canonical local-Path key;working_onuses the same read.EditWorkplaces.capturebatches forge reads by registered repository and qualifies only enumerated/configured roots. It unions branch facts across registered checkouts of one repository instead of overwriting them. An explicit separate clone contributes only its own already-declared root after the same cached worktree reader proves its branch; arbitrary sibling clones are not discovered. No recursive filesystem scan or per-edit Git/network discovery.HeartbeatFramepasses the same registry snapshot andProgressReadinto ownership capture. HUD and overload consume that frame's mapping. Standalone judge/reminders acquire their mapping once at their read boundary.OrderEditRead.ownedremains the one native/external list/count gate. Only a singleton equal to the candidate Todo finalizesEditCall.problem. Original IDs, paths, hunks, clocks, records, cutoff/verdict semantics and merged-order exclusion remain with their existing owners.Production consumer census:
order_tools.py:231list/judge;commander/standing.py:793reminders;commander_hud.py:294HUD;commander/vibe_check.py:143load. Two production capture boundaries:turn_digests.py:239,heartbeat_frame.py:63. Parent/fork tooling continues through the same list owner.Real-record controls — actual split root results above
tests/test_commander_forks.pyadds actual Pi JSONL andExternalTranscripttool starts alongside genuine Git origins, registered roots, retained Todo records and the existing fake-gh subprocess boundary:Preserve meaningful existing parent/fork equality, reminder and merged-order controls.
test_judge_edit_ownership.pynow constructs real allocated Git worktrees and stops expecting scratch to join P2.test_judge_dropped.pyconstructs genuine primary/declared-companion allocations and asserts foreign companion membership before settlement, avoiding a vacuous rewritten check.Source feasibility, not behavioral qualification
Before and after AST inventories each parse 1,188 Python files across
src,tools,tests, 0 parse omissions. The separate-clone delta and subsequent local-cache identity correction were reparsed/recompiled with targeted owner reproof. The declared whole-tree AST check was repeated after the cache correction: 1,188 files, count 0, no omissions. Changed files compile in memory with no product imports. The explicitly permitted whole-tree AST check reports EditWorkplaces.latest count 0, expected 0. Static declaration/inheritance/consumer map and semantically read sites are retained:/home/ts/.cache/agent-scratch/toad-render/p895-source-before.txt/home/ts/.cache/agent-scratch/toad-render/p895-source-after.txt/home/ts/.cache/agent-scratch/toad-render/p895-cache-key-source.txt(rejected-head key and corrected owner/diff)/home/ts/.cache/agent-scratch/toad-render/p895-finding-source.txt(finding declarations, producers, consumers and correction)/home/ts/.cache/agent-scratch/toad-render/p895-capture-source-before.txt,p895-capture-source-after.txt(shared registered Git qualification, separate all-present reservation capture, preserved finding producer/consumer closure and py_compile)AST does not resolve arbitrary dynamic bindings or prove runtime reachability. Executor ran no tests, checker suite, package build, benchmark, service operation, installation, domain-store investigation/replay or provider traffic. Native PR/delivery metadata operations are the explicit ordering exceptions. No performance/installed-success claim.
Original four false memberships — classified, not approved
toolu_01VRsKtt4SJ6ywsAgj3uKHpatoolu_0169cBtPCmiKcgCt9deunbZFohcs-trace/comment.md, non-Git scratch reporttoolu_01P9Lt8TKf2jYx24E98jiH16plugin/introspection_trace.py, non-Git scratch experimenttoolu_01XRfq9FtrLGBCmhDfGKZAhotypemap/probe.py, non-Git scratch; argv is not ownershipClassification comes from root's retained correction artifact, not a new live replay. No original event or verdict was changed. P872's accepted Core757/Toad957 remains untouched.
Overlaps / assumptions
Base/fetched main is
2bcce132379084ea6f689dde916277819f8c2868, preserving merged #3570.commander_hud.pyoverlaps #3538;todos.pyoverlaps #3566 and merged #3570. Necessary shared-owner changes additionally touchheartbeat_frame.pyandchange_report.py. Root integrates overlaps; this branch absorbs no sibling implementation. No P883 matched-builder/checker, P872, installer, UI, notification or claim-delivery repair is included.Assumed: existing registered repository origins, qualified order declarations/links and enumerated Git worktrees are the source witnesses at capture time. No durable per-edit historical provenance exists; this change does not invent it. Original events remain authoritative evidence. Behavioral execution remains root-owned: actual split results are recorded above; the executor ran none. Installed qualification remains root-owned and outstanding.