Commit Graph
2 Commits
Author SHA1 Message Date
DottaandPaperclip a8b652b085 fix(github): preserve review delivery through reconnect (#16259)
## Thinking Path

> - Paperclip is the open source app people use to manage AI agents for
work.
> - GitHub review bots turn authorized repository events into agent
tasks and publish the result on the pull request.
> - Cloud Apps send callbacks through a relay, but the direct callback
recovery worker reported that URL as broken.
> - Reconnecting the same App then prevented a new authorized
publication from updating its existing working comment.
> - Report-only runs could also produce an action-required check when no
complete assessment was available.
> - This pull request preserves comment identity across reconnects,
verifies fresh signed delivery after repair, and keeps report-only
results neutral.
> - Users can receive the review without changing repository access or
making an advisory bot block their pull request.

## Linked Issues or Issue Description

**What happened?**

A review could be saved in Paperclip while GitHub kept showing “Working
on this…”. A same-App reconnect changed the runtime generation and the
existing response receipt then failed with “GitHub response binding
changed”. Cloud callback recovery also compared the relay URL with a
direct tenant URL. An incomplete or failed report-only run could produce
an action-required check.

**Expected behavior**

A fresh authorized attempt updates the same App-owned comment after
reconnect. Stale attempts and revoked people remain denied. Cloud relay
delivery does not produce a direct-callback mismatch. Report-only checks
remain neutral and explain incomplete work honestly.

**Steps to reproduce**

1. Accept a GitHub request and publish its working comment.
2. Reconnect the same App before the agent publishes its result.
3. Submit the result and inspect the working comment and check.
4. Separately, let a report-only run end without an assessment and
inspect its check conclusion.

Related: #16138 addresses the separate classification of local
authorization errors as provider credential failures. This change fixes
the response binding and report-only paths themselves.

## What Changed

- Bind durable response comments to the App and agent. Keep per-attempt
runtime generation checks and provider comment ownership checks,
including legacy receipts.
- Exclude Cloud-managed Apps from the direct webhook recovery scan and
its stale callback-mismatch warning. The Cloud inbox owns relay retries.
- Let a fresh authenticated event from the bound installation verify
webhook delivery after repair. Keep signature, runtime epoch,
installation, and credential lease checks.
- Keep report-only assessment and terminal-run checks neutral. Preserve
incomplete/error text and gated-review behavior.
- Add integration regressions for reconnect races, legacy comments,
failed runs, Cloud recovery, and foreign or forged webhook traffic.
Update connector documentation.
- Correct the assigned-tool search guidance to name the actual input
fields (`offset` and `schemaOffset`) and show valid arguments. The
failed automatic review had an input-schema rejection before tool
execution; this clarifies recovery without expanding tool access.

## Verification

- `pnpm -r typecheck` passed.
- GitHub policy and integration suite: 291 tests passed (`vitest run
server/src/services/chat-github-review-policy.test.ts
server/src/__tests__/chat-channels.integration.test.ts -t
'GitHub|github'`).
- The report-only policy regression failed before the fix and passed
after it.
- `pnpm build` passed.
- Assigned tool discovery: 24 tests passed. Its guidance now maps
returned pagination offsets to the correct input fields and tells the
model to omit unused optional fields.
- Full `pnpm test:run` is running. It encountered a 15-second timeout in
the unrelated authenticated Workspace calendar-write test. That test
passed on one isolated rerun.
- After the review-requested module extraction, server typecheck and
both direct/Cloud recovery activity integration checks passed.
- All 55 non-skipped CI checks passed for
`32e690279d847e44a6946b6c7cfb110fb11aff79`. Greptile scored that commit
5/5 with no unresolved findings. Live staging publication verification
follows deployment.

## Risks

- A durable response can now survive a same-App reconnect. Fresh
authorization, runtime generation checks for each publication attempt,
App ownership, and the publication lease still fence writes.
- Report-only failures intentionally become neutral. The check still
reports the failure; it does not claim a complete assessment.
- Cloud-managed callbacks rely on their existing durable relay inbox.
The direct callback recovery worker remains unchanged for direct
installations.
- No schema migration or permission expansion.

## Model Used

OpenAI Codex (GPT-6), with code inspection, editing, test execution, and
browser verification. The exact deployment model ID and context window
are not exposed in this session.

## Checklist

- [x] I have included a thinking path that traces from project context
to this change
- [x] I have specified the model used (with version and capability
details)
- [x] I have checked ROADMAP.md and confirmed this PR does not duplicate
planned core work
- [x] I have searched GitHub for duplicate or related PRs and linked
them above
- [x] I have either (a) linked existing issues with `Fixes: #` / `Closes
#` / `Refs #` OR (b) described the issue in-PR following the relevant
issue template
- [x] I have not referenced internal/instance-local Paperclip issues or
links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip`
URLs)
- [x] My branch name describes the change (e.g. `docs/...`, `fix/...`)
and contains no internal Paperclip ticket id or instance-derived details
- [x] I have run tests locally and they pass
- [x] I have added or updated tests where applicable
- [x] I have updated relevant documentation to reflect my changes
- [x] I have considered and documented any risks above
- [x] All Paperclip CI gates are green
- [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups
- [x] I will address all Greptile and reviewer comments before
requesting merge

---------

Co-authored-by: Paperclip <noreply@paperclip.ing>
2026-10-13 22:24:41 -05:00
DottaandPaperclip a2ac5c2715 refactor(chat): isolate Telegram, Slack, GitHub and Discord workflows (#15976)
## Thinking Path

> - Paperclip manages AI agents and their work.
> - Chat channels connect that work to external conversations.
> - The shared chat service still contains provider-specific workflows.
> - Those workflows are hard to inspect in a 34,000-line file.
> - This stack extracts Telegram, Slack, GitHub and Discord workflows
into their channel folders.
> - Shared authorization and service lifetime remain explicit.

## Linked Issues or Issue Description

Refs #15823, #15834, #15836, #15844 and #15845. Root of the second
behavior-preserving channel-folder refactor stack. The dependent PRs
#15979, #15981, #15985 and #15987 have merged into this branch; this PR
lands all five extractions on master.

## What Changed

- Move Telegram maintenance and subscription repair into
`chat-channels/telegram/maintenance.ts`.
- Move Slack session stopping and task starts into
`chat-channels/slack/session-stop.ts` and `task-start.ts`.
- Move GitHub webhook intake and recovery into
`chat-channels/github/webhook-ingress.ts`.
- Move Discord command registration and handling into
`chat-channels/discord/commands.ts`.
- Share type-only runtime records while preserving shared authorization,
transactions, live lease checks and worker ownership.
- Reduce the original service from 34,603 to 29,897 lines, removing
4,706 lines (13.6%).

## Verification

- All 205 selected integration cases passed before and after their
respective extractions: Telegram 41, Slack session stop 2 multi-step
scenarios, GitHub 84, Slack task start 25 and Discord 53. Each dependent
PR records its focused reproduction command.
- Server typecheck, module boundaries and whitespace checks passed.
- Per-slice syntax comparisons passed for moved declarations and
remaining service statements, allowing only the documented live callback
substitutions.
- Tests exercise the public service with disposable databases and
fixture provider transports. They do not claim live third-party
execution.
- Cumulative workspace typecheck and build passed on the final stack.
- Current-head CI passed on the combined stack commit
`d5b37dbe512e3e199932b6608cdc17639eda555a`; Greptile is 5/5 with no
unresolved threads.
- The [original serialized
shard](https://github.com/paperclipai/paperclip/actions/runs/38266518817/job/114870022249)
timed out in unchanged `adapter-auth-signal-routes.test.ts`. The exact
case passed locally without changes, and the [single job
retry](https://github.com/paperclipai/paperclip/actions/runs/38266518817/job/114874019933)
passed. The original timeout remains recorded.

Reproduce the original Telegram slice’s focused suite:

```sh
pnpm exec vitest run server/src/__tests__/chat-channels.integration.test.ts -t 'Telegram durable private draft Stop|Telegram maintenance|Telegram webhook|Telegram.*(pause|remove)|Telegram.*commands'
```

- Cumulative local `pnpm test:run` exited 1: the first server group had
**18,770 passed, 105 skipped, and one timeout** in unchanged
`server/src/services/workspace-git-snapshot-streaming.test.ts:56`
(300,000 ms on macOS). The fail-fast runner did not reach its later
workspace and serialized groups. An unchanged isolated rerun passed
using the existing timeout; this does not turn the original full run
green. The test and its Git utility code are outside this refactor; all
current-head CI jobs passed.
- Reproduce that diagnostic alone: `pnpm exec vitest run
server/src/services/workspace-git-snapshot-streaming.test.ts`.

## Risks

- Credential lease ownership and provider-confirmation ordering must
remain unchanged. Existing rotation, lease-loss, recovery and durable
Stop cases pass.
- The channel factories create no workers or timers and preserve live
runtime, shutdown and gateway-ownership reads.

## Model Used

OpenAI Codex, GPT-6 family, with reasoning and repository tools. The
exact runtime model ID and context-window size are not exposed in this
session. No separate agent was used.

## Checklist

- [x] I have included a thinking path that traces from project context
to this change
- [x] I have specified the model used (with version and capability
details)
- [x] I have checked ROADMAP.md and confirmed this PR does not duplicate
planned core work
- [x] I have searched GitHub for duplicate or related PRs and linked
them above
- [x] I have either (a) linked existing issues with `Fixes: #` / `Closes
#` / `Refs #` OR (b) described the issue in-PR following the relevant
issue template
- [x] I have not referenced internal/instance-local Paperclip issues or
links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip`
URLs)
- [x] My branch name describes the change (e.g. `docs/...`, `fix/...`)
and contains no internal Paperclip ticket id or instance-derived details
- [ ] I have run tests locally and they pass
- [x] I have added or updated tests where applicable
- [x] I have updated relevant documentation to reflect my changes
- [x] I have considered and documented any risks above
- [x] All Paperclip CI gates are green
- [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups
- [x] I will address all Greptile and reviewer comments before
requesting merge

---------

Co-authored-by: Paperclip <noreply@paperclip.ing>
2026-10-12 12:03:23 -05:00