AMP-153 Slack: secure and reliable /linear backlog query#98
Open
gabiIsCoding wants to merge 22 commits into
Open
AMP-153 Slack: secure and reliable /linear backlog query#98gabiIsCoding wants to merge 22 commits into
gabiIsCoding wants to merge 22 commits into
Conversation
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…fig for AMP-153 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…n for AMP-153 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…for AMP-153 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… AMP-153 Slack self-service replies were run through markdownToSlackMrkdwn even when the text was already hand-built Slack mrkdwn, so the escaping pass turned <url|AMP-153> into <url|AMP-153> and Slack rendered the raw markup instead of a link. Add an optional textFormat: "mrkdwn" passthrough on SlackSelfServiceReply/reply() and have the Linear backlog handler declare its rendered text as already-mrkdwn, skipping re-escaping without switching the renderer to Markdown-link syntax (which would let Linear-controlled titles inject links via the converter). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… AMP-153 Linear API rejects the query when the same ID variable is used in positions with different type expectations: project(id: String!) and filter comparison (ID). Declare two variables ($projectId: ID!, $projectKey: String!) and pass the same value to both positions to satisfy schema requirements. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… correct for AMP-153
… refresh is honored for AMP-153
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdds read-only Slack ChangesLinear backlog and Slack ingress
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Slack
participant SlackEventsIngress
participant SlackEventProcessor
participant LinearBacklogHandler
participant LinearGraphQL
Slack->>SlackEventsIngress: send event_callback
SlackEventsIngress-->>Slack: return { ok: true }
SlackEventsIngress->>SlackEventProcessor: process asynchronously
SlackEventProcessor->>LinearBacklogHandler: handle /linear
LinearBacklogHandler->>LinearGraphQL: fetch paginated backlog
LinearGraphQL-->>LinearBacklogHandler: return issues
LinearBacklogHandler-->>SlackEventProcessor: return mrkdwn reply
SlackEventProcessor-->>Slack: post source-thread reply
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a read-only /linear Slack command that allows users to list unfinished Linear issues directly in Slack. It adds a dedicated channel allowlist and project router configuration, implements the backlog query logic via Linear's GraphQL API, and introduces a bounded asynchronous executor for Slack Events API callbacks to prevent timeouts. The review feedback highlights two valuable improvements: optimizing the backlog rendering loop by pre-calculating issue counts per state name to avoid O(N * M) complexity, and adding defensive null-safety guards when parsing the GraphQL response to prevent runtime crashes on unexpected API structures.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+81
to
+89
| for (const group of groupShownIssues(shown)) { | ||
| const totalInGroup = backlog.issues.filter((issue) => issue.stateName === group.stateName).length; | ||
| const shownInGroup = group.issues.length; | ||
| const countLabel = shownInGroup < totalInGroup ? `${shownInGroup} of ${totalInGroup}` : String(totalInGroup); | ||
| const emoji = STATE_TYPE_EMOJI[group.stateType] ?? "▫️"; | ||
| lines.push(""); | ||
| lines.push(`${emoji} *${escapeSlackText(group.stateName)} (${countLabel})*`); | ||
| for (const issue of group.issues) lines.push(renderIssueLine(issue)); | ||
| } |
There was a problem hiding this comment.
Performance Optimization
Currently, backlog.issues.filter is called inside a loop for each group shown. If the backlog contains a large number of issues (up to 10,000 based on the 100-page safety limit), this results in
We can optimize this to Map before entering the loop.
const totalByState = new Map<string, number>();
for (const issue of backlog.issues) {
totalByState.set(issue.stateName, (totalByState.get(issue.stateName) ?? 0) + 1);
}
for (const group of groupShownIssues(shown)) {
const totalInGroup = totalByState.get(group.stateName) ?? 0;
const shownInGroup = group.issues.length;
const countLabel = shownInGroup < totalInGroup ? `${shownInGroup} of ${totalInGroup}` : String(totalInGroup);
const emoji = STATE_TYPE_EMOJI[group.stateType] ?? "▫️";
lines.push("");
lines.push(`${emoji} *${escapeSlackText(group.stateName)} (${countLabel})*`);
for (const issue of group.issues) lines.push(renderIssueLine(issue));
}
Comment on lines
+112
to
+116
| if (data.project == null) { | ||
| throw new Error("Linear project not found or inaccessible; check the channel-mapped project ID and the token's access."); | ||
| } | ||
| projectName ??= data.project.name; | ||
| nodes.push(...data.issues.nodes); |
There was a problem hiding this comment.
Defensive Programming / Null Safety
If the GraphQL query succeeds but returns an unexpected structure (e.g., data is null, or data.issues is null/undefined due to partial errors), accessing data.project or data.issues.nodes will throw a TypeError and crash the request.
Adding defensive guards or optional chaining ensures that invalid/unexpected API responses are handled gracefully.
Suggested change
| if (data.project == null) { | |
| throw new Error("Linear project not found or inaccessible; check the channel-mapped project ID and the token's access."); | |
| } | |
| projectName ??= data.project.name; | |
| nodes.push(...data.issues.nodes); | |
| if (data == null || data.project == null) { | |
| throw new Error("Linear project not found or inaccessible; check the channel-mapped project ID and the token's access."); | |
| } | |
| projectName ??= data.project.name; | |
| if (!data.issues?.nodes) { | |
| throw new Error("Linear API returned an invalid response structure (missing issues or nodes)."); | |
| } | |
| nodes.push(...data.issues.nodes); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/cli/src/slack-linear-backlog.ts`:
- Around line 46-53: Update groupShownIssues to key by both issue.stateName and
issue.stateType when retrieving and creating groups, so same-named states with
different types remain separate and retain their correct ordering and emoji.
Apply the same composite-key logic to any related group lookup or count handling
in this function.
- Around line 26-30: Update safeSlackLinkUrl to percent-encode any literal pipe
characters in the URL href before returning it, ensuring URLs inserted into
Slack’s <url|label> format cannot disrupt link parsing while preserving the
existing protocol validation.
In `@packages/slack/src/ingress.ts`:
- Around line 57-87: Update createBoundedAsyncExecutor and the callers around
the delivery acknowledgement paths at lines 280-286 and 401-417 so accepted
tasks are durably persisted before acknowledging delivery. Ensure
dispatcher/Slack failures are retried idempotently and pending work is drained
across shutdown or restart, rather than only logged and discarded; retain the
existing concurrency and maxOutstanding limits.
- Around line 440-441: Update the configuration spread logic near
maxAsyncConcurrency and maxAsyncOutstanding to check for undefined rather than
truthiness, so explicitly provided zero values are passed through to
positiveInteger validation instead of being omitted and replaced by defaults.
In `@packages/slack/test/ingress.test.ts`:
- Around line 45-191: The asynchronous tests in
packages/slack/test/ingress.test.ts (lines 45-191) rely on elapsed-time
thresholds and fixed sleeps; replace them with deferred gates that expose
handler progress and vi.waitFor assertions on completion, while preserving the
immediate acknowledgement checks. In apps/slack-events/test/app.test.ts (lines
9-10), remove the one-tick helper and wait directly with vi.waitFor for each
expected side effect; no timer-based synchronization should remain at either
site.
🪄 Autofix (Beta)
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
Run ID: 706f0420-32f8-462b-8a29-36449aeb6781
📒 Files selected for processing (24)
apps/slack-events/test/app.test.tsdocs/configuration.mddocs/platforms/slack.en.mddocs/platforms/slack.zh-CN.mdpackages/cli/src/config.tspackages/cli/src/doctor.tspackages/cli/src/linear-backlog-config.tspackages/cli/src/slack-linear-backlog.tspackages/cli/src/start.tspackages/cli/test/config.test.tspackages/cli/test/doctor.test.tspackages/cli/test/linear-backlog-config.test.tspackages/cli/test/slack-linear-backlog.test.tspackages/cli/test/start.test.tspackages/linear/src/backlog.tspackages/linear/src/index.tspackages/linear/test/backlog.test.tspackages/slack/src/dispatcher-events.tspackages/slack/src/events.tspackages/slack/src/ingress.tspackages/slack/src/render.tspackages/slack/test/dispatcher-events.test.tspackages/slack/test/events.test.tspackages/slack/test/ingress.test.ts
Comment on lines
+57
to
+87
| function createBoundedAsyncExecutor(input: { | ||
| concurrency: number; | ||
| maxOutstanding: number; | ||
| onError(error: unknown): void; | ||
| }) { | ||
| let active = 0; | ||
| const queue: AsyncTask[] = []; | ||
|
|
||
| function drain(): void { | ||
| while (active < input.concurrency && queue.length > 0) { | ||
| const task = queue.shift()!; | ||
| active += 1; | ||
| void Promise.resolve() | ||
| .then(task) | ||
| .catch(input.onError) | ||
| .finally(() => { | ||
| active -= 1; | ||
| drain(); | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| tryEnqueue(task: AsyncTask): boolean { | ||
| if (active + queue.length >= input.maxOutstanding) return false; | ||
| queue.push(task); | ||
| drain(); | ||
| return true; | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not treat an in-memory enqueue as durable acceptance.
Line 417 acknowledges delivery before processing succeeds, while failures are only logged. Dispatcher/Slack failures or shutdown can therefore permanently lose acknowledged commands. Persist accepted work, or provide an idempotent retry-and-drain mechanism that survives shutdown.
Also applies to: 280-286, 401-417
🤖 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/slack/src/ingress.ts` around lines 57 - 87, Update
createBoundedAsyncExecutor and the callers around the delivery acknowledgement
paths at lines 280-286 and 401-417 so accepted tasks are durably persisted
before acknowledging delivery. Ensure dispatcher/Slack failures are retried
idempotently and pending work is drained across shutdown or restart, rather than
only logged and discarded; retain the existing concurrency and maxOutstanding
limits.
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.
Summary
@OpenTag linear//linearcommand that lists unfinished issues without creating an Agent Run or enabling Linear mutations.(teamId, channelId)allowlist and route each authorized channel to its configured Linear project.event_id.Validation
pnpm vitest run packages/cli/test/start.test.ts packages/linear/test/backlog.test.ts packages/cli/test/slack-linear-backlog.test.ts— 3 files, 87 tests passedpnpm typecheck— passedpnpm lint— passedpnpm vitest run— 124 files, 1654 tests passedgit diff --check— passedNotes
/linearpath remains query-only: no Agent Run, LLM invocation, apply plan, or Linear mutation is created.Summary by CodeRabbit
/linearbacklog lookups./linearbehavior.