Repository navigation
chore: sync with upstream/main (339179f50) - #66
Conversation
…es (pingdotgg#19631) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
…ingdotgg#19630) A delegated subagent waiting on an approval or question now shows on its parent thread: Working section, status badge, notifications, snooze, and the composer banner on web and mobile. Takes over pingdotgg#19614. Co-authored-by: lovelycodemonkey <ghassemimahyad@gmail.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
… command (pingdotgg#19635) After "Always allow this session", the Codex adapter answers later command (or file change) approvals in that thread for the rest of the session. A turn in another runtime mode, or the thread unloading, ends it. Takes over pingdotgg#19598. Co-authored-by: lovelycodemonkey <ghassemimahyad@gmail.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
…dows again (pingdotgg#19656) Windows PowerShell started with detached: true exits 0 without running its command. Native Windows PowerShell launches (reveal and browser) now start attached, unref'd, with ignored stdio. Takes over pingdotgg#10387. Fixes pingdotgg#11780. Co-authored-by: doubletwisted <118005273+doubletwisted@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ingdotgg#19103) Co-authored-by: PR Batch Tester <agent@local.test> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
…ished tool nodes (pingdotgg#19675) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
…g#10984) Co-authored-by: RealDoc06 <73218979+RealDoc06@users.noreply.github.com> Co-authored-by: Theo Browne <me@t3.gg> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ait (pingdotgg#19538) Co-authored-by: maria-rcks <maria@kuuro.net>
…ingdotgg#19521) Co-authored-by: maria-rcks <maria@kuuro.net>
…uestion (pingdotgg#18459) Co-authored-by: maria-rcks <maria@kuuro.net>
Co-authored-by: maria-rcks <maria@kuuro.net>
Merge 21 upstream commits into the fork. - contracts/settings.test.ts conflicted only on import order; it takes upstream's import and keeps the fork's default base branch tests (ADR 0009). - Upstream's delete-branch PR action, settled-worktree cleanup, delegated task questions, and composer vertical caret stops merged beside the fork's Forgejo, existing-branch, schedule-tool, and dead-key changes without adaptation. None of them implements a fork ADR's feature. - No upstream workflow changes; the release define is intact (ADR 0011). - ADRs compare against 339179f. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@fryuni-s-proval Please review this upstream sync at This is a maintainer-requested sync, not a feature PR. Its parents are fork main Review units (
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
I recommend approving. I found nothing that needs changing in how the upstream changes fit with the fork's ADR features. I can only post comments, not submit a formal GitHub review, so treat this as the recommendation. I can't run git, so I couldn't run 1. Contracts test conflict (ADR 0009): OK
2. Delete-branch action and Forgejo (ADRs 0002–0004): OK. Forgejo can't reach the action anywhere:
3. Files changed on both sides: OK
4. CI (ADR 0011): Partly checked. 5. ADRs: OK. The Not checked: I didn't run tests or typecheck. The |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13e4715c28
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| runPullRequestAction: (input) => { | ||
| if (input.action === "delete-branch") { | ||
| return Effect.gen(function* () { | ||
| if (input.stackNumber !== undefined) { |
There was a problem hiding this comment.
Block stacked branch deletion for ordinary action calls
When a merged or closed pull request belongs to a stack, the web and mobile delete controls invoke runAction without stackNumber, so this condition is never true on the user-visible path. The mutation therefore deletes a stacked branch despite explicitly treating stack deletion as unsupported, potentially removing the base branch of remaining stack layers; determine stack membership before mutating, or pass it from the clients and suppress the action.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Out of scope for this sync. GitHubPullRequestApi.ts is byte-identical to upstream/main (git diff upstream/main HEAD -- packages/source-control-github/src/server/GitHubPullRequestApi.ts is empty), and this stack-membership behavior comes from upstream's own delete-branch feature (pingdotgg#19257). The fork does not change GitHub PR actions or their clients, so this is upstream behavior rather than an integration problem. Per the maintainer's instructions, problems already present on either parent are not fixed in the sync PR.
| ); | ||
| const getThreadTranscript = Effect.fn("orchestrationV2.threadManagement.getThreadTranscript")( | ||
| function* (threadId: ThreadId) { | ||
| const { projection } = yield* getThreadSnapshot(threadId); |
There was a problem hiding this comment.
Bound transcript loading before materializing the snapshot
For a long thread, this loads the complete projection—including historical runs, nodes, messages, and turn items—before the subsequent 50 MB transcript check. A dropped oversized thread can therefore force a very large database decode and allocation only to return an error, potentially stalling the environment; read only the transcript rows and stop once the byte budget is exceeded.
AGENTS.md reference: AGENTS.md:L15-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Out of scope for this sync. ThreadManagementService.ts is byte-identical to upstream/main (git diff upstream/main HEAD -- apps/server/src/orchestration-v2/ThreadManagementService.ts is empty), and getThreadTranscript was added by upstream (pingdotgg#15210). The fork does not touch thread snapshots or transcript loading. Per the maintainer's instructions, problems already present on either parent are not fixed in the sync PR.
|
@codex review This is an upstream sync merge (parents: fork main |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
| const { thread } = yield* projectionStore.getThreadRecords(input.threadId, []); | ||
| const providerTurn = yield* thread.worktreePath === null | ||
| ? start(input) | ||
| : withWorkspaceLease(path.resolve(thread.worktreePath), start(input)); | ||
| if (providerTurn !== undefined) yield* providerTurn; |
There was a problem hiding this comment.
⚫️ [PROBLEM] Turn start on idle threads can now wait behind a whole storage-cleanup pass
⚫ [PROBLEM] Turn start now takes the workspace lease (line 1292). Storage cleanup holds that same lease for its entire per-worktree Effect.gen: see apps/server/src/storageCleanup.ts:677, (effect) => withWorkspaceLease(worktreePath, effect). That block includes a lot of slow work:
- the
git.fetchRemoteTrackingBranchnetwork fetch (around storageCleanup.ts:529) gitManager.branchPullRequest(..., { refresh: true })(around :549)- the full recursive
measureWorktreewalk (:662) - two
blockedSinceCheckpasses
Cleanup only considers idle threads, and an idle thread is exactly where a user sends the next message. During a sweep, that message's turn start can block for several seconds or more, with no feedback in the UI. Rollback (CheckpointRollbackService.ts:382) waits on the same lease and gets the same delay. Before this PR, only terminal open contended for the lease.
Suggested fix: in storageCleanup.ts, take the lease only around the final critical section. That is the last blockedSinceCheck, the branch/HEAD/local-changes rechecks, git clean, and git worktree remove. Leave the fetch, the PR lookup, and the size walk outside the lease. The recheck already catches anything that started in the meantime, so the race stays closed, and turn start only waits for the short removal window.
| // A blocking delegate_task wait delivers the question itself. | ||
| if ( | ||
| task.completionWake === "settled_only" && | ||
| event.payload.responseCapability.type === "live" && | ||
| parent.runs.some( | ||
| (run) => | ||
| run.id === task.runId && | ||
| (run.status === "preparing" || run.status === "starting" || run.status === "running"), | ||
| ) | ||
| ) { | ||
| return; | ||
| } | ||
| yield* wakeParent(task, request); |
There was a problem hiding this comment.
⚫️ [PROBLEM] Skip check reads mutable completionWake, so the parent can be told about the same question twice
⚫ [PROBLEM] This guard decides whether the blocking delegate_task wait will deliver the question. It reads task.completionWake === "settled_only" from the projection at the moment the reactor runs (line 133). The problem is that the same runtime-request.updated event also wakes waitForTask in OrchestratorMcpService.ts (lines 1544/1560, hasPendingQuestion). After that wait returns, delegateTask immediately dispatches delegated_task.wake-policy with completionWake: "always" (OrchestratorMcpService.ts:2154-2165).
If that upgrade commits before this reactor reads the parent records, the check fails. wakeParent then queues a queue_after_active message about a question the parent already got in the tool result. Here is how that plays out: the parent passes the question to the user and ends its turn, and then the queued notification starts an extra parent turn about the same question. Whether this happens depends on timing. The reactor tests call handle directly, so they never exercise this ordering.
Suggested fix: base the skip on state that does not change when the wait ends. Some options:
- Have the wait path record that it delivered this request id, for example in the wake-policy command or a separate event.
- Have the wait path dispatch the same deterministic
message:delegated-task-question:<key>command id, so the reactor's dispatch becomes an idempotent no-op. - Make the skip depend on whether the task was launched in wait mode and the launching parent run is still active, not on
completionWake.
|
This is a big, carefully built change. The destructive paths (deleting a branch, removing a worktree) have more layers of guards than usual, and the tests cover the tricky orderings well. I'm raising two problems below, and both are about timing. OverviewSummary
The main risks are in how the new workspace lease and the question reactor interact with work that is already running. Good points
Main Issues
Additional Notes
|
Problem
The fork is 21 commits behind
upstream/main. This sync has to keep the features recorded indocs/adrwhile using upstream's implementations wherever it can.Change
Merged
pingdotgg/t3codemain at339179f50a84dc599cce373dbb48f172f182f44einto the fork. None of the 21 commits implement a fork feature, so every ADR stays accepted and none needs an "implemented upstream" note.packages/contracts/src/settings.test.tsconflicted, on the order of one import. It takes upstream's import and keeps the fork's default base branch tests (ADR 0009).delete-branch, the same as GitLab and Azure DevOps..github. The fork still has onlyci.ymlandserver-linux-release.yml, and the release define is intact.339179f50.git show --remerge-diff 13e4715c2shows the whole fork-side resolution: the ADR bumps and the one import line.Verification
GitManager.test.tsandGitVcsDriverCore.test.ts, and this sync does not changeapps/server/src/gitorapps/server/src/vcs. They come from the local environment: the five known GitManager own-name tests, nine tag tests that fail because the global git config setstag.gpgsign(they pass with it overridden), and one rename-preview test that fails the same way on fork main. CI is the source of truth for these.Model: Claude Opus 5.5; harness: Claude Code in T3 Code.
🤖 Generated with Claude Code