Skip to content

test(secret-file): cover NickServ + account-level symlinks, narrow inspect catch#84713

Merged
RomneyDa merged 1 commit into
mainfrom
secret-file-symlink-nit-followups
May 20, 2026
Merged

test(secret-file): cover NickServ + account-level symlinks, narrow inspect catch#84713
RomneyDa merged 1 commit into
mainfrom
secret-file-symlink-nit-followups

Conversation

@RomneyDa

@RomneyDa RomneyDa commented May 20, 2026

Copy link
Copy Markdown
Member

Summary

Followup to #84711 picking up the three reviewer nits that didn't block the original merge:

  1. Narrow the new 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) re-raises instead of being silently swallowed.
  2. Add a regression test for the IRC NickServ password file symlink path (extensions/irc/src/accounts.ts:118), paralleling the existing top-level passwordFile symlink test.
  3. Add a regression test for the Telegram account-level tokenFile symlink path (extensions/telegram/src/token.ts:149), paralleling the existing channel-level tokenFile symlink test.

Behavior was already correct after #84711; this just locks coverage on the two latent paths and tightens the catch.

Verification

  • node scripts/run-vitest.mjs extensions/telegram/src/account-inspect.test.ts extensions/telegram/src/token.test.ts extensions/irc/src/accounts.test.ts → 3 files, 40 tests passing.

ref #84711

@RomneyDa
test(secret-file): cover NickServ + account-level symlinks, narrow in…
e2a4814 
…spect catch

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.
@openclaw-barnacle openclaw-barnacle Bot added channel: telegram Channel integration: telegram channel: irc size: S maintainer Maintainer-authored PR labels May 20, 2026
@clawsweeper

clawsweeper Bot commented May 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

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
This PR narrows Telegram account token-file inspection to catch only FsSafeError and adds IRC NickServ plus Telegram account-level symlink regression tests.

Reproducibility: yes. source inspection gives a high-confidence path: current main has rejectSymlink: true on both account-level Telegram token files and IRC NickServ password files, and the PR adds the missing regression assertions. I did not execute the tests in this read-only review.

PR rating
Overall: 🐚 platinum hermit
Proof: 🌊 off-meta tidepool
Patch quality: 🐚 platinum hermit
Summary: Focused patch with clear source fit and reported targeted tests; no blocking defects found, with local test execution intentionally skipped for read-only review.

Rank-up moves:

  • none
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: Real behavior proof is not required here because this is a maintainer-labeled MEMBER PR; the PR body provides targeted test verification.

Risk before merge

  • I did not rerun the targeted Vitest command because this review was read-only; the PR body reports node scripts/run-vitest.mjs extensions/telegram/src/account-inspect.test.ts extensions/telegram/src/token.test.ts extensions/irc/src/accounts.test.ts passing 40 tests.
  • The narrowed catch intentionally lets unexpected non-FsSafeError failures propagate from Telegram account inspection instead of returning configured_unavailable.

Maintainer options:

  1. Decide the mitigation before merge
    Land this focused hardening through the maintainer PR workflow after the normal targeted checks confirm the secret-file contract remains intact.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge
No automated repair is needed; this protected maintainer PR should proceed through normal maintainer review and merge handling.

Security
Cleared: No security or supply-chain regression found; the diff tightens credential-file error handling and adds symlink rejection coverage without new dependencies or workflow changes.

Review details

Best possible solution:

Land this focused hardening through the maintainer PR workflow after the normal targeted checks confirm the secret-file contract remains intact.

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

Yes, source inspection gives a high-confidence path: current main has rejectSymlink: true on both account-level Telegram token files and IRC NickServ password files, and the PR adds the missing regression assertions. I did not execute the tests in this read-only review.

Is this the best way to solve the issue?

Yes. Catching only FsSafeError at the Telegram status boundary and adding platform-gated symlink tests is a narrow, maintainable follow-up to the merged secret-file fix.

Label changes:

  • add P2: The PR is a normal-priority credential-file hardening and regression-coverage follow-up with limited surface area.
  • add rating: 🐚 platinum hermit: Current PR rating is 🐚 platinum hermit because proof is 🌊 off-meta tidepool, patch quality is 🐚 platinum hermit, and Focused patch with clear source fit and reported targeted tests; no blocking defects found, with local test execution intentionally skipped for read-only review.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: Real behavior proof is not required here because this is a maintainer-labeled MEMBER PR; the PR body provides targeted test verification.

Label justifications:

  • P2: The PR is a normal-priority credential-file hardening and regression-coverage follow-up with limited surface area.
  • rating: 🐚 platinum hermit: Current PR rating is 🐚 platinum hermit because proof is 🌊 off-meta tidepool, patch quality is 🐚 platinum hermit, and Focused patch with clear source fit and reported targeted tests; no blocking defects found, with local test execution intentionally skipped for read-only review.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: Real behavior proof is not required here because this is a maintainer-labeled MEMBER PR; the PR body provides targeted test verification.

Acceptance criteria:

  • node scripts/run-vitest.mjs extensions/telegram/src/account-inspect.test.ts extensions/telegram/src/token.test.ts extensions/irc/src/accounts.test.ts

What I checked:

  • Protected PR context: The provided live PR context shows author association MEMBER and label maintainer, which are protected keep-open signals for this cleanup workflow.
  • Patch scope: The PR head only changes three files: one Telegram inspection catch and two focused symlink regression tests. (extensions/telegram/src/account-inspect.ts:47, e2a4814329d2)
  • Current Telegram account path: Current main already reads account-level tokenFile through tryReadSecretFileSync(..., { rejectSymlink: true }), so the added test covers a real existing path. (extensions/telegram/src/token.ts:149, 90fd26b602b0)
  • Current IRC NickServ path: Current main reads NickServ passwordFile through tryReadSecretFileSync(..., { rejectSymlink: true }), so the added test covers the latent NickServ path. (extensions/irc/src/accounts.ts:118, 90fd26b602b0)
  • Current broad catch: Current main maps every thrown error from Telegram token-file inspection to configured_unavailable; the PR narrows that to the public fs-safe error type. (extensions/telegram/src/account-inspect.ts:46, 90fd26b602b0)
  • Dependency contract pin: The repository pins @openclaw/fs-safe to 0.2.7, matching the related merged PR's stated fail-closed secret-file contract. (package.json:1783, 90fd26b602b0)

Likely related people:

  • RomneyDa: Authored the current-main merge commit that restored fail-closed secret-file symlink behavior and adjusted the same Telegram/IRC tests this PR follows up on. (role: recent fix owner; confidence: high; commits: 90fd26b602b0; files: src/infra/secret-file.ts, extensions/telegram/src/account-inspect.ts, extensions/telegram/src/token.test.ts)
  • Ayaan Zaidi: Current-line blame attributes the existing Telegram account token-file resolver and IRC NickServ secret-file resolver paths to this author, although the shallow/grafted history makes the older feature trail thin. (role: adjacent area contributor; confidence: low; commits: c289e3ea8730; files: extensions/telegram/src/token.ts, extensions/irc/src/accounts.ts)

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

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal backlog priority with limited blast radius. labels May 20, 2026
@clawsweeper

clawsweeper Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper PR egg

✨ Hatched: 💎 rare Neon Proofling

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: 💎 rare.
Trait: stacks clean commits.
Image traits: location workflow harbor; accessory rollback rope; palette pearl, teal, and neon green; mood watchful; pose pointing at a small proof artifact; shell frosted glass shell; lighting cool dashboard glow; background smooth stones and checkmarks.
Share on X: post this hatch
Copy: My PR egg hatched a 💎 rare Neon Proofling 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.

@RomneyDa RomneyDa merged commit 4d47f9a into main May 20, 2026
144 of 149 checks passed
@RomneyDa RomneyDa deleted the secret-file-symlink-nit-followups branch May 20, 2026 22:35
@clawsweeper clawsweeper Bot mentioned this pull request May 21, 2026
Open
SebTardif pushed a commit to SebTardif/openclaw that referenced this pull request May 24, 2026
@RomneyDa @SebTardif
test(secret-file): cover NickServ + account-level symlinks, narrow in…
280fb09 
…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.
SebTardif pushed a commit to SebTardif/openclaw that referenced this pull request May 24, 2026
@RomneyDa @SebTardif
test(secret-file): cover NickServ + account-level symlinks, narrow in…
4ff0f67 
…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.
SebTardif pushed a commit to SebTardif/openclaw that referenced this pull request May 24, 2026
@RomneyDa @SebTardif
test(secret-file): cover NickServ + account-level symlinks, narrow in…
e582bbf 
…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.
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request May 24, 2026
@RomneyDa
test(secret-file): cover NickServ + account-level symlinks, narrow in…
c566a5a 
…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.
galiniliev pushed a commit to galiniliev/openclaw that referenced this pull request May 25, 2026
@RomneyDa
test(secret-file): cover NickServ + account-level symlinks, narrow in…
eeca8e6 
…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.
SebTardif pushed a commit to SebTardif/openclaw that referenced this pull request May 26, 2026
@RomneyDa @SebTardif
test(secret-file): cover NickServ + account-level symlinks, narrow in…
e34101a 
…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.
SebTardif pushed a commit to SebTardif/openclaw that referenced this pull request May 26, 2026
@RomneyDa @SebTardif
test(secret-file): cover NickServ + account-level symlinks, narrow in…
99c2305 
…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.
SebTardif pushed a commit to SebTardif/openclaw that referenced this pull request May 26, 2026
@RomneyDa @SebTardif
test(secret-file): cover NickServ + account-level symlinks, narrow in…
9277ce1 
…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.
jameslcowan pushed a commit to jameslcowan/openclaw that referenced this pull request Jun 2, 2026
@RomneyDa
test(secret-file): cover NickServ + account-level symlinks, narrow in…
19ed5ac 
…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.
SYU8384 pushed a commit to SYU8384/openclaw that referenced this pull request Jun 3, 2026
@RomneyDa
test(secret-file): cover NickServ + account-level symlinks, narrow in…
baf4835 
…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.
sablehead pushed a commit to sablehead/openclaw that referenced this pull request Jun 10, 2026
@RomneyDa
test(secret-file): cover NickServ + account-level symlinks, narrow in…
1eb4c9e 
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

channel: irc channel: telegram Channel integration: telegram maintainer Maintainer-authored PR P2 Normal backlog priority with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: S status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@RomneyDa