mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-25 09:41:04 +01:00
fix(slack): preserve direct consent handoff across task redirects
This commit is contained in:
1 parent
a03d8b394e
commit
beb854edac
5 files changed
+175
-4
No files matched your search
@@ -53,7 +53,12 @@ and suggest up to three small fixes, with source links and uncertainty. Specify
|
||||
discovery only: no editing, delegation, new tasks, PR or merge.
|
||||
|
||||
Expect one Connect Slack card in Slack and the canonical Paperclip task. The
|
||||
link hands off through the authenticated task to personal OAuth. After the user
|
||||
link hands off automatically through the authenticated task to personal OAuth;
|
||||
it must not require a second Connect Slack click in Paperclip. Sign-in is still
|
||||
required if the browser has no session. The credential-free interaction suffix
|
||||
must survive UUID-to-identifier and company-prefix redirects. Consume it once
|
||||
before starting OAuth; settled requests, different addressees and ordinary task
|
||||
visits must not start authorization. After the user
|
||||
approves, the existing durable intent delivery resumes the original request.
|
||||
The answer keeps its original audience. On legacy v1 tasks, the normal Paperclip
|
||||
composer stays internal and Reply via Slack opts in to that turn's Slack reply.
|
||||
@@ -96,6 +101,27 @@ Slack grant. Do not rotate/revoke the existing bot token to reset a read test.
|
||||
|
||||
## Verification
|
||||
|
||||
September 23 handoff correction: the task's canonical-URL redirect was dropping
|
||||
`/connect-slack-read/:interactionId` from the actual UUID-based Slack link. The
|
||||
redirect now preserves an unconsumed suffix and does not restore one already
|
||||
consumed by the card. Regression tests reproduce the old failure and exercise
|
||||
UUID/wrong-company resolution followed by the real card's automatic start,
|
||||
remount/StrictMode deduplication, ineligible audiences/states and explicit retry
|
||||
after failure. All 154 tests in IssueDetail and ConnectionIntentInteractionBody,
|
||||
UI TypeScript, token gates and the UI production build pass. Provider scopes,
|
||||
OAuth state/session binding and continuation handling are unchanged. A new live
|
||||
OAuth run is not claimed by these tests; do not reset a working grant just to
|
||||
exercise this fix without the user's approval.
|
||||
|
||||
Deployed only to the isolated port-3210 pilot after rebuilding its UI. Live
|
||||
browser smoke: TES-10's actual unprefixed UUID/interaction URL resolved to
|
||||
`/TES/issues/TES-10/connect-slack-read/:interactionId` with the suffix intact.
|
||||
Its already-accepted card stayed connected and did not start another OAuth flow.
|
||||
The browser was returned to the ordinary TES-10 URL; no grant was reset.
|
||||
Subsequently, at the user's explicit request, only the personal channel-read
|
||||
connection was removed through Paperclip to prepare a fresh-consent retest.
|
||||
The CEO chat connection remained active; the user-led fresh OAuth test is pending.
|
||||
|
||||
See [the implementation plan](../plans/2026-09-22-slack-ceo-read-discovery.md)
|
||||
for the current automated/live split and provider references. Run
|
||||
`slack-read-profile.test.ts`, `slack-read-discovery.test.ts`, connection-intent
|
||||
|
||||
@@ -8,6 +8,15 @@ commit: `91339784ce9a740c149b34f013a1e29e7e7febdc`.
|
||||
|
||||
## Current checkpoint: threaded-DM preview
|
||||
|
||||
September 23 direct-consent correction: preserve the credential-free Slack
|
||||
handoff suffix during task UUID/company canonicalization. The existing addressed
|
||||
card then automatically starts the session-bound personal OAuth request, without
|
||||
a second Paperclip Connect click. Already-consumed handoffs are not restored;
|
||||
ordinary visits and settled/non-addressed cards do not auto-start. This is a
|
||||
routing-only fix, not a permission or research-scope expansion. The two relevant
|
||||
UI suites pass all 154 tests; UI TypeScript, token gates and the production build
|
||||
pass. Fresh live OAuth approval remains a separate test, not inferred from mocks.
|
||||
|
||||
The user's immediate request is to test a new Slack DM with a receipt, its own
|
||||
thread and editable status. The isolated port-3210 pilot now runs this narrower
|
||||
preview. Its database was backed up privately before applying migration 0284;
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
// @vitest-environment jsdom
|
||||
|
||||
import { act as reactAct, useState, type ReactNode } from "react";
|
||||
import { act as reactAct, StrictMode, useState, type ReactNode } from "react";
|
||||
import { createRoot, type Root } from "react-dom/client";
|
||||
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
|
||||
import type {
|
||||
@@ -212,6 +212,52 @@ describe("ConnectionIntentInteractionBody states and audience", () => {
|
||||
expect(slackReadMock).toHaveBeenCalledWith(interaction.id);
|
||||
expect(document.querySelector("[data-testid=shared-connection-setup]")).toBeNull();
|
||||
});
|
||||
it("starts a pending Slack handoff only once under StrictMode", async () => {
|
||||
const interaction = { ...pendingConnectionIntentInteraction, payload: {
|
||||
...pendingConnectionIntentInteraction.payload, capabilityProfile: "slack-public-read-v1" as const,
|
||||
} };
|
||||
window.history.replaceState(null, "", `/TES/issues/TES-10/connect-slack-read/${interaction.id}`);
|
||||
renderNode(<StrictMode><ConnectionIntentInteractionBody interaction={interaction}
|
||||
currentUserId={interaction.addresseeUserId} addresseeLabel="Carol" /></StrictMode>);
|
||||
await flush();
|
||||
expect(slackReadMock).toHaveBeenCalledTimes(1);
|
||||
expect(window.location.pathname).toBe("/TES/issues/TES-10");
|
||||
});
|
||||
it.each([
|
||||
{ user: null, status: "pending", matching: true },
|
||||
{ user: "another-user", status: "pending", matching: true },
|
||||
{ user: pendingConnectionIntentInteraction.addresseeUserId, status: "accepted", matching: true },
|
||||
{ user: pendingConnectionIntentInteraction.addresseeUserId, status: "rejected", matching: true },
|
||||
{ user: pendingConnectionIntentInteraction.addresseeUserId, status: "expired", matching: true },
|
||||
{ user: pendingConnectionIntentInteraction.addresseeUserId, status: "pending", matching: false },
|
||||
] as const)("does not auto-start an ineligible handoff: %j", async ({ user, status, matching }) => {
|
||||
const interaction = { ...pendingConnectionIntentInteraction, status, payload: {
|
||||
...pendingConnectionIntentInteraction.payload, capabilityProfile: "slack-public-read-v1" as const,
|
||||
} } as ConnectionIntentInteraction;
|
||||
const pathname = `/TES/issues/TES-10/connect-slack-read/${matching ? interaction.id : "another-interaction"}`;
|
||||
window.history.replaceState(null, "", pathname);
|
||||
renderBody(interaction, user);
|
||||
await flush();
|
||||
expect(slackReadMock).not.toHaveBeenCalled();
|
||||
expect(window.location.pathname).toBe(pathname);
|
||||
});
|
||||
it("leaves a failed automatic handoff on the task for explicit retry without looping", async () => {
|
||||
const interaction = { ...pendingConnectionIntentInteraction, payload: {
|
||||
...pendingConnectionIntentInteraction.payload, capabilityProfile: "slack-public-read-v1" as const,
|
||||
} };
|
||||
slackReadMock.mockRejectedValue(new Error("Please sign in again"));
|
||||
window.history.replaceState(null, "", `/TES/issues/TES-10/connect-slack-read/${interaction.id}`);
|
||||
renderBody(interaction);
|
||||
await waitForAssertion(() => expect(document.querySelector("[role=alert]")?.textContent).toContain("Please sign in again"));
|
||||
expect(window.location.pathname).toBe("/TES/issues/TES-10");
|
||||
await act(() => root?.unmount()); host?.remove(); root = null; host = null;
|
||||
renderBody(interaction);
|
||||
await flush();
|
||||
expect(slackReadMock).toHaveBeenCalledTimes(1);
|
||||
await act(async () => button("Connect Slack")!.click());
|
||||
await flush();
|
||||
expect(slackReadMock).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
it("shows an actionable Slack authorization failure and permits declining", async () => {
|
||||
slackReadMock.mockRejectedValue(new Error("Source channel configuration changed"));
|
||||
const interaction = { ...pendingConnectionIntentInteraction, payload: { ...pendingConnectionIntentInteraction.payload, capabilityProfile: "slack-public-read-v1" as const, sourceChannelId: "CAPPROVED" } };
|
||||
|
||||
@@ -41,6 +41,9 @@ import {
|
||||
} from "../lib/issueDetailBreadcrumb";
|
||||
import { getRecentTasksStorageKey, readRecentTasks } from "../lib/recent-tasks";
|
||||
import { ApiError } from "../api/client";
|
||||
import { connectionIntentsApi } from "../api/connection-intents";
|
||||
import { ConnectionIntentInteractionBody } from "../features/connections/ConnectionIntentInteractionBody";
|
||||
import { pendingConnectionIntentInteraction } from "../fixtures/issueThreadInteractionFixtures";
|
||||
import type { issuesApi } from "../api/issues";
|
||||
|
||||
const mockIssuesApi = vi.hoisted(() => ({
|
||||
@@ -133,6 +136,7 @@ const mockPanelState = vi.hoisted(() => ({ panelVisible: true }));
|
||||
const mockRouteParams = vi.hoisted(() => ({
|
||||
issueId: "PAP-1",
|
||||
companyPrefix: "PAP",
|
||||
interactionId: undefined as string | undefined,
|
||||
}));
|
||||
const mockSidebarState = vi.hoisted(() => ({ isMobile: false }));
|
||||
const mockIssuePropertiesRender = vi.hoisted(() => vi.fn());
|
||||
@@ -1407,6 +1411,8 @@ describe("IssueDetail", () => {
|
||||
mockLocation.state = null;
|
||||
mockRouteParams.issueId = "PAP-1";
|
||||
mockRouteParams.companyPrefix = "PAP";
|
||||
mockRouteParams.interactionId = undefined;
|
||||
window.history.replaceState(null, "", "/issues/PAP-1");
|
||||
});
|
||||
|
||||
afterEach(async () => {
|
||||
@@ -1433,6 +1439,77 @@ describe("IssueDetail", () => {
|
||||
expect(mockIssuesApi.markRead).toHaveBeenCalledWith(canonical.id);
|
||||
});
|
||||
|
||||
it.each(["PAP", "OTHER"])("preserves the Slack OAuth handoff when canonicalizing a UUID link under %s", async (prefix) => {
|
||||
const issue = createIssue({ id: "7c2da70e-d1a2-491f-9f5d-f82e0f88f1d2" });
|
||||
const interaction = {
|
||||
...pendingConnectionIntentInteraction,
|
||||
id: "06df12d7-86e8-44aa-af1f-6cb9bfb32af5",
|
||||
issueId: issue.id,
|
||||
payload: {
|
||||
...pendingConnectionIntentInteraction.payload,
|
||||
capabilityProfile: "slack-public-read-v1" as const,
|
||||
sourceChannelId: "CAPPROVED",
|
||||
},
|
||||
};
|
||||
const handoff = `/connect-slack-read/${interaction.id}`;
|
||||
// Slack publishes /issues/<UUID>/connect-slack-read/<interaction>. The
|
||||
// unprefixed board redirect adds the selected company's prefix unchanged.
|
||||
window.history.replaceState(null, "", `/issues/${issue.id}${handoff}`);
|
||||
mockLocation.pathname = `/${prefix}${window.location.pathname}`;
|
||||
window.history.replaceState(null, "", mockLocation.pathname);
|
||||
mockRouteParams.issueId = issue.id;
|
||||
mockRouteParams.companyPrefix = prefix;
|
||||
mockRouteParams.interactionId = interaction.id;
|
||||
mockIssuesApi.get.mockResolvedValue(issue);
|
||||
mockIssuesApi.listInteractions.mockResolvedValue([interaction]);
|
||||
const start = vi.spyOn(connectionIntentsApi, "startSlackRead").mockResolvedValue({
|
||||
status: "CONNECTED", connectionId: "read-connection",
|
||||
});
|
||||
await act(async () => {
|
||||
root.render(<QueryClientProvider client={queryClient}><IssueDetail /></QueryClientProvider>);
|
||||
});
|
||||
await waitForAssertion(() => expect(mockNavigate).toHaveBeenCalledWith(
|
||||
{ pathname: `/PAP/issues/PAP-1${handoff}`, search: "", hash: "" },
|
||||
{ replace: true, state: null },
|
||||
));
|
||||
// No interactive card is exposed on the outgoing UUID/wrong-company view.
|
||||
expect(start).not.toHaveBeenCalled();
|
||||
const destination = mockNavigate.mock.calls.at(-1)?.[0] as { pathname: string };
|
||||
window.history.replaceState(null, "", destination.pathname);
|
||||
// Mount the real card on the destination. It must start without a second
|
||||
// Connect click, consume the suffix, and stay consumed across a remount.
|
||||
for (const key of ["first-mount", "remount"]) {
|
||||
await act(async () => {
|
||||
root.render(<QueryClientProvider client={queryClient}>
|
||||
<ConnectionIntentInteractionBody key={key} interaction={interaction}
|
||||
currentUserId={interaction.addresseeUserId} addresseeLabel="Carol" />
|
||||
</QueryClientProvider>);
|
||||
});
|
||||
await flushReact();
|
||||
expect(start).toHaveBeenCalledTimes(1);
|
||||
expect(start).toHaveBeenCalledWith(interaction.id);
|
||||
expect(window.location.pathname).toBe("/PAP/issues/PAP-1");
|
||||
}
|
||||
});
|
||||
|
||||
it("does not restore an already consumed Slack handoff during a later company redirect", async () => {
|
||||
const interactionId = pendingConnectionIntentInteraction.id;
|
||||
mockRouteParams.companyPrefix = "OTHER";
|
||||
mockRouteParams.interactionId = interactionId;
|
||||
mockLocation.pathname = `/OTHER/issues/PAP-1/connect-slack-read/${interactionId}`;
|
||||
// The card consumes the handoff synchronously in browser history. Router
|
||||
// state can still contain the old suffix while company data finishes loading.
|
||||
window.history.replaceState(null, "", "/OTHER/issues/PAP-1");
|
||||
mockIssuesApi.get.mockResolvedValue(createIssue());
|
||||
await act(async () => {
|
||||
root.render(<QueryClientProvider client={queryClient}><IssueDetail /></QueryClientProvider>);
|
||||
});
|
||||
await waitForAssertion(() => expect(mockNavigate).toHaveBeenCalledWith(
|
||||
{ pathname: "/PAP/issues/PAP-1", search: "", hash: "" },
|
||||
{ replace: true, state: null },
|
||||
));
|
||||
});
|
||||
|
||||
it.each(["message", "attachment"])("creates an unused conversation only for the first %s and updates its canonical cache", async (kind) => {
|
||||
mockIssuesApi.markRead.mockClear();
|
||||
const agent = createAgent();
|
||||
|
||||
@@ -2848,7 +2848,9 @@ export function IssueDetail({ tasksTab }: { tasksTab?: TaskSidePanelProps["tasks
|
||||
export function TaskDetailSurface({ conversation, tasksTab }: { tasksTab?: TaskSidePanelProps["tasksTab"]; conversation?: {
|
||||
agent: Agent; issue: Issue | null; ensureIssue: () => Promise<Issue>;
|
||||
} }) {
|
||||
const { issueId: routeIssueId, companyPrefix } = useParams<{ issueId: string; companyPrefix: string }>();
|
||||
const { issueId: routeIssueId, companyPrefix, interactionId: slackReadInteractionId } = useParams<{
|
||||
issueId: string; companyPrefix: string; interactionId?: string;
|
||||
}>();
|
||||
const issueId = conversation ? conversation.issue?.id : routeIssueId;
|
||||
const [draftWorkMode, setDraftWorkMode] = useState<IssueWorkMode>("standard");
|
||||
const draftIssue = useMemo(() => conversation ? agentChatDraft(conversation.agent, draftWorkMode) : undefined, [conversation?.agent, draftWorkMode]);
|
||||
@@ -5473,7 +5475,17 @@ export function TaskDetailSurface({ conversation, tasksTab }: { tasksTab?: TaskS
|
||||
nextState,
|
||||
location.search,
|
||||
);
|
||||
const taskPath = createIssueDetailPath(canonicalRef);
|
||||
const handoffSuffix = slackReadInteractionId
|
||||
? `/connect-slack-read/${slackReadInteractionId}`
|
||||
: "";
|
||||
// Slack publishes UUID task links. Keep its credential-free handoff when
|
||||
// resolving the canonical task/company so the card can start OAuth without
|
||||
// another click. Consult live history: the card may already have consumed
|
||||
// the suffix while router state/company data is still catching up.
|
||||
const pendingHandoff = handoffSuffix && window.location.pathname.endsWith(handoffSuffix)
|
||||
? handoffSuffix
|
||||
: "";
|
||||
const taskPath = `${createIssueDetailPath(canonicalRef)}${pendingHandoff}`;
|
||||
navigate(
|
||||
{
|
||||
pathname: taskCompany
|
||||
@@ -5493,6 +5505,7 @@ export function TaskDetailSurface({ conversation, tasksTab }: { tasksTab?: TaskS
|
||||
loadedIssue,
|
||||
loadedIssueCompany,
|
||||
companyPrefix,
|
||||
slackReadInteractionId,
|
||||
issueId,
|
||||
navigate,
|
||||
location.state,
|
||||
|
||||
Reference in new issue
Block a user