Repository navigation
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 724000d33b
ℹ️ 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".
| if (summary.unarchivedDescendantCount > 0) { | ||
| throw new ArchiveThreadConfirmationRequired( | ||
| summary.unarchivedDescendantCount, | ||
| summary.workspaceRemovalDelayMs ?? null, |
There was a problem hiding this comment.
When confirmation is enabled and the target is the last live thread in its environment but has no unarchived descendants, the summary contains a numeric workspaceRemovalDelayMs, yet this condition is false and execution proceeds directly to archiveAll. That common single-thread case therefore removes the workspace without ever showing the new warning; a numeric removal delay also needs to trigger confirmation.
Useful? React with 👍 / 👎.
| export const threadChildSummaryResponseSchema = z.object({ | ||
| nonDeletedChildCount: z.number().int().nonnegative(), | ||
| unarchivedDescendantCount: z.number().int().nonnegative(), | ||
| workspaceRemovalDelayMs: z.number().int().nonnegative().nullable().optional(), |
There was a problem hiding this comment.
bb thread archive
This field makes the warning available through the SDK and UI only. The existing CLI path at apps/cli/src/commands/thread/actions.ts:271-308 calls sdk.threads.archive immediately and neither queries childSummary nor exposes another command that reports the removal delay, so CLI users still lose a workspace without the new warning. Add the corresponding CLI behavior and documentation required for an end-user feature.
AGENTS.md reference: AGENTS.md:L24-L27
Useful? React with 👍 / 👎.
| const keepsEnvironment = listLiveThreadsInEnvironment(db, { | ||
| environmentId: thread.environmentId, | ||
| }).some((liveThread) => !archivedThreadIds.has(liveThread.id)); |
There was a problem hiding this comment.
Each child-summary request now materializes every live thread row in the environment and filters the array in JavaScript. This is especially expensive for shared project-checkout environments, where the provider retains workspaces but the code still loads the entire project’s thread set before eventually returning null; use a targeted existence query for a live thread outside the archive set, and preferably check the provider policy before querying threads.
AGENTS.md reference: AGENTS.md:L32-L35
Useful? React with 👍 / 👎.
| const count = Math.round(delayMs / unit.ms); | ||
| return `Its workspace is removed ${count} ${unit.label}${count === 1 ? "" : "s"} after archiving.`; |
There was a problem hiding this comment.
For provider-defined delays that are not exact units, Math.round can promise more time than users actually have. For example, a 90-second grace period is displayed as “2 minutes,” and a 36-hour grace period as “2 days,” even though removal happens substantially earlier; format compound units, round down, or label the value as approximate rather than overstating the deadline.
Useful? React with 👍 / 👎.
This branch has not been deployed
1 participant
Human comments
What was wrong
The archive confirmation dialog never said that archiving can remove the thread's workspace. When archiving leaves an environment with no unarchived threads, the server schedules removal after the environment provider's
retireGraceMs(5 minutes by default;nullkeeps it). That policy only existed on the server, so the app had nothing to show.What changed
GET /api/v1/threads/:id/child-summarynow also returnsworkspaceRemovalDelayMs. It is the provider'sretireGraceMswhen archiving this thread plus everything archive-all would take with it (children, lifecycle dependents, hidden forks) leaves its environment with no unarchived threads. It isnullwhen other threads keep the environment, when the provider keeps workspaces (retireGraceMs: null, e.g. project checkout), when there's no provider environment, or when removal is already scheduled or running. The newgetEnvironmentRetireGraceMsinenvironment-engine.tsapplies the same checks the retire sweep does.threadChildSummaryResponseSchemagains an optional, nullableworkspaceRemovalDelayMs. The change only adds a field: older servers omit it and the app treats a missing value asnull.sdk.threads.childSummarypicks it up through the shared type. No daemon/protocol change.ThreadArchiveDialogadds "Its workspace is removed 5 minutes after archiving." only when the delay is a number. It says nothing fornull, and the existing sentences are unchanged.docs/debugging-and-qa.md(Archive Confirmation Counts) documents the field.How you verified
provider-orchestration.test.tschecks the route against a real in-memory DB and provider: grace + last thread → delay, grace + child in same env → delay, grace + unrelated live thread →null,retireGraceMs: null→null.ThreadArchiveDialog.test.tsxcovers the sentence for 5 min, 1 hour and 0, and that it's absent fornull. Existing child-summarytoEqualassertions are updated. CI runs remotely.Screenshots (last thread in a worktree environment)
Before (merge base
9a47d42)After (PR head)
BB-Thread: Show workspace cleanup timing when archiving
🤖 Generated with Claude Code