Make MCP usable for planning: fix the consent dead end, add the ceremony tools - #89
Merged
Conversation
Connecting an MCP client dead ended. After signing in, the consent screen demanded a passkey step up on top of the sign in that had just happened, and when that second prompt did not complete there was nothing to do: the decision endpoint kept answering passkey_required, the form raised a toast, and the client was left waiting for a redirect that never came. A session that authenticated within the last five minutes now satisfies the step up on its own, because that sign in already proved possession. An older session still has to present a passkey, which is the case the step up was written for. The window is closed rather than open at the edge, an unparseable timestamp never counts as fresh, and neither does one stamped in the future, so a skewed clock cannot skip the check. When consent cannot be completed for any reason, the screen now offers to return to the client with access_denied instead of stranding it, so the client reports a refused connection rather than hanging. Discovery also advertised only the four default scopes while the server issues orbit.read and orbit.write, so a client that trusts the metadata never asked for the permissions the consent screen describes. Verified end to end against a registered client: authorize, consent, code exchange, then tools/list over the issued token.
The MCP server offered 23 tools and none of them touched the ceremony the team actually runs. A standup could not be opened, walked, recorded, or reviewed from a client, sprints could only be read rather than opened or closed, and milestones were absent entirely, so planning through MCP stopped at the issue list. Sixteen tools close that gap. The full ceremony is drivable: open the room, start it, walk it forwards and backwards, record what each person said with their attendance, raise a blocker against a turn and clear it later, read the open blockers a team is carrying, and see who is overloaded before the meeting. The rotation that decides the speaking order and the scrum master seat can be read and set. Sprints can be opened, moved, and completed with unfinished work rolling into the next one. Milestones can be listed and created against a project. Every tool goes through the same service layer the web app uses, so the team and workspace rules that apply in the UI apply here too, including the rule that everyone in a standup has to be on the team. Verified against a live client over the issued OAuth token: 39 tools listed, a room opened and started through the protocol.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 13 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (19)
📝 WalkthroughWalkthroughThe pull request adds OAuth step-up handling, issue search, Scrum MCP tools, sprint lifecycle pages and APIs, document access and transfer workflows, realtime synchronization, query reconciliation, and Playwright coverage. ChangesOAuth consent and session verification
Issue search and reconciliation
Sprint lifecycle
Document access and transfer
Scrum MCP and synchronization
End-to-end and test support
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…est timeout where bun reads it A sprint date arrived as a bare string, so a client that produced "next Monday" got an error raised deep inside the cycle service rather than a clear rejection naming the field. Both sprint tools now check the value parses as a date and say so at the boundary, matching the precedent heldOn already set. The consent screen also raised its blocked notice on any failure, including a failed deny, which left two buttons that did the same thing. It now only appears when approving is what failed, and clears when a new attempt starts. Separately, packages/core carried timeout = 20000 under [test] in bunfig.toml, which bun does not read: the suite kept timing out at the 5000ms default and an earlier run of three clean passes was luck rather than proof. The timeout moves to the CLI flag bun actually honours, and the other database backed packages get it too, since they share the same contention. Three consecutive full runs of the core suite now pass.
…make search find issues Dragging a card to another column removed it from the board. The move reached the server and the badges moved, but the card itself rendered nowhere: the destination column keeps its own cached list, and the reconciler was told never to insert a row it had not already seen. That guard was added to stop a newly created issue appearing in all seven columns at once, but belongsInList already prevents that on its own by matching the state the list is scoped to, so the flag only ever suppressed the legitimate case. A list now admits a row whose scope it can verify. When a list carries an encoded filter group the client cannot evaluate, it asks the server to settle instead of guessing, which is the same rule the realtime path already follows. The board drag and drop test that covered this was disabled as flaky under synthetic mouse events. Driving the pointer through a dozen intermediate moves turns out to be perfectly stable, so the behaviour is covered again for real: the card lands in the target column, leaves the old one, and is still there after a reload. The test fails against the previous reconciler. Search was a separate gap. The palette bound to the sidebar search box only ever matched command names, so typing an issue title returned "No matching commands" and never called the server, even though the issue list has supported a text query all along. It now searches issues as you type, debounced, and opens the one you pick, while still matching commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
packages/mcp-server/src/tools/scrum.ts (1)
464-464: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConstrain
targetDateto the documented format.The description states
YYYY-MM-DD, but the schema accepts any non-empty string. An invalid value fails insidecreateMilestoneinstead of at the tool boundary, so the caller receives a less specific error. TheheldOnschema at lines 39-42 already defines this pattern.Reuse that pattern here.
♻️ Proposed change
- targetDate: z.string().optional().describe('YYYY-MM-DD.'), + targetDate: z + .string() + .regex(/^\d{4}-\d{2}-\d{2}$/) + .optional() + .describe('The milestone target day, as YYYY-MM-DD.'),🤖 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 `@packages/mcp-server/src/tools/scrum.ts` at line 464, Update the targetDate schema in the relevant Scrum tool definition to reuse the existing heldOn YYYY-MM-DD validation pattern, while preserving its optional behavior and description.packages/mcp-server/src/tools.test.ts (1)
344-357: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for standup idempotency.
The
open_standupdescription promises that opening the same day twice returns the same room.openStandupimplements this with an advisory lock and an existing-row short circuit (packages/core/src/work/standup-service.tslines 192-215). No test covers it, so a regression that creates a second room for the same day would pass.Call
open_standuptwice for oneheldOnvalue and assert that both calls return the sameid.As per coding guidelines: "A feature is not complete until it has tests that would fail if the feature broke."
🤖 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 `@packages/mcp-server/src/tools.test.ts` around lines 344 - 357, Add an idempotency assertion to the existing standup test by calling admin.result('open_standup') twice with the same team and heldOn value, then assert both returned standup records have the same id. Keep the existing get_standup and missing-day assertions unchanged.Source: Coding guidelines
🤖 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/e2e/board-drag.spec.ts`:
- Around line 44-75: Update the test case around the card selection and drag
flow to avoid mutating shared board data: create a uniquely identifiable Todo
issue for this test, use it as moving, and delete it in a finally block. Ensure
cleanup runs on assertion or drag failure as well as success, while preserving
the existing reload persistence assertions.
In `@apps/web/src/app/`(auth)/oauth/authorize/consent-form.tsx:
- Line 57: Add a colocated test for the consent form’s blocked approval
transition, covering the approval-error path around the blocked state and
related UI branches. Assert that the blocked panel appears, the passkey notice
is hidden, and selecting cancel follows the deny flow and returns the user
appropriately. Use the existing consent-form test patterns and visible component
symbols rather than changing production behavior.
In `@apps/web/src/lib/query/use-issues.ts`:
- Around line 248-253: Update settleFilteredLists and its caller reconciliation
flow to capture which filtered queries contained each moved issue before
removal, then invalidate a query when either that recorded old membership or
belongsInList(search, issue) indicates membership. Preserve the existing
admitsNewRows behavior and avoid relying solely on the post-reconciliation issue
state.
In `@apps/web/src/test/render.tsx`:
- Around line 7-11: Move the QueryClient creation out of the Providers component
and into the shared render-helper scope, then have Providers reuse that stable
client. Preserve the existing defaultOptions while ensuring rerenders retain the
same QueryClientProvider instance and query cache.
In `@packages/mcp-server/src/tools/scrum.ts`:
- Around line 108-123: The participants array in open_standup and the members
array in set_standup_rotation accept unbounded person references, causing one
concurrent resolveUserId lookup per entry. Add the same appropriate .max()
length bound to both schemas at packages/mcp-server/src/tools/scrum.ts lines
108-123 and 338-346, preserving the existing optional array behavior.
- Around line 348-357: Update the async rotation handler around setRotation to
build and publish the corresponding SyncAction after applying the rotation
changes, then return the rotation result as before. Reuse the existing action
build/publish utilities and ensure publishing occurs before the handler returns
so realtime listeners receive the update.
---
Nitpick comments:
In `@packages/mcp-server/src/tools.test.ts`:
- Around line 344-357: Add an idempotency assertion to the existing standup test
by calling admin.result('open_standup') twice with the same team and heldOn
value, then assert both returned standup records have the same id. Keep the
existing get_standup and missing-day assertions unchanged.
In `@packages/mcp-server/src/tools/scrum.ts`:
- Line 464: Update the targetDate schema in the relevant Scrum tool definition
to reuse the existing heldOn YYYY-MM-DD validation pattern, while preserving its
optional behavior and description.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ef4b6fa-9e02-4886-9f73-6506672dde9a
📒 Files selected for processing (23)
apps/web/e2e/board-drag.spec.tsapps/web/src/app/(auth)/oauth/authorize/consent-form.tsxapps/web/src/app/(auth)/oauth/authorize/decision/route.tsapps/web/src/app/(auth)/oauth/authorize/step-up.test.tsapps/web/src/app/(auth)/oauth/authorize/step-up.tsapps/web/src/app/.well-known/oauth-authorization-server/route.tsapps/web/src/components/command-palette.test.tsxapps/web/src/components/command-palette.tsxapps/web/src/components/keyboard-hints.test.tsxapps/web/src/components/layout/app-shell.test.tsxapps/web/src/lib/query/keys.tsapps/web/src/lib/query/use-issue-search.tsapps/web/src/lib/query/use-issues.tsapps/web/src/test/render.tsxpackages/core/bunfig.tomlpackages/core/package.jsonpackages/db/package.jsonpackages/mcp-server/package.jsonpackages/mcp-server/src/tools.test.tspackages/mcp-server/src/tools/index.tspackages/mcp-server/src/tools/scrum.tspackages/realtime-server/package.jsonpackages/services/package.json
💤 Files with no reviewable changes (1)
- packages/core/bunfig.toml
The team says sprint, the product said cycle. Every label, heading, menu entry, empty state and MCP tool description now reads sprint, and the two routes move to /sprints and /team/<key>/sprint/active with permanent redirects from the old paths so existing links and bookmarks still land. The database column, the API field and the internal identifiers stay cycle. A schema rename would have meant a migration, a breaking API change and a rewrite of every query for no user visible gain, so the change stops at the surface a person actually reads. Cycle time keeps its name in the analytics panel, because that is the name of the metric rather than a reference to the sprint. Also adds the test timeout flag to apps/realtime, which was missed when the other database backed packages got it, and which was still failing at the 5000ms default under contention. Three consecutive runs pass.
…ne home Moving the routes left the breadcrumb table matching the old paths, so /sprints and the team sprint page were not recognised as navigable and rendered without a trail. The active sprint page also kept its old metadata title and heading. Six places separately reconstructed a sprint's display name when the stored name was blank, each writing "Cycle N". They now share sprintLabel in @orbit/shared, and new sprints are created named "Sprint N" rather than "Cycle N", so the name a person sees no longer depends on which screen they are looking at. Cycle time keeps its name in the analytics panel: it is the started to completed duration, the sibling of lead time, not a reference to the sprint.
Completing a sprint moves every unfinished issue into the next one, so a closed sprint's live issue set contains only the work that landed. Every history query built on those rows reports a shrunken scope and close to a hundred percent completion, which is the opposite of useful. The outcome is now written into the progress_snapshot column the schema has always carried and never used: scope, completed, canceled, rolled over, and points, captured before the rollover runs. A regression test asserts the recorded scope is larger than what the live query reports afterwards, which is the whole point. Sprints were also unreachable from the product. createCycle, updateCycle, deleteCycle and completeCycle had no callers outside the MCP tools, so a person using the web app could not open a sprint or close one, and therefore never produced any history at all. They now have routes under /api/cycles. On top of that: pastCycles and getCycleByNumber, a history list on the sprints page showing what each closed sprint shipped, and a per sprint page at /team/<key>/sprint/<number> so a finished sprint can be opened rather than only the running one. Sprints closed before this change say so rather than showing numbers that would be wrong.
The permission model was already complete in the service layer and completely unreachable from the product: no route called setDocAccess, no screen listed who a doc was shared with, and the share menu offered only workspace, link and public, so a private or team doc could not be created at all. Every part of the feature existed except the part a person could use. The share menu now offers all five visibilities, and a restricted doc opens a dialog that searches the workspace by name, grants read or write to a person or a whole team, and revokes again. Granting reaches the grantee live, because the action is published on their own scope as well as the doc's. The service itself needed hardening before it could be exposed. It trusted the grant array rather than parsing it, never checked that a named subject was even in the workspace, silently kept duplicate rows for the same person, and published nothing, so a grant was invisible to every other client including the sharer's other tabs. It also returned "that doc does not exist" to somebody who could plainly see the doc, which now reads as the refusal it is. Restricting a doc no longer demands publish rights. Tightening access from workspace to private was going through the same permission gate as publishing to the open web, which had it backwards: only sharing outward needs doc:publish. A person picked in the dialog was also being sent to the server double encoded, because apiFetch already serialises the body it is given.
… stop tests sharing state A filtered list was only invalidated when the moved issue belonged to it after the move. An issue leaving a filtered list was removed by the reconciler before that check ran, so the list stayed a row short with stale pagination and nothing ever refetched it. Membership is now recorded before reconciliation, and a list is settled when either its old or its new membership changed. setRotation changed the speaking order and the scrum master seat without publishing anything, so every other client kept the previous rotation until a reload. It now returns an action on the team scope, standup_rotation joins the sync models, and the catch up loader covers it like every other model. The shared test render helper built a new QueryClient on each render, which meant a component could lose the cache it had just written to. The client is now created once per render call. The board drag test moved whichever issue happened to be first in Todo and left it there, so repeated runs slowly emptied the column and parallel runs fought over the same row. It creates its own issue now. It also asserts persistence through the API rather than relying on a reload repainting within a fixed window, which is what made it fail on slower CI while passing locally. Both MCP tools that accept a list of people now bound it, and the consent screen has the regression test its blocked state was missing: the panel appears only when approving fails, the passkey notice gives way to it, and the way out returns the client a denial rather than nothing.
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (2)
apps/web/src/features/cycles/sprint-history.tsx (1)
62-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the shared interaction helper.
Replace the direct
transition-colorsandhover:bg-surface-2classes withcardHover. This keeps the sprint card aligned with the shared hover, focus, duration, and reduced-motion policy.As per coding guidelines, use shared interaction tokens for color transitions. Based on learnings, use
cardHoverfor selectable card border and background behavior.🤖 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/features/cycles/sprint-history.tsx` around lines 62 - 66, Update the Link in the sprint history card to use the shared cardHover interaction helper instead of the direct transition-colors and hover:bg-surface-2 classes, while preserving the remaining layout and styling classes.Sources: Coding guidelines, Learnings
apps/web/src/features/docs/doc-people-access.tsx (1)
108-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the shared hover interaction helpers.
Replace the custom
transition-colorsandhover:bg-surface-2classes with the applicable helper from@/lib/interaction.ts.Based on learnings, hover and focus color transitions must use the shared interaction helpers and motion tokens.
Also applies to: 151-154
🤖 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/features/docs/doc-people-access.tsx` around lines 108 - 112, Update the buttons in the doc access entry actions around the button element and the corresponding block at the second referenced location to replace the custom transition-colors and hover:bg-surface-2 classes with the applicable shared hover interaction helper from `@/lib/interaction.ts`, preserving the existing layout and motion behavior.Sources: Coding guidelines, Learnings
🤖 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/e2e/board-drag.spec.ts`:
- Around line 55-69: Update the API calls in the test setup around the bootstrap
lookup and issue creation to validate HTTP success before parsing, then parse
each JSON payload with Zod schemas instead of TypeScript assertions. Ensure the
schemas validate the optional teams/team fields and created issue identifier,
and fail immediately at the API boundary when the ENG team, response status, or
persisted issue data is invalid; apply the same response-validation change to
the code referenced around lines 83–88.
In `@apps/web/e2e/sprints.spec.ts`:
- Around line 22-45: Add response schemas for the /api/bootstrap and /api/cycles
POST envelopes, then parse each response body with its schema before accessing
teams, id, or number in the sprint test. Replace the current type assertions in
the evaluate callbacks while preserving the existing team lookup and
created-cycle behavior.
In `@apps/web/src/app/`(app)/team/[key]/cycle/active/page.tsx:
- Around line 8-9: Validate params.key against teamCreateSchema.key in the
active cycle page before constructing the redirect, and reject invalid keys
rather than redirecting them. Only pass the validated key to permanentRedirect,
preserving the existing legacy route behavior for valid team keys.
In `@apps/web/src/app/`(app)/team/[key]/sprint/[number]/page.tsx:
- Line 52: Update SprintPage’s CyclePanel usage so an unloaded upcoming-sprint
schedule is not represented by an empty array. Either load and pass the
applicable upcoming list, or add an explicit unavailable state that CyclePanel
uses to hide the “Nothing scheduled after this one” section until data is
available.
- Around line 12-25: Validate both dynamic route parameters with the shared Zod
schemas before any metadata generation or data lookup: in
apps/web/src/app/(app)/team/[key]/sprint/[number]/page.tsx lines 12-25, replace
sprintNumber’s Number-based parsing with the appropriate team-key and digit-only
positive sprint-number schemas, handling invalid values with the existing
notFound behavior; in apps/web/src/app/(app)/team/[key]/sprint/active/page.tsx
lines 12-22, parse the team key schema before metadata generation and team
lookup.
In `@apps/web/src/app/`(auth)/oauth/authorize/consent-form.test.tsx:
- Line 13: Replace the prohibited AI-tooling client name and host references in
the consent-form test fixture, including the clientName and corresponding values
in the nearby fixture entries, with neutral OAuth client names and URLs.
Preserve the fixture structure and test behavior.
- Around line 80-82: Update the denial-request test around the deny button
interaction to first wait for the failed denial request to enter its pending
state, then await the asynchronous rejection/handler completion before asserting
that consent-blocked is absent. Keep the existing assign-not-called assertion
while ensuring the UI assertion cannot run before the failure path finishes.
In `@apps/web/src/app/api/cycles/route.ts`:
- Around line 7-12: Add a shared Zod schema for the GET /api/cycles query
parameters and parse the result of searchParamsOf(request) before accessing
teamId, status, or limit. Use the parsed values in the route’s pastCycles call,
enforcing the intended limit type and range rather than relying on
Number.isFinite or downstream clamping; preserve the existing default behavior
for omitted parameters and empty cycles responses.
In `@apps/web/src/features/cycles/data.ts`:
- Around line 62-68: Update readOutcome to validate the entire external value
with sprintOutcomeSchema.safeParse(), including canceled, rolledOver, points,
and closedAt, and return the parsed SprintOutcome only on success; return null
for any incomplete or invalid snapshot so SprintHistory uses its
unavailable-outcome path. Add a regression test covering a partial sprint
snapshot.
In `@apps/web/src/features/docs/doc-people-access.tsx`:
- Around line 29-40: Update the access controls around useDocAccess and the
grants useMemo so pending or failed queries render an appropriate loading/error
state instead of treating missing data as an empty grant set. Keep controls
disabled or unavailable until access.data contains the current grants, and
ensure the commit path cannot call commit before that data is available.
In `@apps/web/src/features/docs/doc-share-menu.tsx`:
- Around line 65-74: Update DocShareMenuProps and the DocShareMenu component to
accept a separate canPublish permission, then use it to hide or disable the
external visibility options (`link` and `public`) while preserving
canManageAccess for access-management controls. Ensure writers without
doc:publish cannot select those options.
In `@apps/web/src/features/issues/issue-properties.tsx`:
- Around line 226-242: Complete the sprint terminology migration: in
apps/web/src/features/issues/issue-properties.tsx lines 226-242, update the
PropertyRow label to “Sprint”; in
apps/web/src/features/filters/filter-fields.tsx lines 237-247 and
apps/web/src/features/filters/grouping.ts lines 105-114, change “No cycle”
fallbacks to “No sprint”; in apps/web/src/features/analytics/cycle-panel.tsx
lines 192-209, update the selector label, zero state, and velocity entity copy
to “Sprint” while preserving “Cycle time” as the metric name.
In `@apps/web/src/lib/query/use-doc-access.ts`:
- Around line 33-44: Update useSetDocAccess to apply an optimistic grant
snapshot before the mutation, preserve the previous docAccess cache for rollback
on failure, and invalidate or reconcile the query after completion. Also
serialize mutations per docId, or disable overlapping submissions while one is
pending, so an older snapshot cannot overwrite a newer grant set.
In `@packages/core/src/realtime/backfill.ts`:
- Around line 655-673: The standup_rotation backfill currently emits member-row
updates instead of the team-level snapshot produced by setRotation, losing empty
and replacement rotations. Update the standup_rotation loader and persistence
path to retain a team-level rotation revision/tombstone with modelId equal to
the teamId, return the full rotation payload, and apply limit by team syncId
rather than member rows; add coverage for empty and replacement rotation
updates.
In `@packages/core/src/work/cycle-service.test.ts`:
- Around line 343-347: Extend the test around the persisted outcome assertion
for the cycle progress snapshot to create estimated completed and canceled
issues, pass a fixed completion time through the cycle completion flow, and
assert every SprintOutcome field: scope, completed, canceled, points,
rolledOver, and closedAt. Keep the existing assertions and verify the stored
values match the created issues and fixed timestamp.
In `@packages/shared/src/utils/index.ts`:
- Around line 147-149: Update sprintLabel to return the normalized, trimmed
sprint.name when it is non-empty, while retaining the existing Sprint number
fallback for blank names. Add coverage for names with surrounding whitespace to
verify the returned label is trimmed.
---
Nitpick comments:
In `@apps/web/src/features/cycles/sprint-history.tsx`:
- Around line 62-66: Update the Link in the sprint history card to use the
shared cardHover interaction helper instead of the direct transition-colors and
hover:bg-surface-2 classes, while preserving the remaining layout and styling
classes.
In `@apps/web/src/features/docs/doc-people-access.tsx`:
- Around line 108-112: Update the buttons in the doc access entry actions around
the button element and the corresponding block at the second referenced location
to replace the custom transition-colors and hover:bg-surface-2 classes with the
applicable shared hover interaction helper from `@/lib/interaction.ts`, preserving
the existing layout and motion behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 847ff6e8-0769-44c4-adcf-7b006cec7991
📒 Files selected for processing (59)
apps/realtime/package.jsonapps/web/e2e/board-drag.spec.tsapps/web/e2e/doc-sharing.spec.tsapps/web/e2e/sprints.spec.tsapps/web/src/app/(app)/cycles/page.tsxapps/web/src/app/(app)/sprints/loading.tsxapps/web/src/app/(app)/sprints/page.tsxapps/web/src/app/(app)/team/[key]/cycle/active/page.tsxapps/web/src/app/(app)/team/[key]/sprint/[number]/page.tsxapps/web/src/app/(app)/team/[key]/sprint/active/loading.tsxapps/web/src/app/(app)/team/[key]/sprint/active/page.tsxapps/web/src/app/(auth)/oauth/authorize/consent-form.test.tsxapps/web/src/app/(auth)/oauth/authorize/consent-form.tsxapps/web/src/app/api/cycles/[id]/complete/route.tsapps/web/src/app/api/cycles/[id]/route.tsapps/web/src/app/api/cycles/route.tsapps/web/src/app/api/docs/[id]/access/route.tsapps/web/src/components/command-palette.test.tsxapps/web/src/features/analytics/burndown-chart.tsxapps/web/src/features/analytics/cycle-panel.tsxapps/web/src/features/cycles/cycle-board.tsxapps/web/src/features/cycles/data.tsapps/web/src/features/cycles/sprint-history.tsxapps/web/src/features/docs/doc-people-access.tsxapps/web/src/features/docs/doc-share-menu.tsxapps/web/src/features/docs/doc-surface.tsxapps/web/src/features/filters/filter-fields.tsxapps/web/src/features/filters/grouping.tsapps/web/src/features/issues/issue-card.tsxapps/web/src/features/issues/issue-properties.tsxapps/web/src/features/issues/issue-row.tsxapps/web/src/lib/breadcrumbs.tsapps/web/src/lib/navigation.tsapps/web/src/lib/query/fetcher.tsapps/web/src/lib/query/keys.tsapps/web/src/lib/query/use-doc-access.tsapps/web/src/lib/query/use-issues.tsapps/web/src/test/render.tsxpackages/core/src/analytics/burndown.tspackages/core/src/analytics/distribution.tspackages/core/src/analytics/snapshot.tspackages/core/src/content/doc-comment-service.test.tspackages/core/src/content/doc-service.test.tspackages/core/src/content/doc-service.tspackages/core/src/org/team-service.tspackages/core/src/realtime/backfill.test.tspackages/core/src/realtime/backfill.tspackages/core/src/work/cycle-service.test.tspackages/core/src/work/cycle-service.tspackages/core/src/work/standup-service.test.tspackages/core/src/work/standup-service.tspackages/mcp-server/src/tools/issues.tspackages/mcp-server/src/tools/planning.tspackages/mcp-server/src/tools/scrum.tspackages/shared/src/constants/onboarding.tspackages/shared/src/events/sync.tspackages/shared/src/filters/index.tspackages/shared/src/utils/index.tspackages/shared/src/validators/doc.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/web/src/components/command-palette.test.tsx
- apps/web/src/lib/query/use-issues.ts
- packages/mcp-server/src/tools/scrum.ts
- apps/web/src/app/(auth)/oauth/authorize/consent-form.tsx
…nish the wording Two ways the share dialog could destroy access. While the grant list was still loading, or after it had failed, the component treated the unknown set as an empty one, so adding a single person replaced everybody who already had access. It now waits for the list, and says so when it cannot load it. Separately, two quick edits each sent the whole set, so a slow first request could land after a fast second and restore what the second had just removed. Mutations are serialized per doc and settle against the server. A sprint outcome from before this change, or any partial snapshot, was accepted on the strength of two fields and then dereferenced for a third, which threw instead of falling back to the unavailable state the UI already has. It parses with a schema now, and the test covers every persisted field rather than three of them, including a canceled issue and the point totals. The rotation catch up loader emitted one entry per member row while the producer emits one snapshot per team, so a reconnecting client saw a different shape than a connected one, and a page limit could truncate a rotation mid team. The loader now rebuilds the same team snapshot. Writers without publish rights were offered the link and public options and got a server rejection for taking them. Those options are hidden unless the person can actually publish. Route parameters are validated: a sprint number must be digits, the legacy cycle redirect refuses a key that is not shaped like a team key, and the sprint list query is parsed rather than coerced by hand. The numbered sprint page also loads the upcoming schedule instead of passing an empty list, which had it claiming nothing was scheduled. Also finishes the wording sweep, which still said cycle in the issue sidebar, both filter menus, the analytics panel and the marketing copy, and trims the sprint name so a padded one does not keep its whitespace in every label. The consent test fixture no longer names a real vendor, and its denial case waits for the request to settle rather than asserting before it runs.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
apps/web/src/app/(app)/team/[key]/cycle/active/page.tsx (1)
3-12: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse the shared Zod team-key schema.
Line 11 validates request input with a local regular expression. Parse
params.keywith the shared team-key schema and redirect with the parsed value. This keeps route validation aligned with team creation.As per coding guidelines, “validate every external input with Zod.”
#!/bin/bash set -euo pipefail ast-grep outline packages/shared/src/validators/team.ts --items all rg -n -C 3 'teamCreateSchema|teamKey.*Schema|key:.*z\.' packages/shared/src/validators apps/web/src/app🤖 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/app/`(app)/team/[key]/cycle/active/page.tsx around lines 3 - 12, Replace the local TEAM_KEY regex validation in ActiveCycleRedirect with the shared team-key Zod schema from the validators module. Parse params.key using that schema, use the parsed value in permanentRedirect, and preserve notFound behavior for invalid keys.Source: Coding guidelines
🤖 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/features/landing/landing-page.tsx`:
- Line 593: Update the visible palette result labels near the “Move to sprint”
span so “Move to Cycle 14” and “Move to Cycle 15” use “sprint” instead, keeping
the mock terminology consistent for the same action.
In `@apps/web/src/lib/query/zz-audit3.test.tsx`:
- Around line 119-140: The tests currently only log state instead of verifying
behavior. In apps/web/src/lib/query/zz-audit3.test.tsx lines 119-140, replace
timed waits and console.log calls with waitFor assertions covering source and
destination column contents during and after the move. In
packages/core/src/content/scratch-escalation.test.ts lines 35-46, add assertions
for public visibility, anonymous document retrieval, expected content, and
inclusion in listPublicDocs().
In `@packages/core/src/realtime/backfill.ts`:
- Around line 656-665: The distinct team query in catchUp must deterministically
prioritize teams by their earliest newer standup rotation syncId before applying
the page limit. Update the query around the standupRotation selection to group
each team’s matching rows, order by the minimum syncId ascending, then limit the
results; add a catchup test covering older and newer changed teams to verify no
team is skipped.
In `@packages/shared/src/validators/cycle.ts`:
- Around line 23-33: Update sprintOutcomeSchema so closedAt uses the existing
instantSchema instead of a nonempty string validator, matching startsAt and
endsAt. Adjust CycleService.outcomeOf if necessary so its returned closedAt type
is Date and remains consistent with the schema.
---
Duplicate comments:
In `@apps/web/src/app/`(app)/team/[key]/cycle/active/page.tsx:
- Around line 3-12: Replace the local TEAM_KEY regex validation in
ActiveCycleRedirect with the shared team-key Zod schema from the validators
module. Parse params.key using that schema, use the parsed value in
permanentRedirect, and preserve notFound behavior for invalid keys.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 515b96a9-140f-4db4-be60-ada1616299fc
📒 Files selected for processing (27)
apps/web/e2e/api.tsapps/web/e2e/board-drag.spec.tsapps/web/e2e/doc-sharing.spec.tsapps/web/e2e/sprints.spec.tsapps/web/src/app/(app)/team/[key]/cycle/active/page.tsxapps/web/src/app/(app)/team/[key]/sprint/[number]/page.tsxapps/web/src/app/(auth)/oauth/authorize/consent-form.test.tsxapps/web/src/app/api/cycles/route.tsapps/web/src/app/layout.tsxapps/web/src/app/llms.txt/route.tsapps/web/src/features/analytics/cycle-panel.tsxapps/web/src/features/cycles/data.tsapps/web/src/features/docs/doc-people-access.tsxapps/web/src/features/docs/doc-share-menu.tsxapps/web/src/features/docs/doc-surface.tsxapps/web/src/features/filters/filter-fields.tsxapps/web/src/features/filters/grouping.tsapps/web/src/features/issues/issue-properties.tsxapps/web/src/features/landing/landing-meta.tsapps/web/src/features/landing/landing-page.tsxapps/web/src/lib/query/use-doc-access.tsapps/web/src/lib/query/zz-audit3.test.tsxpackages/core/src/content/scratch-escalation.test.tspackages/core/src/realtime/backfill.tspackages/core/src/work/cycle-service.test.tspackages/shared/src/utils/index.tspackages/shared/src/validators/cycle.ts
🚧 Files skipped from review as they are similar to previous changes (17)
- apps/web/src/features/docs/doc-surface.tsx
- apps/web/src/features/filters/filter-fields.tsx
- apps/web/src/features/issues/issue-properties.tsx
- packages/shared/src/utils/index.ts
- apps/web/src/features/filters/grouping.ts
- apps/web/e2e/sprints.spec.ts
- apps/web/e2e/board-drag.spec.ts
- apps/web/src/app/api/cycles/route.ts
- apps/web/src/app/(app)/team/[key]/sprint/[number]/page.tsx
- apps/web/src/features/cycles/data.ts
- apps/web/src/lib/query/use-doc-access.ts
- apps/web/e2e/doc-sharing.spec.ts
- apps/web/src/features/docs/doc-share-menu.tsx
- apps/web/src/app/(auth)/oauth/authorize/consent-form.test.tsx
- apps/web/src/features/docs/doc-people-access.tsx
- apps/web/src/features/analytics/cycle-panel.tsx
- packages/core/src/work/cycle-service.test.ts
…ps another Closing a sprint looked for the next one by number, but numbers are handed out in creation order while windows sit on a calendar, so the two disagree the moment somebody backfills an earlier sprint. Completing that earlier sprint then minted a successor starting where it ended, straight through the middle of a sprint that already existed. createCycle refuses exactly that window, so completion was creating state the product will not let you create by hand, and activeCycle resolves the running sprint by date, so which one is active became arbitrary. The successor is now the earliest sprint that starts at or after the completed one ends, and only when there is none does it mint a new one, starting after the last sprint the team has rather than after the one just closed. Its number sits above every existing number instead of one above the closed sprint, which was the other way duplicates could appear. Found by an adversarial review agent, whose reproduction is now a test asserting that no two of a team's sprint windows clash after a completion. It fails against the by-number lookup. Also adds Markdown export and import and PDF through print. Export rewrites attachment links to absolute urls so they still resolve outside the app, and adds the title as a heading unless the body already opens with it. Import reads a front matter title, falls back to the first heading and lifts it out of the body, then to the file name, and refuses a file past the content limit rather than failing at the server. A print stylesheet drops the shell so the page carries the document rather than the application around it.
…st the database A write grantee could publish somebody else's private doc to the open web. Making restriction not require publish rights had left widening gated only on being able to write, and a write grant is exactly what you hand a collaborator. Widening the audience of a restricted doc is now the author's or an admin's call, on both the share and the update path, while narrowing it stays open to any writer because narrowing takes nothing away from anyone. The passkey step up could be skipped. The hook stamping last used ran after every call to the verification endpoint, whether or not the assertion verified, so a deliberately failed attempt was enough to satisfy the freshness check for the next two minutes. It only stamps on a verified assertion now. Completing a sprint was not concurrency safe. Two completions read completed_at as null at read committed, both passed the guard, and the loser overwrote the winner's recorded outcome with a snapshot claiming nothing rolled over, which is the one thing sprint history exists to record. Completion now takes the same advisory lock the create path already used, re-reads under it, and the closing update refuses a sprint that is already closed. The successor also has to be a sprint that is still open, so unfinished work can no longer be filed into a sprint that was closed weeks ago. The MCP grant was written before the consent code was validated, so a failed authorization still left a client connected to a workspace. The grant is written after, and from the client and scopes recorded on the consent itself rather than from the request body, which the browser could otherwise choose freely. A failing mutation rolled the whole issue cache back to a snapshot taken before it started, undoing any sibling mutation that had landed in between. Each rollback now restores only the row its own mutation touched. A newly created issue was also only placed into team-prefixed lists, so it never appeared in My Issues even though the header count moved. Also: the standup rotation route published nothing and double wrapped its response, and pressing Previous on the first speaker marked them done and left the room with nobody speaking.
…tool The access token carries the scopes the user consented to, and nothing looked at them. Consent asked for orbit.read and orbit.write separately, the screen listed them as different permissions, and then every tool was registered regardless, so a client granted read only could create issues, move them, open and close sprints, change the rotation and invite members. The permission screen was describing a boundary that did not exist. Tools now register only when the token allows them: defineTool already knows whether a tool is read only, so a token without orbit.write simply never sees the write half of the server. A read only client lists 23 read tools rather than 39, every one of them annotated read only, and a write call comes back as an unknown tool rather than being carried out. Also orders the standup rotation catch up by the earliest new sync id before taking a page, since selectDistinct with a limit picks rows arbitrarily and could skip a team whose rotation changed earlier, and validates the recorded sprint close time as an instant rather than any non empty string.
… scope on revocation Display options did nothing on a board whose columns fetch their own rows. Hiding sub issues or narrowing completed work filtered the flat first page, which is the only list those options ever reached, while each column rendered the server rows straight through. Turning an option on visibly changed nothing. The column now applies the same filter, and its header shows the rendered count rather than the server total whenever a filter is trimming rows and there is no further page, so the number above a column matches what is under it. Revoking a doc grant left the reader's live subscription in place. The socket authorized a doc scope once, at subscribe time, and never looked again, so somebody removed from a private doc kept receiving its updates and its comment bodies until they reloaded. A doc or doc comment action now revalidates that scope on every connection holding it and drops it when the reader may no longer read the doc, failing closed if the check itself fails.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
packages/core/src/realtime/backfill.ts (1)
656-671:⚠️ Potential issue | 🟠 MajorPreserve catch-up for an empty rotation.
When
setRotationremoves all rows for a team,touchedhas no row to select. The loader then returns nostandup_rotationaction. A client that missed the live Redis action keeps the old rotation after reconnect.Persist a team-level rotation revision or tombstone, or read a durable team-scoped sync record during catch-up. Add a regression test in
packages/core/src/realtime/backfill.test.ts.This is the same unresolved gap noted in the previous review. Based on learnings, an empty rotation has no
standupRotationrow that can carry a newsyncId; catch-up cannot represent that transition. As per coding guidelines, the feature needs a regression test for this path.🤖 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 `@packages/core/src/realtime/backfill.ts` around lines 656 - 671, Update the catch-up flow around the touched query in the backfill loader so teams whose rotation rows were all removed still produce a standup_rotation action by reading or persisting a durable team-scoped revision/tombstone, rather than relying only on standupRotation rows. Preserve existing non-empty rotation behavior, and add a regression test in backfill.test.ts covering reconnect catch-up after setRotation empties a team’s rotation.Sources: Coding guidelines, Learnings
🤖 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/`(auth)/oauth/authorize/decision/route.ts:
- Around line 62-68: Update the approval flow around finalizeMcpConsent and
recordMcpGrant so consent finalization and MCP grant persistence execute within
the same database transaction. Ensure a failure in either operation rolls back
both changes, allowing the consent code to be retried, while preserving the
existing approved client, user, organization, and scope values.
In `@apps/web/src/app/globals.css`:
- Around line 829-832: Replace the hardcoded print colors in the body rule with
semantic CSS custom properties, and define corresponding print background and
foreground variables in the existing theme rules. Preserve the current
light-theme values while providing appropriate values for supported themes.
In `@apps/web/src/features/docs/doc-export-menu.tsx`:
- Around line 44-60: Add colocated Bun behavioral tests for
apps/web/src/features/docs/doc-export-menu.tsx lines 44-60, covering Markdown
download generation and window.print invocation. Add tests for
apps/web/src/features/docs/doc-import.tsx lines 42-73 covering file selection,
create mutation input, successful navigation, busy state, and failed-import
error feedback.
In `@apps/web/src/features/docs/doc-import.tsx`:
- Around line 25-36: Update the take function to validate the selected file
metadata and the parsed title/content with the appropriate xSchema Zod schemas
before calling create.mutateAsync. Keep the existing content-length check, and
ensure mutation receives only schema-validated values.
- Around line 25-29: Update take to validate file.size against a bounded byte
limit before calling file.text(), rejecting oversized files without reading
them; retain the existing text.length check after decoding to enforce the
content limit as well.
In `@packages/core/src/work/cycle-service.test.ts`:
- Around line 392-449: Add a concurrency test alongside the existing completion
tests that invokes completeCycle for the same sprint twice concurrently via
Promise.allSettled. Assert exactly one operation fulfills, the other rejects
with the expected conflict, and querying the team’s cycles confirms only one
successor was created.
- Around line 436-447: Strengthen the test around firstCycle and cycle
completion by storing the preexisting sprint ID, making that sprint ineligible
for successor selection, and recording the IDs returned by listCycles before
completion. After completeCycle, assert closed.nextCycle has a new ID not among
the preexisting cycles, then verify its number exceeds the highest preexisting
sprint number.
---
Duplicate comments:
In `@packages/core/src/realtime/backfill.ts`:
- Around line 656-671: Update the catch-up flow around the touched query in the
backfill loader so teams whose rotation rows were all removed still produce a
standup_rotation action by reading or persisting a durable team-scoped
revision/tombstone, rather than relying only on standupRotation rows. Preserve
existing non-empty rotation behavior, and add a regression test in
backfill.test.ts covering reconnect catch-up after setRotation empties a team’s
rotation.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 16ad15ca-4b34-4cc4-aa29-b05aea50e753
📒 Files selected for processing (24)
apps/web/src/app/(auth)/oauth/authorize/decision/route.tsapps/web/src/app/api/standups/rotation/route.tsapps/web/src/app/globals.cssapps/web/src/features/docs/doc-export-menu.tsxapps/web/src/features/docs/doc-import.tsxapps/web/src/features/docs/doc-surface.tsxapps/web/src/features/docs/doc-transfer.test.tsapps/web/src/features/docs/doc-transfer.tsapps/web/src/features/docs/docs-empty-pane.tsxapps/web/src/features/landing/landing-page.tsxapps/web/src/lib/auth/server.tsapps/web/src/lib/query/use-issues.tspackages/core/src/auth/mcp-token.tspackages/core/src/content/doc-service.test.tspackages/core/src/content/doc-service.tspackages/core/src/realtime/backfill.tspackages/core/src/work/cycle-service.test.tspackages/core/src/work/cycle-service.tspackages/core/src/work/standup-service.tspackages/mcp-server/src/server.tspackages/mcp-server/src/test-helpers.tspackages/mcp-server/src/tools.test.tspackages/mcp-server/src/tools/support.tspackages/shared/src/validators/cycle.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/mcp-server/src/tools.test.ts
- apps/web/src/features/landing/landing-page.tsx
- packages/shared/src/validators/cycle.ts
- packages/core/src/work/standup-service.ts
… its labels Dropping a card within its own column showed nothing until the server answered. The reconciler replaced the row in place and returned the list unsorted, so the new sortOrder it had just written had no effect on the order rendered. It sorts after replacing now, which is what the insert path already did. A cross column drag on a filtered board also repainted the card in the column it had just left. Filtered lists were being settled from onMutate, so the refetch raced the mutation and read pre move state. They settle once the server has answered instead. An issue arriving over the socket lost its labels, because the delta carries the issue row and labels live in a join table, so the card rendered bare until the next refetch. A create now publishes the labels it was created with.
…c's grantees Deleting a sprint left every issue in it pointing at a row that no longer exists. The foreign key nulls the column, but nothing told any client, so open boards kept the issues filed under a sprint that had gone until somebody reloaded. The issues are detached explicitly now and each one is announced. A restricted doc addressed its deltas at the author's own user scope and the doc scope. Nothing subscribed to either from the shell, so a change to a private doc reached nobody in real time, and a person the doc had just been shared with was never told. The shell now subscribes to the reader's own user scope, and a sharing change is addressed at each grantee, by user or by team.
… the transfer controls Consuming the consent code and writing the workspace grant were two separate statements, so a grant that failed to write left the user holding a code that had already been spent and no way to retry. They land in one transaction now. Importing a file read the whole thing into a string before checking the limit, so picking a very large file could take the tab down before any check ran. The size is checked first, and the parsed title and body go through a schema before they reach the create call rather than being trusted because the file picker had an accept attribute. Print colours come from theme tokens instead of two hardcoded hex values. Adds the coverage the transfer controls were missing: exporting downloads a file named after the doc, the PDF action hands off to the browser rather than rendering anything, an import creates the doc and opens it and carries the collection it started from, and a refused or oversized import neither creates a doc nor navigates. Also adds the concurrent completion case, which asserts one sprint completion wins, the other is refused, and exactly one successor exists.
Selection was an index into the filtered list, so a notification arriving above the one being read silently moved the pane onto a different item, and a list that shrank left the index pointing past the end, which took two presses to recover from. Selection is keyed by notification id now, so what is open stays open whatever arrives. The panes also shared the shell's single scroll container, so a long list pushed the whole page down and the reading pane scrolled away with it. Each pane scrolls on its own. The entity a notification points at was read from the database and then dropped by the mapping layer, so nothing downstream could open the thing the notification was about. It is carried through now, from both the initial load and the socket.
|
Too many files changed for review (103 files, 100 file limit). Bypass the limit by tagging |
This was referenced Aug 5, 2026
This was referenced Aug 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Makes MCP usable for planning, fixes the board bugs that made the tracker feel broken, renames cycles to sprints, adds per-document sharing and Markdown export/import, and closes the security holes an adversarial review reproduced against the database.
The board was dropping cards
Dragging to another column removed the card from the board. The move reached the server and the badges moved, but the card rendered nowhere: each column keeps its own cached list, and the reconciler had been told never to insert a row it had not already seen.
belongsInListalready prevents a new issue appearing in every column, so that guard only ever suppressed the legitimate case.The drag-and-drop test covering this was disabled as flaky. It isn't. It now creates its own issue, drags it, and asserts persistence through the API as well as the render.
Also fixed: an in-column reorder showed no movement until the server answered; a cross-column drag on a filtered board repainted the card in the column it had just left; display options did nothing on server-backed columns while the header showed a total that disagreed with the rows; a failing mutation rolled the whole issue cache back, undoing sibling mutations that had already succeeded; a created issue never reached My Issues; and an issue arriving over the socket lost its labels.
Search never searched
⌘K matched command names only. Typing an issue title returned "No matching commands" and never called the server. It searches issues as you type now, debounced.
MCP
Connecting dead-ended: after signing in, the consent screen demanded a passkey on top of the sign-in that had just happened, and when that prompt did not complete the client waited for a redirect that never came. A session that authenticated within five minutes now satisfies the step-up; an older one still needs a passkey; and when consent cannot complete the screen returns the client
access_deniedrather than stranding it.Sixteen new tools, because the server had 23 and none touched the ceremony: the whole standup is drivable, sprints can be opened, moved and completed, milestones listed and created.
Scopes are now enforced. The token carries what the user consented to and nothing looked at it, so a client granted
orbit.readcould create issues, close sprints and invite members. A read-only token now sees 23 read tools instead of 39.Sprints
Renamed everywhere a person looks, with
/sprintsand permanent redirects from the old paths. The database column stayscycle, so no migration.History told a lie. Completing a sprint moves unfinished issues into the next one, so a closed sprint's live rows contain only what landed, and every history query reported near-100% completion. The outcome is written to the
progress_snapshotcolumn the schema always carried and never used, captured before the rollover.Completion also wasn't concurrency safe (two completions both passed the guard, the loser overwrote the winner's outcome), filed unfinished work into already-closed sprints, and minted successors that overlapped existing sprints — a window
createCycleitself refuses. All three are fixed and tested. Sprints were unreachable from the web app at all; they have routes now, plus a history list and a per-sprint page.Docs
Per-document sharing with named people and teams. The permission model was complete in the service layer and unreachable: no route called
setDocAccess, nothing listed who a doc was shared with, and a private doc could not be created.Hardening that came with it: a write grantee could publish someone else's private doc to the open web; the grant list being unknown was treated as empty, so adding one person wiped everybody; two quick edits could restore what the later one removed; revoking a grant left the reader's live subscription in place; and a restricted doc's deltas were addressed at a scope nothing subscribed to.
Plus Markdown export and import, and PDF through a print stylesheet.
Other security fixes
The passkey step-up was bypassable — the hook stamping
lastUsedAtran whether or not the assertion verified, so a deliberately failed attempt satisfied the freshness check. The MCP grant was written before the consent code was validated, and took its client and scopes from the request body rather than the consent record.Verification
bun run verifygreen. 11 e2e specs pass, covering cross-column drag, palette search, sprint completion and history, the old cycle URLs, and sharing a private doc with a named person who then gains access. Regression tests were checked against the pre-fix code where the behaviour was subtle.Not in this PR
Docs folders and tree, anchored comments, the inbox reading pane, the project tabs, and moving tests into a dedicated tree.
Greptile Summary
This PR covers five distinct improvements: fixing the board drag-drop bug that dropped moved cards, wiring issue search into the command palette, repairing the MCP OAuth consent dead-end and adding sixteen ceremony tools, renaming cycles to sprints everywhere the user sees, capturing sprint completion snapshots before rollover so history is accurate, and surfacing the previously-unreachable doc-access model in the UI.
reconcilenow usesadmitsNewRowsto decide whether a moved issue can enter a previously unseen list;settleFilteredListshandles filtered columns by invalidating and refetching, covering the half that left moved cards invisible.signedInWithinallows a session created within the last five minutes to satisfy the passkey step-up, and the consent route now readsclientId/scopefrom the stored consent record instead of trusting the request body, so a client cannot forge a wider grant.atClosecaptures all non-archived issues before the rollover runs, thenoutcomeOfwrites the pre-rollover totals intoprogressSnapshot; the advisory lock oncycle:teamIdprevents a double-complete race.Confidence Score: 5/5
Safe to merge. The auth, board, and sprint completion paths are all correctly implemented and covered by new tests.
The MCP consent flow, board reconciler, sprint snapshot, and doc-access validation are each well-implemented and covered by new tests. The one real-time gap (revoked doc grantees not receiving a cache-invalidation event) is a UX limitation, not a correctness or security issue, because
loadReadableDocenforces access on every server call.Files Needing Attention:
packages/core/src/content/doc-service.ts- thesetDocAccesssync broadcast is worth revisiting to include previously-granted scopes so revoked users see an immediate update.Important Files Changed
setDocAccessnow validates subjects are in the workspace, deduplicates grants, and publishes a sync action. Real-time broadcast scope does not include previously-granted users whose access is revoked, leaving their cache stale until next navigation.pg_advisory_xact_lock+ re-read pattern to prevent double-complete, and finds the next sprint by date rather than number. NewpastCyclesandgetCycleByNumberhelpers added.clientIdandscopefrom the stored consent record rather than the request body, closing a potential grant-widening vector; deny path unaffected.!Number.isFinite(started)andstarted <= now.abandon()path that sends deny to the client with a redirect instead of leaving it stranded;blockedis only set on allow failures, so the Cancel button does not appear alongside Deny on deny errors.reconcilenow usesadmitsNewRowsto admit newly-scoped issues into unfiltered lists;settleFilteredListsinvalidates filtered lists that either held the moved issue or now should contain it.instantrefine validates parsability at the tool boundary for date fields.scopes.user(currentUserId)to the realtime subscription list so user-scoped events reach the client.advanceStandupnow short-circuits when direction ispreviousand already at the first turn.setRotationnow returns actions and the route publishes them.Sequence Diagram
sequenceDiagram participant MCP as MCP Client participant Web as /oauth/authorize participant Decision as /decision route participant DB as Postgres MCP->>Web: "GET /authorize?client_id=..." Web->>MCP: Render consent screen MCP->>Decision: "POST {decision:allow, consentCode, organizationId}" Decision->>DB: read session.createdAt alt session created within 5 min Decision->>DB: finalizeMcpConsent(accept:true) DB-->>Decision: "{redirectUri, clientId, scope}" Decision->>DB: recordMcpGrant(clientId, scope from consent record) Decision->>MCP: "{redirectUri}" else older session + passkey required Decision->>MCP: "{status:passkey_required}" MCP->>Web: trigger passkey prompt MCP->>Decision: retry POST Decision->>DB: finalizeMcpConsent + recordMcpGrant Decision->>MCP: "{redirectUri}" end alt allow fails Web->>Web: setBlocked(message), show Cancel button MCP->>Decision: "POST {decision:deny}" Decision->>DB: finalizeMcpConsent(accept:false) Decision->>MCP: "{redirectUri with access_denied}" endReviews (11): Last reviewed commit: "fix(sprints,docs): announce a deleted sp..." | Re-trigger Greptile
Context used: