fix: address Copilot review of #198 (scope-denied sync result, legacy ref migration write, lockfile)

src/ui/index.tsx

@@ -18,6 +18,7 @@ import { normalizeCompanyAssigneeOptionsResponse, type GitHubSyncAssigneeOption
 import { buildPaperclipUrl, fetchJson, fetchPaperclipHealth, resolveCliAuthPollUrl } from './http.ts';
 import { resolveInstalledGitHubSyncPluginId, resolvePluginSettingsHref } from './plugin-installation.ts';
 import {
+  hasLegacyPluginSecretRefs,
   mergePluginConfig,
   type GitHubSyncPluginConfig,
   type GitHubSyncPluginConfigPatch,
@@ -5847,9 +5848,13 @@ function buildPluginConfigUrl(pluginId: string, companyId: string): string {
   return `/api/plugins/${pluginId}/config?companyId=${encodeURIComponent(companyId)}`;
 }
 
-async function readPluginConfig(pluginId: string, companyId: string): Promise<GitHubSyncPluginConfig> {
+async function readRawPluginConfig(pluginId: string, companyId: string): Promise<unknown> {
   const currentConfigResponse = await fetchJson<PluginConfigResponse | null>(buildPluginConfigUrl(pluginId, companyId));
-  return normalizePluginConfig(currentConfigResponse?.configJson);
+  return currentConfigResponse?.configJson;
+}
+
+async function readPluginConfig(pluginId: string, companyId: string): Promise<GitHubSyncPluginConfig> {
+  return normalizePluginConfig(await readRawPluginConfig(pluginId, companyId));
 }
 
 async function syncTrustedPaperclipApiBaseUrl(
@@ -6923,10 +6928,17 @@ export async function patchPluginConfig(
     throw new Error('Company context is required to save GitHub Sync plugin config.');
   }
 
-  const currentConfig = await readPluginConfig(pluginId, companyId);
+  const rawCurrentConfig = await readRawPluginConfig(pluginId, companyId);
+  const currentConfig = normalizePluginConfig(rawCurrentConfig);
   const nextConfig = mergePluginConfig(currentConfig, patch);
-
-  if (JSON.stringify(nextConfig) === JSON.stringify(currentConfig)) {
+  // A legacy row (bare secret-id strings) normalizes to the same bindings the patch produces, so
+  // the normalized shapes match even though the stored row still needs upgrading.
+  const isLegacySecretRefMigration = hasLegacyPluginSecretRefs(rawCurrentConfig);
+  const isNormalizedConfigUnchanged = JSON.stringify(nextConfig) === JSON.stringify(currentConfig);
+
+  // Compare on the normalized shape only when the stored row is already in binding form; otherwise
+  // the write is what upgrades the host row and clears `binding_missing`.
+  if (!isLegacySecretRefMigration && isNormalizedConfigUnchanged) {
     return;
   }
 
@@ -6937,6 +6949,13 @@ export async function patchPluginConfig(
       throw error;
     }
 
+    // Pre-2026.831 hosts reject binding refs, and the bare secret-id strings they already store are
+    // the shape they understand. `stripPluginSecretRefConfig` would drop those refs entirely, so
+    // when the legacy migration is the only reason for this write, leave the stored row untouched.
+    if (isLegacySecretRefMigration && isNormalizedConfigUnchanged) {
+      return;
+    }
+
     const safeConfig = stripPluginSecretRefConfig(nextConfig);
     if (JSON.stringify(safeConfig) === JSON.stringify(nextConfig)) {
       throw error;

src/ui/plugin-config.ts

@@ -94,6 +94,23 @@ function normalizeCompanySecretRefMap(value: unknown): Record<string, PluginSecr
   return Object.fromEntries(entries);
 }
 
+/**
+ * Reports whether a raw plugin config row still stores any secret ref in the legacy bare
+ * secret-id string shape. Such rows normalize to the same `{ type: "secret_ref" }` bindings a
+ * patch produces, so callers must not skip a write based on normalized equality alone: the host
+ * only binds (and the worker only resolves) refs stored as binding objects.
+ */
+export function hasLegacyPluginSecretRefs(value: unknown): boolean {
+  if (!isPlainRecord(value)) {
+    return false;
+  }
+
+  return [value.githubTokenRefs, value.paperclipBoardApiTokenRefs].some((refs) =>
+    isPlainRecord(refs)
+    && Object.values(refs).some((secretRef) => !isPluginSecretRefBinding(secretRef) && Boolean(normalizeOptionalString(secretRef)))
+  );
+}
+
 export function normalizePluginConfigBoardTokenRefs(value: unknown): PluginConfigBoardTokenRefs | undefined {
   return normalizeCompanySecretRefMap(value);
 }

src/worker.ts

@@ -16083,13 +16083,29 @@ function mergeRememberedTrustedConfig(): Record<string, unknown> {
   return merged;
 }
 
+interface ReadTrustedConfigOptions {
+  /**
+   * Sync execution must not silently continue for a company the host refuses to scope: when the
+   * host denies the company scope and it never delivered a config for that company (no saved
+   * plugin-config row), rethrow so the caller records `COMPANY_SCOPE_DENIED_SYNC_MESSAGE` instead
+   * of running with an empty or worker-local fallback config.
+   */
+  requireCompanyScope?: boolean;
+}
+
 /**
  * Reads the company-scoped plugin config from the host. Paperclip 2026.831 requires a company
  * scope for `ctx.config.get()`; when the host cannot derive one (scheduled jobs, actions without a
  * company, companies without a saved config row) the worker falls back to the config the host
- * delivered through `configChanged` so company-keyed settings still resolve.
+ * delivered through `configChanged` so company-keyed settings still resolve. Data/settings paths
+ * keep that lenient behavior so the UI can render an empty config; sync execution opts into
+ * `requireCompanyScope` so the scope-denied condition surfaces as a recorded sync error.
  */
-async function readTrustedConfig(ctx: PluginSetupContext, companyId?: string): Promise<Record<string, unknown>> {
+async function readTrustedConfig(
+  ctx: PluginSetupContext,
+  companyId?: string,
+  options: ReadTrustedConfigOptions = {}
+): Promise<Record<string, unknown>> {
   const normalizedCompanyId = normalizeCompanyId(companyId);
 
   try {
@@ -16103,6 +16119,9 @@ async function readTrustedConfig(ctx: PluginSetupContext, companyId?: string): P
     const remembered = normalizedCompanyId
       ? rememberedTrustedConfigByCompanyId.get(normalizedCompanyId)
       : mergeRememberedTrustedConfig();
+    if (options.requireCompanyScope && normalizedCompanyId && !remembered && isCompanyScopeDeniedError(error)) {
+      throw error;
+    }
     const warningKey = `${normalizedCompanyId ?? '<instance>'}:${getErrorMessage(error)}`;
     if (!reportedTrustedConfigReadFailures.has(warningKey)) {
       reportedTrustedConfigReadFailures.add(warningKey);
@@ -16119,9 +16138,13 @@ async function readTrustedConfig(ctx: PluginSetupContext, companyId?: string): P
   }
 }
 
-async function getResolvedConfig(ctx: PluginSetupContext, companyId?: string): Promise<GitHubSyncConfig> {
+async function getResolvedConfig(
+  ctx: PluginSetupContext,
+  companyId?: string,
+  options: ReadTrustedConfigOptions = {}
+): Promise<GitHubSyncConfig> {
   const [savedConfig, externalConfig] = await Promise.all([
-    readTrustedConfig(ctx, companyId),
+    readTrustedConfig(ctx, companyId, options),
     readExternalConfig(ctx)
   ]);
 
@@ -22258,10 +22281,22 @@ async function startSync(
   await reconcileOrphanedRunningSyncState(ctx, options.target?.companyId);
 
   const targetCompanyId = normalizeCompanyId(options.target?.companyId);
-  const [config, persistedSettings] = await Promise.all([
-    getResolvedConfig(ctx, targetCompanyId),
-    ctx.state.get(SETTINGS_SCOPE).then((value) => normalizeSettings(value))
-  ]);
+  let config: GitHubSyncConfig;
+  let persistedSettings: GitHubSyncSettings;
+  try {
+    [config, persistedSettings] = await Promise.all([
+      getResolvedConfig(ctx, targetCompanyId, { requireCompanyScope: true }),
+      ctx.state.get(SETTINGS_SCOPE).then((value) => normalizeSettings(value))
+    ]);
+  } catch (error) {
+    if (!isCompanyScopeDeniedError(error)) {
+      throw error;
+    }
+    // The host refused to scope plugin config for this company and never delivered one, so the
+    // company has no saved GitHub Sync plugin config. Record the operator-facing sync error
+    // instead of continuing with an empty or worker-local fallback config.
+    return createUnexpectedSyncErrorResult(ctx, trigger, error, targetCompanyId);
+  }
   const token = await resolveGithubToken(ctx, {
     companyId: targetCompanyId,
     config,