feat(watchsync): add per-connection plugin config - #550
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughWatch-provider plugins can now declare per-connection configuration schemas. The runtime validates and merges these values during API-key authentication. The API and settings UI expose, collect, and submit the configuration. Local watch exports now use provider-specific history filtering and batch sizes. ChangesWatch-provider connection setup
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant WatchProvidersSettings
participant WatchProvidersAPI
participant WatchsyncService
participant PluginProvider
participant WatchsyncPlugin
User->>WatchProvidersSettings: enter API key and connection fields
WatchProvidersSettings->>WatchProvidersAPI: submit API key and connection_config
WatchProvidersAPI->>WatchsyncService: ConnectAPIKeyWithConfig
WatchsyncService->>PluginProvider: ConnectWithAPIKeyConfig
PluginProvider->>PluginProvider: validate and merge configuration
PluginProvider->>WatchsyncPlugin: ExchangeAPIKey
WatchsyncPlugin-->>PluginProvider: return credentials
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
web/src/hooks/queries/watchProviders.ts (1)
6-14: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueMake
descriptionrequired.ConfigSchemaViewalways serializesdescription;json_schema,required, and optionaladmin_formalready match.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/hooks/queries/watchProviders.ts` around lines 6 - 14, Update the WatchProviderSummary type to require the description field, matching ConfigSchemaView serialization; add it alongside the existing summary properties while leaving json_schema, required, and optional admin_form handling unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/watchsync/plugin_provider.go`:
- Around line 175-179: Update the authentication flow around connectionConfig
and the returned TokenSet so validated connectionValues are stored in
TokenSet.SecretAttributes before returning, using the existing encrypted
credential-attribute mechanism. Ensure later connection-specific plugin calls
reconstruct their configuration from those persisted attributes rather than
falling back to installation configuration, while preserving the existing token
response behavior.
In `@web/src/pages/settings/WatchProvidersSettings.tsx`:
- Around line 287-297: Update the APIKeyBlock call site and component flow to
pass providerKey, then include providerKey in the SchemaForm idPrefix so each
provider card generates unique DOM ids even when schema.key is shared.
- Around line 236-255: Update the configValid calculation in
WatchProvidersSettings so it evaluates only schemas with an admin_form, matching
the schemas included by configuredValues and rendered by the form. Keep the
existing configValidity fallback for rendered schemas, and do not let required
schemas without an admin_form disable Connect.
---
Nitpick comments:
In `@web/src/hooks/queries/watchProviders.ts`:
- Around line 6-14: Update the WatchProviderSummary type to require the
description field, matching ConfigSchemaView serialization; add it alongside the
existing summary properties while leaving json_schema, required, and optional
admin_form handling unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3bf0180a-60eb-4028-83fe-e7334b4387ce
📒 Files selected for processing (15)
cmd/silo/main.godocs/superpowers/specs/2026-07-25-plugin-watch-sync-provider-design.mdinternal/api/handlers/plugins.gointernal/api/handlers/watch_providers.gointernal/api/handlers/watch_providers_test.gointernal/plugins/config_schema_view.gointernal/plugins/watch_sync_config_test.gointernal/watchsync/plugin_provider.gointernal/watchsync/plugin_provider_test.gointernal/watchsync/registry.gointernal/watchsync/service.gointernal/watchsync/types.goweb/src/hooks/queries/watchProviders.test.tsweb/src/hooks/queries/watchProviders.tsweb/src/pages/settings/WatchProvidersSettings.tsx
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4542bb53c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Redact connection secrets from exchange faults
When a connection schema contains a password or secret field and the plugin echoes that submitted value in its fault message—for example, password X was rejected—this call redacts only the API key, so the new connection secret can be returned verbatim by the API and displayed in the UI. Include the values from connectionValues.SecretValues in the secrets passed to watchSyncFaultError, just as the API key and stored credentials are already protected.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Addressed the remaining Codex security review in host commit 9c9e10e: API-key exchange faults now redact every connection-config secret (including nested JSON-schema credentials), not only the API key. The focused regression makes the plugin echo both a top-level and nested submitted secret and asserts neither reaches the returned error. Validation: host race tests for plugins/watchsync/handlers/cmd, vet, golangci-lint (0 issues), frontend lint/format/tests/build. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4edf0068e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/src/components/admin/plugins/configSchemaAdminForm.ts`:
- Around line 15-28: Update the JSON schema property descriptor parsed in
configSchemaAdminForm to include an optional default value, and copy valid
primitive property.default values into the inferred field’s default_value for
string, number, and boolean defaults. Preserve existing behavior for unsupported
or absent defaults, and add a schema-only regression test covering string,
number, and true boolean defaults used by PluginConfigForm and
watchProviderConnectionConfig.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c187c013-0917-4959-86cd-a8346982d1f1
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
cmd/silo/main_test.gogo.modinternal/plugins/config_secrets.gointernal/watchsync/plugin_provider.gointernal/watchsync/plugin_provider_test.goweb/src/components/admin/plugins/PluginConfigForm.test.tsxweb/src/components/admin/plugins/PluginConfigForm.tsxweb/src/components/admin/plugins/configSchemaAdminForm.tsweb/src/pages/settings/WatchProvidersSettings.tsxweb/src/pages/settings/watchProviderConnectionConfig.test.tsweb/src/pages/settings/watchProviderConnectionConfig.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/pages/settings/WatchProvidersSettings.tsx
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f445c1cda
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10d4624222
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5400717c3e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d12f467dd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
internal/watchsync/plugin_provider.go (2)
706-710: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReject an empty connection-config key explicitly.
validateWatchSyncConnectionConfigSchemasaccepts a schema whose key trims to"".connectionConfigthen skips that schema at line 600, and the schema never reaches the UI. The manifest author gets no error and no field. Two empty keys also produce the confusing messageconnection config "" is duplicated.♻️ Proposed check
key := strings.TrimSpace(schema.GetKey()) + if key == "" { + return errors.New("connection config key must not be empty") + } if _, exists := seen[key]; exists {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/watchsync/plugin_provider.go` around lines 706 - 710, Update validateWatchSyncConnectionConfigSchemas around the key extraction to reject a trimmed empty schema key with a clear validation error before checking seen. Preserve duplicate detection for non-empty keys, while ensuring empty keys cannot reach connectionConfig or produce a duplicate-key error.
599-641: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider rejecting undeclared nested fields.
Line 587 rejects an undeclared top-level schema key. The inner loop at lines 621-639 does not apply the same rule to nested fields. Any field that is absent from
publicFieldNamesis forwarded inSecretValues. The default-secret routing is a safe fallback, so submitted data is not exposed. The plugin still receives keys that its manifest never declared.If additive tolerance is intended here, keep the current behavior. If manifest-declared fields are meant to be exhaustive, reject unknown nested fields with the same sanitized error path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/watchsync/plugin_provider.go` around lines 599 - 641, Decide whether nested manifest fields are exhaustive; if so, update the inner field loop in the connection-config validation flow to reject fields absent from the schema’s declared field set before routing them to Values or SecretValues. Return the rejection through sanitizedConnectionConfigError, preserving the existing behavior for declared public and secret fields; otherwise explicitly retain the current additive-tolerance behavior.web/src/components/admin/plugins/SchemaForm.test.tsx (1)
101-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the negative case for each conditional test.
Both tests assert only that the conditional field and the conditional section appear when the controlling default is
true. An implementation that returnedtruefromevaluateShowWhenunconditionally would still pass. Add a companion assertion withdefault_value: falseandqueryByText(...)returningnull. That pins the condition itself, not just the default lookup.Also applies to: 223-258
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/admin/plugins/SchemaForm.test.tsx` around lines 101 - 126, Add negative companion cases to the conditional field test around SchemaForm, and the corresponding conditional section test: set the controlling field’s default_value to false and assert the conditional content is absent with queryByText(...), returning null. Preserve the existing true-default assertions so both visibility outcomes are covered.web/src/components/admin/plugins/configSchemaAdminForm.test.ts (1)
22-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for a
falseboolean default.The test covers
true. The implementation keepsfalseonly because it checkstypeof property.default === "boolean". A future change to a truthiness check would dropfalsesilently and this suite would still pass.falseis the regression-prone value.💚 Proposed addition
verify_tls: { type: "boolean", default: true }, + follow_redirects: { type: "boolean", default: false }, }, }), }), ); expect(form?.fields.map(({ key, default_value }) => ({ key, default_value }))).toEqual([ { key: "base_url", default_value: "https://floppy.example.com" }, { key: "port", default_value: 8080 }, { key: "verify_tls", default_value: true }, + { key: "follow_redirects", default_value: false }, ]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/admin/plugins/configSchemaAdminForm.test.ts` around lines 22 - 41, Add a boolean property with default false to the existing “preserves primitive JSON Schema defaults on inferred fields” test and include its expected field/default_value pair in the assertion, ensuring false is explicitly preserved.internal/watchsync/service.go (1)
1643-1661: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid passing an unused fallback in the single-batch path.
watchedExportBatchSizeusesfallbackfor non-batched exporters at line 1550. InlimitWatchedExportBatch, the type assertion already succeeds, so use the asserted exporter directly when calculating the limit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/watchsync/service.go` around lines 1643 - 1661, Update limitWatchedExportBatch to reuse its successful singleBatchWatchedExporter assertion when calculating the limit, rather than calling watchedExportBatchSize with an unused fallback; preserve the existing fallback behavior in watchedExportBatchSize for other callers and non-batched exporters.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/watchsync/plugin_provider.go`:
- Around line 776-795: Update the property validation loop around explicit[key]
so any non-inferable property type is accepted when it has a renderable explicit
admin_form field, including enum-only or $ref properties. Preserve the existing
array item-type validation for properties without an explicit field, and retain
the current error for unsupported properties that lack one.
- Around line 830-837: Update the numeric validation flow around
connectionConfigNumber to also parse string values with strconv when the field
declares a numeric control or has a numeric minimum/maximum bound. Apply the
existing min/max checks to successfully parsed numeric strings, while leaving
non-numeric strings on the existing string-validation path.
---
Nitpick comments:
In `@internal/watchsync/plugin_provider.go`:
- Around line 706-710: Update validateWatchSyncConnectionConfigSchemas around
the key extraction to reject a trimmed empty schema key with a clear validation
error before checking seen. Preserve duplicate detection for non-empty keys,
while ensuring empty keys cannot reach connectionConfig or produce a
duplicate-key error.
- Around line 599-641: Decide whether nested manifest fields are exhaustive; if
so, update the inner field loop in the connection-config validation flow to
reject fields absent from the schema’s declared field set before routing them to
Values or SecretValues. Return the rejection through
sanitizedConnectionConfigError, preserving the existing behavior for declared
public and secret fields; otherwise explicitly retain the current
additive-tolerance behavior.
In `@internal/watchsync/service.go`:
- Around line 1643-1661: Update limitWatchedExportBatch to reuse its successful
singleBatchWatchedExporter assertion when calculating the limit, rather than
calling watchedExportBatchSize with an unused fallback; preserve the existing
fallback behavior in watchedExportBatchSize for other callers and non-batched
exporters.
In `@web/src/components/admin/plugins/configSchemaAdminForm.test.ts`:
- Around line 22-41: Add a boolean property with default false to the existing
“preserves primitive JSON Schema defaults on inferred fields” test and include
its expected field/default_value pair in the assertion, ensuring false is
explicitly preserved.
In `@web/src/components/admin/plugins/SchemaForm.test.tsx`:
- Around line 101-126: Add negative companion cases to the conditional field
test around SchemaForm, and the corresponding conditional section test: set the
controlling field’s default_value to false and assert the conditional content is
absent with queryByText(...), returning null. Preserve the existing true-default
assertions so both visibility outcomes are covered.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c44c8dc5-2d8d-4f9b-a780-c457020522ab
📒 Files selected for processing (13)
internal/pluginhost/handshake.gointernal/watchsync/plugin_provider.gointernal/watchsync/plugin_provider_test.gointernal/watchsync/repository.gointernal/watchsync/service.gointernal/watchsync/service_test.goweb/src/components/admin/plugins/SchemaForm.test.tsxweb/src/components/admin/plugins/SchemaForm.tsxweb/src/components/admin/plugins/configSchemaAdminForm.test.tsweb/src/components/admin/plugins/configSchemaAdminForm.tsweb/src/components/admin/plugins/schemaFormUtils.test.tsweb/src/components/admin/plugins/schemaFormUtils.tsweb/src/pages/settings/WatchProvidersSettings.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- web/src/pages/settings/WatchProvidersSettings.tsx
- internal/watchsync/plugin_provider_test.go
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2dd4069009
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 936f1990af
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f39667b469
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| if (explicitFields.length > 0) { | ||
| const explicitKeys = new Set(explicitFields.map((field) => field.key)); |
There was a problem hiding this comment.
When a property gets format: "password" or writeOnly: true through a local $ref, an explicit TEXT field remains non-secret because sensitiveKeys inspects only the referencing property's direct annotations. The backend explicitly resolves local references in ConfigSchemaFieldSets, and validateConnectionSchemaIsRenderable accepts referenced properties with explicit controls, so this supported schema displays the credential in a plain-text input. Fresh evidence beyond the resolved direct-annotation issue is that the new merge still does not follow $ref; resolve referenced schemas when deriving sensitivity or use the server-derived classification.
Useful? React with 👍 / 👎.
| // A property whose shape comes from enum/const/$ref cannot be | ||
| // inferred from type alone, but an explicit scalar form control is | ||
| // still a complete input mechanism. Direct object properties remain | ||
| // unsupported because none of these controls produces an object. | ||
| if property.Type != "object" && connectionAdminFieldRendersValue(field) { | ||
| continue |
There was a problem hiding this comment.
When a property has no direct type but constrains a non-string scalar through $ref, enum, or const, this branch accepts any explicit value-producing control. The frontend's parseFieldTypes also reads only direct type, so a static SELECT or TEXT field for a referenced number/boolean emits a string and every Connect request is rejected by JSON-schema validation. Fresh evidence beyond the resolved direct-type control issue is this explicit acceptance path for indirect types; resolve the effective scalar type and validate/coerce the control, or reject the descriptor.
Useful? React with 👍 / 👎.
| .filter(([, property]) => property.writeOnly === true || property.format === "password") | ||
| .map(([key]) => key), | ||
| ); | ||
| return { |
There was a problem hiding this comment.
When a JSON-schema property is listed in required but its matching explicit admin-form field omits required: true, this merge retains the explicit field unchanged and discards the inferred required flag. SchemaForm therefore reports an empty required connection block as valid, enables Connect, and submits an object that the backend rejects on every attempt until the user guesses which visible field is mandatory. Unlike the resolved omitted-field case, the key is present in the explicit form here; merge JSON-schema requiredness onto matching fields or reject the inconsistent descriptor.
Useful? React with 👍 / 👎.
| length := utf8.RuneCountInString(text) | ||
| if minimum := int(validation.GetMinLength()); minimum > 0 && length < minimum { | ||
| return fmt.Errorf("connection config %q field %q must be at least %d characters", schema.GetKey(), field.GetKey(), minimum) | ||
| } | ||
| if maximum := int(validation.GetMaxLength()); maximum > 0 && length > maximum { | ||
| return fmt.Errorf("connection config %q field %q must be at most %d characters", schema.GetKey(), field.GetKey(), maximum) |
There was a problem hiding this comment.
When an admin-form min_length or max_length is applied to text containing an astral Unicode character such as an emoji, the server counts Unicode code points here while SchemaForm uses JavaScript raw.length, which counts UTF-16 code units. A one-emoji value can consequently be rejected by the UI even though the server accepts it for max_length: 1, or be submitted and then rejected for min_length: 2; use the same code-point counting rule on both sides.
Useful? React with 👍 / 👎.
f39667b to
d0678b2
Compare
Connection config fields absent from a schema's declared sets are classified as secrets fail-closed, but the fault-redaction list collected only declared secret fields. A plugin echoing such a value in a fault returned it in the clear. Redact every flattened secret value instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
go mod tidy removes the v0.13.1 hash pair left behind by the v0.13.2 bump. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
d0678b2 to
420e3ef
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 420e3ef581
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if err := publicconfig.ValidateValue(schema, "watch sync connection config", schema.GetKey(), value); err != nil { | ||
| return nil, nil, sanitizedConnectionConfigError(err, secrets) |
There was a problem hiding this comment.
When an undeclared field fails a permitted additionalProperties schema (for example, an unknown API token rejected by a format or pattern), secrets contains only schema-declared secret fields at this point, so a validator error that includes the rejected value is returned through the API unsanitized. Fresh evidence after the undeclared-secret fault-redaction fix is that undeclared values are appended to the redaction list only after validation succeeds; collect fail-closed secret values before calling ValidateValue as well.
Useful? React with 👍 / 👎.
| if number, ok := connectionConfigNumber(raw); ok { | ||
| if validation.GetHasMin() && number < validation.GetMin() { | ||
| return fmt.Errorf("connection config %q field %q must be at least %g", schema.GetKey(), field.GetKey(), validation.GetMin()) | ||
| } | ||
| if validation.GetHasMax() && number > validation.GetMax() { | ||
| return fmt.Errorf("connection config %q field %q must be at most %g", schema.GetKey(), field.GetKey(), validation.GetMax()) | ||
| } | ||
| } |
There was a problem hiding this comment.
When an admin-form NUMBER field has min/max constraints but its JSON Schema property is unconstrained (such as {}), a direct v1 request can submit "abc": JSON-schema validation accepts it, connectionConfigNumber returns false, and this block silently skips every numeric constraint before forwarding the string to the plugin. Fresh evidence after the server-side admin-form validation addition is that nonnumeric values still bypass that validator; require a numeric parse for NUMBER controls whenever numeric validation is declared. This matters for supported third-party API clients, not only the web form.
AGENTS.md reference: AGENTS.md:L19-L22
Useful? React with 👍 / 👎.
1 participant
Problem
Plugin watch providers could receive only installation-wide provider configuration during API-key setup. For self-hosted providers such as Floppy, that forced every profile on one Silo server to use the same remote instance.
Closes #549. Follow-up to #475.
Approach
The released SDK v0.13 contract already provides capability config_schema, WatchSyncProviderConfig, and encrypted WatchSyncCredentials.secret_attributes, so this change requires no SDK protobuf or release update.
The /api/v1 change is additive. Built-in providers and plugins without capability setup fields keep their previous behavior.
Risk and compatibility
Validation
Passed on commit b4542bb:
The production frontend build completed with the repository's existing default.woff2 and large-chunk warnings only.
Adversarial review
The review found that directly forwarding form drafts would send numeric and boolean fields as strings and could omit displayed defaults; submission now uses the shared schema coercion/default builder. It also found a duplicate admin-form DTO bridge that could drift, so both plugin-admin and watch-provider surfaces now share one converter with the existing completeness regression test. Finally, exposing an optional legacy global Floppy URL still contradicted the requested model; the Floppy manifest removes that setting entirely while the host's fail-closed undeclared-config path preserves old encrypted values as an invisible upgrade fallback.
AI Disclosure
Summary by CodeRabbit
New Features
Bug Fixes