[2witstudios]

Why

PageSpace agents had no planning affordance. Read-only mode already frames itself as plan mode — READ_ONLY_CONSTRAINT literally says "Focus on exploring, analyzing, and planning / Create actionable plans for the user to execute later" — but nothing taught the agent how to produce a plan, and nothing let a conversation remember which plan it was working against.

Prior art is unanimous that the plan must be a durable artifact (Cursor .plan.md, Cline's /deep-planning → implementation_plan.md, Claude Code's plan file, Copilot Workspace attaching it to the PR), and the stated reason is context compaction: a file/page re-read is deterministic where a summary only approximately remembers. The documented anti-pattern is re-read thrash, which argues for a small durable pointer plus deliberate re-reads rather than bulk re-injection.

PageSpace had the compaction problem and none of the plumbing. conversations had no page association at all; "where am I" was rebuilt from the browser URL every turn and thrown away.

Two tiers of skill, split by ownership

A built-in skill teaches PageSpace's own mechanics (how a CANVAS page works, what SUBTASKS_INCOMPLETE means). Users have no reason to edit those; a deploy is the right way to change them.

A starter skill is methodology, where the useful property is that the user can disagree with it and rewrite it. So /plan seeds once as a real DOCUMENT at Home / Skills / Plan plus a personal commands row, and then belongs to the user — the registry is a seed, not a live source, and edits there never reach an existing install.

Idempotence rides users.starterSkillsInstalledAt, deliberately not "does a command with this trigger exist?". Only a stamp both refuses to resurrect a starter the user deliberately deleted and lets the backfill re-run safely after a partial failure.

• New signups install inside provisionHomeDriveIfNeeded — both drive-creation branches, same transaction, web + admin copies kept in sync.
• Existing users: one-shot scripts/backfill-starter-skills.ts (--dry-run supported). No login-path change, so the auth hot path is untouched.

The binding, and why it lives in the cache-stable prompt

conversations.planPageId (migration 0246), set by new set_plan / clear_plan tools, renders an ACTIVE PLAN: section into the cache-stable system prompt.

That placement is the requirement, not an optimization. The compaction summary at ModelMessages[0] is lossy, so a pointer carried in the message stream gets summarized away exactly when it is most needed — the same reason TasksDropdown's message-derived task binding empties on truncation. The system prompt is never compacted, so the pointer survives by construction.

It stays cache-safe because the value changes only on an explicit tool call and never on navigation: one prefix invalidation per plan change, the same trade Agent Memory already accepts.

Plan page titles are user-authored and land verbatim in the system prompt, so they pass clipDescription first — an unsanitized title with newlines could otherwise forge a whole instruction section. There's a test for that.

Authorization is principal-aware, not user-level. The page-chat route also serves scoped MCP and OAuth tokens, and unlike the neighbouring Agent Memory section (whose page is by construction a child of the agent page, hence always same-drive) a plan binding can point at a page in any drive. So getActivePlan takes an injected check — canPrincipalViewPage on the page-chat route, defaulting to the user-level check for the session-only Global Assistant — and set_plan uses canActorViewPage. "Can this human see it" is strictly wider than "may this token reach it"; without the distinction a token scoped to drive A could read the title and id of a plan bound in drive B.

PlanChip sits beside TasksDropdown in both chat headers but reads the persisted binding, so it survives a reload and a summary.

Where plan artifacts go (resolved an open question in the epic)

• In a drive → find-or-create a Plans folder at that drive's root; plans live next to the work, visible to drive members.
• No drive in view (global assistant) → the user's Home drive.

To make the second half reachable, buildLocationTurnPrompt now renders the Home drive id in its no-location branch only — list_drives exposes neither drive kind nor a Home marker, so an agent standing nowhere previously had no way to name Home. The standing "do NOT assume the Home drive" guard is unchanged; the skill body carries the narrow exception.

Drive-by fix

buildLocationTurnPrompt was silently dropping currentPage.id even though LocationContext carried it — the model could only address pages by title, which is ambiguous across drives and changes under it on rename. Now rendered, in the volatile block, at zero cache cost. Dead isTaskLinked removed from this prompt only (still live in the page tree and page-read tools).

Deliberate calls worth a reviewer's eye

• set_plan/clear_plan are not in WRITE_TOOLS so they survive read-only mode. They mutate conversation metadata, not user content, and binding an existing plan doc is a legitimate read-only action.
• Read-only still strips create_page, so /plan cannot write its artifact there. The body says so plainly and offers to save once write mode is on, rather than thrashing. A carve-out would need a new predicate alongside filterToolsForReadOnly — out of scope here.
• A skill cannot change the active toolset mid-conversation (prepareStep returns only { messages }; there is no experimental_activeTools). /plan instructs, it does not enforce.

Verification

Monorepo typecheck, build, and lint clean.

Suite	Result
@pagespace/lib	8,748 passed
web	16,184 passed
admin	178 passed

Run against a scratch Postgres with all 246 migrations applied. Three initially-failing web files and the admin broadcast failures were an unmigrated local test DB (Postgres 42703); one (grouping.test.ts) is a pre-existing TZ dependency in a file this PR never touches and passes under TZ=UTC. Each was verified individually rather than assumed.

The generated 0246_snapshot.json accounts for ~20.9k of the diff; the hand-written change is ~1,600 lines.

Review round (commits 2 and 3)

All eight CodeRabbit threads were verified against source before acting; none was dismissed. Beyond the literal findings:

• The reviewer flagged the read path for principal scope; the write path (set_plan) had the same hole, so it was fixed too.
• On the description-budget mismatch I picked neither stated number arbitrarily: 200 is COMMAND_CATALOG_FULL_CLIP, the point past which the catalog clips anyway. But a per-description cap cannot catch N starters crowding out the user's own commands, so the aggregate guard (≤40% of COMMAND_CATALOG_CHAR_BUDGET) is the one that will actually bite when orchestrate/research/execute land.
• The installer's onConflictDoNothing skip left an orphaned page in the Skills folder — a document that looks like a skill but can never be invoked. It's now deleted, not just reported correctly.
• The lock-ordering assertion was mutation-tested: moving the FOR UPDATE below the stamp read fails it, restoring it passes.

Follow-ups (not in this PR)

• orchestrate / research / execute / brainstorm reuse this machinery — but not for existing users until a starter ledger lands. Review found that users.starterSkillsInstalledAt is a one-shot marker, not the high-water mark it was documented as: adding a second starter reaches new signups only, and the backfill cannot see already-stamped users. Adding it without resurrecting a starter the user deleted needs per-trigger provenance, i.e. a schema change. Recorded in the installer docblock and on the epic so the next PR can't miss it.
• Hiding the internal platform skills (canvas-websites et al.) from the / picker.
• TasksDropdown could read a persisted binding instead of useAggregatedTasks, fixing its truncation behavior and the model's blindness to it in one move.
• The dead todo_list message path (TodoListMessage, CompactTodoListMessage, and a loader calling /api/ai/tasks/by-message/{id} — a route that does not exist) is dead code worth deleting.

Deploy note

Requires bun run db:migrate before the new image takes traffic (both new columns are nullable, so the old image tolerates them).

🤖 Generated with Claude Code

https://claude.ai/code/session_011KebqPHDM6zHpKV7VPt2Fa

Summary by CodeRabbit

• New Features

  • Added starter planning skills to new and existing Home drives.
  • AI conversations can now bind to, view, update, and clear an active plan.
  • Added plan status indicators and quick access to linked plan pages.
  • AI responses now incorporate active-plan and Home-drive context when relevant.

• Documentation

  • Updated documentation to reflect 79 available workspace tools.
  • Added guidance for starter skills and conversation plan binding.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[coderabbitai]

📝 Walkthrough

Walkthrough

This change adds seeded starter skills, transactional Home-drive installation, and existing-user backfill. It also adds persisted conversation plan bindings, plan tools, prompt context, APIs, chat UI, schema changes, tests, and updated tool-count documentation.

Changes

Starter Skills and Conversation Plans

Layer / File(s)	Summary
Skill contracts and persistence schema packages/lib/src/commands/*, packages/db/src/schema/*, packages/db/drizzle/*, docs/specs/agent-skills.md	Defines the seeded plan skill, installation timestamp, conversation plan reference, migration metadata, and catalog rules.
Starter-skill installation packages/lib/src/commands/starter-skill-installer.ts, packages/lib/src/commands/__tests__/*, packages/lib/package.json	Installs skill pages and personal commands into a Home-drive Skills folder. It handles idempotence, existing triggers, conflicts, and missing users.
Onboarding and user backfill apps/admin/src/lib/onboarding/*, apps/web/src/lib/onboarding/*, scripts/backfill-starter-skills.ts, apps/web/src/app/api/auth/__tests__/me.test.ts	Installs starter skills during Home-drive provisioning and adds a transactional backfill for existing users.
Plan tools and prompt context apps/web/src/lib/ai/core/*, apps/web/src/lib/ai/tools/plan-tools.ts, apps/web/src/app/api/ai/chat/route.ts, apps/web/src/app/api/ai/global/[id]/messages/route.ts	Adds set_plan and clear_plan, active-plan lookup, stable prompt rendering, Home-drive hints, tool registration, and validation tests.
Plan API and interface apps/web/src/app/api/ai/conversations/*, apps/web/src/components/ai/*, apps/web/src/components/layout/*, apps/web/src/app/api/ai/*	Adds authenticated plan retrieval and clearing, plan status renderers, PlanChip, header integration, and conversation fixture updates.
Documentation and tool counts README.md, apps/marketing/src/app/docs/getting-started/page.tsx	Updates the documented workspace tool count from 77 to 79.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant ChatUI
  participant PlanTools
  participant Database
  participant SystemPrompt
  User->>ChatUI: Request planning action
  ChatUI->>PlanTools: set_plan
  PlanTools->>Database: Validate page and conversation
  PlanTools->>Database: Persist planPageId
  ChatUI->>SystemPrompt: Build next conversation prompt
  SystemPrompt->>Database: Load active plan
  Database-->>SystemPrompt: Plan identity and title
  SystemPrompt-->>ChatUI: Prompt with ACTIVE PLAN context

Loading

Possibly related PRs

• 2witstudios/PageSpace#2244: Adds the unified skill system that this change extends with starter-skill provisioning and plan binding.
• 2witstudios/PageSpace#2298: Changes related AI workspace-tool registry and tool-count infrastructure.
• 2witstudios/PageSpace#2326: Overlaps in AI chat-route and tool-filtering infrastructure while changing payer-tier sandbox eligibility.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 55.56% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely summarizes the two primary changes: the editable /plan starter skill and durable conversation-to-plan binding.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch pu/system-skills-in-hoem

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 7

🧹 Nitpick comments (2)

apps/web/src/lib/ai/core/plan-binding.ts (1)

90-90: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Keep the stable prompt independent of mutable page titles.

A page rename changes plan.title without a set_plan or clear_plan call. This invalidates the provider cache despite the prompt contract stating that plan changes are the only invalidation source.

Render the stable pageId only, or move title rendering to a deliberately volatile section.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/web/src/lib/ai/core/plan-binding.ts` at line 90, Update the stable
prompt construction around the plan context sentence to remove the mutable
plan.title value from the cached content, retaining only the stable plan.pageId;
if the title must remain visible, render it in the deliberately volatile prompt
section instead.

scripts/backfill-starter-skills.ts (1)

69-74: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

The dry-run count over-reports installs.

The dry-run branch adds STARTER_SKILL_TRIGGERS.length for every scanned user. It ignores triggers the user already owns, which the real run skips. The dry run therefore reports a higher number than the real run will install.

Query the existing commands triggers per batch and subtract the collisions, so the dry run predicts the real result. This also lets you populate summary.skippedCollision on a dry run.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/backfill-starter-skills.ts` around lines 69 - 74, Update the dry-run
path in the batch loop of the backfill script to query each batch’s existing
command triggers, count overlaps with STARTER_SKILL_TRIGGERS, and increment
summary.installed only for non-colliding triggers. Also increment
summary.skippedCollision for those overlaps so dry-run totals match the real
run.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@apps/web/src/app/api/ai/conversations/`[conversationId]/plan/route.ts:
- Around line 60-113: Add direct route tests for the GET and DELETE handlers,
covering ownership isolation, trashed or otherwise inaccessible plans, DELETE
rejection when CSRF validation fails, and successful authorized unbinding that
clears planPageId and returns the expected response. Reuse the route’s existing
authentication, database, and audit behavior in the tests without changing the
handlers’ contracts.

In `@apps/web/src/components/ai/shared/PlanChip.tsx`:
- Around line 38-45: Update the plan-tool success handling in set_plan and
clear_plan to invalidate or revalidate the SWR key used by PlanChip,
`/api/ai/conversations/${conversationId}/plan`, after each mutation succeeds.
Reuse the existing SWR mutation mechanism and ensure both binding and clearing
refresh the chip state; add integration coverage for successful set_plan and
clear_plan flows.

In `@apps/web/src/lib/ai/core/plan-binding.ts`:
- Line 68: Update getActivePlan to accept a principal-aware authorization check
in addition to userId, and use that check for row.pageId instead of relying
solely on canUserViewPage. Ensure the bound plan is returned only when the
authenticated principal, including MCP drive-scoped tokens, is authorized for
the page’s drive.

In `@docs/specs/agent-skills.md`:
- Around line 144-147: The starter description limit is inconsistent between the
specification and test guard. In docs/specs/agent-skills.md lines 144-147, align
the documented budget with the enforced value or lower the test to approximately
140 characters; in packages/lib/src/commands/__tests__/starter-skills.test.ts
lines 22-28, set MAX_DESCRIPTION_CHARS to the agreed documented limit and update
its comment to reference the specification.

In `@packages/lib/src/commands/starter-skill-installer.ts`:
- Around line 97-103: Update installStarterSkills to acquire a row-level lock
when reading the user record, then perform the starter-skills installation and
starterSkillsInstalledAt stamp update within the same transaction. Add proper
conflict handling so concurrent callers cannot both proceed after observing
NULL, and ensure this protection does not depend on locks held by callers.
- Around line 143-159: Update the command insertion flow using the existing
insert chain and onConflictDoNothing to call returning(), then only push
skill.trigger into installed when a row was returned; otherwise record the
trigger in skipped. Change skipped from const to a mutable declaration if
needed, preserving accurate backfill reporting when the command already exists.

In `@scripts/backfill-starter-skills.ts`:
- Around line 95-105: Update the final noHomeDrive calculation in the backfill
flow to exclude users recorded in summary.failed, so failed transactions are not
reported as benign no-Home-drive users. Ensure the count is scoped to users
present at the start of the walk, or rename the field and its “left unstamped”
reporting to accurately describe the broader set being counted.

---

Nitpick comments:
In `@apps/web/src/lib/ai/core/plan-binding.ts`:
- Line 90: Update the stable prompt construction around the plan context
sentence to remove the mutable plan.title value from the cached content,
retaining only the stable plan.pageId; if the title must remain visible, render
it in the deliberately volatile prompt section instead.

In `@scripts/backfill-starter-skills.ts`:
- Around line 69-74: Update the dry-run path in the batch loop of the backfill
script to query each batch’s existing command triggers, count overlaps with
STARTER_SKILL_TRIGGERS, and increment summary.installed only for non-colliding
triggers. Also increment summary.skippedCollision for those overlaps so dry-run
totals match the real run.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 8f0b5c8f-ba82-4a79-b0cc-32aad717255e

📥 Commits

Reviewing files that changed from the base of the PR and between 3f8f0a5 and 030a58b.

📒 Files selected for processing (44)

• README.md
• apps/admin/src/lib/onboarding/__tests__/home-drive.test.ts
• apps/admin/src/lib/onboarding/home-drive.ts
• apps/marketing/src/app/docs/getting-started/page.tsx
• apps/web/src/app/api/ai/chat/route.ts
• apps/web/src/app/api/ai/conversations/[conversationId]/plan/route.ts
• apps/web/src/app/api/ai/global/[id]/messages/__tests__/stream-socket-events.test.ts
• apps/web/src/app/api/ai/global/[id]/messages/route.ts
• apps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/__tests__/route.test.ts
• apps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/messages/__tests__/route.test.ts
• apps/web/src/app/api/auth/__tests__/me.test.ts
• apps/web/src/app/api/v1/chat/completions/__tests__/route-backfill.test.ts
• apps/web/src/app/api/v1/chat/completions/__tests__/route.test.ts
• apps/web/src/app/api/v1/conversations/__tests__/route.test.ts
• apps/web/src/components/ai/shared/PlanChip.tsx
• apps/web/src/components/ai/shared/chat/tool-calls/PlanBindingRenderer.tsx
• apps/web/src/components/ai/shared/chat/tool-calls/registry.tsx
• apps/web/src/components/ai/shared/index.ts
• apps/web/src/components/layout/middle-content/page-views/dashboard/GlobalAssistantView.tsx
• apps/web/src/components/layout/right-sidebar/ai-assistant/SidebarChatTab.tsx
• apps/web/src/lib/ai/core/__tests__/ai-tools.test.ts
• apps/web/src/lib/ai/core/__tests__/location-prompt.test.ts
• apps/web/src/lib/ai/core/__tests__/plan-binding.test.ts
• apps/web/src/lib/ai/core/ai-tools.ts
• apps/web/src/lib/ai/core/home-drive-hint.ts
• apps/web/src/lib/ai/core/location-prompt.ts
• apps/web/src/lib/ai/core/plan-binding.ts
• apps/web/src/lib/ai/core/skill-catalog.ts
• apps/web/src/lib/ai/tools/plan-tools.ts
• apps/web/src/lib/onboarding/__tests__/home-drive.test.ts
• apps/web/src/lib/onboarding/home-drive.ts
• docs/specs/agent-skills.md
• packages/db/drizzle/0246_sticky_venus.sql
• packages/db/drizzle/meta/0246_snapshot.json
• packages/db/drizzle/meta/_journal.json
• packages/db/src/schema/auth.ts
• packages/db/src/schema/conversations.ts
• packages/lib/package.json
• packages/lib/src/commands/__tests__/starter-skill-installer.test.ts
• packages/lib/src/commands/__tests__/starter-skills.test.ts
• packages/lib/src/commands/starter-bodies/plan.ts
• packages/lib/src/commands/starter-skill-installer.ts
• packages/lib/src/commands/starter-skills.ts
• scripts/backfill-starter-skills.ts

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

apps/web/src/components/ai/shared/PlanChip.tsx (1)

116-118: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Add an accessible name to the active-plan trigger.

The icon-only button has no explicit accessible name. Add aria-label="Active plan" so assistive technology identifies the control.

Proposed fix

-        <Button variant="ghost" size="icon" className="relative h-8 w-8" title="Active plan">
+        <Button
+          variant="ghost"
+          size="icon"
+          className="relative h-8 w-8"
+          title="Active plan"
+          aria-label="Active plan"
+        >

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/web/src/components/ai/shared/PlanChip.tsx` around lines 116 - 118, Add
aria-label="Active plan" to the icon-only Button in PlanChip so assistive
technology has an explicit accessible name, while preserving its existing title
and styling.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/lib/src/commands/__tests__/starter-skill-installer.test.ts`:
- Around line 206-213: Update the test around installStarterSkills to have the
fake client record operation order, then assert that the user-row lock occurs
before the first user-stamp read. Keep the existing lockedRows assertion, but
ensure the new assertion distinguishes lock-before-read from merely confirming
execute ran.

---

Outside diff comments:
In `@apps/web/src/components/ai/shared/PlanChip.tsx`:
- Around line 116-118: Add aria-label="Active plan" to the icon-only Button in
PlanChip so assistive technology has an explicit accessible name, while
preserving its existing title and styling.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: f02456db-6faa-4aae-8b30-06aed3dcad29

📥 Commits

Reviewing files that changed from the base of the PR and between 030a58b and d8edc15.

📒 Files selected for processing (14)

• apps/web/src/app/api/ai/chat/route.ts
• apps/web/src/app/api/ai/conversations/[conversationId]/plan/__tests__/route.test.ts
• apps/web/src/components/ai/shared/PlanChip.tsx
• apps/web/src/components/ai/shared/__tests__/PlanChip.test.tsx
• apps/web/src/components/layout/middle-content/page-views/dashboard/GlobalAssistantView.tsx
• apps/web/src/components/layout/right-sidebar/ai-assistant/SidebarChatTab.tsx
• apps/web/src/lib/ai/core/__tests__/plan-binding.test.ts
• apps/web/src/lib/ai/core/plan-binding.ts
• apps/web/src/lib/ai/tools/plan-tools.ts
• docs/specs/agent-skills.md
• packages/lib/src/commands/__tests__/starter-skill-installer.test.ts
• packages/lib/src/commands/__tests__/starter-skills.test.ts
• packages/lib/src/commands/starter-skill-installer.ts
• scripts/backfill-starter-skills.ts

🚧 Files skipped from review as they are similar to previous changes (9)

• apps/web/src/components/layout/middle-content/page-views/dashboard/GlobalAssistantView.tsx
• apps/web/src/components/layout/right-sidebar/ai-assistant/SidebarChatTab.tsx
• packages/lib/src/commands/starter-skill-installer.ts
• packages/lib/src/commands/tests/starter-skills.test.ts
• scripts/backfill-starter-skills.ts
• docs/specs/agent-skills.md
• apps/web/src/app/api/ai/chat/route.ts
• apps/web/src/lib/ai/core/plan-binding.ts
• apps/web/src/lib/ai/tools/plan-tools.ts

[2witstudios]

Deep review round (commits b831da7 + 3fc99d8)

Beyond the CodeRabbit threads I ran a multi-agent adversarial review over the branch diff (33 agents, one finder per correctness angle plus an independent verifier per finding location). It surfaced six confirmed issues nobody had flagged, including one I introduced while fixing the first round.

Fixed

Issue	Why it mattered
set_plan required strict conversation ownership	/api/ai/chat explicitly authorizes non-owners to post to a shared conversation (ownsIt || isSharedConversation). A collaborator running /plan would watch the agent write the plan page and then be told the binding failed — the exact lost-after-compaction hole the feature exists to close. Writability now mirrors the route's own gate.
PlanChip revalidation was dead code on the Global Assistant	set_plan is not in CORE_TOOL_NAMES and that route always splits its toolset, so the agent can only reach it as execute_tool({tool_name:'set_plan'}) — a tool-execute_tool part my top-level name match never saw. The fix I shipped in the previous commit therefore did nothing on the one surface where the chip is most visible. Now unwrapped.
resolveHomeDriveHint leaked past MCP drive scope	The same leak I had just fixed for the plan section, in the file next door. A drive-scoped token got a Home driveId; since the /plan body sends driveless plans to Home, it was then steered into a create_page that driveOutsideMcpScope rejects every time.
Two installer docblocks were false, not merely unclear	starterSkillsInstalledAt is a one-shot marker, not the "high-water mark" I called it — appending to STARTER_SKILLS later reaches new users only, and the backfill cannot even see stamped users. That silently blocks the orchestrate/research/execute follow-ups this PR promises, so the limitation and what fixing it needs (per-trigger provenance — a schema change) are now spelled out. Separately, the FOR UPDATE said to make serialization "owned by this function" was a no-op under the default client = db, since each statement autocommits; the default is removed so a transaction is required, which both real callers already pass.
Self-contradictory prompt	"Do NOT assume the Home drive" sat immediately above a line handing the model the Home driveId. Resolving that ambiguity the wrong way is the bug the guard was added to prevent. The guard is now scoped to general work and the id labelled off-limits unless named.

Refuted — verified, no change

The claim that removing the isTaskLinked branch drops task-linked guidance. Neither producer on the AI location path (resolvePageContext, pageContextToLocationContext) has ever set that flag; the page-tree APIs that populate it are a different code path. The branch was genuinely dead.

Documented rather than fixed

Three findings were really "the comment overclaims", so 3fc99d8 makes the claims true:

• The section changes on set_plan/clear_plan, on a rename of the bound page, and on a visibility change. The property that actually justifies the cache-stable placement is narrower than I wrote: none of those track navigation.
• ACTIVE PLAN renders on two routes while set_plan ships in the shared tool set, so on three others the binding persists but is not echoed into that surface's own prompt — the same callable-but-unadvertised position load_skill already occupies.
• Bind time uses canActorViewPage (agent-aware), render time uses canPrincipalViewPage. Closing that needs one actor abstraction shared by the prompt and tool layers; recorded with what it would take rather than left looking like an oversight.

Verification

Monorepo typecheck, build and lint clean. lib 8,748 · web 16,184 · admin 178 passing against a freshly migrated database.

One note on method: three global-route test files failed after the scope fix because their @/lib/auth mocks were incomplete. I completed the mocks rather than weakening the code — but in the earlier round the same symptom meant the code was genuinely fragile (a missing dependency threw before .catch() could attach), and there I fixed the code instead. Worth distinguishing the two.

[2witstudios]

⚠️ Two commits of review fixes landed after the merge point

This merged at d9b4e26f1, but review work continued after that commit. b831da7a and 3fc99d8 were pushed to the branch afterwards and are not in master.

They contain confirmed correctness fixes, so these are live in master right now:

• set_plan fails for collaborators on a shared conversation. This route explicitly authorizes non-owners to post (ownsIt || isSharedConversation), but the plan tools required strict ownership — so a collaborator watches the agent write the plan page and is then told the binding failed.
• The PlanChip revalidation added in d9b4e26 is dead code on the Global Assistant. set_plan is not a CORE tool and that route always splits its toolset, so it only ever arrives wrapped in execute_tool — a part shape the detector did not match.
• resolveHomeDriveHint leaks the Home driveId past MCP drive scope — the same leak fixed here for the plan section, in the file next door.
• Two installer docblocks were false rather than unclear, one of which (starterSkillsInstalledAt being a one-shot marker, not a high-water mark) silently blocks the orchestrate/research/execute follow-ups this PR promises for every existing user.

Carried forward in #2370, cherry-picked cleanly onto post-merge master and re-verified there from scratch: typecheck/build/lint clean, lib 8,748 · web 16,184 · admin 178 passing.

No action needed on this PR — flagging so the gap between the merge point and the branch tip is on the record.