Repository navigation
fix(clients): remote pairing keeps the scopes the link grants - #197
Conversation
github-actions
Bot
added
the
vouch:trusted
github-actions
Bot
added
the
size:S
1e824f2 to
df079c7
Compare
ab5c539 to
e4dd584
Compare
|
Astra review (gpt-6-astra, 2026-09-24) of this PR after the rebase onto current
Built by Claude Fable 5.1 in Claude Code. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedWe couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: Jacksondr5/j5code/.coderabbit.yaml Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (12)
💤 Files with no reviewable changes (7)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. 📝 WalkthroughWalkthroughClient pairing and DPoP exchanges no longer request a fixed scope set. Pairing tests cover grants with different scopes. The connection registry also updates observers when a bearer environment is re-paired with replacement credentials. ChangesPairing and connection updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to Remote pairing preserves the link’s granted permissions, and observers follow replacement credentials. No identified issue blocks merging after normal checks. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation PR Resolution Implement or include the remaining Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 4 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
bryantderosier
left a comment
bryantderosier
left a comment
There was a problem hiding this comment.
The client changes look right. I'd fix one FORK.md slip before merge.
Should fix
FORK.md: this hunk drops theapps/mobile/src/state/thread-order.test.tssentence from the "CI test fixtures reflect runtime contracts" entry. That test delta still exists onj5/main, so the inventory would no longer record a live upstream edit. (inline)
I checked the rest: the six client-runtime/web/mobile hunks match upstream pingdotgg#9785. The server still limits requested scopes to what the link grants, default links stay standard, and desktop-secondary still pins the standard scopes. Admin-link sessions keeping the admin scope is the direction upstream is taking.
| - **Committed Stop wins.** `RestartContinuation.ts` refuses continuation at admission and execution; `ProjectionStore.ts` includes completed `run_interrupt_request` items for active recovery runs. `Orchestrator.ts` checks the same durable Stop while holding the thread dispatch lock: a new steer cannot enter a stopped run, and a previously accepted non-delegated steer cannot become an automatic follow-up after Stop. This also applies to human steers; an explicit new message can start new work. The mailbox keeps its own disposed-cohort policy. Tests cover Stop before steer admission, after admission but before a normal provider completion, and restart recovery before provider acknowledgement. Upstream interrupted/failed terminal handling is otherwise adopted. Watch signal: upstream resolves `TODO(interrupt-hardening)` and honors committed Stop in restart and completed-steer follow-up. Remove these guards once equivalent behavior is proven. | ||
| - **MCP idle HTTP connection race**. Files: `apps/server/src/server.ts`, `apps/server/src/mcpHttpConnections.ts`, and its focused test. The Node server disables keep-alive only for `/mcp` responses, avoiding reuse of a connection racing Node's idle timeout. Responses and streams finish normally; MCP session identity is independent of the HTTP connection. Tool POSTs are never automatically retried because a failed response can follow a committed side effect. Nine focused transport tests and live real-provider acceptance cover this seam. Watch signal: the upstream Node/provider transport prevents this idle-reuse failure while preserving streaming, session identity and exactly-once tool effects. Exit: remove the delegate and helper when equivalent transport behavior is verified. | ||
|
|
||
| - **CI test fixtures reflect runtime contracts**. `apps/server/src/entrypoint.test.ts` and `provider/AntigravityInstallation.test.ts` expect canonical filesystem paths, including macOS `/private/var`; `textGeneration/CodexTextGeneration.test.ts` preserves `PATH` when its fake CLI receives an explicit environment. `orchestration-v2/FoundationPersistence.test.ts` supplies the turn-item list inspected by J5's restart admission guard; this is a fixture consequence of that guard, not an independent upstream defect. `apps/mobile/src/features/review/shikiReviewHighlighter.test.ts` fixes the clock during its cold-init comparison so Shiki's 500 ms tokenization budget cannot make runner load change token boundaries; the assertion and runtime budget remain intact. `apps/mobile/src/state/thread-order.test.ts` uses the existing complete thread-shell fixture instead of casting a partial object that lacks the lineage read by section membership. Watch signal: upstream supplies these portable fixtures and deterministic timing. Exit: remove each matching test delta when that correction lands. `orchestration-v2/testkit/OrchestratorScenario.ts` awaits the replay gate's arrival notification instead of exhausting a fixed event-loop counter; the gate testkit and scenario tests cover early/late arrival, frame hold/release, missing labels and the former polling path. The Claude composed-prompt assertion remains the separate case 8 contract. |
There was a problem hiding this comment.
This looks like a rebase slip. The replacement line drops the sentence about apps/mobile/src/state/thread-order.test.ts using the complete thread-shell fixture, but that delta is still on j5/main (f229e78), so the inventory would stop recording a live upstream edit. I'd restore that sentence and keep the new "Remote pairing keeps the scopes" bullet as the only addition.
There was a problem hiding this comment.
Done in 66fffa00fd: the thread-order sentence is restored; the new bullet is the only addition.
66fffa0 to
dca6209
Compare
dca6209 to
eab1c4e
Compare
eab1c4e to
0c204ba
Compare
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
0c204ba to
97321aa
Compare
github-actions
Bot
added
size:XXL
97321aa to
0ca0422
Compare
github-actions
Bot
added
size:S
Every client pinned the standard scope set when pairing a remote environment, so a startup URL's administrative grant was silently narrowed and the remote session could never hold access:write. The peer-credential route requires that scope, so the "Peer with…" dialog could not introduce a remote environment from the web client. Take the client hunks of upstream PR pingdotgg#9785 verbatim: ClientPresentation loses its scopes field and the pairing, bearer-refresh and DPoP exchanges send no scope, so the server issues exactly what the link carries. Desktop secondary backends keep requesting the standard set, as upstream does. FORK.md records the patch under the temporary in-place patches, with the exit being to take upstream's files once pingdotgg#9785 merges. Built by Claude Fable 5.1 in Claude Code. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
0ca0422 to
0495271
Compare
This branch was successfully deployed
2 participants
Stack 7/8. Depends on #177. Merge in order, retargeting to
j5/mainas predecessors land.Full stack:
Testing:
scripts/j5/pr-env.sh <pr> --peer(#255) serves this PR's build as two peered servers, Local and Remote. Rebased ontoj5/mainon 2026-09-30, after the Effect rc.115 upstream sync and the Playbooks and Crews migrations landed; peer migrations are now 023–027 and the FORK.md case is 48. A test state directory that ran an earlier cut of this stack recorded the peer tables under other ids, so rerunpr-env.shwith--freshfor such environments.Stack 6 of the cross-server peering build. Base: #177 (Connections UI). Unblocks the "Peer with…" dialog for a remote environment paired from the web client.
Problem
Testing #177 on two servers showed the readiness notice "This connection to … cannot manage access (it lacks access:write)". It was correct. Every client pinned
AuthStandardClientScopeswhen pairing a remote environment (apps/web/src/connection/platform.ts, the mobile equivalent, and the shared pairing and DPoP exchanges), and the server grants what is requested rather than what the link carries. A startup URL's administrative grant was therefore silently narrowed, the remote session never heldaccess:write, and the peer-credential route from #173 would refuse it. Peering from the web client to any remote environment was impossible.Upstream has the same diagnosis and an open maintainer fix: pingdotgg#9785 "fix(auth): preserve explicitly granted pairing scopes" (juliusmarminge, last pushed 2026-09-19). Community PR pingdotgg#4496 reported it first, in July.
What this does
Takes the client hunks of pingdotgg#9785 verbatim, so a later upstream sync either merges cleanly or drops the fork's copy:
ClientPresentationloses itsscopesfield.scope, so the server issues exactly what the pairing link carries. An administrative startup URL yields an administrative remote session; a link fromj5 auth pairing createstill yields standard scopes.registry.tscompares catalog entries by reference (Stream.changesWith) so re-pairing an unchanged environment rebinds its observers to the replacement supervisor. Without it, upgrading a saved standard-scope connection to administrative scope for peering left consumers on the old credential. Upstream's observer test comes with it.scopeis sent, and two new cases prove a read-only and an administrative grant survive pairing unchanged (mirroring upstream's tests).Not taken from pingdotgg#9785: its server-side validate-before-consume, the CLI
--scopeflags, and browser re-pair session replacement. Peering does not need them.FORK.md
Recorded under "Temporary in-place patches (watching upstream for a fix)". Watch signal: pingdotgg#9785 or its successor merges into upstream main. Exit: take upstream's versions of these files outright at that rebase, including any revision upstream makes before merging; the fork holds no other delta in them.
Surfaces
Verification
vp test runononboarding.test.ts,layer.test.ts,resolver.test.ts,supervisor.test.ts: 76 passed.packages/client-runtime,apps/web,apps/mobile: clean.Built by Claude Fable 5.1 in Claude Code.
🤖 Generated with Claude Code
Summary by CodeRabbit