Skip to content

Commit 0df9a9e

Browse files
authored
Merge pull request #4 from pylon-code/upstream/2026-08-11-pull-requests
feat: adopt the multi-provider pull requests page (DEF-1, DEF-2)
2 parents 2faf7c4 + c7406e0 commit 0df9a9e

146 files changed

Lines changed: 34423 additions & 448 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

.agents/upstream-review.md

Lines changed: 58 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -601,6 +601,8 @@ so `L1` and `L2` may merge in either order.
601601
DEF-1 and DEF-2 were re-evaluated against this head and both **stay deferred**.
602602
Neither gate holds: the date is before 2026-08-25, and the fourteen-day churn
603603
check on the pull-requests files still returns `cad2c9361` and `a7b0366cb`.
604+
_Superseded the same day: the developer chose to adopt both anyway — see the
605+
sixth batch below._
604606

605607
| Change set | Upstream | Decision | Pylon reference | Rationale or revisit condition |
606608
| ---------- | --------------------- | -------- | --------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
@@ -614,8 +616,62 @@ Verification: `themePalette`, `themeBoot`, `vscodeThemeImport`, and
614616
after the color-space change. Palette math that computes cleanly can still shift
615617
perceptibly, so L2 wants one pass in a real client after merge.
616618

619+
## 2026-08-11 (sixth batch) — deferred work adopted: `#4849`, `#6049`, `#6061`
620+
621+
**DEF-1 and DEF-2 adopted, closing the deferred register.** The developer chose
622+
to bring them in ahead of the recorded revisit gates, which had not come due —
623+
the date gate was 2026-08-25 and the churn check was still non-empty. That is a
624+
deliberate override, not a gate that passed, and it is recorded as such: the
625+
pull-requests surface may still move upstream, so expect follow-up commits.
626+
627+
No new upstream range is involved, so the cursor does not move. Every commit
628+
here sits at or before the current `reviewed-through`.
629+
630+
| Change set | Upstream | Decision | Pylon reference | Rationale or revisit condition |
631+
| ---------- | --------------------------------- | -------- | --------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
632+
| M1 | `cad2c9361` / `#4849` | adopted | (this branch) | Multi-provider pull requests page with in-app reviews: 143 files, 34,133 insertions, covering GitHub, GitLab, Bitbucket, and Azure DevOps providers plus the diff-comment review surface. Only **three** conflicts against a much-changed `pylon`, all resolved Pylon-first — see below. |
633+
| M2 | `a7b0366cb` / `#6049` | adopted | (this branch) | PR page header accounts for Windows window controls. Clean once M1 was in; it conflicted only because it patches a file M1 creates. |
634+
| M3 | `f5fce7416` / `#6061` (was DEF-2) | adopted | (this branch) | Routes self-hosted GitLab remotes through the pull-requests service. Clean once M1 supplied `PullRequestService.ts`, which is exactly why it was deferred with DEF-1 rather than skipped. |
635+
636+
Conflict resolutions, all Pylon-first:
637+
638+
- **`index.css`** — upstream reintroduces `--animate-status-pulse` alongside a
639+
new `--animate-ghost-pulse`. **Only `ghost-pulse` was taken.** The new
640+
`PullRequestGhosts.tsx` depends on it in six places, whereas
641+
`animate-status-pulse` is the continuously repainting dot Pylon deliberately
642+
replaced with DotMatrix, and `ServerUpdateAction.test.tsx` asserts its
643+
absence. Pylon's `-1s` skeleton delay was kept over upstream's undelayed one.
644+
- **`SidebarChrome.tsx`** — kept Pylon's usage Back button from K9 and added
645+
upstream's `pullRequestsSupported` Pull Requests entry. **This also completes
646+
`#6031`:** K9 took only its usage half because Pylon had no PR page, so the
647+
pull-requests Back button is now restored, giving that page the same way out.
648+
- **`RightPanelTabs.tsx`** — kept Pylon's "Pylon desktop app" wording and took
649+
upstream's new `terminal` disabled-reason key, which neither side had before.
650+
651+
Verification: server `pullRequest` and `sourceControl`, contracts
652+
`pullRequest`, and client-runtime `pullRequestDiffHttp` pass (496 tests); web
653+
`pullRequest`, `diffs`, `openPullRequestLink`, `rightPanelStore`,
654+
`reviewCommentContext`, `useLiveRefresh`, and `ServerUpdateAction` pass (198).
655+
Contracts, client-runtime, web, mobile, and server typecheck with no errors —
656+
the two `pullRequest` Effect diagnostics are upstream suggestions, not errors.
657+
Lint clean and format clean across all 145 changed files. The generated
658+
`routeTree.gen.ts` carries the new route.
659+
660+
Cheaper than the original DEF-1 assessment feared: **no lockfile change, no new
661+
external dependency, no `vite.config.ts` change, and no migrations.** The only
662+
manifest edit is a `./state/pull-requests` subpath export in
663+
`packages/client-runtime/package.json`.
664+
665+
**Not verified:** nothing here was exercised in a real client. This adds a whole
666+
product surface across web, desktop, and mobile, and its remote and
667+
multi-environment behavior is untested in Pylon.
668+
617669
## Deferred register
618670

671+
_The register is currently empty: DEF-1 and DEF-2 were adopted on 2026-08-11
672+
(see the sixth batch above). Entries are removed once adopted or skipped, so an
673+
empty register means nothing is waiting._
674+
619675
Upstream work that has been reviewed and consciously _not_ adopted yet, with
620676
the condition that should trigger a fresh look. Entries stay here until they
621677
are adopted or skipped outright — a deferral that nobody revisits is the same
@@ -625,7 +681,5 @@ Every review must read this register before reporting new candidates,
625681
re-evaluate each `Revisit when` against the current upstream head, and report
626682
the outcome. See Phase 2.5 of the `review-t3-upstream` skill.
627683

628-
| ID | Upstream | Deferred on | Revisit when | Why deferred |
629-
| ----- | ----------------------------------------------- | ----------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
630-
| DEF-1 | `cad2c9361` / `#4849` and `a7b0366cb` / `#6049` | 2026-08-11 | **Both** gates must hold, because an empty log alone is also what day one looks like: (1) the date is on or after **2026-08-25**, and (2) `git log --oneline --since="14 days ago" t3code-upstream/main -- apps/web/src/routes/_chat.pull-requests.tsx packages/contracts/src/pullRequest.ts packages/client-runtime/src/state/pullRequests.ts` is empty. Then review as its own dedicated batch with a real product decision, not as part of a range sync. | Multi-provider pull requests page with in-app reviews: **34,121 insertions across 143 files**, larger than the other 59 commits of the 2026-08-11 range combined. It is a new product surface, not a fix — an 800-line `pullRequest.ts` contract, ~149 lines of new RPC, new HTTP endpoints, new client-runtime state, new dependencies, a `vite.config.ts` change — and Pylon would own all of it across web, desktop, mobile, and every connection mode. Three reasons to wait rather than skip: (1) **it will churn** — in this same range upstream rewrote `UsagePage` (`#5823`) and deleted the project-settings routes it had just added (`#5923`) within days, and `#4849` landed one day before the reviewed head with `#6049` already following it; (2) it touches `openPullRequestLink` and `sourceControlPresentation`, the same area as the merged-badge/auto-settle fix made in the previous batch, so landing it now risks masking that; (3) there is no security, correctness, or provider-compatibility pressure. "Does Pylon want in-app PR review?" deserves its own product decision, not a line in a 60-commit sync. |
631-
| DEF-2 | `f5fce7416` / `#6061` | 2026-08-11 | Adopt together with DEF-1, as part of the same dedicated pull-requests batch. It has no standalone revisit condition: without the pull-requests surface there is no `PullRequestService.ts` to patch. | Routes self-hosted GitLab remotes through the pull-requests service. Two of its five files (`apps/server/src/pullRequest/PullRequestService.ts` and its test) do not exist in Pylon. The `SourceControlProviderRegistry` half could in principle be lifted out, but it exists to serve the PR service and carries no benefit alone. First application of the standing policy: work that depends on the deferred pull-requests surface is deferred with it, never skipped, so it is not lost when DEF-1 is eventually taken. |
684+
| ID | Upstream | Deferred on | Revisit when | Why deferred |
685+
| --- | -------- | ----------- | ------------ | ------------ |

apps/server/src/auth/RpcAuthorization.test.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,17 @@ describe("RPC authorization scopes", () => {
3737
expect(requiredScopeForRpcMethod(WS_METHODS.cloudInstallRelayClient)).toBe(AuthRelayWriteScope);
3838
});
3939

40+
it("reads the reviewer menu under the same scope as the pull request it belongs to", () => {
41+
// The candidate list is a read like the detail beside it, and asking somebody for a review is
42+
// a write like every other pull request operation.
43+
expect(requiredScopeForRpcMethod(WS_METHODS.pullRequestsReviewerCandidates)).toBe(
44+
requiredScopeForRpcMethod(WS_METHODS.pullRequestsDetail),
45+
);
46+
expect(requiredScopeForRpcMethod(WS_METHODS.pullRequestsRequestReviewers)).toBe(
47+
requiredScopeForRpcMethod(WS_METHODS.pullRequestsComment),
48+
);
49+
});
50+
4051
it("rejects unknown RPC method names", () => {
4152
for (const method of ["server.notRegistered", "toString", "constructor"]) {
4253
expect(() => requiredScopeForRpcMethod(method)).toThrow(

apps/server/src/auth/RpcAuthorization.ts

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,23 @@ export const RPC_REQUIRED_SCOPES = {
5555
[WS_METHODS.serverGetBackgroundPolicy]: AuthOrchestrationReadScope,
5656
[WS_METHODS.cloudGetRelayClientStatus]: AuthRelayReadScope,
5757
[WS_METHODS.cloudInstallRelayClient]: AuthRelayWriteScope,
58+
[WS_METHODS.pullRequestsList]: AuthOrchestrationReadScope,
59+
[WS_METHODS.pullRequestsListStats]: AuthOrchestrationReadScope,
60+
[WS_METHODS.pullRequestsDetail]: AuthOrchestrationReadScope,
61+
[WS_METHODS.pullRequestsActivity]: AuthOrchestrationReadScope,
62+
[WS_METHODS.pullRequestsDiffFileContents]: AuthOrchestrationReadScope,
63+
[WS_METHODS.pullRequestsRunAction]: AuthOrchestrationOperateScope,
64+
[WS_METHODS.pullRequestsComment]: AuthOrchestrationOperateScope,
65+
[WS_METHODS.pullRequestsSubmitReview]: AuthOrchestrationOperateScope,
66+
[WS_METHODS.pullRequestsReplyToThread]: AuthOrchestrationOperateScope,
67+
[WS_METHODS.pullRequestsSetThreadResolution]: AuthOrchestrationOperateScope,
68+
// Read scope like the reads it un-caches: refreshing is part of reading, and a read-only
69+
// client pressing refresh must not be told it may not look again.
70+
[WS_METHODS.pullRequestsInvalidate]: AuthOrchestrationReadScope,
71+
// The candidate list is a read like the detail beside it; asking somebody for a review is a
72+
// write like every other one.
73+
[WS_METHODS.pullRequestsReviewerCandidates]: AuthOrchestrationReadScope,
74+
[WS_METHODS.pullRequestsRequestReviewers]: AuthOrchestrationOperateScope,
5875
[WS_METHODS.sourceControlLookupRepository]: AuthOrchestrationReadScope,
5976
[WS_METHODS.sourceControlCloneRepository]: AuthOrchestrationOperateScope,
6077
[WS_METHODS.sourceControlPublishRepository]: AuthOrchestrationOperateScope,

apps/server/src/cloud/bootService.test.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,8 @@ const makeHarness = Effect.fn("test.make_boot_service_harness")(function* (
8181
timedOut: false,
8282
stdoutTruncated: false,
8383
stderrTruncated: false,
84+
stdoutInvalidUtf8: false,
85+
stderrInvalidUtf8: false,
8486
};
8587
}),
8688
});

apps/server/src/cloud/pinnedRuntime.test.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,8 @@ const successfulRunner = (fs: FileSystem.FileSystem, path: Path.Path) =>
3131
timedOut: false,
3232
stdoutTruncated: false,
3333
stderrTruncated: false,
34+
stdoutInvalidUtf8: false,
35+
stderrInvalidUtf8: false,
3436
};
3537
}),
3638
});

apps/server/src/cloud/selfUpdate.test.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,8 @@ const makeHarness = Effect.fn("test.make_self_update_harness")(function* (
4545
timedOut: false,
4646
stdoutTruncated: false,
4747
stderrTruncated: false,
48+
stdoutInvalidUtf8: false,
49+
stderrInvalidUtf8: false,
4850
};
4951
}
5052
order.push("preflight");
@@ -64,6 +66,8 @@ const makeHarness = Effect.fn("test.make_self_update_harness")(function* (
6466
timedOut: false,
6567
stdoutTruncated: false,
6668
stderrTruncated: false,
69+
stdoutInvalidUtf8: false,
70+
stderrInvalidUtf8: false,
6771
};
6872
}),
6973
});

apps/server/src/environment/ServerEnvironment.test.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,7 @@ it.layer(NodeServices.layer)("ServerEnvironmentLive", (it) => {
6969
expect(first.environmentId).toBe(second.environmentId);
7070
expect(second.capabilities.repositoryIdentity).toBe(true);
7171
expect(second.capabilities.connectionProbe).toBe(true);
72+
expect(second.capabilities.pullRequests).toBe(true);
7273
expect(second.capabilities.threadTitleRegeneration).toBe(true);
7374
}),
7475
);

apps/server/src/environment/ServerEnvironment.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,7 @@ export const make = Effect.gen(function* () {
143143
capabilities: {
144144
repositoryIdentity: true,
145145
connectionProbe: true,
146+
pullRequests: true,
146147
threadSettlement: true,
147148
threadSnooze: true,
148149
threadPinning: true,

apps/server/src/environment/ServerEnvironmentLabel.test.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,8 @@ describe("resolveServerEnvironmentLabel", () => {
8181
timedOut: false,
8282
stdoutTruncated: false,
8383
stderrTruncated: false,
84+
stdoutInvalidUtf8: false,
85+
stderrInvalidUtf8: false,
8486
}),
8587
);
8688

@@ -120,6 +122,8 @@ describe("resolveServerEnvironmentLabel", () => {
120122
timedOut: false,
121123
stdoutTruncated: false,
122124
stderrTruncated: false,
125+
stdoutInvalidUtf8: false,
126+
stderrInvalidUtf8: false,
123127
}),
124128
);
125129

@@ -223,6 +227,8 @@ describe("resolveServerEnvironmentLabel", () => {
223227
timedOut: false,
224228
stdoutTruncated: false,
225229
stderrTruncated: false,
230+
stdoutInvalidUtf8: false,
231+
stderrInvalidUtf8: false,
226232
}),
227233
);
228234

@@ -264,6 +270,8 @@ describe("resolveServerEnvironmentLabel", () => {
264270
timedOut: false,
265271
stdoutTruncated: false,
266272
stderrTruncated: false,
273+
stdoutInvalidUtf8: false,
274+
stderrInvalidUtf8: false,
267275
}),
268276
);
269277

0 commit comments

Comments
 (0)