Repository navigation
Open pull requests and resume feedback reads in xmd repl - #970
Merged
taras merged 17 commits intoOct 21, 2026
Merged
Conversation
…blication (#967) Checkpoint 1 of Layer B's foundation: the private record an entry writes before it contacts a Git host, and the history walk that reads one back beside the publication it was opened over. No entry declaration, no transport and no upsert path is enabled here, so no pull-request capability is presented yet. `retained-repository/pull-request-records.ts` is the accepted closed version-1 `repl_repository_pull_request_request`: `version,owner,site,requested,received, resolved`, each reading carrying its place and the content the document wrote, and `resolved` carrying the covering checkout, its common directory, the admitted credential-free origin and the complete pull-request inputs including the head this attempt read. Its name is the existing strict canonical digest over `["executablemd.repl.pull-request-request",1,owner,site,requested]`, so the answer takes no part in it and a saved request is found without resolving anything again. The body is verbatim and may be empty, the number is a positive integer or null, and the received content and Repository are held to the inputs resolved from them. No credential, endpoint, object source, selection token or publication flag enters it. `push-admission.ts` now admits both protocols in the one existing walk rather than growing a second. The queue per coroutine holds what was actually asked, in order, because a definitive failed envelope names no kind: it is identified by the request standing at its position and that request's own expected outcome fingerprint, never assumed to be a Push. A successful envelope is held to its own stated kind as well, unrelated well-formed kinds still pass over, an unreadable success still refuses, and orphan, duplicate, foreign and mismatched records are refused while a trailing pending request stays valid. The walk now answers with the acknowledged successful Push pairs in physical outcome order, carrying the checkout provenance a receipt alone does not, for the publication consumer the accepted plan already specifies. `retained-repository/pull-request-admission.ts` is that consumer's proof: the last relevant acknowledged pair decides, held to the same resolved checkout root, common directory and exact head, with the existing fixed missing, conflicting and unreadable categories and no journal payload in its sentences. Failed and pending Pushes grant nothing. Tier RPR carries the checkpoint-1 portion of B6 — closed round trips and refusals, both object-id widths, null number and empty verbatim body, owner/site/name/input/placement checks, and mixed successful and definitive failed pairing across coroutines. Tier RP's thirteen Push controls are preserved unchanged and pass beside it, which is the compatibility half. The runtime origin readback, the complete saved-read question and envelope admission, and actual registration before replay remain checkpoint 2. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…#967) Two bounded authoring corrections to checkpoint 1. No behavior, structure, record shape, pairing rule or evidence scenario changes. Code Rule 6 prohibits type casting to infer a type, and the RPR4 fixture used `as unknown as ReplPullRequestContent` to hand the parser a number member it deliberately malforms. The parser's input is already `unknown`, so the fixture's type is now inferred and passed as it stands. The same five negative cases — zero, a negative, a fraction, a string and an absent member — are unchanged. Code Rule 4 keeps comments for surprising behavior, and eight of these narrated a declaration or a straightforward comparison instead: the durable type and name prefix constants, the request payload builder, the two reading parsers, the two content and ask comparisons, and the publication-destination predicate. They are removed. What stays is the reasoning a reader cannot recover from the code — why the answer takes no part in the record's name, why the body is verbatim, why the received content is held to the resolved inputs, and why the last acknowledged pair decides. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…eads (#967) An entry declares all six external repository names and owns the policy beneath four of them. A `<PullRequest>` writes its request down before the Git host is contacted and reconciles that request afterwards, so an entry reopened between the two asks for the title, body, head and destination it originally named. What admits one is this entry's own last acknowledged publication of that branch: another checkout's, another entry's, a synthetic answer and a failed or pending Push authorize nothing. Each evidence read installs its own transport descriptor under the stable Api name, in a scope of that one read's own, whose default closes over that invocation's admitted callback — so a nested or concurrent read cannot consume another's. Restoration still passes current URL and allowed-list admission and still composes through live outer handlers, and an authored read after the question reaches today's service. The entry transport is handed the platform source every other installation of this adapter is handed; without it the resolver had none to build and every entry read and upsert would have refused as unavailable. The retained read admission recomputes the delivered configuration fingerprint and the name it produces, so a description whose question does not produce its own name — an empty configuration included — is not admission.
#967) An entry read dispatched straight into its own transport descriptor, so an ordinary `PullRequestOperations.around({ read })` handler was never called. A lifecycle handler that answers a read itself is a supported surface, and skipping it let this entry open a session and append a snapshot for a read the document had already been answered about — different durable history from the same document, and a break in the contextual-operation compatibility boundary that lexical replacement rests on. The read now dispatches through both ordinary chains, as the upsert beside it already did. Each layer's terminal is this invocation's own, installed in a scope of that one read's own: the semantic terminal delegates to the transport terminal, and the transport terminal still ends in the admitted callback closed over this entry's owner and that invocation's site. Owner, site and callback reach no public option, registry or Context, and no mechanism is added. The configuration and the expansion position move into the delegated half, so a semantic answer that never delegates asks this entry's transport nothing. B5's evidence now composes one handler at each layer over an unchanged restored snapshot, and a second row proves a non-delegating semantic answer opens no transport and retains no read. Reverting only the previous read module fails both rows and leaves the neighbouring four passing.
…d on (#967) Seven scenarios the matrix names were absent or could not discriminate. Each is now written where it belongs, against the real entry assembly. Metadata parity compares all six declarations rather than two. A bound body's Push child and admitted read are shown acknowledging before the parent's own request, a refusing child leaves the parent with no request and nothing contacted, and a repository-local component of either name answers ahead of the entry's own policy and retains nothing. Two real publishers of one destination replace two publishers of two: their acknowledged order is arranged with readiness gates, they restore in the opposite order, and the later pair still decides. A checkout moved back to an earlier published commit refuses before a request or a credential exists. Pending recovery is held to the whole saved request — number, title, verbatim body, draft, base, head and destination — with HEAD and the branch moved underneath it, and an upsert handler that would retarget fresh work is shown never to run. Concurrency is gated on the first snapshot becoming history rather than on elapsed time. A retained checks item naming another commit and a saved request whose origin is not its Repository's both refuse through the runtime. Every released fixture now passes the admission a `repl` entry performs today. Cancelling during credential acquisition joins the broker and retains the request alone.
…#967) OW6 proved the order its resource closed in, but nothing in it established that acknowledgement waits: a journal append racing a teardown that had merely started would have produced the same list. The attempt now runs in a task of the row's own, and its resource says when teardown has begun and waits for the row to say it may finish. Held there — after observe, perform and validation, before cleanup is done — the journal holds no Git-host outcome and the caller has not returned. Released, the order is closed before appended before answered. Two explicit signals and no elapsed time: a readiness signal from inside the teardown, and a release the row controls. The release is armed before anything waits on it, so an assertion that fails under the gate cannot leave the attempt stranded in a cleanup nobody will finish. This is where the proof belongs. The entry opens its access inside this same owned attempt, so what this row holds is the teardown an entry's pull request is acknowledged after. The shipped session declares no asynchronous teardown of its own, and adding one to make it gateable would be production arranged for a test. OW7's cleanup failures, the entry's own credential-acquisition cancellation and every production module are unchanged.
…er one restores (#967) Q7's two gates both sat after the publications they were meant to order, so a schedule that restored the first publisher's Push, blocked at its question, then restored the second satisfied every assertion while leaving the original restoration order intact. The question-return order it asserted is not the publication-restoration order B2 asks about. The earlier publisher now reads evidence above its own `<Git.Push />`. That read is an ordinary admitted occurrence, and on continuation it restores through the live semantic chain — so a handler there parks that branch while its publication is still unreached, which document order alone guarantees. The later publisher's post-publication checkpoint waits for that parking rather than assuming it, records whether the branch was still held at that moment, and only then releases it. So the two publications demonstrably restore opposite the order the journal acknowledged them in, and the pull request still names the later head. An evidence list accumulated in restoration order would offer the earlier pair here, whose commit this head's own provenance refuses. Both gates are armed before anything waits on either, so a failing assertion releases them during unwinding instead of stranding a branch. The same destination, the two distinct native commits, the bare-remote reading, the later-head result and Q7a are unchanged, as are OW6, OW7, Q31 and every production module.
#967) One shared journey, run by the source REPL and by the freshly built binary. It selects a managed Repository and a Worktree, commits inside the captured path, publishes the branch, opens a pull request over it and reads reviews, comments and checks. Then it leaves at the question, lets the checkout advance from a shell and the service's feedback change, reopens the printed location, and finds the original number and URL and all three arrays — with no second pull request, no collection asked for again and no `git` run at all. Answering runs the suffix once, and the read written after the question reaches the service and sees what is there now. The document names `github.com`, because that is what selects the adapter. Git reaches a bare repository on disk instead through one invocation-local mapping the existing shim adds to its delegation, with protocols restricted to files so a missed mapping refuses rather than contacting anything. A configuration file could not say this: the host pins global and system configuration off, and that is left exactly as it is. Real Git still makes every checkout, object, commit and ref. What the entry bound is written into files, because a long source scrolls the transcript and a file is read whatever the screen shows. Node and Bun gain their own row: `<PullRequest>` and an evidence read refuse there with nothing contacted and no credential read. The four contract surfaces now describe the implemented behavior instead of its absence.
…it (#967) Twenty modules of entry policy and grammar leave `@executablemd/git` for `packages/cli/src/repl/repositories/`, and Git keeps only the domain behind a credential-encapsulated boundary: a twelve-method repository handle, a two-method pull-request facade and the issue transport installer. The four `repl` exports are gone with no wrapper, and `repl` is now the one command that assembles the bundled Plugin — because what an entry wrote down about its checkout is this command's record, and the admission that reads it back has to be inside the same contribution as the vocabulary. Raw-host fixtures move to the operation they stand for. Where a claim was only visible below the handle, it moves to the suite that owns the invocation rather than being dropped: the interrupted Add child and the unclassified-Add classification to `git-add`, the held-read child and scratch removal to `git-observations-deno`, the clone and worktree-add children and the pending descriptor and directory deliveries to `repository-replay`.
The expectation named two of them. It has named two since #181 delivery 4, and the entry began declaring `<PullRequest>` and its three evidence reads in 92b1300 — so the row has been asserting the wrong list, and its comment has been saying those four stay the registered defaults, since before the ownership move touched anything. The list is the six the entry declares. The comment now says why it is six: each of them needs the invocation, because a durable boundary that has to enclose the public chain cannot be installed inside it, and each evidence read is named after the occurrence that asked for it. The four Git reads and the file work beneath them stay out of it, being middleware under the component rather than a policy named after an invocation.
…ed (#967) The boundary was only mostly closed. `GitSession` and `RepositoryHost` were still published as types — invisible to an entrypoint control that reads `Object.keys()` on a loaded module, which is how two of the amendment's withheld rows survived. `AdmittedReadExecution` was missing. The observation admissions and the Repository identity codecs had drifted onto `/api`, and `admitLocator` onto `/deno` — which pulled the two provider-neutral record codecs into the Deno graph, the one place a module that has to run anywhere must not reach. All of that is now on its frozen route, and the partition suite reads the entrypoints as source text so a type-only row cannot hide. The CLI fixtures stop reaching into `packages/git/src`. Seeding goes through one ordinary `git` executable, the Issue fixture source and origin marker move to Git's own test support, the semantic pull-request chain is composed from the published name and interface the way production composes it, and the released configuration variable and sidecar file name are stated as the data they are. `repl-boundaries` now walks that whole corpus and holds it to the four published routes, with a counterexample so the rule cannot go vacuous. The four cancelled-attempt resource proofs are real again. OW12 through OW15 build the same handle the public factory builds, over leaves they can hold, and reconcile through the actual engine: a scratch space still owed, one already delivered, a credential session that arrives after its own cleanup started, and an `ls-remote` child that is genuinely running when the halt lands. The earlier claim that existing containment and session suites already covered these is withdrawn — they cover neighbouring facts, and the ledger now says which. The inert-constructor control also measured the wrong host, so it would have passed had construction acquired anything. It now runs the package-private builder the public factory delegates to, over the counting leaf that construction would acquire through, and a planted eager acquisition fails it.
The shared fixture helper started `execFile` inside a promise it built itself and then waited on it. Cancelling that wait stops the waiting; it does not stop the command. A scope that unwound mid-command would leave a `git` still running and still writing into a fixture directory nobody was watching any more. `exec()` is the process owner this repository already has. It registers the child's release before it spawns, puts it in its own process group, and kills and joins that group when the asking scope unwinds. Both helpers go through it and keep what they had: the argument vector, the working directory, the isolated configuration and stated identity, trimmed output, the raised diagnostic with what Git printed, and a non-zero exit read as an answer rather than a fault. The environment is still layered over the caller's own, because `env` replaces rather than extends and a command without `PATH` finds no `git`. `PullRequestReadKind` and `PullRequestReadResult` were already published through `/api`, so the two root aliases added beside them bought nothing. They are gone and the moved read policy takes both from the route the plan gives them. `parseCompletePullRequestReadResult`, `PULL_REQUEST_READ_KINDS` and `readRequest` stay where they were planned.
…ored (#967) The journey proved restoration with two files written before the question. Their surviving bytes say only that nothing rewrote them — a reopening that had lost a binding, or resolved one afresh, would leave them exactly as they are. So the question now renders every value the entry learned: the pull request's number, URL and state, and the length of each evidence array. That text is evaluated again as the entry is projected, and the service has meanwhile gained two reviews, a comment and a check — so a reopening that asked anything again would print `2/1/1` where the screen has to show `0/0/0`. The two files stay, with comments that now say what they actually establish. Beside the screen, the journal itself. The printed location is decoded with the command's own codec, the per-user data root and `pairedLocation` say where that execution's history lives, and `readRecords` reads it — the same reader the command opens a retained history with. After leaving there is one saved pull-request request naming the published branch, head and verbatim body, one performed pull-request reconciliation beside the publication's own, and one complete snapshot for each collection. Reopening changes and duplicates none of it. Answering adds exactly one reviews snapshot, for the occurrence written after the question. Selected records are compared rather than whole-file bytes, because projecting an entry legitimately appends to its history. `bareHead` no longer reads synchronously. The fixture service answers each request in an Effection task already, so resolving a ref is ordinary suspended work there; the loose ref and the packed table are both read that way, and a branch in neither still answers with nothing. Also corrected: the comment claiming only two of the six operations are bound over their Api. Four of them gained a REPL entry's boundary in this issue, and `<PullRequest>` and its three reads take their operation as an argument now.
Reading the entry's journal needed one durable type name from `@executablemd/git/deno`, and importing it at the top of the shared journey put Workflow's storage adapter — and so `node:sqlite` — on the import graph of `repl-push-pty.test.ts`. That suite also holds the unsupported-runtime journey, whose whole subject is that Node and Bun install no entry provider. ESM evaluates a static import before either journey is chosen, so on a runtime without `node:sqlite` the absence this suite observes would have become a module it could not load. Deno and the compiled binary never showed it. The positive journey now loads that entrypoint itself, with the `until(import())` this repository already uses, and hands the type to the snapshot selection. The unsupported journey's graph reaches no adapter: the edge is dynamic, so it is followed only when the journey that needs it runs. Every assertion, the one shared journey and the single read of each collection are unchanged. Three comments above `ASKED`, `held` and `asked` are gone. They restated what the next line already says. The explanations that carry a reason — why the live dialog is the restoration evidence, what the first-run files do and do not establish, and why a pull-request outcome is selected apart from the publication's — stay where they are.
CI found what a local run could not. Three lists name the relocated fixtures and suites by path, and the ownership move left all three pointing at where those files used to be. `tsconfig.node.json` excluded the lifecycle suite because it drives Deno's own `Command`; at its new path the exclusion stopped matching and Node's typecheck began reading it. `.oxfmtrc.json` ignored the released history journals for the reason its own comment gives — reserializing one changes the bytes the evidence is — and at their new path the formatter reached them, which is exactly how they were damaged twice during this work. Both entries now follow their files: the same single path each, repointed. The Node check also reported `toSorted` on an ES2022 target. The linter prefers it, but preferring is a warning and the target is a hard error, so the covered files sort a spread copy instead — non-mutating either way. Two accumulators in the journey helper inferred `never[]` under those settings and now state their element types. `api-entrypoint` expected eight contextual Apis and recognized nine. The ninth is `IssueOperations`, which the approved inventory publishes on `/api` — so the suite names it, with its two operations, rather than the count being relaxed. Its minted name is not published beside it, and the comment says why. The Linux journey looked for the entry's journal under the default data home while the command had written under the terminal's own. `allocate()` exports `XDG_DATA_HOME` into every shell it opens, so it now also states that root, and the journey resolves the path from the environment the command actually had. macOS ignores that variable, which is why only Linux saw it. The frozen-lock diagnostics needed no repair: they are `frozen-entry.test.ts` asserting that a stale lockfile stops every dependency-loading task, and all five of its cases passed. Nothing about the lock or the dependencies is touched.
taras
commented
Oct 21, 2026
taras
left a comment
taras
left a comment
Owner
Author
There was a problem hiding this comment.
Review of head 23bce226 (15 commits on 94a20441). Every non-test source hunk read; deno check on the changed modules and deno task lint pass; the new suite passes locally (32 steps, 54s). CI is green except review, which fails on test literals, not code.
Verdict: no blocking defect in the retained paths. One behavioral defect on the read decline path, one contract change that needs an explicit decision, and several duplications that will drift.
Should fix before merge
-
Outer read handlers run twice on the decline path.
packages/cli/src/repl/repositories/deno/pull-request-reads.ts:327hands the GitHub handler anextthat calls the publicPullRequestAPI.operations.readfrom the top. The ordinary profile installs the same handler withPullRequestAPI.around(packages/git/src/deno/composition/pull-request-reads.ts:276), so itsnextcontinues down the same chain. When the handler declines (a non-GitHub URL, or one outside the allowed list), every outerPullRequestAPI.aroundhandler the document installed runs a second time before the refusal. A counting or URL-rewriting handler sees two calls. Fix: install the handler witharoundinside the scoped block and call the public read once. -
An existing Git contract test was inverted.
packages/git/tests/plugin.test.ts:238flipsdeclaresFor({ command: "repl" })fromtruetofalsewith a new description. The behavior is deliberate (xmd replnow assembles the bundled Plugin itself,packages/cli/src/repl/repositories/plugin.ts), but it is a contract change to@executablemd/git, and the PR body records no authorization for it. Decide the contract, not just the test. -
The
reviewcheck. The review document journals the diff, and the repo's own@executablemd/secretlint-rule-credentialshits four credential-shaped test literals. RenameTOKEN = "github-credential-for-this-test"and?token=hunter2inpackages/cli/tests/retained-repository-pull-request.test.ts(lines 164, 2639), andTOKEN = "pull-request-server-credential"andpassword=journey-git-credentialinpackages/cli/tests/support/repl-pull-request-journey.ts(lines 96, 630). #945 failed the same way twice.
Duplications that will drift
pull-request.ts:322defines a privatedestinationRefForwhilepush.tsimports the exported one from@executablemd/git.pull-request-admission.ts:123compares the two strings; if the push recipe changes, every<PullRequest>refuses withmissing-push-evidenceagainst a valid acknowledged Push.replPullRequestPlacesHeld(push-admission.ts:492) is a copy ofreplPushPlacesHeld(:142) differing only in member paths. One placement rule, two bodies.pull-request-host.ts:274re-implements thegitHubPullRequestAccessrecipe instead of reusing it.run-profile.ts:96andplugin.ts:73each hard-code thereplspecial case. Expose one predicate.
Dead or repeated checks
upsertProvider(pull-request.ts:522) repeats the fingerprint and placement checksreadbackalready enforced on the same frozen record, and throws a different error class for a condition that cannot differ.reconcileparses the reconciliation record twice (:488,:494).pull-request-reads.ts:302: thedomain === undefinedbranch is unreachable; the provider always passes a built handle. Type it non-optional, or gate it on the sameusableflag the upsert uses if that was the intent.pull-request-admission.ts:79catches everything from the walk and throws one genericunreadable-push-evidencewith nocause. The walk's specific reason (duplicate-outcome,foreign-owner, …) is lost.
The double history walk (measured, not blocking)
Every <PullRequest> calls walkReplRetainedPublications over readAll(), although replPushAdmission performed the identical walk at journal open and discarded the result (admitReplPushHistory returns void). Measured: readAll() on an entry is an in-memory clone of the entry's own segment (entries.ts:267), the walk skips every record that is not a placement, request or Git-host outcome, and each matching record costs one parse plus one SHA-256 at ~25 µs (1,000 digests = 25 ms). The released fixtures hold 13–15 events with one push each. So this is a structural duplication, not a cost. A plain cache of the open-time result would be wrong, because the upsert must see the <Git.Push /> this entry appended after open; the fix is to keep the validated publications on the entry and fold in later appends. Fine as a follow-up.
Size and CI weight
+11,680 / −2,813 over 98 files: tests and test support 7,193; production source +4,312 / −2,446, of which most is the move of 20 entry-policy modules from packages/git to packages/cli/src/repl/repositories/ (commits 9 and 11); docs 132. New production code excluding comments is ~1,700 lines.
Measured at this head (Measure test weights run 39569268964): files new to this PR add ~40s Deno (retained-repository-pull-request.test.ts 33.8s, -records 6.4s), ~2s Node, 0s Bun; the moved suites keep their cost. The unknown is repl-push-pty.test.ts (52.4s Deno), which now carries the PR journey as its second row.
Review in two passes: git diff 94a20441..4bd72e53 (the feature) and git diff -M 4bd72e53..23bce226 (the package cut and corrections).
Tests: keep / drop
Keep whole: retained-repository-pull-request-records.test.ts (RPR1–17), the B9/B10 PTY rows and the compiled twin, retained-repository-history.test.ts, the cut guards (repl-boundaries S6, module-partition ×3, run-profile ×1, repl-push C6, github-issues ×4), and git-host-owned OW12–15 (relocated P20–P22, not new weight).
Keep in the Q suite (22 rows): Q1, Q3, Q4, Q5, Q6, Q7a · Q26, Q27 · Q8, Q9, Q10 · Q11, Q13 · Q14, Q15, Q16, Q17, Q18 · Q19, Q20, Q21 · Q23, Q24, Q25.
Drop or fold (10 rows, ~14s, ~500 lines):
- Q7 — Q7a proves "last pair decides" deterministically; Q7 is the readiness-gated race and the flake candidate.
- Q2 — covered by Q7a.
- Q12 — move its
resumed.env/methodsCalledassertions into Q11. - Q22 — C6 and
run-composition-lazyalready hold the factory and acquisition facts. - Q28 — the binding suite's A1 rows hold shadowing for all sixteen names.
- Q29, Q30 — fold into Q24's tamper table.
- Q31 — Q21 keeps the request on cancel, OW14 releases the credential session.
run-composition-lazy: keep the three acquisition rows, collapse the four shape rows into one.github-pull-requests: the three facade rows collapse to one.
The larger saving is harness, not rows: the Q file carries 1,257 lines of private setup including its own loopback GitHub model, while support/pull-request-server.ts is a second one used by the journey. One shared server removes several hundred lines.
Housekeeping
The PR body is stale: it names head 4bd72e53 and three pending corrections that 87712615 already made, and its review guide points at packages/git/src/deno/retained-repository/pull-request.ts, which the move relocated to packages/cli/src/repl/repositories/deno/pull-request.ts.
…#967) A REPL entry's evidence read installs its own transport terminal under the stable Api name, and handed the adapter a continuation that called the public `PullRequestAPI.operations.read` again. That descriptor is the end of the chain, so there was nothing below it to continue into: re-entering from the top ran every outer handler the document installed a second time before the refusal, and an outer handler that counts or rewrites a URL saw one read twice. The decline now raises the public surface's own answer where the chain actually ends. Installing the handler with `PullRequestAPI.around` instead would also run the outer handlers once, but it publishes this occurrence's terminal under a name the chain resolves — and a read an outer handler starts for itself would then reach this occurrence's retention callback, which Q18 holds must be impossible. Q14a counts one outer transport handler across a declined read. It reports 2 against the previous continuation and 1 against this one. `@executablemd/git` also stops declaring its vocabulary for `repl`, which the command profile's own prose still described as a document command. What an entry retained is that command's record, and the admission that reads it back has to travel inside the same contribution as the vocabulary, so `xmd repl` assembles the bundled Plugin itself. The journey's credential helper binds what it answers with before writing it into the line, the way `credential-home.ts` already does. Spelled inline it read to this repository's own review as a credential-named field with a long value, and refused the `review` check four times over — once for each place the PR context serializes that line.
Six duplications and dead checks the review named, with no behavior moved. `destinationRefFor` was a private copy of the one `@executablemd/git` exports and `push.ts` already imports; if the push recipe changed, every `<PullRequest>` would have refused with `missing-push-evidence` against a valid acknowledged Push. One placement rule now has one body, over whichever three readings a Push or a pull-request record supplied. The adapter's access recipe is the adapter's own, so the provider shares the read's memoization instead of re-deriving it from the resolver's two halves. `upsertProvider` re-asked both questions `readback` had already answered on the same frozen record — and carried its own error class for a verdict that cannot differ, since placements are rebuilt by selections that already happened and nothing drops one. The read's handle is typed non-optional, because the provider builds one per entry and always passes it; gating it on `usable` instead would refuse the read with no managed root that Q23 requires. A history the publication walk refuses now carries that walk's own reason as the cause, which the generic `unreadable-push-evidence` was discarding; the sentence still carries nothing the history held. `repl` is written down once, where the wrapper that knows about it lives, and `run-profile.ts` asks that instead of spelling the comparison again. Three rows go. Q7's deterministic core is Q7a's and it was the readiness-gated race; Q22's six names and three admissions are asserted verbatim against the production selector in C6, so it keeps only what building it costs; the handle and facade shape rows each proved their own types twice over. Q2, Q12, Q28, Q29, Q30 and Q31 stay. Each holds a fact its proposed cover does not: Q11 reads `XMD_WORKFLOW_GITHUB_PULL_REQUESTS` on restore, so Q12's "reads no configuration" cannot move into it; Q24's table tampers with a reviews read and cannot host a checks envelope or a saved request; A1 holds the registry, not a local component shadowing a declaration and retaining nothing; and Q21 cancels an HTTP call in flight, not a credential broker.
PR #970: Open pull requests and resume feedback reads in
|
taras
marked this pull request as ready for review
October 21, 2026 14:46
taras
commented
Oct 21, 2026
taras
left a comment
taras
left a comment
Owner
Author
There was a problem hiding this comment.
Second round, on head 0fc3fe1a (two commits on the reviewed 23bce226). Both returned items are settled; no new findings.
The six returned rows: keep all six
Each reason holds, checked against the row bodies:
- Q2 is the acceptance side of the rule Q7a refuses: a create opens over the later of two publications, and asserts the request's
headShais the later commit. With Q7 gone it is the only row that says so. - Q12 isolates the upsert. Q11 restores three reads, and a restored read re-checks URL permission, which reads
XMD_WORKFLOW_GITHUB_PULL_REQUESTS; so "no configuration read" cannot move into Q11. - Q28 proves a repository-local component shadows the entry policy and retains nothing. A1 holds the registry, not retention.
- Q29 / Q30 need a checks envelope and a saved request; Q24's table drives reviews-read descriptions only.
- Q31 cancels inside the credential broker; Q21 cancels an HTTP call in flight; OW14 is below the handle, not through the entry.
Net: Q7 dropped, Q22 trimmed to construction cost, Q14a added, the four run-composition-lazy shape rows and three github-pull-requests facade rows collapsed. 32 rows remain.
The contract change: recorded correctly
The body's "Contract change" section, the rewritten comment above DOCUMENT_COMMANDS in packages/git/src/plugin.ts, and the plugin.test.ts assertion now say the same thing and say why. Restoring declaresFor(repl) === true would have handed a raw-plugin host the vocabulary with no entry admission beside it, which main never did either, so declining is the safe half of the old contract.
The decline-path fix: verified
declinedRead raises the public surface's own answer at the terminal instead of re-entering PullRequestAPI.operations.read. The reason for not using around is right: it would publish this occurrence's retention callback under a name a handler's nested read resolves, which Q18 forbids. Q14a discriminates (2 before, 1 after).
Spot-checked the dedup commit: upsertProvider is now the one call readback left it; the admission refusal carries the walk's reason as cause without the history's content; repl is named once in the wrapper and run-profile.ts asks it.
Evidence run at 0fc3fe1a
deno task test packages/cli/tests/retained-repository-pull-request.test.ts packages/cli/tests/retained-repository-pull-request-records.test.ts packages/git/tests/plugin.test.ts packages/cli/tests/run-profile.test.ts— 14 passed, 68 steps, 0 failed (57s).deno task build, thendeno test --allow-all --frozen scripts/tests/repl-push-compiled.test.ts— E1 27s and B9 19s, 0 failed. This closes the gap the body names ("last run at4bd72e53"); the body can say so.
CI at the time of writing: 42 checks green, composability skipped as expected on a PR, one test-node shard still running.
Still open before merge
- The double history walk, as the follow-up the body already names.
- The two loopback GitHub models, as a separate commit or PR after this one.
- The xmd-review bot's size warnings are the package cut; nothing to do about them in this round.
taras
deleted the
agent/repository-pr-945-layer-b-main94-20261020T110400Z
branch
October 21, 2026 14:49
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.
Why
Authors can publish a branch, open or update its pull request, read reviews/comments/checks, and leave at a question in one
xmd replentry. Reopening restores acknowledged work; an explicitly authored later read observes current feedback.Draft for inspection. Tracks #967.
What changes
Before: REPL entries provide Push and Issue policies, while PullRequest and its evidence reads lack an entry provider.
After: Deno and compiled REPL entries provide the existing pull-request vocabulary inside the selected Repository or Worktree:
How it works
A fresh upsert requires this entry's last matching acknowledged Push, including checkout and commit provenance. The complete PR request is acknowledged before remote reconciliation. Interrupted work resumes those saved inputs; completed work restores its result without acquiring a provider or credentials.
Each admitted read retains its provider snapshot. Current URL permissions and outer semantic/transport composition still apply on reopen. A new read has its own occurrence and reaches the service.
Review guide
Start with:
packages/cli/src/repl/repositories/deno/pull-request.tsandpull-request-reads.ts— the move in commits 9 and 11 relocated these out ofpackages/git.Then review:
packages/cli/src/repl/repositories/.Look carefully at:
pull-request-reads.ts. That descriptor ends the chain, so a declined read raises the public surface's own answer from there. It must not re-enterPullRequestAPI.operations.read, which would run every outer handler twice, and it must not be installed witharound, which would publish this occurrence's retention callback under a name a handler's own nested read resolves.Contract change
@executablemd/gitno longer declares its vocabulary forrepl:declaresFor({ command: "repl" })answersfalseandDOCUMENT_COMMANDSdrops the name.What an entry retained about the checkout it was standing in is
xmd repl's record, and canonical core applies an admission within each execution's own journal read — so the admission that reads it back has to travel inside the same contribution as the vocabulary.xmd repltherefore assembles the bundled Plugin itself (packages/cli/src/repl/repositories/plugin.ts) and contributes both together, fromgitCompositionInstallation().Delivered behavior is identical either way, because that wrapper intercepts a
replrequest beforedeclaresForis consulted. What changes is what a host that installs the rawgitPluginunder areplcommand receives: the vocabulary with no entry admission beside it, or nothing. The second was chosen and authorized by the maintainer in this review round;packages/git/tests/plugin.test.tsstates it, andpackages/cli/tests/plugin-cli.test.tsandrun-profile.test.tshold what the assembled value declares.What must stay true
Q14a, which reports 2 against the previous continuation.Q18.How to verify it
Run at
0fc3fe1a:deno task test packages/cli/tests/retained-repository-pull-request.test.ts packages/cli/tests/retained-repository-pull-request-records.test.ts— 49 steps, 0 failed (1m3s).deno task test packages/git/tests/plugin.test.ts packages/git/tests/module-partition.test.ts packages/git/tests/public-entrypoint.test.ts packages/git/tests/pull-request-read.test.ts packages/cli/tests/plugin-cli.test.ts packages/cli/tests/run-profile.test.ts packages/cli/tests/repl-boundaries.test.ts— 136 steps, 0 failed (37s).deno task test packages/git/tests/git-host-owned.test.ts packages/cli/tests/repl-push.test.ts packages/cli/tests/retained-repository-history.test.ts— 27 steps, 0 failed (10s).deno task test packages/git/tests/run-composition-lazy.test.ts packages/git/tests/github-pull-requests.test.ts— 49 steps, 0 failed.deno task test packages/cli/tests/repl-push-pty.test.ts— both terminal rows, 0 failed (50s).deno task checkanddeno task lint— clean.Q14awas also run against the previous continuation, where it reportsseenas 2 rather than 1. That is what says it discriminates the defect rather than only passing beside it.The compiled terminal row (
scripts/tests/repl-push-compiled.test.ts, afterdeno task build) was last run at4bd72e53and has not been re-run at this head.Scope
Included
architecture.md,specs/repl-spec.md,specs/executable-mdx-spec.mdandspecs/workflow-workspace-spec.md.Intentionally unchanged
gitHubPullRequestAccesskeeps its signature; the shared recipe moved behindgitHubPullRequestReadResolver, whichmodule-partition.test.tswithholds from every bare package specifier.Risks and limitations
reviewcheck failed four times on one test literal — one credential-shaped line, counted once for each place the PR context serializes it. The journey's credential helper now binds what it answers with before writing it into the line, aspackages/git/tests/support/credential-home.tsalready does. Verified by running this repository's own scanner over the base-to-head diff and over every changed file's contents: 0 findings, from 4.<PullRequest>walksreadAll()for retained publications althoughreplPushPlacesHeld's admission walked the identical history at journal open and discarded it. Measured as structural, not costly: the released fixtures hold 13–15 events with one push each, and each matching record costs one parse plus one SHA-256 at ~25 µs. A plain cache would be wrong, because the upsert must see the<Git.Push />this entry appended after open. Left as a follow-up.XMD_WORKFLOW_GITHUB_PULL_REQUESTSon restore, so Q12's "reads no configuration" cannot move into it; Q24's tamper table runs one reviews read and cannot host Q29's checks envelope or Q30's saved request; A1 holds the registry rather than a local component shadowing a declaration and retaining nothing; Q21 cancels an HTTP call in flight rather than Q31's credential broker; and with Q7 gone, Q2 is the only row left asserting that a create opens over the later of two publications. Returned for the reviewer to decide rather than deleted.Scope confirmation