Repository navigation
fix(api): name the rejected plugin id and secret binding destination in 400s - #1431
Merged
Merged
Conversation
…in 400s A plugin selection the selected Plugin Driver does not offer named the Driver but not the selection, so with several selections the caller had to guess. The error now carries the rejected selection key: the message names it and HTTP adds a /plugins/<id> INVALID_VALUE detail, the same JSON-pointer mechanism the log route uses for /cursor. A Secret binding with a reserved destination returned a static 400 that named neither the key nor the rule. normalizeSecretBindings now says which rule broke (reserved prefix, reserved name, or invalid format) and names a well-formed destination; request admission adds a /secretBindings/<key> (or /configuration/secretBindings/<key> for provisioning) detail. No Secret value or ID is echoed. Worker and Compute discard this message, so their behaviour is unchanged.
Contributor
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
clawsweeper
Bot
added
P3
Contributor
|
Codex review: needs changes before merge. Reviewed October 5, 2026, 7:42 PM ET / 23:42 UTC (Revision 2). ClawSweeper reviewWhat this changesThe PR makes API rejection messages identify unsupported plugin selections and reserved Secret binding destinations, adds field pointers, and extends tests and documentation. Merge readiness⛔ Needs changes before merge - 2 items remain The diagnostics remain useful and are absent from current main. No blocking runtime or security defect was found; one small documentation correction remains. Priority: P3 Review scores
Verification
How this fits togetherOpenClaw Control Plane validates Agent plugin selections and Configuration Secret bindings before accepting resource changes. Its HTTP error mapper turns rejected input into bounded messages and field pointers for callers. flowchart TB
A[Agent or Configuration request] --> B[API admission]
B --> C[Plugin selection validation]
B --> D[Secret destination validation]
C --> E[HTTP error mapping]
D --> E
E --> F[400 response with rule and field pointer]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Keep the bounded field-specific diagnostics and document the distinction between reserved-name pointers and malformed-name map pointers. Do we have a high-confidence way to reproduce the issue? Yes, source establishes the diagnostic gap: submit a plugin from the other Driver or a reserved Secret destination through the existing API routes. Current main returns no pointer identifying the rejected selection or destination; this review did not execute those requests. Is this the best way to solve the issue? Yes, extending the existing validation errors and HTTP detail mechanism is a focused solution. The flow documentation needs to reflect the final malformed-key safety behavior. Full review comments:
Overall correctness: patch is correct AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 0d9bb168a060. LabelsLabel changes: No label changes. Label justifications:
EvidenceAcceptance criteria:
What I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
…d keys (review) The plugin message named the ID before the rule, so a long Codex ID let the 256-character cap cut "does not offer". Keep the original "A plugin selection names a plugin that the selected Plugin Driver (X) does not offer" prefix and append the ID after it. A malformed Secret binding destination was echoed raw into the detail path, control characters included. Point INVALID_FORMAT details at the binding map instead. A conformance test drives normalizeRequestSecretBindings through requestFailure for reserved prefix, reserved name and invalid format, and checks the Secret ID never appears.
This was referenced Oct 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Two
400 INVALID_REQUESTresponses said what was wrong but not where:A plugin selection names a plugin that the selected Plugin Driver (codex-plugin) does not offer…. With several selections the caller has to guess which one was rejected, for example after an Installation switches its Plugin Driver.A secret binding uses a reserved or invalid environment destination.This names neither the binding key nor the rule (reserved prefix, reserved name, or invalid format). A Configuration with ten bindings gave no hint which one to change.Change
Plugin ID. The runtime translator's plugin-ID errors carry the rejected selection key.
PluginPolicyValidationError("unknownPlugin", driverId, pluginId)appends the key to the existing message after the rule (A plugin selection names a plugin that the selected Plugin Driver (occ-plugin) does not offer: codex-plugin:linear@openai-curated-remote. Check each plugin ID…), so the 256-character cap cuts the advice before the rule, and exposes it aspluginId. HTTP adds adetailsentry{ path: "/plugins/<id>", code: "INVALID_VALUE" }. This is the JSON-pointer mechanism #1409 added for/cursor, with~and/escaped. The key is the caller's own selection key, which the API contract limits to[A-Za-z0-9._~:@-]{1,253}.Secret binding destination.
normalizeSecretBindings(contracts) now reports which rule a destination broke, and the destination key as error properties:A secret binding destination uses the reserved prefix OPENCLAW_*: OPENCLAW_TOKEN.A secret binding destination is a reserved process or platform variable name: PATH.A secret binding destination is not a valid environment variable name: it must match ^[A-Za-z_][A-Za-z0-9_]*$ and have at most 253 characters.(the key is not echoed)The rule comes before the key, so the 256-character message cap never cuts the rule.
normalizeRequestSecretBindingspasses the key intoSecretBindingValidationError.destination. HTTP turns that into a detail:/secretBindings/<key>for Configuration create and update, and/configuration/secretBindings/<key>for Agent provisioning. Reserved destinations getINVALID_VALUEat the key. A malformed key is never echoed: itsINVALID_FORMATdetail points at the binding map itself. A detail path over the contract's 512-character limit also points at the map.SecretBindingValidationErrors, such as cross-Namespace references, keep their message and have no details.revisionSecretBindings) and Kubernetes Compute both catchnormalizeSecretBindingsfailures and replace the message with their own static result, so their behavior is unchanged. Preset validation keeps its own static message.path. TheErrorDetailschema (path,code) is unchanged, so no OpenAPI change.Docs:
docs/reference/agent-plugins.md(failure list),docs/flows/configuration-driver.md(binding admission), and a changelog line indocs/flows/agent-provisioning.md.Verification
Review (opus): APPROVE WITH NITS. The third commit (
cf4228d68) addresses the message order, malformed-key echo and test-gap nits. The delta re-review found two stale flow-doc lines, fixed inacb334cd5.node --test tests/conformance/http-errors.test.mjs tests/conformance/plugin-driver.test.mjs: 104/104 pass.tests/conformance/presets.test.mjspasses too. A new http-errors case drivesnormalizeRequestSecretBindingsthroughrequestFailurefor a reserved prefix, a reserved name (provisioning path) and a malformed key (INVALID_FORMATat the map), and checks that the Secret ID is never in the failure.node --test tests/integration/secret-api.test.mjs tests/integration/occ-api.test.mjs: 75/75 pass.apps/andpackages/changes reverted, the extended tests fail. Seven tests fail:HTTP error mapping: a plugin selection the selected Plugin Driver does not offerHTTP error mapping: a reserved Secret binding destinationPlugin Drivers name themselves when a selection names a plugin they do not offerAgent plugin maps reject structural errors…(the PATCH with an OCC and a Codex selection gets the ID and/plugins/<id>detail)Configuration and Agent writes reject invalid Secret bindings as invalid requestsAgent provisioning API validates inline configuration with existing Secret referencesHarness source admission rejects foreign references…(nine reserved destinations: exact message,/secretBindings/<key>detail, and no Secret ID in the body)tests/conformanceandtests/integrationrun: 2915 pass, 423 skipped, 18 fail. The 18 are the Podman/dev-up, CLI release, and repository-credential container and Git suites. The same files fail on amaintree on this host (16/81 in those files), so they are environment failures, not this change.tsc --buildclean;eslint --max-warnings 0andprettier --checkon touched files clean.Open PRs touching the same files
I ran
gh pr diff --name-onlyacross all 94 open PRs, then comparedgit merge-treeagainst each overlapping PR for this head and fororigin/main, by file and by hunk:drivers/plugin/index.ts,occ/src/errors.tsplugin/index.ts)drivers/plugin/index.ts,agent-plugins.md,plugin-driver.test.mjsplugin/index.ts,agent-plugins.md)runtime-translator.ts,plugin-driver.test.mjshttp/errors.ts,occ/src/errors.tshttp/errors.ts)secret-bindings.ts,occ/src/errors.tssecret-bindings.ts,occ/src/errors.ts)http/errors.ts,occ/src/errors.tsagent-provisioning.md,occ-api.test.mjsconfiguration-driver.mdocc-api.test.mjs(#115 alsoplugin-driver.test.mjs)occ-api.test.mjs)occ-api.test.mjsThis PR adds no new conflicted files or hunks. An earlier revision conflicted with #914 and #1370 in flow docs; the second commit moves those notes out of their hunks.
Risks
A secret binding uses a reserved or invalid environment destination.) no longer match. Nothing in the repository (console,occ, docs) matches it. The unknown-plugin message keeps its full original prefix,A plugin selection names a plugin that the selected Plugin Driver (<id>) does not offer.console/api-client.mjs) drops paths containing:,@or., so it ignores the plugin detail. The message still names the ID, and the console is unchanged.plugins, the rejected selections come from the stored Agent. In that case/plugins/<id>points at the Agent'spluginsfield rather than the request body.