Skip to content

fix: resolve UUID secret refs with host contract - #192

Open
steingran wants to merge 3 commits into
alvarosanchez:mainfrom
steingran:fix/structured-secret-ref-host-compat
Open

fix: resolve UUID secret refs with host contract#192
steingran wants to merge 3 commits into
alvarosanchez:mainfrom
steingran:fix/structured-secret-ref-host-compat

Conversation

@steingran

Copy link
Copy Markdown
Contributor

What changed

  • Convert UUID-backed saved secret references to the structured { type: "secret_ref", secretId } form required by newer Paperclip hosts.
  • Preserve legacy non-UUID references unchanged.
  • Permit the existing company-scoped token fallback only for documented host-unavailable/invalid-secret-reference errors; unrelated secret-provider failures still fail closed.
  • Add focused regression coverage for structured refs, scoped fallback behavior, and fail-closed provider errors.

Root cause

The published worker normalizes saved secret refs to UUID strings and passes those strings directly to ctx.secrets.resolve. Newer Paperclip hosts require UUID-backed refs as structured secret_ref objects, so configured GitHub operations fail before the already provisioned company-scoped fallback can be considered.

Impact

This restores compatibility with the current host secret-reference contract without rotating, reconnecting, or logging credentials. It does not widen fallback behavior for permission failures or arbitrary resolver errors.

Validation

The originating implementation run passed:

  • pnpm typecheck
  • focused structured-ref/fallback tests (3/3)
  • full pnpm test
  • pnpm build
  • git diff --check

Publication recovery verified this branch is exactly one commit ahead of upstream main and changes only src/worker.ts and tests/plugin.spec.ts (104 insertions, 9 deletions).

@steingran
steingran marked this pull request as ready for review July 24, 2026 02:04
Copilot AI review requested due to automatic review settings July 24, 2026 02:04

Copy link
Copy Markdown
Contributor Author

Ready for review at b1a747ca4089c2362c9970d80761ba6ba00ed8d6 (one commit ahead of main; only src/worker.ts and tests/plugin.spec.ts). CI run 552 passed. The change keeps non-UUID refs unchanged and limits fallback to documented host-unavailable/invalid-ref cases; unrelated secret-provider errors remain fail-closed. No credentials were changed, logged, rotated, or reconnected.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the plugin worker to be compatible with newer Paperclip hosts that require UUID-backed secret references to be passed as structured { type: "secret_ref", secretId } objects (rather than raw UUID strings), while keeping legacy non-UUID references unchanged. It also tightens fallback behavior so the company-scoped token fallback is only used for specific “secret ref unavailable/invalid ref” host errors, and adds targeted regression tests.

Changes:

  • Add a wrapper (resolvePluginSecret) that converts UUID-shaped secret refs into structured secret_ref objects before calling ctx.secrets.resolve.
  • Refine the “secret refs unavailable” error classification to include the documented invalid-secret-reference host error.
  • Add regression tests for structured secret refs, scoped fallback behavior, and fail-closed resolver/provider errors.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/worker.ts Adds UUID→structured secret ref conversion and narrows fallback eligibility to specific host error cases.
tests/plugin.spec.ts Adds regression tests covering structured secret refs, fallback gating, and fail-closed resolver errors.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/worker.ts Outdated
Comment thread src/worker.ts Outdated
@steingran

Copy link
Copy Markdown
Contributor Author

@alvarosanchez Please review the updated head 4f8a497 after CI completes. It resolves both secret-reference threads (UUIDv6/v7/v8 structured refs and resolver method binding); focused regression, typecheck, full test suite, and build passed.

@alvarosanchez alvarosanchez left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes because the patch does not work against either side of the repository's stated host-compatibility range.

  1. Blocking — the newer host contract requires an explicit company scope, but every new call omits it (src/worker.ts:2929-2936). Since Paperclip v2026.720.0, PluginSecretsClient.resolve forwards options.companyId/configPath, and the host rejects a call without companyId before resolution. resolvePluginSecret widens only the first argument and invokes secrets.resolve(...) with one argument, so all four changed paths still fail with companyId is required for this operation. The new unit tests replace harness.ctx.secrets.resolve with a permissive one-argument stub, so they cannot detect this production RPC failure.

  2. Blocking — the plugin still stores refs as UUID strings, so newer hosts do not create the binding that structured resolution requires (src/manifest.ts:57-69, src/ui/plugin-config.ts:1-6, and the config writes around src/ui/index.tsx:12025-12028 / 12132-12135). Current Paperclip extracts/binds object-shaped { type: "secret_ref", secretId, version? } values from company-scoped plugin config and deliberately ignores UUID strings outside schema-declared secret fields. This manifest declares these map values as plain strings, and the UI normalizers discard object values. Converting the UUID only at src/worker.ts:2923-2926 is therefore too late: after adding companyId, the host still returns binding_missing because no plugin binding was registered.

  3. Blocking — unconditional UUID conversion breaks the repository's enforced Paperclip 2026.626.0 baseline (src/worker.ts:2923-2926; baseline asserted in tests/build-script.spec.mjs:10,54-58). That host accepts only a UUID string. Passing the new object produces Invalid secret reference: [object Object], which the broadened fallback classifier does not recognize because the message contains neither secret_ref nor the old disabled-host wording. GitHub and board secret resolution therefore regress on the supported baseline.

The full local pnpm typecheck, pnpm test (335/335), and pnpm build pass at head 4f8a497e1851a28b774d05144bac246f85173f19, but those tests exercise mocked resolver shapes rather than either real host bridge. Please either implement and test an explicit dual-host path, or intentionally raise/synchronize the minimum host+SDK boundary; for the newer path, persist structured refs in trusted company-scoped config and call resolve(ref, { companyId, configPath }) through an SDK version that forwards those options.


✨ This message was AI-generated using gpt-5.6-sol

@steingran

Copy link
Copy Markdown
Contributor Author

@alvarosanchez Fresh maintainer review requested for exact head 3ef0dfd. This addresses the dual-host secret-ref findings: structured company-scoped binding persistence, companyId/configPath forwarding for new hosts, raw UUID one-argument resolution for the Paperclip 2026.626.0 baseline, receiver binding, and fail-closed unrelated resolver errors. Verification passed: focused plugin suite, typecheck, full test suite, build, and git diff --check. Please review this exact SHA.

@alvarosanchez alvarosanchez left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review result: changes requested

Reviewed exact head: 3ef0dfde146e0f0ef830d29d1f479be1e15f3920

The worker-side company scope and legacy retry are improved, but the end-to-end current-host path remains broken.

1. Blocking: current-host config requests omit the required company scope

Locations: src/ui/index.tsx:6895-6907, with new structured-reference writes around 12025-12031 and 12132-12138.

Paperclip v2026.722.0 requires:

  • GET /api/plugins/:pluginId/config?companyId=...
  • POST /api/plugins/:pluginId/config with { companyId, configJson }

patchPluginConfig() has no company argument. It performs the old unscoped GET and posts only configJson. The GET therefore fails with 400: "companyId" is required before a structured reference can be saved or bound.

The updated test around tests/plugin.spec.ts:6073+ models the old unscoped endpoint and body, so it does not exercise the v2026.722.0 contract.

2. Blocking: existing UUID-string references are not migrated into bindable config

Locations: src/worker.ts:23986-23991, src/ui/index.tsx:11382-11386, src/ui/index.tsx:11459-11463, and src/ui/plugin-config.ts:25-29.

An installation upgraded from the supported v2026.626.0 host can contain equal raw UUID strings in state and config without a worker-local plaintext fallback.

  • settings.registration treats equal raw strings as already synchronized, so it does not request repair.
  • If config is absent, the repair effect writes the same raw string back.
  • Paperclip v2026.722.0 creates company secret bindings only from structured { type: "secret_ref", secretId, version? } values.

The reference therefore remains unbound and raw resolution is rejected. The original no-reconnect upgrade case remains broken even after finding 1 is fixed.

3. Blocking: the declared SDK minimum does not support the implementation

Locations: package.json:44, src/worker.ts:13, and src/worker.ts:2983-2984.

The lockfiles select @paperclipai/[email protected], but package.json still advertises ^2026.626.0.

Installing the exact declared minimum and running typecheck fails because v2026.626.0:

  • does not export EnvSecretRefBinding; and
  • accepts only the one-argument secret resolver, so it cannot forward { companyId, configPath } to a newer host.

Package consumers do not inherit this repository's lockfile. The runtime dependency minimum must match the SDK contract used by the implementation. The old-host compatibility retry can remain in the plugin.

4. Correctness: structured references are compared by object identity

Location: src/worker.ts:23987-23990.

Both normalizers create fresh objects. As a result, semantically identical configured and saved structured references are always reported as requiring config synchronization.

An adversarial registration test using equivalent structured references returned:

githubTokenNeedsConfigSync: true

Use the existing semantic reference comparator instead of !== for both GitHub and board references.

Verification

On a disposable checkout pinned to the exact head:

  • Frozen dependency install with SDK v2026.722.0: passed
  • Focused secret tests: 6/6 passed
  • pnpm typecheck: passed
  • Full tests: 5 build-script tests + 337 main tests passed
  • pnpm build: passed
  • git diff --check: passed
  • CI verify: green on the exact SHA
  • SDK v2026.626.0 compatibility probe: reproduced finding 3
  • Structured-reference equality probe: reproduced finding 4

The green suite does not exercise the current host's company-scoped config endpoint or the SDK version declared as the package minimum.


✨ This message was AI-generated using gpt-5.6-sol

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants