apps/web/src/components/ui/scroll-area.tsx-26-28 (1)

26-28: ⚠️ Potential issue | 🟡 Minor

Prevent gutter spacing when hideScrollbars is enabled.

If a caller passes both scrollbarGutter and hideScrollbars, the viewport still reserves pe-2.5/pb-2.5 even though Lines 34-40 remove the scrollbar UI. That leaves a blank strip on the right/bottom edge.

Suggested fix

-          scrollbarGutter && "data-has-overflow-y:pe-2.5 data-has-overflow-x:pb-2.5",
+          scrollbarGutter &&
+            !hideScrollbars &&
+            "data-has-overflow-y:pe-2.5 data-has-overflow-x:pb-2.5",

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/components/ui/scroll-area.tsx` around lines 26 - 28, The gutter
spacing is still applied when both scrollbarGutter and hideScrollbars are true;
update the conditional that adds the "data-has-overflow-y:pe-2.5
data-has-overflow-x:pb-2.5" classes so it only applies when scrollbarGutter is
true AND hideScrollbars is false (e.g., change the expression to scrollbarGutter
&& !hideScrollbars) inside the ScrollArea component where those class strings
are assembled; this ensures the pe-2.5/pb-2.5 gutter is not reserved when the
scrollbar UI is hidden.

apps/web/src/index.css-123-139 (1)

123-139: ⚠️ Potential issue | 🟡 Minor

Quote Consolas and Menlo in this stack.

Lines 136 and 138 hit the current Stylelint value-keyword-case rule because those family names are unquoted. Quoting them preserves the intended font names and clears the lint error.

💡 Proposed fix

   --terminal-font-family:
     "Symbols Nerd Font Mono",
     "Symbols Nerd Font",
     "JetBrainsMono Nerd Font Mono",
     "JetBrainsMonoNL Nerd Font Mono",
     "Hack Nerd Font Mono",
     "SauceCodePro Nerd Font Mono",
     "FiraCode Nerd Font Mono",
     "MesloLGS NF",
     "CaskaydiaMono Nerd Font Mono",
     "Geist Mono",
     "SF Mono",
     "SFMono-Regular",
-    Consolas,
+    "Consolas",
     "Liberation Mono",
-    Menlo,
+    "Menlo",
     monospace;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/index.css` around lines 123 - 139, The --terminal-font-family
CSS variable contains unquoted family names "Consolas" and "Menlo" which
triggers Stylelint's value-keyword-case rule; update the font-family list in the
--terminal-font-family declaration to wrap Consolas and Menlo in quotes
(preserve the existing order, commas and other entries) so those font family
names are treated as strings and the lint error is resolved.

apps/web/src/lib/terminalFont.ts-1-29 (1)

1-29: ⚠️ Potential issue | 🟡 Minor

The fallback now applies the dark terminal stack in light mode too.

--terminal-font-family is only defined under the dark variant in apps/web/src/index.css, so Lines 25-29 fall back to DEFAULT_TERMINAL_FONT_FAMILY whenever light mode is active. Because that default is the same Nerd-font-first stack as the dark override, light-theme terminals pick up the new font list as well. Keep the fallback aligned with the non-dark stack, or define a :root value and override it only in dark mode.

💡 One way to keep the change dark-only

 const DEFAULT_TERMINAL_FONT_FAMILY = [
-  '"Symbols Nerd Font Mono"',
-  '"Symbols Nerd Font"',
-  '"JetBrainsMono Nerd Font Mono"',
-  '"JetBrainsMonoNL Nerd Font Mono"',
-  '"Hack Nerd Font Mono"',
-  '"SauceCodePro Nerd Font Mono"',
-  '"FiraCode Nerd Font Mono"',
-  '"MesloLGS NF"',
-  '"CaskaydiaMono Nerd Font Mono"',
   '"Geist Mono"',
   '"SF Mono"',
   '"SFMono-Regular"',
   "Consolas",
   '"Liberation Mono"',
   "Menlo",
   "monospace",
 ].join(", ");

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/lib/terminalFont.ts` around lines 1 - 29, The fallback constant
DEFAULT_TERMINAL_FONT_FAMILY and resolveTerminalFontFamily() currently return
the dark-mode Nerd-font-first stack when the CSS variable is absent (light
mode); change the fallback to the original non-dark stack or ensure a :root CSS
variable is defined so light mode supplies a proper value. Concretely, update
DEFAULT_TERMINAL_FONT_FAMILY to the non-dark/default terminal font list (the
stack you used before the dark update) or add a :root --terminal-font-family in
CSS and keep the dark override only in the dark variant; keep references to
DEFAULT_TERMINAL_FONT_FAMILY and resolveTerminalFontFamily() unchanged so the
function uses the corrected fallback.

.plans/16c-pr89-remediation-checklist.md-202-202 (1)

202-202: ⚠️ Potential issue | 🟡 Minor

Keep the thread IDs exact here.

The inserted * characters make these no longer match the real GitHub thread identifiers, which breaks copy/paste lookup and audit traceability.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.plans/16c-pr89-remediation-checklist.md at line 202, The listed thread
identifiers (e.g., "PRRT*kwDORLtfbc5widJw", "PRRT_kwDORLtfbc5wnWv*",
"PRRT_kwDORLtfbc5w0_g7", "PRRT_kwDORLtfbc5w1C36") contain stray asterisks that
alter the actual GitHub thread IDs; update the entries to the exact IDs by
removing the inserted "*" characters so they match the real identifiers (for
example change "PRRT*kwDORLtfbc5widJw" → "PRRT_kwDORLtfbc5widJw" and
"PRRT_kwDORLtfbc5wnWv*" → "PRRT_kwDORLtfbc5wnWv") ensuring copy/paste lookup and
audit traceability work.

apps/server/src/open.test.ts-126-142 (1)

126-142: ⚠️ Potential issue | 🟡 Minor

Clean up the temporary fixture directory.

This test creates a real directory tree but never removes it, so repeated runs leave junk behind in the OS temp dir. Wrap the fixture setup in try/finally and fs.rmSync(..., { recursive: true, force: true }).

♻️ Suggested cleanup

   it.effect("uses the containing directory when terminal editors receive a file path", () =>
     Effect.gen(function* () {
       const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "t3-open-ghostty-"));
-      const filePath = path.join(tempDir, "nested", "AGENTS.md");
-      fs.mkdirSync(path.dirname(filePath), { recursive: true });
-      fs.writeFileSync(filePath, "# test\n", "utf8");
-
-      const ghosttyFileTarget = yield* resolveEditorLaunch(
-        { cwd: `${filePath}:48`, editor: "ghostty" },
-        "linux",
-      );
-
-      assert.deepEqual(ghosttyFileTarget, {
-        command: "ghostty",
-        args: [`--working-directory=${path.dirname(filePath)}`],
-      });
+      try {
+        const filePath = path.join(tempDir, "nested", "AGENTS.md");
+        fs.mkdirSync(path.dirname(filePath), { recursive: true });
+        fs.writeFileSync(filePath, "# test\n", "utf8");
+
+        const ghosttyFileTarget = yield* resolveEditorLaunch(
+          { cwd: `${filePath}:48`, editor: "ghostty" },
+          "linux",
+        );
+
+        assert.deepEqual(ghosttyFileTarget, {
+          command: "ghostty",
+          args: [`--working-directory=${path.dirname(filePath)}`],
+        });
+      } finally {
+        fs.rmSync(tempDir, { recursive: true, force: true });
+      }
     }),
   );

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/open.test.ts` around lines 126 - 142, The test inside the
it.effect block creates a temporary directory (tempDir, filePath) but never
deletes it; wrap the fixture setup and assertions in a try/finally so the
tempDir is removed after the test, and in the finally call fs.rmSync(tempDir, {
recursive: true, force: true }) to clean up; locate the it.effect test that
calls resolveEditorLaunch and add the try/finally around the
mkdtempSync/fs.writeFileSync/setup and the assert so cleanup always runs.

apps/web/src/components/ChatView.tsx-2347-2357 (1)

2347-2357: ⚠️ Potential issue | 🟡 Minor

The sidebar carry-over is immediately overwritten on thread switch.

The later useEffect([activeThread?.id]) below closes the sidebar again, so planSidebarOpenOnNextThreadRef never survives navigation. This makes the “open on next thread” path effectively dead unless the user reopens the sidebar manually.

Suggested consolidation

 useEffect(() => {
   setExpandedWorkGroups({});
   setPullRequestDialogState(null);
-  if (planSidebarOpenOnNextThreadRef.current) {
-    planSidebarOpenOnNextThreadRef.current = false;
-    setPlanSidebarOpen(true);
-  } else {
-    setPlanSidebarOpen(false);
-  }
+  setIsRevertingCheckpoint(false);
+  const shouldOpenPlanSidebar = planSidebarOpenOnNextThreadRef.current;
+  planSidebarOpenOnNextThreadRef.current = false;
+  setPlanSidebarOpen(shouldOpenPlanSidebar);
   planSidebarDismissedForTurnRef.current = null;
 }, [activeThread?.id]);

Then remove the later duplicate useEffect([activeThread?.id]).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/components/ChatView.tsx` around lines 2347 - 2357, The current
useEffect that runs on activeThread?.id sets planSidebarOpen based on
planSidebarOpenOnNextThreadRef but is then countermanded by a later duplicate
useEffect, so the "open on next thread" flag never takes effect; consolidate the
logic by removing the later duplicate useEffect([activeThread?.id]) and ensure
the remaining useEffect (the one shown) both honors
planSidebarOpenOnNextThreadRef.current (setting setPlanSidebarOpen(true) and
clearing the ref) and resets planSidebarDismissedForTurnRef.current as needed;
update references to planSidebarOpenOnNextThreadRef,
planSidebarDismissedForTurnRef, setPlanSidebarOpen, setExpandedWorkGroups, and
setPullRequestDialogState so all thread-switch cleanup and sidebar-opening
behavior live in this single effect.

apps/server/src/git/Layers/GitManager.ts-55-74 (1)

55-74: ⚠️ Potential issue | 🟡 Minor

Don't guess the fork repo from the PR URL.

For cross-repo PRs, pullRequest.url identifies the base repo, not necessarily the head repo. If headRepositoryNameWithOwner is missing, this fallback can synthesize the wrong <owner>/<repo> and wire fetch/upstream against the wrong repository. Returning null here is safer because the caller already has a PR-ref fallback path.

Safer fallback

 function resolveHeadRepositoryNameWithOwner(
   pullRequest: ResolvedPullRequest & PullRequestHeadRemoteInfo,
 ): string | null {
   const explicitRepository = pullRequest.headRepositoryNameWithOwner?.trim() ?? "";
   if (explicitRepository.length > 0) {
     return explicitRepository;
   }
-
-  if (!pullRequest.isCrossRepository) {
-    return null;
-  }
-
-  const ownerLogin = pullRequest.headRepositoryOwnerLogin?.trim() ?? "";
-  const repositoryName = parseRepositoryNameFromPullRequestUrl(pullRequest.url);
-  if (ownerLogin.length === 0 || !repositoryName) {
-    return null;
-  }
-
-  return `${ownerLogin}/${repositoryName}`;
+  return null;
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/git/Layers/GitManager.ts` around lines 55 - 74, In
resolveHeadRepositoryNameWithOwner, stop guessing the head repo from
pullRequest.url; remove the parseRepositoryNameFromPullRequestUrl fallback and
instead return null whenever headRepositoryNameWithOwner is empty (including
cross-repo cases) or owner is missing — i.e., if explicitRepository is empty
return null, and do not synthesize `${owner}/${repo}` from pullRequest.url;
callers (which already have the PR-ref fallback) will handle the absent head
repo safely.

apps/server/src/git/Layers/GitCore.test.ts-1201-1226 (1)

1201-1226: ⚠️ Potential issue | 🟡 Minor

This test passes even if fetchPullRequestBranch() is a no-op.

feature/pr-fetch already exists locally before the method under test runs, so git branch --list feature/pr-fetch doesn't validate the fetch behavior. Delete the local branch first or assert that the ref now points at the PR head commit.

Make the assertion prove the fetch happened

         yield* git(tmp, ["push", "-u", "origin", "feature/pr-fetch"]);
+        const prHeadSha = yield* git(tmp, ["rev-parse", "HEAD"]);
         yield* git(tmp, ["push", "origin", "HEAD:refs/pull/55/head"]);
         yield* git(tmp, ["checkout", initialBranch]);
+        yield* git(tmp, ["branch", "-D", "feature/pr-fetch"]);
 
         yield* fetchGitPullRequestBranch({
           cwd: tmp,
           prNumber: 55,
           branch: "feature/pr-fetch",
         });
 
-        const localBranches = yield* git(tmp, ["branch", "--list", "feature/pr-fetch"]);
-        expect(localBranches).toContain("feature/pr-fetch");
+        expect(yield* git(tmp, ["rev-parse", "feature/pr-fetch"])).toBe(prHeadSha);
         const currentBranch = yield* git(tmp, ["branch", "--show-current"]);
         expect(currentBranch).toBe(initialBranch);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/git/Layers/GitCore.test.ts` around lines 1201 - 1226, The
test is false-positive because feature/pr-fetch exists locally before calling
fetchGitPullRequestBranch; remove the local branch (e.g., run git(...,
["branch", "-D", "feature/pr-fetch"]) or delete via helper) before invoking
fetchGitPullRequestBranch, or instead assert the branch/refs/pull/55/head commit
changed by looking up the commit hash (use git(..., ["rev-parse",
"refs/pull/55/head"]) or git(..., ["rev-parse", "feature/pr-fetch"]) and
compare) to prove the fetch actually updated the ref; update the test around
fetchGitPullRequestBranch to perform one of these actions and then assert the
expected commit identity or branch existence.

apps/web/src/components/Sidebar.tsx-1913-1917 (1)

1913-1917: ⚠️ Potential issue | 🟡 Minor

Keep SortableContext.items aligned with the rendered projects.

During thread search you return null for projects with no matches, but items still contains every project id. That makes reorder state depend on invisible projects, so clearing the search can leave the persisted order surprising. Either disable reordering while search is active or drive both the map and items from the same visible-project list.

Also applies to: 1931-1933

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/components/Sidebar.tsx` around lines 1913 - 1917, The
SortableContext is being fed all project ids (via items={projects.map(p =>
p.id)}) while the render maps can return null for non-matching projects during a
thread search, causing hidden items to affect reorder state; fix by computing a
visibleProjects array (e.g. filter projects by the same search predicate used in
the render) and use items={visibleProjects.map(p => p.id)} and map over
visibleProjects for rendering, or alternatively disable reordering when a search
is active (e.g. pass an empty items list or skip SortableContext when
threadSearchActive); update references in the Sidebar component where
SortableContext and the project rendering occur so both use the same visible
list.

apps/web/src/components/Sidebar.tsx-1515-1519 (1)

1515-1519: ⚠️ Potential issue | 🟡 Minor

Use Element in the global mousedown guard.

The target for the PR/provider/terminal icons can be an SVGElement, so this HTMLElement narrow turns safe clicks inside a data-thread-item into null and clears the current multi-select unexpectedly. Switching this check to event.target instanceof Element and widening shouldClearThreadSelectionOnMouseDown accordingly avoids that false reset.

💡 Suggested fix

-      const target = event.target instanceof HTMLElement ? event.target : null;
+      const target = event.target instanceof Element ? event.target : null;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/components/Sidebar.tsx` around lines 1515 - 1519, The mousedown
handler's target narrowing uses `event.target instanceof HTMLElement`, which
returns null for SVG elements and causes valid clicks on PR/provider/terminal
icons to be treated as misses; change the guard in the onMouseDown function to
`event.target instanceof Element` and update the signature/logic of
shouldClearThreadSelectionOnMouseDown to accept an Element (or broader) instead
of HTMLElement so SVGElement targets are handled correctly; ensure
clearSelection is only called when shouldClearThreadSelectionOnMouseDown returns
true with the new Element type.

apps/server/src/provider/Layers/CodexAdapter.ts (1)

1318-1318: Remove the no-op identity map.

.pipe(Effect.map((session) => session)) is an identity transformation that adds no value. It likely remained from a previous refactor.

🧹 Proposed fix

-      }).pipe(Effect.map((session) => session));
+      });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/provider/Layers/CodexAdapter.ts` at line 1318, Remove the
no-op identity mapping in CodexAdapter by deleting the
`.pipe(Effect.map((session) => session))` call (the identity transform using
Effect.map) and directly return the preceding Effect/stream result instead;
locate the pipe chain on the session-producing expression in the CodexAdapter
implementation and remove the Effect.map identity to simplify the pipeline.

apps/web/src/threadSelectionStore.test.ts (1)

58-65: Consider adding explicit test for deselecting the anchor thread.

The test "preserves anchor when deselecting a non-anchor thread" covers deselecting a thread that isn't the anchor. It may be worth adding an explicit test case for when the anchor thread itself is deselected via toggleThread, verifying that the anchor remains (which is the current behavior based on the implementation). This would document the intentional distinction from removeFromSelection, which clears the anchor when the anchor thread is removed.

📝 Suggested test case

it("preserves anchor when deselecting the anchor thread itself", () => {
  const store = useThreadSelectionStore.getState();
  store.toggleThread(THREAD_A);
  store.toggleThread(THREAD_B); // anchor = B
  store.toggleThread(THREAD_B); // deselect B (the anchor)

  const state = useThreadSelectionStore.getState();
  expect(state.selectedThreadIds.has(THREAD_B)).toBe(false);
  expect(state.anchorThreadId).toBe(THREAD_B); // anchor preserved for shift-click continuity
});

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/threadSelectionStore.test.ts` around lines 58 - 65, Add an
explicit unit test to cover deselecting the anchor thread via toggleThread:
using useThreadSelectionStore.getState(), toggle THREAD_A then THREAD_B to make
B the anchor, then toggleThread(THREAD_B) to deselect the anchor and assert that
selectedThreadIds no longer contains THREAD_B but anchorThreadId still equals
THREAD_B; reference useThreadSelectionStore, toggleThread, selectedThreadIds and
anchorThreadId to locate where to add the test and distinguish this behavior
from removeFromSelection which should clear the anchor.

apps/web/src/composerDraftStore.ts (1)

29-50: Well-designed debounced storage wrapper for single-key persistence.

The implementation correctly wraps the base storage with debounced writes. One note: the shared Debouncer instance means only the last call's arguments are retained across all keys. This works perfectly for Zustand persist (which uses a single key), but could be surprising if this exported function is reused elsewhere with multiple keys.

Consider adding a brief JSDoc comment documenting this single-key semantic:

+/**
+ * Creates a storage wrapper that debounces setItem calls.
+ * Note: Uses a single shared debouncer, so only the last setItem call's
+ * arguments are retained. Designed for single-key usage (e.g., Zustand persist).
+ */
 export function createDebouncedStorage(baseStorage: StateStorage): DebouncedStorage {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/composerDraftStore.ts` around lines 29 - 50, The
createDebouncedStorage wrapper uses a single shared Debouncer instance
(Debouncer) so only the most recent maybeExecute arguments are retained across
all keys, which is fine for single-key usage (Zustand persist) but surprising
for multi-key use; add a brief JSDoc above createDebouncedStorage describing
this single-key semantic, noting that Debouncer is shared and only the last
key/value is persisted when multiple keys are used, and advise callers to not
reuse this wrapper for multi-key persistence or to construct separate debounced
storages per key.

apps/server/src/provider/Layers/ClaudeCodeAdapter.ts (1)

847-850: Consider adding providerThreadRef(context) to the fallback providerRefs for consistency.

The normal completion path (lines 932-935) includes ...providerThreadRef(context) in providerRefs, but this fallback path omits it. If context.resumeSessionId is available, the providerThreadId would be missing from the event in the fallback case.

♻️ Suggested fix for consistency

           providerRefs: {
+            ...providerThreadRef(context),
             ...(fallbackTurnId ? { providerTurnId: String(fallbackTurnId) } : {}),
           },

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/provider/Layers/ClaudeCodeAdapter.ts` around lines 847 - 850,
The fallback branch that builds providerRefs only spreads providerTurnId when
fallbackTurnId exists but omits the providerThreadRef built by
providerThreadRef(context), so events lose providerThreadId when
context.resumeSessionId is present; update the fallback providerRefs object (the
code around providerRefs in ClaudeCodeAdapter) to also spread
...providerThreadRef(context) alongside ...(fallbackTurnId ? { providerTurnId:
String(fallbackTurnId) } : {}) so the fallback path includes the same
providerThreadId logic as the normal completion path.

apps/web/src/components/ChatView.browser.tsx (1)

293-311: Keep the root snapshot timestamp aligned with the injected plan state.

This helper makes the nested thread/plan newer than snapshot.updatedAt, which creates a read-model state the app should not normally receive. Keeping the top-level timestamp in sync will make the fixture less surprising for any time/order-sensitive UI logic.

♻️ Proposed fix

   return {
     ...snapshot,
+    updatedAt: isoAt(1_001),
     threads: snapshot.threads.map((thread) =>
       thread.id === THREAD_ID
         ? Object.assign({}, thread, {
             proposedPlans: [

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/components/ChatView.browser.tsx` around lines 293 - 311, The
top-level snapshot.updatedAt must be kept in sync with the injected thread/plan
timestamps so the fixture doesn't create an impossible read model; when mapping
snapshot.threads and injecting the proposedPlans for THREAD_ID (with createdAt:
isoAt(1_000) and updatedAt: isoAt(1_001)), also set the returned
snapshot.updatedAt to isoAt(1_001) (or otherwise align it to the thread's
updatedAt) so the root snapshot timestamp matches the nested plan state.

apps/web/src/components/ui/toast.logic.test.ts (1)

18-63: Consider one more normalization regression case.

These tests lock in the happy path and undefined, but buildVisibleToastLayout also collapses null, negative, and non-finite heights to 0. A tiny table-driven case for those inputs would make future refactors safer.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/components/ui/toast.logic.test.ts` around lines 18 - 63, Add a
new test case in apps/web/src/components/ui/toast.logic.test.ts that exercises
buildVisibleToastLayout with heights that should be normalized to zero (e.g.,
null, negative numbers, NaN, Infinity) using a small table-driven input array;
assert that layout.frontmostHeight === 0 and that each item’s offsetY (and
visibleIndex if relevant) collapses to the expected sequence (offsetY all zero
and visibleIndex incrementing from 0) to lock in the normalization behavior for
null/negative/non-finite values.

.plans/08-precommit-format-and-lint.md (1)

20-22: Clarify formatter tooling choice.

The CI workflow uses oxfmt (bun run fmt:check), but this plan proposes biome format --write and biome check for pre-commit hooks. If both tools are intended for different purposes (e.g., oxfmt for package.json sorting, biome for code formatting), consider documenting this distinction. Otherwise, align the tooling to avoid inconsistencies between local pre-commit checks and CI.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.plans/08-precommit-format-and-lint.md around lines 20 - 22, Clarify and
align the formatter tooling: either document why `oxfmt` (invoked by `bun run
fmt:check`) is used in CI and `biome format --write` / `biome check` are used in
pre-commit (e.g., note `oxfmt` is for package.json sorting and `biome` is for
code styles), or change the staged-file tasks to use the same command as CI
(`bun run fmt:check` / `oxfmt`) to avoid mismatches; update the plan text around
the staged tasks (`biome format --write`, `biome check`) to explicitly state the
intended roles or replace them so the pre-commit hooks and CI use the identical
formatter command.

.plans/10-unify-process-session-abstraction.md (1)

34-42: Make PTY coverage part of the done criteria.

Line 32 calls PTY behavior the main risk, but Line 37 still leaves PTY-path tests optional. That makes it too easy to ship the abstraction with only child-process coverage. Please require at least one PTY-backed write/exit contract test in the done criteria.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.plans/10-unify-process-session-abstraction.md around lines 34 - 42, Update
the Done Criteria to require PTY-path coverage: add a bullet saying at least one
PTY-backed contract test must exist that exercises write and exit behavior
(write/kill/killAll) so PTY-backed session backends are exercised; reference the
existing test harness (processManager.test.ts) as the baseline and require
adding a PTY-backed test that verifies write and kill/killAll semantics for the
Session backend before the item is considered done.

apps/web/src/routes/_chat.$threadId.tsx (1)

18-19: Centralize the composer width heuristic.

This callback now hard-codes a second layout threshold (208 + actions + gap) alongside the compact rules in apps/web/src/components/composerFooterLayout.ts. If either side changes, the resize guard and the actual footer layout can disagree. Consider moving the minimum-width calculation into the shared composer layout module so both decisions stay in sync.

Also applies to: 95-119

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/routes/_chat`.$threadId.tsx around lines 18 - 19, The hard-coded
COMPOSER_COMPACT_MIN_LEFT_CONTROLS_WIDTH_PX in _chat.$threadId.tsx duplicates
the layout threshold logic in apps/web/src/components/composerFooterLayout.ts;
move the minimum-width calculation into the shared composer layout module (where
composerFooterLayout lives), export a single function/constant (e.g.,
getComposerCompactMinWidth or COMPOSER_COMPACT_MIN_WIDTH) and replace the
literal in _chat.$threadId.tsx (and the other occurrences around lines 95–119)
and the resize guard to call/import that shared value so the guard and the
footer layout use the exact same heuristic. Ensure the exported symbol accounts
for actions+gap logic currently encoded in both places.

apps/web/src/pullRequestReference.ts (1)

1-3: URL pattern only supports public GitHub.

The regex is limited to github.com. If enterprise GitHub instances (e.g., github.mycompany.com) need to be supported in the future, this pattern would need adjustment.

If this is intentional (public GitHub only), consider adding a brief comment documenting this scope.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/pullRequestReference.ts` around lines 1 - 3, The URL regex
GITHUB_PULL_REQUEST_URL_PATTERN currently only matches github.com and will not
accept enterprise or self-hosted domains like github.mycompany.com; either
broaden the pattern to allow optional subdomains (e.g., permit *.github.* or any
host matching /https?:\/\/[^/\s]+\/[^/\s]+\/pull\/(\d+)/i) or, if public
github.com is intentional, add a concise comment above
GITHUB_PULL_REQUEST_URL_PATTERN (and optionally PULL_REQUEST_NUMBER_PATTERN)
documenting that only public github.com URLs are supported and why.

apps/server/src/git/Layers/GitHubCli.test.ts (1)

125-125: Consider using toContain for clearer assertion.

The current assertion works but expect().toContain() provides clearer failure messages:

-      assert.equal(error.message.includes("Pull request not found"), true);
+      expect(error.message).toContain("Pull request not found");

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/git/Layers/GitHubCli.test.ts` at line 125, Replace the
brittle assert.equal check in the test with a Jest-style containment assertion
for clearer failures: in GitHubCli.test.ts change the assertion using
assert.equal(error.message.includes("Pull request not found"), true) to use
expect(error.message).toContain("Pull request not found") (or the test
framework's equivalent) so failures report the actual message and mismatch more
clearly.

apps/web/src/components/BranchToolbar.tsx (1)

133-134: Consider simplifying optional prop forwarding.

The conditional spread pattern works but is verbose. Since React ignores undefined props, you could pass them directly:

onCheckoutPullRequestRequest={onCheckoutPullRequestRequest}
onComposerFocusRequest={onComposerFocusRequest}

However, I acknowledge this matches the existing pattern on line 134, so keeping it consistent is reasonable if this is an intentional style choice in the codebase.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/components/BranchToolbar.tsx` around lines 133 - 134, The
conditional prop-spread for onCheckoutPullRequestRequest and
onComposerFocusRequest is verbose; simplify by passing the props directly (e.g.,
use onCheckoutPullRequestRequest={onCheckoutPullRequestRequest} and
onComposerFocusRequest={onComposerFocusRequest}) because React ignores undefined
props—update the JSX in BranchToolbar (where those spreads appear) to remove the
{...(cond ? { prop } : {})} pattern and pass the props directly.

apps/server/src/wsServer.ts (1)

629-644: Race condition: clients connecting before health check completes receive empty providers.

The providers variable starts as an empty array and is populated asynchronously via Effect.forkIn. Clients connecting before the health check completes will receive an empty providers array in both the initial serverGetConfig response (line 1041) and any serverConfigUpdated pushes triggered by keybinding changes (line 660).

Consider initializing with a loading/pending state or awaiting the first health check before accepting connections, depending on the desired UX.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/wsServer.ts` around lines 629 - 644, The current logic
assigns providers = [] and then populates it asynchronously from
providerHealth.getStatuses (used in the Effect.flatMap that calls broadcastPush
to WS_CHANNELS.serverConfigUpdated), causing serverGetConfig responses and
subsequent pushes to send an empty providers list if clients connect before the
first health check completes; fix by treating providers as a pending state or
awaiting the first emission from providerHealth.getStatuses before accepting or
responding to connections: either initialize providers to a sentinel (e.g., null
or {status: "loading"}) and update serverGetConfig and the broadcastPush
payloads to convey a loading state, or block/await the first
providerHealth.getStatuses emission (subscribe synchronously or await the first
value from providerHealth.getStatuses) before forking subscriptionsScope so
providers is populated before any broadcastPush or serverGetConfig runs.

.plans/01-shared-model-normalization.md (1)

22-28: Plan may conflict with contracts package guideline.

The proposal to add normalizeModelSlug, resolveModelSlug, and DEFAULT_MODEL to packages/contracts may conflict with the established guideline that the contracts package should remain schema-only with no runtime logic.

Consider placing these utilities in packages/shared (e.g., t3tools/shared/model) instead, keeping only the canonical model list schema in contracts.

Based on learnings: "In packages/contracts, keep this package schema-only — no runtime logic. Use explicit subpath exports in packages/shared."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.plans/01-shared-model-normalization.md around lines 22 - 28, The contracts
package must remain schema-only: remove runtime utilities normalizeModelSlug,
resolveModelSlug, and the DEFAULT_MODEL constant from the contracts model module
and keep only the canonical model list/schema there; then implement those
utilities in the shared package (create a module that defines
normalizeModelSlug, resolveModelSlug, DEFAULT_MODEL and imports the canonical
model list from the contracts module) and expose them via explicit subpath
exports from the shared package; finally update the contracts package exports to
only export the schema/canonical list and ensure all consumers import the
runtime helpers from the new shared subpath.

packages/contracts/src/git.ts (1)

19-21: Consider consolidating the duplicated PR schemas.

GitStatusPrState/GitPullRequestState and GitStatusPr/GitResolvedPullRequest now describe the same contract twice. Reusing one shared schema here would reduce the chance of the status and PR-resolution payloads drifting later.

Also applies to: 38-45

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/contracts/src/git.ts` around lines 19 - 21, There are duplicate
schemas for PR state and PR objects; replace duplicated definitions by creating
a single canonical schema (e.g., GitPullRequestState) and reuse it everywhere
instead of GitStatusPrState and GitPullRequestState, and likewise consolidate
the PR object schemas by making one shared schema (e.g., GitPullRequest /
GitResolvedPullRequest) and exporting/referencing it from GitStatusPr and
GitResolvedPullRequest locations; update all references to use the shared names
so the contracts don’t diverge (apply the same consolidation to the other
duplicates around the GitStatusPr/GitResolvedPullRequest definitions).

apps/server/src/provider/Layers/ProviderService.ts (1)

471-487: ⚠️ Potential issue | 🟠 Major

Let listThreadIds() failures surface instead of returning partial sessions.

Line 471 still turns a directory listing failure into [], so callers get active sessions with dropped persisted overrides like resumeCursor and runtimeMode and cannot distinguish that from “no bindings”. Keep the per-binding fallback on Line 478, but avoid swallowing the outer listing failure.

Suggested narrowing of the fallback

         const persistedBindings = yield* directory.listThreadIds().pipe(
           Effect.flatMap((threadIds) =>
             Effect.forEach(
               threadIds,
               (threadId) =>
                 directory
                   .getBinding(threadId)
                   .pipe(Effect.orElseSucceed(() => Option.none<ProviderRuntimeBinding>())),
               { concurrency: "unbounded" },
             ),
           ),
-          Effect.catchCause((cause) => {
-            return Effect.logWarning("failed to list persisted thread bindings", { cause }).pipe(
-              Effect.map(() => [] as Array<Option.Option<ProviderRuntimeBinding>>),
-            );
-          }),
         );

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/provider/Layers/ProviderService.ts` around lines 471 - 487,
The current implementation swallows failures from directory.listThreadIds() by
catching the cause and returning an empty array, causing callers to mistake a
listing error for "no bindings"; instead, remove or narrow the Effect.catchCause
that wraps directory.listThreadIds() so that listThreadIds() errors propagate,
while keeping the per-binding fallback around directory.getBinding(threadId)
(the Effect.orElseSucceed for each getBinding) to tolerate missing individual
bindings; in practice, locate the persistedBindings computation that calls
directory.listThreadIds().pipe(...) and change it to let listThreadIds()
failures surface (i.e., do not map them to []), preserving only the per-thread
getBinding fallback.

apps/web/src/components/Sidebar.tsx (1)

17-29: ⚠️ Potential issue | 🟠 Major

Add keyboard support to project reordering.

Project reordering is pointer-only. The dnd-kit library supports keyboard sorting via KeyboardSensor with sortableKeyboardCoordinates. Keyboard-only users currently cannot reorder projects.

♿ Suggested fix

 import {
   DndContext,
   type DragCancelEvent,
   type CollisionDetection,
+  KeyboardSensor,
   PointerSensor,
   type DragStartEvent,
   closestCorners,
   pointerWithin,
   useSensor,
@@ -23,6 +24,7 @@ import {
 import { SortableContext, useSortable, verticalListSortingStrategy } from "@dnd-kit/sortable";
+  sortableKeyboardCoordinates,

Update the sensor setup:

   const projectDnDSensors = useSensors(
     useSensor(PointerSensor, {
       activationConstraint: { distance: 6 },
     }),
+    useSensor(KeyboardSensor, {
+      coordinateGetter: sortableKeyboardCoordinates,
+    }),
   );

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/components/Sidebar.tsx` around lines 17 - 29, Project reordering
currently only registers PointerSensor so keyboard users cannot reorder items;
update the DnD sensor setup used by DndContext to include KeyboardSensor
alongside PointerSensor by importing KeyboardSensor and
sortableKeyboardCoordinates and adding useSensor(KeyboardSensor, { coordinates:
sortableKeyboardCoordinates }) to the useSensors call (keep the existing
PointerSensor entry) so DndContext uses both sensors and enables keyboard-based
sorting for the sortable list components (e.g., where DndContext, useSensor,
useSensors, PointerSensor, and verticalListSortingStrategy are used).

apps/web/src/store.ts (1)

400-408: Prefer keyed sharing for activities too.

updateOrderedArrayWithStructuralSharing() recreates every later activity when the server inserts or reorders one in the middle. Since activities already have stable ids, using the keyed helper here would preserve identity better and cut avoidable rerenders.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/store.ts` around lines 400 - 408, The current mapActivities uses
updateOrderedArrayWithStructuralSharing which rebuilds later activities on
mid-list inserts; switch to the keyed variant (e.g.,
updateOrderedArrayWithKeyedStructuralSharing) so identity is preserved by key:
call that helper with incoming, previous, mapItem: normalizeActivity and a key
selector that returns each activity's stable id (e.g., item => item.id) so
reorders/inserts won't recreate unaffected activities.

apps/web/src/components/ChatView.tsx (1)

2347-2357: Merge this into the existing thread-reset effect.

This effect repeats the same expandedWorkGroups / plan-sidebar reset that's already happening later on activeThread?.id. Keeping both means two state-reset passes on every thread navigation. Folding the PR-dialog cleanup into the existing effect would keep this logic in one place.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/components/ChatView.tsx` around lines 2347 - 2357, Duplicate
thread-reset logic: remove this standalone useEffect and fold its cleanup
actions into the existing effect that already depends on activeThread?.id;
specifically, in the effect that currently resets expandedWorkGroups and
plan-sidebar state, also call setPullRequestDialogState(null),
setExpandedWorkGroups({}), handle planSidebarOpenOnNextThreadRef.current (set to
false and call setPlanSidebarOpen(true) or setPlanSidebarOpen(false)
accordingly), and set planSidebarDismissedForTurnRef.current = null so all
thread-reset work lives in the single effect tied to activeThread?.id.