fix(infra): restore symlink rejection in tryReadSecretFileSync by RomneyDa · Pull Request #84711 · openclaw/openclaw

RomneyDa · 2026-05-20T21:53:43Z

Summary

Drop the swallow-all try/catch wrapper around tryReadSecretFileSync so the upstream @openclaw/fs-safe@0.2.7 contract surfaces: undefined for blank/missing/not-found, FsSafeError for symlink/oversize/non-regular/hardlink.
Unblocks the infra-state CI shard's throws from the try helper for rejected files case.
Closes the production gap: rejectSymlink: true callers (Telegram, LINE, Zalo, IRC, Nextcloud Talk credential loaders) fail closed on symlinked credentials again.
Align stale plugin caller tests with the same fail-closed contract: 5 runtime resolvers now expect a thrown FsSafeError; inspectTelegramAccount catches at its boundary and reports configured_unavailable (matching its existing return type).

Behavior change to flag

Operators with a symlinked credential file for Telegram/LINE/Zalo/IRC/Nextcloud Talk will now see a startup-time FsSafeError (clear "must not be a symlink" message) instead of the channel silently appearing as "no credentials configured." This is the intended fs-safe 0.2.7 contract and matches the docs claim at docs/channels/telegram.md:1065 that tokenFile symlinks are rejected; inspectTelegramAccount (status/doctor surfaces) still reports configured_unavailable rather than throwing.

Verification

node scripts/run-vitest.mjs src/infra/secret-file.test.ts extensions/zalo/src/token.test.ts extensions/line/src/accounts.test.ts extensions/telegram/src/token.test.ts extensions/irc/src/accounts.test.ts extensions/telegram/src/account-inspect.test.ts extensions/nextcloud-talk/src/setup.test.ts → 7 files, 87 tests passing.

ref 9e4eca0

The local wrapper added in 9e4eca0 swallowed all errors from @openclaw/fs-safe@0.2.7's tryReadSecretFileSync via a bare try/catch, silently downgrading every rejectSymlink: true caller (Telegram, LINE, Zalo, IRC, Nextcloud Talk credential files) to accept symlinked credential files. It also broke the infra-state CI shard's symlink expectation that #84595 had just realigned with the new fail-closed upstream contract. Restore the direct re-export so the upstream contract surfaces: undefined for blank/missing/not-found, FsSafeError for symlink, oversize, non-regular file, and hardlink validation failures.

clawsweeper · 2026-05-20T21:55:19Z

Codex review: needs maintainer review before merge.

Workflow note: Future ClawSweeper reviews update this same comment in place.

How this review workflow works

ClawSweeper keeps one durable marker-backed review comment per issue or PR.
Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
Maintainers can also comment @clawsweeper review to request a fresh review only.
Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

Summary
The branch removes OpenClaw's local catch-all tryReadSecretFileSync wrapper, re-exports the @openclaw/fs-safe helper directly, and adds a changelog entry for fail-closed secret-file validation.

Reproducibility: yes. Current main source catches all upstream tryReadSecretFileSync errors while the focused test expects a symlink rejection to throw; I did not run the test because this review had to keep the checkout read-only.

PR rating
Overall: 🦞 diamond lobster
Proof: 🌊 off-meta tidepool
Patch quality: 🦞 diamond lobster
Summary: Patch quality is above average: it is small, aligns with the upstream contract, has focused claimed validation, and has no blocking findings.

What the crustacean ranks mean

🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Real behavior proof
Not applicable: This is a maintainer/member PR with the protected maintainer label, so the external-contributor real behavior proof gate does not apply.

Risk before merge

Merging intentionally changes upgrade behavior for symlinked credential file paths: affected channel plugins can now fail closed instead of treating the rejected file as missing or falling back.
This read-only review did not execute the PR's Vitest command, so CI or maintainer local proof should confirm the focused secret-file shard before merge.

Maintainer options:

Accept fail-closed credentials (recommended)
Merge after maintainer accepts that symlinked credential file paths for affected plugins will now throw instead of being treated as missing or falling back.
Preserve compatibility first
If that upgrade break is not acceptable, keep the current missing-file behavior by default and add an explicit strict mode or migration warning before enforcing throws.
Pause if CI points elsewhere
If the infra-state failure is not the catch-all wrapper, pause this PR and narrow the fix to the actual failing contract.

Next step before merge
No repair is needed; maintainer review should accept the fail-closed credential compatibility impact and let CI or a focused local run prove the shard before merge.

Security
Cleared: The diff restores the dependency's fail-closed secret-file contract and does not add new dependency, execution, permission, or secret-handling surface.

Review details

Best possible solution:

Land the direct upstream re-export after maintainer acceptance of the deliberate fail-closed credential behavior and focused CI proof.

Do we have a high-confidence way to reproduce the issue?

Yes. Current main source catches all upstream tryReadSecretFileSync errors while the focused test expects a symlink rejection to throw; I did not run the test because this review had to keep the checkout read-only.

Is this the best way to solve the issue?

Yes. Re-exporting the upstream helper is the narrowest maintainable fix because OpenClaw's wrapper duplicated and weakened the @openclaw/fs-safe contract that callers already opted into with rejectSymlink: true.

Label changes:

add P1: The PR addresses a current-main regression in security-sensitive credential loading and an infra-state CI shard expectation.
add merge-risk: 🚨 compatibility: Existing setups that configured symlinked credential files may stop working because the helper will throw instead of returning undefined.
add merge-risk: 🚨 auth-provider: The affected code path controls channel credential resolution for Telegram, LINE, Zalo, IRC, and Nextcloud Talk.
add rating: 🦞 diamond lobster: Current PR rating is 🦞 diamond lobster because proof is 🌊 off-meta tidepool, patch quality is 🦞 diamond lobster, and Patch quality is above average: it is small, aligns with the upstream contract, has focused claimed validation, and has no blocking findings.
add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: This is a maintainer/member PR with the protected maintainer label, so the external-contributor real behavior proof gate does not apply.

Label justifications:

P1: The PR addresses a current-main regression in security-sensitive credential loading and an infra-state CI shard expectation.
merge-risk: 🚨 compatibility: Existing setups that configured symlinked credential files may stop working because the helper will throw instead of returning undefined.
merge-risk: 🚨 auth-provider: The affected code path controls channel credential resolution for Telegram, LINE, Zalo, IRC, and Nextcloud Talk.
rating: 🦞 diamond lobster: Current PR rating is 🦞 diamond lobster because proof is 🌊 off-meta tidepool, patch quality is 🦞 diamond lobster, and Patch quality is above average: it is small, aligns with the upstream contract, has focused claimed validation, and has no blocking findings.
status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: This is a maintainer/member PR with the protected maintainer label, so the external-contributor real behavior proof gate does not apply.

What I checked:

Protected PR state: The provided GitHub context lists authorAssociation: MEMBER and the maintainer label, so this cleanup review must not auto-close the PR even when the patch looks correct.
Current main swallows fs-safe rejections: Current main wraps tryReadSecretFileSyncImpl in a broad try/catch and returns undefined for every thrown error, including policy errors from fs-safe. (src/infra/secret-file.ts:17, 384451343191)
Current test contract expects symlink rejection to throw: The focused secret-file test already expects tryReadSecretFileSync(..., { rejectSymlink: true }) to throw for a symlinked credential file. (src/infra/secret-file.test.ts:114, 384451343191)
Upstream dependency contract: The @openclaw/fs-safe@0.2.7 tarball returns undefined only for blank or not-found inputs and throws FsSafeError for other rejected outcomes such as symlink, non-file, too-large, hardlink, or path mismatch. (@openclaw/fs-safe@0.2.7:dist/secret-file.js:136)
Affected credential callers: Telegram, LINE, Zalo, IRC, and Nextcloud Talk credential loaders all call the public helper with rejectSymlink: true, so the wrapper changes real credential-loading behavior rather than only a unit-test helper. (extensions/telegram/src/token.ts:149, 384451343191)
PR head restores direct dependency export: The PR head re-exports tryReadSecretFileSync from @openclaw/fs-safe/secret and removes the local catch-all wrapper. (src/infra/secret-file.ts:5, 46495863d01d)

Likely related people:

Kevin Lin: git blame shows the broad catch-all wrapper on current main was added in commit 9e4eca0. (role: introduced current wrapper behavior; confidence: high; commits: 9e4eca00ff0f; files: src/infra/secret-file.ts)
Ayaan Zaidi: Commit c0ac456 created the secret-file wrapper module, public SDK exports, and initial focused tests for this helper surface. (role: introduced secret-file helper surface; confidence: high; commits: c0ac4564f787; files: src/infra/secret-file.ts, src/infra/secret-file.test.ts, src/plugin-sdk/secret-file-runtime.ts)
Xu Xiang: Commit d5cc0d5 changed the secret-file test to expect the try helper to throw for rejected symlink files. (role: recent test contract contributor; confidence: high; commits: d5cc0d53b7e3; files: src/infra/secret-file.test.ts)

Codex review notes: model gpt-5.5, reasoning high; reviewed against 384451343191.

clawsweeper · 2026-05-20T22:01:43Z

ClawSweeper PR egg

✨ Hatched: 🌱 uncommon Sunspot Branchling

Hatch command

Comment @clawsweeper hatch when this PR is hatchable.

Hatchability rules:

Merged PRs are hatchable.
Open PRs are hatchable when they are status: 👀 ready for maintainer look, status: 🚀 automerge armed, or labeled clawsweeper:automerge.
Closed unmerged PRs are hatchable only when one of those hatchable labels is still present in the durable record.

Rarity: 🌱 uncommon.
Trait: sleeps inside passing CI.
Image traits: location proof lagoon; accessory shell-shaped keyboard; palette plum, gold, and soft gray; mood watchful; pose standing beside its cracked shell; shell polished stone shell; lighting golden review-room light; background small green status lights.
Share on X: post this hatch
Copy: My PR egg hatched a 🌱 uncommon Sunspot Branchling in ClawSweeper.

What is this egg doing here?

Eggs appear after the PR passes real-behavior proof. It is here for vibes, not verdicts: it does not change labels, ratings, merge decisions, or automation.
The shell reacts to review momentum: open follow-up work warms it up, re-review makes it wobble, and a clean final review lets it hatch.
Hatchability usually comes from sufficient real-behavior proof, no blocking P0/P1/P2 findings, no security attention needed, and clean correctness. A merged PR is already final, so merge makes the egg hatchable independently.
The hatch is seeded from this repository and PR number, so the same PR keeps the same creature; the reviewed head SHA can only change safe visual details.
Rarity is just collectible sparkle: 🥚 common, 🌱 uncommon, 💎 rare, ✨ glimmer, and 🌈 legendary.

5 token/account resolver tests still asserted the pre-fs-safe-0.2.7 "silent skip" behavior (token: "", source: "none") on rejected symlinks; they passed only because the swallow-all wrapper in secret-file.ts hid the throw. Restoring the upstream fail-closed contract surfaces the throw, so update the tests to expect FsSafeError. inspectTelegramAccount reports credential status (its return type has an explicit configured_unavailable state for "configured but unreadable"), so its callsite is the right boundary to catch the FsSafeError and map it to configured_unavailable rather than letting the throw bubble. Affected: - extensions/zalo/src/token.test.ts - extensions/line/src/accounts.test.ts - extensions/telegram/src/token.test.ts - extensions/irc/src/accounts.test.ts - extensions/nextcloud-talk/src/setup.test.ts - extensions/telegram/src/account-inspect.ts (catch + report status)

RomneyDa · 2026-05-20T22:14:42Z

@clawsweeper please reevaluate labels

clawsweeper · 2026-05-20T22:16:34Z

🦞👀
ClawSweeper is taking a look at your question.

I asked ClawSweeper to answer this maintainer mention in the next review comment. Tiny claws, bounded scope: this is a read-only assist pass unless it produces one of the existing structured safe-action markers.

Request: please reevaluate labels

…spect catch (#84713) Followup nits from the #84711 review: - Narrow the inspectTokenFile catch in extensions/telegram/src/account-inspect.ts to FsSafeError so only fs-safe validation throws map to configured_unavailable; any other throw (programmer error, unexpected I/O) is rethrown. - Add a regression test for the IRC NickServ password file symlink rejection path (extensions/irc/src/accounts.ts:118), paralleling the existing top-level passwordFile test. - Add a regression test for the Telegram account-level tokenFile symlink rejection path (extensions/telegram/src/token.ts:149), paralleling the existing channel-level tokenFile test. Behavior was already correct after #84711; this just locks coverage and tightens the catch.

…law#84711) * fix(infra): restore symlink rejection in tryReadSecretFileSync The local wrapper added in 9e4eca0 swallowed all errors from @openclaw/fs-safe@0.2.7's tryReadSecretFileSync via a bare try/catch, silently downgrading every rejectSymlink: true caller (Telegram, LINE, Zalo, IRC, Nextcloud Talk credential files) to accept symlinked credential files. It also broke the infra-state CI shard's symlink expectation that openclaw#84595 had just realigned with the new fail-closed upstream contract. Restore the direct re-export so the upstream contract surfaces: undefined for blank/missing/not-found, FsSafeError for symlink, oversize, non-regular file, and hardlink validation failures. * test(plugins): align stale symlink tests with fail-closed contract 5 token/account resolver tests still asserted the pre-fs-safe-0.2.7 "silent skip" behavior (token: "", source: "none") on rejected symlinks; they passed only because the swallow-all wrapper in secret-file.ts hid the throw. Restoring the upstream fail-closed contract surfaces the throw, so update the tests to expect FsSafeError. inspectTelegramAccount reports credential status (its return type has an explicit configured_unavailable state for "configured but unreadable"), so its callsite is the right boundary to catch the FsSafeError and map it to configured_unavailable rather than letting the throw bubble. Affected: - extensions/zalo/src/token.test.ts - extensions/line/src/accounts.test.ts - extensions/telegram/src/token.test.ts - extensions/irc/src/accounts.test.ts - extensions/nextcloud-talk/src/setup.test.ts - extensions/telegram/src/account-inspect.ts (catch + report status)

…spect catch (openclaw#84713) Followup nits from the openclaw#84711 review: - Narrow the inspectTokenFile catch in extensions/telegram/src/account-inspect.ts to FsSafeError so only fs-safe validation throws map to configured_unavailable; any other throw (programmer error, unexpected I/O) is rethrown. - Add a regression test for the IRC NickServ password file symlink rejection path (extensions/irc/src/accounts.ts:118), paralleling the existing top-level passwordFile test. - Add a regression test for the Telegram account-level tokenFile symlink rejection path (extensions/telegram/src/token.ts:149), paralleling the existing channel-level tokenFile test. Behavior was already correct after openclaw#84711; this just locks coverage and tightens the catch.

…law#84711) * fix(infra): restore symlink rejection in tryReadSecretFileSync The local wrapper added in 9e4eca0 swallowed all errors from @openclaw/fs-safe@0.2.7's tryReadSecretFileSync via a bare try/catch, silently downgrading every rejectSymlink: true caller (Telegram, LINE, Zalo, IRC, Nextcloud Talk credential files) to accept symlinked credential files. It also broke the infra-state CI shard's symlink expectation that openclaw#84595 had just realigned with the new fail-closed upstream contract. Restore the direct re-export so the upstream contract surfaces: undefined for blank/missing/not-found, FsSafeError for symlink, oversize, non-regular file, and hardlink validation failures. * test(plugins): align stale symlink tests with fail-closed contract 5 token/account resolver tests still asserted the pre-fs-safe-0.2.7 "silent skip" behavior (token: "", source: "none") on rejected symlinks; they passed only because the swallow-all wrapper in secret-file.ts hid the throw. Restoring the upstream fail-closed contract surfaces the throw, so update the tests to expect FsSafeError. inspectTelegramAccount reports credential status (its return type has an explicit configured_unavailable state for "configured but unreadable"), so its callsite is the right boundary to catch the FsSafeError and map it to configured_unavailable rather than letting the throw bubble. Affected: - extensions/zalo/src/token.test.ts - extensions/line/src/accounts.test.ts - extensions/telegram/src/token.test.ts - extensions/irc/src/accounts.test.ts - extensions/nextcloud-talk/src/setup.test.ts - extensions/telegram/src/account-inspect.ts (catch + report status)

…spect catch (openclaw#84713) Followup nits from the openclaw#84711 review: - Narrow the inspectTokenFile catch in extensions/telegram/src/account-inspect.ts to FsSafeError so only fs-safe validation throws map to configured_unavailable; any other throw (programmer error, unexpected I/O) is rethrown. - Add a regression test for the IRC NickServ password file symlink rejection path (extensions/irc/src/accounts.ts:118), paralleling the existing top-level passwordFile test. - Add a regression test for the Telegram account-level tokenFile symlink rejection path (extensions/telegram/src/token.ts:149), paralleling the existing channel-level tokenFile test. Behavior was already correct after openclaw#84711; this just locks coverage and tightens the catch.

…law#84711) * fix(infra): restore symlink rejection in tryReadSecretFileSync The local wrapper added in 9e4eca0 swallowed all errors from @openclaw/fs-safe@0.2.7's tryReadSecretFileSync via a bare try/catch, silently downgrading every rejectSymlink: true caller (Telegram, LINE, Zalo, IRC, Nextcloud Talk credential files) to accept symlinked credential files. It also broke the infra-state CI shard's symlink expectation that openclaw#84595 had just realigned with the new fail-closed upstream contract. Restore the direct re-export so the upstream contract surfaces: undefined for blank/missing/not-found, FsSafeError for symlink, oversize, non-regular file, and hardlink validation failures. * test(plugins): align stale symlink tests with fail-closed contract 5 token/account resolver tests still asserted the pre-fs-safe-0.2.7 "silent skip" behavior (token: "", source: "none") on rejected symlinks; they passed only because the swallow-all wrapper in secret-file.ts hid the throw. Restoring the upstream fail-closed contract surfaces the throw, so update the tests to expect FsSafeError. inspectTelegramAccount reports credential status (its return type has an explicit configured_unavailable state for "configured but unreadable"), so its callsite is the right boundary to catch the FsSafeError and map it to configured_unavailable rather than letting the throw bubble. Affected: - extensions/zalo/src/token.test.ts - extensions/line/src/accounts.test.ts - extensions/telegram/src/token.test.ts - extensions/irc/src/accounts.test.ts - extensions/nextcloud-talk/src/setup.test.ts - extensions/telegram/src/account-inspect.ts (catch + report status)

…spect catch (openclaw#84713) Followup nits from the openclaw#84711 review: - Narrow the inspectTokenFile catch in extensions/telegram/src/account-inspect.ts to FsSafeError so only fs-safe validation throws map to configured_unavailable; any other throw (programmer error, unexpected I/O) is rethrown. - Add a regression test for the IRC NickServ password file symlink rejection path (extensions/irc/src/accounts.ts:118), paralleling the existing top-level passwordFile test. - Add a regression test for the Telegram account-level tokenFile symlink rejection path (extensions/telegram/src/token.ts:149), paralleling the existing channel-level tokenFile test. Behavior was already correct after openclaw#84711; this just locks coverage and tightens the catch.

…law#84711) * fix(infra): restore symlink rejection in tryReadSecretFileSync The local wrapper added in d024283 swallowed all errors from @openclaw/fs-safe@0.2.7's tryReadSecretFileSync via a bare try/catch, silently downgrading every rejectSymlink: true caller (Telegram, LINE, Zalo, IRC, Nextcloud Talk credential files) to accept symlinked credential files. It also broke the infra-state CI shard's symlink expectation that openclaw#84595 had just realigned with the new fail-closed upstream contract. Restore the direct re-export so the upstream contract surfaces: undefined for blank/missing/not-found, FsSafeError for symlink, oversize, non-regular file, and hardlink validation failures. * test(plugins): align stale symlink tests with fail-closed contract 5 token/account resolver tests still asserted the pre-fs-safe-0.2.7 "silent skip" behavior (token: "", source: "none") on rejected symlinks; they passed only because the swallow-all wrapper in secret-file.ts hid the throw. Restoring the upstream fail-closed contract surfaces the throw, so update the tests to expect FsSafeError. inspectTelegramAccount reports credential status (its return type has an explicit configured_unavailable state for "configured but unreadable"), so its callsite is the right boundary to catch the FsSafeError and map it to configured_unavailable rather than letting the throw bubble. Affected: - extensions/zalo/src/token.test.ts - extensions/line/src/accounts.test.ts - extensions/telegram/src/token.test.ts - extensions/irc/src/accounts.test.ts - extensions/nextcloud-talk/src/setup.test.ts - extensions/telegram/src/account-inspect.ts (catch + report status)