Conversation
Config paths whose segment is nsec, and paths that contain webhookUrl, now use the existing password field that does not preload the stored value. - Add ConfigPathSensitivity and route SchemaConfigEditor.IsSensitive through it - Cover nostr nsec, googlechat webhookUrl, and the existing secret names Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
|
🦞👀 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. |
|
Codex review: needs real behavior proof before merge. Reviewed September 30, 2026, 4:49 PM ET / 20:49 UTC (Revision 25). ClawSweeper reviewWhat this changesThe Windows Config editor masks Nostr keys and webhook URLs, hides stored sensitive arrays and objects, and adds blank replacement and confirmed-clear controls. Merge readiness⛔ Blocked before merge - 3 items remain Current main still exposes these config fields through ordinary controls. The branch addresses that behavior, and its latest commit appears to repair the prior staged-replacement finding. The remaining blocker is current-head running-UI proof of that final cancel-and-save sequence. Priority: P2 Review scores
Verification
How this fits togetherThe Windows tray Config page receives configuration and schema data from the Gateway, builds editable controls, and sends saved changes back to the Gateway. This PR changes how that page displays and edits values classified as sensitive. flowchart LR
A[Gateway config] --> C[Config page]
B[Config schema] --> C
C --> D{Sensitive field?}
D -->|Yes| E[Hidden value editor]
D -->|No| F[Ordinary controls]
E --> G[Validated changes]
F --> G
G --> H[Gateway config save]
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep the agreed hidden-value editing contract and verify the final replacement decision reaches the Gateway save request on the current head. Do we have a high-confidence way to reproduce the issue? Yes, from source: current main does not classify the reported path segments as sensitive and sends them to ordinary controls. I did not run the Windows app in this read-only review. Is this the best way to solve the issue? Yes, the branch follows the maintainer's bounded hidden-value editing decision. Its final cancel-and-save behavior still needs current-head UI confirmation. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 04a880fe9488. LabelsLabel changes: No label changes. Label justifications:
EvidenceWhat 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 (24 earlier review cycles; latest 8 shown)
|
|
Hi @SebTardif, Copilot here helping Scott triage your OpenClaw fixes. Thank you for the careful follow-ups and for keeping the repairs on the original PR branches. There are now 23 open PRs from this batch, and we want to give each one a fair, timely review rather than let useful fixes get lost in the queue. Could you focus on closing ClawSweeper's current-head, changed-behavior proof requests on a smaller set before adding more follow-ups? A concise proof on the PR itself is ideal: exact head SHA, the smallest full required build/Shared/Tray and focused test counts, and a redacted real-path observation of the behavior the patch changes. Please mark anything unavailable as For this PR, the next decisive item is an isolated running Config page view showing stored Nostr |
|
Global triage: HOLD_FOR_AUTHOR. Take confidence 15%; recommendation confidence 99%; effort small code fix plus medium proof; risk medium. The real Nostr credential path remains unmasked. The classifier recognizes an exact Please classify the exact The failed E2E lanes reproduce on the exact base in shared Gateway restart setup; CI Gate is derivative. Core, Tray, and UI/accessibility checks passed at this head, but the required exact build, Shared, and Tray closeout results are not reported in the PR body. |
The editor treated an exact nsec segment as secret, but the Gateway property is channels.nostr.privateKey. That stored value still loaded into a plain text box. Match an exact privateKey segment the same way as nsec. A longer name such as privateKeyExtra stays plain. Validation: ./build.ps1 exit 0. Shared 4107 passed, 32 skipped, 0 failed (4139 total). ConfigPathSensitivityTests 19 passed. Tray 3086 passed and 5 failed (3091 total). Those five are the existing LF source checks. Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
|
Maintainer review at exact head The current-head screenshot and redacted patch trace are useful proof for the two scalar fields shown: There is one concrete in-scope blocker before this can reach the requested >=90% landing confidence:
Please make sensitivity control rendering, not only classification:
Validation on this exact head:
No maintainer patch or commit was added, so Seb’s authorship and branch remain unchanged. |
A sensitive string array, including webhookUrls, uses a password box and does not copy the stored value into it. A blank box keeps the existing item. The schema JSON view and the fallback array view do the same. ./build.ps1 exit 0. Shared tests: 4107 passed, 32 skipped. Tray tests: 3087 passed, 5 failed on the LF source-contract mismatch tracked in openclaw#1518. ConfigPathSensitivityTests: 20 passed. Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
|
@SebTardif, thank you. The Please address two bounded gaps on this original branch:
Please keep this correction in the existing editor/classifier/model/test scope. Our broader local preview-redaction/localization/fixture experiment was stopped and has not been pushed; it is not a request to reproduce that expansion. No redesign or new service is needed for this follow-up. After the fix, include the exact head, focused regression results plus the required build/Shared/Tray floor, and a redacted isolated UI proof that actually applies/saves a replacement and exercises confirmed clear/cancel with dummy values. Existing green CI is useful but does not cover these two paths. We will re-review this original PR once those targeted fixes are present. |
Sensitive schema objects, and schema-less sensitive objects, use the same blank replace and confirmed clear editor as complex arrays. Stored values stay off the control. Replace all checks the JSON kind and each array item kind before staging. Invalid input keeps the original value and blocks Save. Cancelling Clear all drops the abandoned validation error. ./build.ps1 exit 0. Shared 4107 passed, 32 skipped. Tray 3100 passed, 5 failed on the known LF source contracts (openclaw#1518). Focused editor and model tests passed. Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
A classified object with a properties map was expanded into child controls before the hidden editor ran. Decide that case first, keep the stored children off the page, and cover the properties schema. Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
A sensitive object stays hidden when its schema declares properties. Bundle Id was not on the page. Saving the dummy replacement and the confirmed clear both reached the patch. Cancelling Clear all cleared the validation error from the rejected item, and the stored marker was absent from the changed fields. Head On the isolated Config page, Webhook URLs and Secret Bundle each opened as one hidden entry. Saving CI on this head is still running. |
Shared failed only ExtractTarBz2Async_CancellationIsBoundedAndKillsExtractor because the extractor process was still running. That test passed locally. Revocation recovery failed in wsl-create: the Ubuntu-24.04 install exited -1 with no output after 900 seconds. The tree matches 0a3a1c6. Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
|
The red checks on the previous head are not this config change. Shared failed one cancellation test because the piper extractor process was still running, and that same test passed locally. Revocation recovery stopped while creating the WSL instance: the Ubuntu install exited -1 with no output after 900 seconds. Head CI on this head is still running. |
|
Head
This pull already used its empty retrigger, so this head stays. |
…celed A valid Replace all stayed on the edit session after a later invalid draft was rejected. Canceling Clear all cleared the error and left the pending change removed, so a later save kept the stored secret. Cancel clear now stages the committed replacement again. SensitiveArray_RejectedRetryThenCanceledClear_RestoresCommittedReplacement passed. ./build.ps1 exit 0. Shared 4107 passed, 32 skipped. Tray 3102 passed and 5 failed, the LF source-contract mismatch in openclaw#1518. Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
What Problem This Solves
Fixes: the config editor shows a Nostr private key and a Google Chat webhook URL in a normal text box.
User Impact
User impact: those fields use the same password box as other secrets, and a stored value is not preloaded. A stored secret object that declares properties shows how many entries are configured. Replace all starts blank. Clear all asks before it removes the entries.
Why This Change Was Made
The schema editor and the no-schema fallback share
ConfigPathSensitivity. An exact path segment namednsec,privateKey,webhookUrl, orwebhookUrlstakes the existing sensitive field.webhookUrlExtrastays a normal field. A sensitive string array uses a password box. The stored item is kept by the editor, not on the control. A blank box keeps that item.A sensitive array of objects stays off the page. The page shows how many entries are configured. Replace all opens a blank JSON box and the patch contains only that new array. Clear all asks, then sends an empty array. Cancel, or a save that does not use those actions, leaves the stored array out of the patch. The no-schema fallback uses the same editor.
A sensitive object uses that same hidden editor, including when its schema declares properties. Child titles and stored children stay off the page. An object that is not itself sensitive still expands. A sensitive string stays a password field. Replace all checks the expected JSON kind before it stages a value. A wrong kind leaves the stored value in place and keeps Save disabled. Cancelling that replacement, or cancelling Clear all, clears the blocking validation state.
Evidence
Terminal output from commit
4184c01c.Head
92b1cd635d83f3f038f42f18ceea4d6559b21c5b.ConfigPathSensitivityTests: 22 passed, includingchannels.slack.webhookUrls,channels.googlechat.webhookUrlExtraas plain, andSchemaEditor_HidesStoredValuesInSensitiveArrays. The parent capture below is29faa6b2.The running Config page on this head, after Display Name was changed to
after-edit, Save, and Refresh:Display Name, both Webhook Urls rows, Webhook Url Extra, and Show JSON are inside this frame. The two stored array items show
Leave blank to keep existing valueand do not show the stored URLs. Webhook Url Extra showsbenign-near-match. Show JSON is visible, so the JSON preview is collapsed. The stored array strings are not on screen.Redacted patch trace from the same run:
This was a loopback stub on
127.0.0.1:18794, not a product OpenClaw gateway. The earlier full-frame capture6aa74002is parent7e488a40. That run did not include an empty array entry.Head
92b1cd635d83f3f038f42f18ceea4d6559b21c5bkeeps a loaded empty string in the array. The same page, after Display Name was changed toafter-editand Save:Both rows show
Leave blank to keep existing value. The stored URL is not on screen. Show JSON is visible.This was a loopback stub on
127.0.0.1:18795, not a product OpenClaw gateway.Head
26cf9b9fd8fe22ad05550afe215a7b5d3b2c4999was the read-only note. Headb3c02a5bc0f9a3e37da143b860a0bae4d08e85cereplaces that dead end with Replace all and Clear all. The stored objects are still not copied into the editor.Change Type
Scope
winnodeRequired proof pools
windows-winui-interactive: isolated tray on headb2c836d4b9dae9d24fa430e0c8633cc2ac206ac3, Config page. That head is an empty retrigger, and the tree matches0a3a1c67c876c281efba0739bc088bd9f23e0437. A sensitive array of objects and a sensitive object that declares properties each show a count. The capture applies a replacement, saves it, confirms clear, and cancels a clear. Stored values stay off the page.Validation
b2c836d4b9dae9d24fa430e0c8633cc2ac206ac3. Empty retrigger. The tree matches0a3a1c67c876c281efba0739bc088bd9f23e0437../build.ps1: exit 0.UseHiddenObjectEditor_SensitiveObjectWithProperties_StaysHidden,ArrayItemKinds_RejectsTheWrongJsonKind,ArrayItemKinds_AcceptsObjectAndStringReplacements,SensitiveObject_UnrelatedEditAndCancel_PreserveStoredObject, andSensitiveObject_ReplaceSendsOnlyTheNewObject_ClearSendsEmptyObject.webhookUrlExtrastays plain.Head
3f81db4d114cba09c3bb2cb66c8fb113bf511152:./build.ps1: exit 0.SensitiveArray_RejectedRetryThenCanceledClear_RestoresCommittedReplacementpassed.SchemaEditor_HidesStoredValuesInSensitiveArrayspassed.Real Behavior Proof
b2c836d4b9dae9d24fa430e0c8633cc2ac206ac3(empty retrigger, same tree as0a3a1c67c876c281efba0739bc088bd9f23e0437). Loopback stubws://127.0.0.1:18796. RegistryisLocalfalse. Fake shared token. Deep linkopenclaw://hub/config../build.ps1, Shared, and Tray for this head are in Validation above.["not-an-object"]to Webhook URLs. Opened Clear all and chose Cancel. Applied[{"url":"https://example.invalid/hook-dummy"}]to Webhook URLs and{"id":"replacement-dummy"}to Secret Bundle, then saved. After reconnect, confirmed Clear all on Webhook URLs and saved again.webhookUrlsto the dummy array andsecretBundleto{"id":"replacement-dummy"}. The second patch setwebhookUrlsto[]and leftsecretBundleas that dummy object. The stored marker was absent from both changed fields. Display Name stayedafter-edit. Webhook URL Extra stayedbenign-near-match.b2c836d4b9dae9d24fa430e0c8633cc2ac206ac3tree, captured on0a3a1c67. The three frames after those are heada18cd305, where Apply, the confirmation Clear all, and Save were not clicked. The pictures in Evidence are earlier heads. The password-field capture6aa74002on7e488a40was not recaptured.127.0.0.1:18796. The rejected-retry then canceled-clear restore was not opened on the Config page. The session test keeps the committed replacement, and Cancel clear stages that replacement again instead of dropping it.Security Impact
NoYesNoNoNoYes, explain the risk and mitigation: Stored objects are not copied into the editor, including a sensitive object whose schema declares properties. Replace all sends only the newly entered JSON after the item kind checks. Clear all sends an empty array or an empty object after confirmation. A save that does not use those actions leaves the stored value out of the patch. A rejected replacement keeps Save disabled.Compatibility and Migration
YesNoNoReview Conversations