Repository navigation
test(configuration): consolidate the save-rule tests - #2604
Conversation
Share one refusal check across Configuration create, update and Preset save, build slot rows from their JSON pointer, and hoist the refusal texts and the Filesystem save fixture. Test names are unchanged.
…e the refusal check The Dedicated Codex apiKey, allowlist and media rows now run on create, update and Preset save. The size, stored-credential and plaintext cases use the shared refusal check.
Keep realistic sibling fields (a proxy's enabled, a process's command and an exec secret source) beside the rows at() builds, take options objects in assertRefused and refusedAtSaveAndDeploy, share storedRowAgent, give the two slot helpers one argument order, write the models-list setting paths out, restore the zero-width space escape, and name the large-roster refusal with literalKey. The two Configuration HTTP baseUrl and models-list tests now use the Filesystem save fixture with a ready Compute, because Preset saves need it; their rules do not depend on Compute.
|
🦞👀 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. |
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
clawsweeper
Bot
added
P3
|
Codex review: needs maintainer review before merge. What this changesThis PR shares integration-test fixtures and assertions while extending refusal cases across Configuration creation, updates, and Preset saves. Example: Saving a model provider with baseUrl "ftp://models.example/v1".
Review scores
ProductKind: Test only · Worth it: Yes · Fix scope: Complete Merge readiness✅ Ready for maintainer review Keep open: this PR usefully consolidates save-rule coverage without changing production behavior; no actionable defect was found, and required CI passed for the reviewed head. Priority: P3 Before mergeNone. FindingsNone. Agent review detailsHow this fits togetherThe integration suite sends authenticated requests through the controller's real HTTP handlers into OCC validation and selected Configuration and Compute Drivers, then checks responses and stored state. flowchart TB
Rows[Refusal cases] --> Helper[Shared save assertions]
Helper --> HTTP[Controller HTTP handlers]
HTTP --> OCC[OCC admission]
OCC --> Rules[Shared and Driver rules]
Rules --> Result[Named refusal or accepted save]
Result --> Checks[Response and storage assertions]
Technical reviewBest possible solution: Retain the shared assertions and expanded save-path coverage while preserving independent expected messages and existing deployment-admission checks. Do we have a high-confidence way to reproduce the issue? This PR consolidates existing regression tests rather than repairing a new product bug; source inspection confirms the added requests reach the real save handlers. Is this the best way to solve the issue? The consolidation is appropriate: it reuses the existing integration boundary, preserves distinct inputs and expected refusals, and adds no production testing seams. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against df01c5fe9fd0. Provenance checked
TestingProof path: in-process harness. SecurityNone. EvidenceWhat I checked:
Likely related people:
Review metrics
LabelsLabel changes:
Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment Reviewed October 16, 2026, 11:41 AM ET / 15:41 UTC. |
|
I found one coverage distinction worth preserving and opened the small follow-up #2606 after this landed. No production change or revert is proposed. I temporarily made the shared model-provider validation depend on a Compute driver being selected. Before this consolidation, two tests correctly caught the API returning The follow-up restores one no-Compute HTTP create/update check using the new helpers. It fails for the deliberate fault, and the complete file on landed main passes 235/235 with the unmodified production code. |
Problem
tests/integration/configuration-controller.test.mjstook 24 commits in five days (#2341-#2436, #2582, #2594) as the Configuration and Preset save rules grew: URL userinfo, baseUrl scheme and slashes, literal secrets in sensitive slots, provider/model selection, size limit, stale generation. Each rule family brought its own fixture, refusal loop and message helpers, and each table ran on whichever save paths its author picked: some on Configuration create only, some on create and Preset save, a few on create, update and Preset save. #2394 and #2396 were exactly that kind of gap: a rule that ran on one save path but not another.Change
Test-only; one file.
refusedOnSave(context, values, message, { item, sentinel })asserts the 400,INVALID_REQUESTand the exact message (and that no answer echoes the sentinel) on Configuration create, an update of an existing Configuration, and a Preset save. Every refusal table now runs through it, so each row is pinned on all three paths:refusedAtSaveAndDeployshares the roster and Kubernetes-settings flow (refused on create and update, an older row still reads, deploy refuses it, an update that keeps it is refused);assertRefused,acceptedOnSave,assertNothingStored,savePreset,saveFixturereplace per-test copies.at("/channels/telegram/botToken", sentinel)replaces 5-15-line nested literals; the pointer also names the refused field.setting,userinfo,literalKey,literalHeader,literalModelKey,absoluteUrl,modelApi,gatewayPassword, approval policy texts) are defined once.models: {}and a record of model rows with a literalx-api-key); "Configuration HTTP refuses a model provider models setting that is not a list" runs both, on the same fixture and on more paths, and its "list first" row moved there too.validateValuesso the model-provider test can save Presets.Verification
model-auth.ts,model-provider-settings.ts, Kubernetesconfiguration-rules.ts,openai-endpoint.ts,configuration-settings.ts,openclaw-roster.ts) plus 20 new ones over the save paths: Preset save skipping the content or Driver rules, create/update skipping the size check, content rules or Driver rules, size checked after content, the 1 MiB limit off by one or doubled, provisioning skipping the size check, and the stale-generation check dropped or reduced to<. Run against this file alone, one at a time, product file restored after each (git statusclean). The killed sets are identical. Mutants are now killed by more tests where a path was thin: "update skips the Driver's rules" 3 -> 113 failing tests, "Preset save skips the content rules" 41 -> 58.eslint --max-warnings 0andprettier --checkpass on the file; the file stays inchecks-baseline-1.json.git merge-treeof this head against Draft implementation: managed outbound HTTPS for dedicated Gateways #2518, Credential Proxy Implementation #2487 (the open PRs touching this file), fix(console): explain when provisioning blocks a Configuration save #2593, feat(console): stop and delete Agents from the Agents list #2524, fix: preserve native plugin permissions across Harness selection and admission #2258, fix(console): grant Agent access to accepted Configuration Secrets #2255, Enable dedicated OpenClaw from Console with pinned native runtime #1685 and fix(compute): stub every model provider and strip request params on workspace-node gateways #830 gives the same conflicted files as against main.Risks
Test-only. Every refusal now also runs on paths it did not before; all hold today. A future rule added on one path only will fail these tables, which is the intent. Probing the deploy path for rows an older release stored found one gap outside this PR: deployment admits a stored model provider baseUrl that every save refuses (
ftp://..., or one slash after the scheme); it is tracked separately and not changed here.