Greptile Summary

This PR fixes a credential-leak bug in skills.update by wrapping the RPC response config in the existing redactConfigObject() utility, mirroring the approach already used by config.get. The fix is minimal (+2 lines), well-tested, and correctly leverages pattern-based guessing (since no uiHints are provided) to redact fields such as apiKey and any env.* keys whose names match sensitive patterns (e.g. GEMINI_API_KEY).

Confidence Score: 5/5

Safe to merge — the redaction logic is correct and the only finding is a minor test assertion style issue.

The production change is a single, well-understood call to an already-proven utility. All remaining feedback is P2 (weak test assertion style) and does not block merge.

No files require special attention.

This is a comment left during a code review.
Path: src/gateway/server-methods/skills.update.normalizes-api-key.test.ts
Line: 91-95

Comment:
**Weak redaction assertions allow false positives**

The `.not.toBe()` checks pass for any value other than the plaintext secret — including `undefined`, `null`, or an empty string — so a bug that silently drops the `apiKey` key or replaces it with `null` would not be caught. Asserting the exact sentinel value would make these tests meaningfully verify the redaction contract.

```suggestion
    expect(config.apiKey).toBe("__OPENCLAW_REDACTED__");
    const env = config.env as Record<string, string>;
    expect(env.GEMINI_API_KEY).toBe("__OPENCLAW_REDACTED__");
```

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "chore(changelog): add entry for skills.u..." | Re-trigger Greptile}

cc @hxy91819 .

Related work from PRtags group knowing-oarfish-aymf

Title: Open PR duplicate: [Bug]: skills.update echoes raw apiKey and env secrets in gateway response

* This PR

Codex automated review: keeping this open.

Keep this PR open. Current main still returns the updated skills.update config object directly, so the linked credential-echo bug remains unfixed on main. The latest PR head is a focused implementation candidate and now addresses the previously raised redacted-sentinel round-trip concern; related duplicate PRs are closed unmerged, and the canonical bug remains open.

Best possible solution:

Keep this PR open. The best path is to finish review on the current head, resolve or explain the failing checks, then land the focused gateway change plus regression tests so it closes #66769. The latest PR head is a better candidate than closing/reopening duplicate attempts because it fixes the main leak and preserves existing secrets when clients round-trip redacted sentinel values.

What I checked:

• Current main still echoes the updated skill config: On current main, config-mode skills.update writes the new config and responds with config: current, so plaintext apiKey and secret-like env values can still be included in the RPC response. (src/gateway/server-methods/skills.ts:344, a35ad200d1d1)
• The request schema includes the secret-bearing fields named by the bug: SkillsUpdateParamsSchema accepts skillKey, optional apiKey, and optional env, matching the leak surface described by the PR and linked bug. (src/gateway/protocol/schema/agents-models-skills.ts:228, a35ad200d1d1)
• Existing redaction infrastructure supports the intended behavior: REDACTED_SENTINEL is the established gateway response redaction value, and the surrounding comment documents write-side restoration expectations. Existing redaction tests already cover skills.entries.*.env.* secret redaction and restoration. (src/config/redact-snapshot.ts:87, a35ad200d1d1)
• Latest PR head is a focused fix candidate: The current PR patch imports redactConfigObject and REDACTED_SENTINEL, returns config: redactConfigObject(current), skips writing the sentinel back over existing apiKey/env secrets, and adds tests for both response redaction and sentinel round-trips. (src/gateway/server-methods/skills.ts:313, 61fc06f33f29)
• PR security review found no supply-chain expansion: The PR file list contains only CHANGELOG.md, src/gateway/server-methods/skills.ts, and the colocated skills.update unit test. It does not touch workflows, dependencies, lockfiles, package metadata, install/build/release scripts, downloaded artifacts, or generated/vendor code. (61fc06f33f29)
• Related work does not supersede this PR: The linked bug [Bug]: skills.update echoes raw apiKey and env secrets in gateway response #66769 is still open. PRtags links fix(gateway): redact apiKey and secret env values in skills.update response #66928 and fix: mask secrets in skills.update response #66986 as related duplicate attempts, but both are closed unmerged, so they do not supersede this active implementation candidate.

Remaining risk / open question:

• The current PR head is not ready to land without CI follow-up: GitHub check-runs report failures in checks-node-core and checks-node-agentic-plugins on 61fc06f33f29804dcc1c3ec90cbd66f1946df714.
• This is a security-sensitive response-shaping change; before merge, maintainers should ensure the failing checks are unrelated or fixed and that the new regression tests run in the intended changed lane.

Codex Review notes: model gpt-5.5, reasoning high; reviewed against a35ad200d1d1.

@codex review

Codex Review: Didn't find any major issues. 🚀

Merged via squash.

• Prepared head SHA: 61fc06f33f29804dcc1c3ec90cbd66f1946df714
• Merge commit: 27ee5c00980101fc7f25a653d70aa830d830afa3

Thanks @Ziy1-Tan!