PaperClipAI

Jannik/PaperClipAI

0 Fork 0

mirror of https://github.com/paperclipai/paperclip.git synced 2026-10-28 12:04:41 +01:00

Commit Graph

Author	SHA1	Message	Date
DottaandPaperclip	4413bb262f	refactor(tools): isolate OAuth protocol boundaries (#17147 ) ## Thinking Path > - Paperclip manages AI agents and their access to external tools. > - Tool connections use OAuth to obtain credentials from remote providers. > - The tool-access service mixed provider protocol parsing with database and secret writes. > - This made protocol changes hard to review without reading the full coordinator. > - This PR gives discovery, endpoint policy, and client exchange explicit owners. > - The coordinator keeps authorization, leases, persistence, and audit decisions. > - A real local provider fixture checks the boundary through public routes. ## Linked Issues or Issue Description What existing behavior does this improve? The ownership of OAuth discovery, registration, token exchange, and client metadata. Current behavior The tool-access service contains over 20,000 lines. Provider responses, endpoint policy, and stateful operations share the same closure. Proposed behavior Small protocol modules accept settings and named request capabilities. They return normalized results. The service owns durable state and existing public exports. This is part 3 of the stack: #17144, then #17146, then this PR. It targets #17146's branch. Review only this PR's diff. ## What Changed - Extract endpoint policy, metadata parsing/discovery, registration/token exchange, provider error normalization, and the public client metadata document. - Validate remote registration responses in the protocol module. Return client material to the coordinator for secret and connection writes. - Remove the unused raw provider response from the internal token result. Keep every consumed field. - Preserve process-wide registration single-flight, service-local maps, refresh leases, membership checks, callback checks, and transaction ownership. - Reuse connection-method URL normalization. Keep the endpoint setting and clock callbacks live. - Add a real local HTTP OAuth/MCP route fixture for public and Basic-authenticated clients. - Reduce `tool-access.ts` from 20,917 lines / 808,777 bytes to 19,293 lines / 749,158 bytes. The seven OAuth modules total 1,697 lines; the largest has 582 lines. The stack removes 2,232 lines from the original service. - Record the next profile-resolution and token-broker boundaries in the dated plan. ## Verification - Baseline: 466 service cases and 80 protocol/provider-policy cases pass. The new two-case HTTP fixture passes before moving production code. - After: `pnpm exec vitest run server/src/__tests__/tool-access-oauth-protocol.integration.test.ts server/src/__tests__/tool-access-service.test.ts server/src/__tests__/remote-mcp-connectors.test.ts server/src/__tests__/remote-http-rebinding.test.ts server/src/__tests__/oauth-client-metadata.test.ts` — 508 tests pass with one worker and disposable PostgreSQL. - The fixture drives connect, metadata discovery, DCR, PKCE code exchange, callback, catalog review, and finish. It checks persisted active state, secret references, issuer/auth-method binding, and wire-level authentication. - `pnpm -r --workspace-concurrency=2 typecheck` passes. The sandbox-only attempt could not open a local IPC socket; the same command passed with local process permissions. - `node scripts/check-module-boundaries.mjs` and `git diff --check` pass. - AST comparison confirms 37 moved bodies are unchanged. Registration protocol and persistence statements match their original halves. Token exchange only drops its unused raw result. Preflight uses explicit defaults and a clock callback. - Initial fixture authoring used a nonexistent catalog enabled field; the corrected assertion checks persisted status and reviewer identity. An initial typecheck found optional defaults at the new boundary; the caller now passes its already-validated defaults. - Final-stack local verification: `pnpm -r --workspace-concurrency=2 typecheck` and `pnpm build` pass. Remote CI is green at `3dc46c132408556ecb31038a9b9621ac20aacefc`. Greptile is 5/5 with no actionable findings or open inline threads. - Full local-suite limitation: `pnpm test:run` was interrupted after 79 minutes during the serial general-server phase, after CI had completed the full test matrix. Before interruption, it flagged the unchanged `company-search-service.test.ts` suite (26 database cases skipped). One isolated rerun passed all 28 cases without code changes. The original log and a pre-interruption copy are preserved. The original failure cause was not reproduced. The full local suite and its later groups are not claimed as passed; the complete CI matrix is green on this stack. ## Risks - OAuth endpoint checks and issuer/resource binding are security boundaries. Existing denial, redirect, scope, DCR, secret-redaction, and refresh tests cover the preserved paths. - A moved closure could capture stale policy or split registration/refresh ownership. The new modules hold no mutable state. Policy is read through the existing callback, and state stays in the service. - The local fixture proves real HTTP and database behavior. It does not claim a live third-party provider sign-in. - No schema, API, authorization policy, or persisted-data migration changes are intended. ## Model Used OpenAI Codex, GPT-6 family, with code execution and repository tools. The exact serving model ID and context-window size 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-21 22:21:41 -05:00
DottaandPaperclip	8e95caf9ae	refactor(tools): isolate connection method projection (#17146 ) ## Thinking Path > - Paperclip manages AI agents and their access to external tools. > - Connection methods define operator settings and managed tool arguments. > - Setup, catalog discovery, and gateway invocation must use the same projection rules. > - These rules were embedded in a large database service. > - This PR gives them one module with a narrow settings contract. > - Existing provider policy modules still decide their provider-specific restrictions. ## Linked Issues or Issue Description What existing behavior does this improve? Connection method configuration and tool input projection. Current behavior The tool-access database service owns settings validation and pure input/schema transforms. Callers import these through the service. Proposed behavior A connection-method module owns these transforms and accepts only connection settings. Existing imports remain compatible. This PR depends on #17144 and targets its branch. Review only this PR's diff. It is part 2 of the three-PR stack. #17147 follows this PR. ## What Changed - Move method selection, settings validation, and header/schema/argument projection into `tool-access/connection-methods.ts`. - Replace database row parameter types with required config and transport settings. No new validation is added inside typed calls. - Keep Honcho, Google Chat, and Gmail policy in their existing modules. Preserve their call order. - Retain the service's public exports for routes, gateway consumers, and tests. - Reduce `tool-access.ts` from 21,237 lines / 820,116 bytes to 20,917 lines / 808,777 bytes. The new module is 359 lines / 12,665 bytes. ## Verification - Baseline: all 466 tool-access service tests and 80 protocol/provider-policy cases pass before the move. - After: `pnpm exec vitest run server/src/__tests__/tool-access-service.test.ts server/src/__tests__/memory-connectors.test.ts server/src/services/google-chat-tool-policy.test.ts server/src/services/gmail-tool-governance.test.ts` — 506 tests pass with one worker and disposable PostgreSQL. - `pnpm --filter @paperclipai/server exec tsc --noEmit` passes after the existing workspace prerequisites were built. - AST body comparison confirms the 11 moved functions and private record parser are unchanged. `git diff --check` passes. - The first typecheck found optional config in the public DTO. The new input type now states the existing required-config invariant directly. - Final-stack local verification: `pnpm -r --workspace-concurrency=2 typecheck` and `pnpm build` pass. Greptile is 5/5 at `2d0bb86b5830c0306580e3ddc8637c4994b2a75a` with no actionable findings. - CI passed all non-browser gates. [The first browser attempt](https://github.com/paperclipai/paperclip/actions/runs/39675461620/job/119484152569) failed in the unchanged Cases read-only test: the status filter remained All statuses while the announcement-dismiss locator handler interrupted the assertion. The same test passed on #17144. The failure log is preserved. One retry of only the failed jobs passed without code changes; all CI gates are now green on the same head. - Full local-suite limitation: `pnpm test:run` was interrupted after 79 minutes during the serial general-server phase, after CI had completed the full test matrix. Before interruption, it flagged the unchanged `company-search-service.test.ts` suite (26 database cases skipped). One isolated rerun passed all 28 cases without code changes. The original log and a pre-interruption copy are preserved. The original failure cause was not reproduced. The full local suite and its later groups are not claimed as passed; the complete CI matrix is green on this stack. ## Risks - A projection-order change could alter managed arguments or advertised schemas. Function bodies and call order are unchanged, and tests exercise setup, discovery, and gateway consumers. - Legacy Gmail method aliases, GitHub toolset headers, missing unrelated settings, and disabled method filtering retain their previous behavior. - The module has no database dependency, new network work, or mutable state. ## Model Used OpenAI Codex, GPT-6 family, with code execution and repository tools. The exact serving model ID and context-window size 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-21 22:21:41 -05:00
DottaandPaperclip	9db3a5726e	refactor(tools): isolate GitHub access discovery (#17144 ) ## Thinking Path > - Paperclip manages AI agents and their access to external tools. > - Tool connections discover repositories through GitHub account grants. > - The tool-access service mixes GitHub response parsing with grant authorization and persistence. > - These responsibilities have different inputs and failure rules. > - This PR puts GitHub discovery in one provider module. > - The service keeps current authority checks and stored grant updates. ## Linked Issues or Issue Description What existing behavior does this improve? The ownership and review of GitHub access discovery in the tool-access service. Current behavior The 21,525-line service contains GitHub pagination, account metadata validation, and grant lifecycle code. Proposed behavior A GitHub module returns normalized access metadata. Existing service exports remain compatible. This is the first PR in a three-part refactor stack. No behavior change is intended. A search found no existing tool-access refactor PR or issue. Stack order: this PR, #17146, then #17147. Each PR targets its predecessor. ## What Changed - Move six functions into `tool-access/github-access-discovery.ts`. Their bodies are unchanged. - Keep grant selection, credential resolution, refresh retry, membership locks, revision checks, and webhook updates in the service. - Add public-entry characterization tests for HTTP error classification and invalid pagination. - Record five candidate boundaries and the initial three-PR scope in the dated plan. - Reduce `tool-access.ts` from 21,525 lines / 829,519 bytes to 21,237 lines / 820,116 bytes. The new module is 294 lines / 9,722 bytes. ## Verification - Before and after: `pnpm exec vitest run server/src/__tests__/github-grant-metadata.test.ts server/src/__tests__/project-repositories.test.ts` — 13 tests pass. - Before and after: `pnpm exec vitest run server/src/__tests__/tool-access-service.test.ts -t 'GitHub\|github'` — 11 selected tests pass with disposable PostgreSQL; 455 cases are outside this selection. - `pnpm --filter @paperclipai/server typecheck` passes. - Exact text comparison confirms all six moved bodies match the base. `git diff --check` passes. - The initial baseline attempt failed because the plugin SDK had not been built. The normal workspace prerequisite build resolved it before behavioral validation. - Final-stack local verification: `pnpm -r --workspace-concurrency=2 typecheck` and `pnpm build` pass. CI is green at `07e78dd1b1c26e0bd4234006766444c71e234451`. Greptile is 5/5 with no actionable findings. - Full local-suite limitation: `pnpm test:run` was interrupted after 79 minutes during the serial general-server phase, after CI had completed the full test matrix. Before interruption, it flagged the unchanged `company-search-service.test.ts` suite (26 database cases skipped). One isolated rerun passed all 28 cases without code changes. The original log and a pre-interruption copy are preserved. The original failure cause was not reproduced. The full local suite and its later groups are not claimed as passed; the complete CI matrix is green on this stack. ## Risks - Import wiring could break existing consumers. Existing imports remain supported and all four production call sites are unchanged. - Provider requests retain their ordering, timeout, fixed GitHub origin, and response redaction. The new module has no database dependency or shared mutable state. - Tests simulate GitHub responses. They do not qualify live GitHub access. ## Model Used OpenAI Codex, GPT-6 family, with code execution and repository tools. The exact serving model ID and context-window size 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-21 22:21:40 -05:00

Author

SHA1

Message

Date

DottaandPaperclip

4413bb262f

refactor(tools): isolate OAuth protocol boundaries (#17147 )

## Thinking Path

> - Paperclip manages AI agents and their access to external tools.
> - Tool connections use OAuth to obtain credentials from remote
providers.
> - The tool-access service mixed provider protocol parsing with
database and secret writes.
> - This made protocol changes hard to review without reading the full
coordinator.
> - This PR gives discovery, endpoint policy, and client exchange
explicit owners.
> - The coordinator keeps authorization, leases, persistence, and audit
decisions.
> - A real local provider fixture checks the boundary through public
routes.

## Linked Issues or Issue Description

**What existing behavior does this improve?**

The ownership of OAuth discovery, registration, token exchange, and
client metadata.

**Current behavior**

The tool-access service contains over 20,000 lines. Provider responses,
endpoint policy, and stateful operations share the same closure.

**Proposed behavior**

Small protocol modules accept settings and named request capabilities.
They return normalized results. The service owns durable state and
existing public exports. This is part 3 of the stack: #17144, then
#17146, then this PR. It targets #17146's branch. Review only this PR's
diff.

## What Changed

- Extract endpoint policy, metadata parsing/discovery,
registration/token exchange, provider error normalization, and the
public client metadata document.
- Validate remote registration responses in the protocol module. Return
client material to the coordinator for secret and connection writes.
- Remove the unused raw provider response from the internal token
result. Keep every consumed field.
- Preserve process-wide registration single-flight, service-local maps,
refresh leases, membership checks, callback checks, and transaction
ownership.
- Reuse connection-method URL normalization. Keep the endpoint setting
and clock callbacks live.
- Add a real local HTTP OAuth/MCP route fixture for public and
Basic-authenticated clients.
- Reduce `tool-access.ts` from 20,917 lines / 808,777 bytes to 19,293
lines / 749,158 bytes. The seven OAuth modules total 1,697 lines; the
largest has 582 lines. The stack removes 2,232 lines from the original
service.
- Record the next profile-resolution and token-broker boundaries in the
dated plan.

## Verification

- Baseline: 466 service cases and 80 protocol/provider-policy cases
pass. The new two-case HTTP fixture passes before moving production
code.
- After: `pnpm exec vitest run
server/src/__tests__/tool-access-oauth-protocol.integration.test.ts
server/src/__tests__/tool-access-service.test.ts
server/src/__tests__/remote-mcp-connectors.test.ts
server/src/__tests__/remote-http-rebinding.test.ts
server/src/__tests__/oauth-client-metadata.test.ts` — 508 tests pass
with one worker and disposable PostgreSQL.
- The fixture drives connect, metadata discovery, DCR, PKCE code
exchange, callback, catalog review, and finish. It checks persisted
active state, secret references, issuer/auth-method binding, and
wire-level authentication.
- `pnpm -r --workspace-concurrency=2 typecheck` passes. The sandbox-only
attempt could not open a local IPC socket; the same command passed with
local process permissions.
- `node scripts/check-module-boundaries.mjs` and `git diff --check`
pass.
- AST comparison confirms 37 moved bodies are unchanged. Registration
protocol and persistence statements match their original halves. Token
exchange only drops its unused raw result. Preflight uses explicit
defaults and a clock callback.
- Initial fixture authoring used a nonexistent catalog enabled field;
the corrected assertion checks persisted status and reviewer identity.
An initial typecheck found optional defaults at the new boundary; the
caller now passes its already-validated defaults.
- Final-stack local verification: `pnpm -r --workspace-concurrency=2
typecheck` and `pnpm build` pass. Remote CI is green at
`3dc46c132408556ecb31038a9b9621ac20aacefc`. Greptile is 5/5 with no
actionable findings or open inline threads.

- Full local-suite limitation: `pnpm test:run` was interrupted after 79
minutes during the serial general-server phase, after CI had completed
the full test matrix. Before interruption, it flagged the unchanged
`company-search-service.test.ts` suite (26 database cases skipped). One
isolated rerun passed all 28 cases without code changes. The original
log and a pre-interruption copy are preserved. The original failure
cause was not reproduced. The full local suite and its later groups are
not claimed as passed; the complete CI matrix is green on this stack.

## Risks

- OAuth endpoint checks and issuer/resource binding are security
boundaries. Existing denial, redirect, scope, DCR, secret-redaction, and
refresh tests cover the preserved paths.
- A moved closure could capture stale policy or split
registration/refresh ownership. The new modules hold no mutable state.
Policy is read through the existing callback, and state stays in the
service.
- The local fixture proves real HTTP and database behavior. It does not
claim a live third-party provider sign-in.
- No schema, API, authorization policy, or persisted-data migration
changes are intended.

## Model Used

OpenAI Codex, GPT-6 family, with code execution and repository tools.
The exact serving model ID and context-window size 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-21 22:21:41 -05:00

DottaandPaperclip

8e95caf9ae

refactor(tools): isolate connection method projection (#17146 )

## Thinking Path

> - Paperclip manages AI agents and their access to external tools.
> - Connection methods define operator settings and managed tool
arguments.
> - Setup, catalog discovery, and gateway invocation must use the same
projection rules.
> - These rules were embedded in a large database service.
> - This PR gives them one module with a narrow settings contract.
> - Existing provider policy modules still decide their
provider-specific restrictions.

## Linked Issues or Issue Description

**What existing behavior does this improve?**

Connection method configuration and tool input projection.

**Current behavior**

The tool-access database service owns settings validation and pure
input/schema transforms. Callers import these through the service.

**Proposed behavior**

A connection-method module owns these transforms and accepts only
connection settings. Existing imports remain compatible. This PR depends
on #17144 and targets its branch. Review only this PR's diff. It is part
2 of the three-PR stack. #17147 follows this PR.

## What Changed

- Move method selection, settings validation, and header/schema/argument
projection into `tool-access/connection-methods.ts`.
- Replace database row parameter types with required config and
transport settings. No new validation is added inside typed calls.
- Keep Honcho, Google Chat, and Gmail policy in their existing modules.
Preserve their call order.
- Retain the service's public exports for routes, gateway consumers, and
tests.
- Reduce `tool-access.ts` from 21,237 lines / 820,116 bytes to 20,917
lines / 808,777 bytes. The new module is 359 lines / 12,665 bytes.

## Verification

- Baseline: all 466 tool-access service tests and 80
protocol/provider-policy cases pass before the move.
- After: `pnpm exec vitest run
server/src/__tests__/tool-access-service.test.ts
server/src/__tests__/memory-connectors.test.ts
server/src/services/google-chat-tool-policy.test.ts
server/src/services/gmail-tool-governance.test.ts` — 506 tests pass with
one worker and disposable PostgreSQL.
- `pnpm --filter @paperclipai/server exec tsc --noEmit` passes after the
existing workspace prerequisites were built.
- AST body comparison confirms the 11 moved functions and private record
parser are unchanged. `git diff --check` passes.
- The first typecheck found optional config in the public DTO. The new
input type now states the existing required-config invariant directly.
- Final-stack local verification: `pnpm -r --workspace-concurrency=2
typecheck` and `pnpm build` pass. Greptile is 5/5 at
`2d0bb86b5830c0306580e3ddc8637c4994b2a75a` with no actionable findings.
- CI passed all non-browser gates. [The first browser
attempt](https://github.com/paperclipai/paperclip/actions/runs/39675461620/job/119484152569)
failed in the unchanged Cases read-only test: the status filter remained
All statuses while the announcement-dismiss locator handler interrupted
the assertion. The same test passed on #17144. The failure log is
preserved. One retry of only the failed jobs passed without code
changes; all CI gates are now green on the same head.

- Full local-suite limitation: `pnpm test:run` was interrupted after 79
minutes during the serial general-server phase, after CI had completed
the full test matrix. Before interruption, it flagged the unchanged
`company-search-service.test.ts` suite (26 database cases skipped). One
isolated rerun passed all 28 cases without code changes. The original
log and a pre-interruption copy are preserved. The original failure
cause was not reproduced. The full local suite and its later groups are
not claimed as passed; the complete CI matrix is green on this stack.

## Risks

- A projection-order change could alter managed arguments or advertised
schemas. Function bodies and call order are unchanged, and tests
exercise setup, discovery, and gateway consumers.
- Legacy Gmail method aliases, GitHub toolset headers, missing unrelated
settings, and disabled method filtering retain their previous behavior.
- The module has no database dependency, new network work, or mutable
state.

## Model Used

OpenAI Codex, GPT-6 family, with code execution and repository tools.
The exact serving model ID and context-window size 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-21 22:21:41 -05:00

DottaandPaperclip

9db3a5726e

refactor(tools): isolate GitHub access discovery (#17144 )

## Thinking Path

> - Paperclip manages AI agents and their access to external tools.
> - Tool connections discover repositories through GitHub account
grants.
> - The tool-access service mixes GitHub response parsing with grant
authorization and persistence.
> - These responsibilities have different inputs and failure rules.
> - This PR puts GitHub discovery in one provider module.
> - The service keeps current authority checks and stored grant updates.

## Linked Issues or Issue Description

**What existing behavior does this improve?**

The ownership and review of GitHub access discovery in the tool-access
service.

**Current behavior**

The 21,525-line service contains GitHub pagination, account metadata
validation, and grant lifecycle code.

**Proposed behavior**

A GitHub module returns normalized access metadata. Existing service
exports remain compatible. This is the first PR in a three-part refactor
stack. No behavior change is intended. A search found no existing
tool-access refactor PR or issue.

Stack order: this PR, #17146, then #17147. Each PR targets its
predecessor.

## What Changed

- Move six functions into `tool-access/github-access-discovery.ts`.
Their bodies are unchanged.
- Keep grant selection, credential resolution, refresh retry, membership
locks, revision checks, and webhook updates in the service.
- Add public-entry characterization tests for HTTP error classification
and invalid pagination.
- Record five candidate boundaries and the initial three-PR scope in the
dated plan.
- Reduce `tool-access.ts` from 21,525 lines / 829,519 bytes to 21,237
lines / 820,116 bytes. The new module is 294 lines / 9,722 bytes.

## Verification

- Before and after: `pnpm exec vitest run
server/src/__tests__/github-grant-metadata.test.ts
server/src/__tests__/project-repositories.test.ts` — 13 tests pass.
- Before and after: `pnpm exec vitest run
server/src/__tests__/tool-access-service.test.ts -t 'GitHub|github'` —
11 selected tests pass with disposable PostgreSQL; 455 cases are outside
this selection.
- `pnpm --filter @paperclipai/server typecheck` passes.
- Exact text comparison confirms all six moved bodies match the base.
`git diff --check` passes.
- The initial baseline attempt failed because the plugin SDK had not
been built. The normal workspace prerequisite build resolved it before
behavioral validation.
- Final-stack local verification: `pnpm -r --workspace-concurrency=2
typecheck` and `pnpm build` pass. CI is green at
`07e78dd1b1c26e0bd4234006766444c71e234451`. Greptile is 5/5 with no
actionable findings.

- Full local-suite limitation: `pnpm test:run` was interrupted after 79
minutes during the serial general-server phase, after CI had completed
the full test matrix. Before interruption, it flagged the unchanged
`company-search-service.test.ts` suite (26 database cases skipped). One
isolated rerun passed all 28 cases without code changes. The original
log and a pre-interruption copy are preserved. The original failure
cause was not reproduced. The full local suite and its later groups are
not claimed as passed; the complete CI matrix is green on this stack.

## Risks

- Import wiring could break existing consumers. Existing imports remain
supported and all four production call sites are unchanged.
- Provider requests retain their ordering, timeout, fixed GitHub origin,
and response redaction. The new module has no database dependency or
shared mutable state.
- Tests simulate GitHub responses. They do not qualify live GitHub
access.

## Model Used

OpenAI Codex, GPT-6 family, with code execution and repository tools.
The exact serving model ID and context-window size 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-21 22:21:40 -05:00