Skip to content

fix(mattermost): keep status read-only for SecretRef tokens#71024

Open
afurm wants to merge 2 commits intoopenclaw:mainfrom
afurm:af/fix-mattermost-status-secretref
Open

fix(mattermost): keep status read-only for SecretRef tokens#71024
afurm wants to merge 2 commits intoopenclaw:mainfrom
afurm:af/fix-mattermost-status-secretref

Conversation

@afurm
Copy link
Copy Markdown
Contributor

@afurm afurm commented Apr 24, 2026 •
edited
Loading

Summary

  • Problem: openclaw status text mode crashed when Mattermost botToken used an unresolved exec:keychain SecretRef.
  • Why it matters: Mattermost could be healthy at runtime, but read-only diagnostics failed before printing the channel table.
  • What changed: Added a Mattermost read-only account inspector that reports configured-but-unavailable credential metadata without dereferencing the secret, and taught status snapshots to preserve that state.
  • What did NOT change (scope boundary): Runtime Mattermost token resolution remains strict; this only changes read-only inspection/status behavior.

Change Type (select all)

  • Bug fix
  • Feature
  • Refactor required for the fix
  • Docs
  • Security hardening
  • Chore/infra

Scope (select all touched areas)

  • Gateway / orchestration
  • Skills / tool execution
  • Auth / tokens
  • Memory / storage
  • Integrations
  • API / contracts
  • UI / DX
  • CI/CD / infra

Linked Issue/PR

Root Cause (if applicable)

  • Root cause: Mattermost had strict runtime account resolution but no read-only inspectAccount path. Status rendering fell back to resolveAccount, which throws on unresolved SecretRefs.
  • Missing detection / guardrail: No Mattermost regression test covered read-only status inspection with an unresolved SecretRef credential.
  • Contributing context (if known): Telegram, Discord, and Slack already had read-only inspectors; Mattermost was missing the equivalent path.

Regression Test Plan (if applicable)

  • Coverage level that should have caught this:
    • Unit test
    • Seam / integration test
    • End-to-end test
    • Existing coverage already sufficient
  • Target test or file:
    • extensions/mattermost/src/mattermost/accounts.test.ts
    • extensions/mattermost/src/channel-actions-setup-status.contract.test.ts
  • Scenario the test should lock in: unresolved Mattermost SecretRef bot tokens are reported as configured/unavailable in read-only inspection without throwing.
  • Why this is the smallest reliable guardrail: It tests the plugin resolver and the status snapshot contract directly without requiring a live Mattermost server.
  • Existing test that already covers this (if any): None.
  • If no new test is added, why not: N/A.

User-visible / Behavior Changes

openclaw status no longer crashes for Mattermost accounts whose bot token is configured as an unresolved SecretRef. It reports degraded credential availability instead.

Diagram (if applicable)

Before:
openclaw status -> Mattermost resolveAccount -> unresolved SecretRef throw -> CLI crash

After:
openclaw status -> Mattermost inspectAccount -> configured_unavailable metadata -> status table renders

Security Impact (required)

  • New permissions/capabilities? (No)
  • Secrets/tokens handling changed? (Yes)
  • New/changed network calls? (No)
  • Command/tool execution surface changed? (No)
  • Data access scope changed? (No)
  • If any Yes, explain risk + mitigation: Read-only status now detects configured SecretRefs without resolving them. It does not expose token values, and runtime resolution remains strict.

Repro + Verification

Environment

  • OS: macOS local checkout
  • Runtime/container: Node 22 / pnpm
  • Model/provider: N/A
  • Integration/channel (if any): Mattermost
  • Relevant config (redacted): channels.mattermost.botToken as exec:keychain SecretRef plus baseUrl

Steps

  1. Configure Mattermost with botToken as an unresolved SecretRef.
  2. Run openclaw status.
  3. Inspect the Mattermost status row.

Expected

  • Status renders and reports Mattermost as configured with unavailable credentials.

Actual

  • Fixed by this PR. Before the fix, status crashed with an unresolved SecretRef error.

Evidence

  • Failing test/log before + passing after
  • Trace/log snippets
  • Screenshot/recording
  • Perf numbers (if relevant)

Validation:

  • pnpm test extensions/mattermost/src/mattermost/accounts.test.ts extensions/mattermost/src/channel-actions-setup-status.contract.test.ts
  • pnpm check:changed

Human Verification (required)

  • Verified scenarios:
    • Mattermost account inspection preserves unresolved SecretRef state without dereferencing the secret.
    • Mattermost status snapshots include botTokenStatus: configured_unavailable.
    • Changed-lane validation passed.
  • Edge cases checked:
    • Runtime resolveMattermostAccount still throws on unresolved SecretRefs.
    • Read-only inspection treats SecretRef + baseUrl as configured but unavailable.
  • What you did not verify:
    • A live Mattermost server connection.
    • A real exec:keychain secret provider returning a token.

Review Conversations

  • I replied to or resolved every bot review conversation I addressed in this PR.
  • I left unresolved only the conversations that still need reviewer or maintainer judgment.

Compatibility / Migration

  • Backward compatible? (Yes)
  • Config/env changes? (No)
  • Migration needed? (No)
  • If yes, exact upgrade steps: N/A.

Risks and Mitigations

  • Risk: Status may treat a configured-but-unavailable SecretRef as configured even when the command cannot read it.
    • Mitigation: The row is marked with unavailable credential metadata, and runtime token use still requires successful strict resolution.

@openclaw-barnacle openclaw-barnacle Bot added channel: mattermost Channel integration: mattermost size: S labels Apr 24, 2026
@afurm afurm force-pushed the af/fix-mattermost-status-secretref branch from c85286c to cdf63f7 Compare April 24, 2026 08:32
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 24, 2026

Greptile Summary

This PR adds a read-only inspectMattermostAccount path that detects configured-but-unresolved SecretRef bot tokens without dereferencing them, fixing a crash in openclaw status for Mattermost accounts whose botToken is an exec:keychain SecretRef. The new path is wired into mattermostConfigAdapter.inspectAccount and the status snapshot preserves the botTokenStatus: configured_unavailable field through resolveAccountSnapshot.

Confidence Score: 5/5

Safe to merge; the fix is well-scoped, runtime token resolution is unchanged, and tests cover the primary scenario.

Only one P2 edge-case finding remains: the botTokenSource/botTokenStatus priority when a configured_unavailable SecretRef coexists with an env token. This scenario (both sources simultaneously present) is uncommon in practice, and the primary crash fix is correct and well-tested.

extensions/mattermost/src/mattermost/accounts.ts — the env-token vs SecretRef priority order in inspectMattermostAccount's source/status derivation.

Prompt To Fix All With AI 
This is a comment left during a code review.
Path: extensions/mattermost/src/mattermost/accounts.ts
Line: 185-198

Comment:
When a config-level SecretRef is `configured_unavailable` **and** an env token is also present, `botTokenSource` is forced to `"config"` and `botTokenStatus` is forced to `"configured_unavailable"` even though `botToken` is actually set to the live `envToken`. This means `probeAccount` (which reads `account.botToken`) would probe with the env token while the snapshot reports the account as credential-unavailable — a misleading inconsistency. The env-fallback branch should be evaluated before the SecretRef status takes precedence in both the source and status chains.

```suggestion
  const botTokenSource: MattermostTokenSource = tokenInspection.botToken
    ? "config"
    : envToken
      ? "env"
      : tokenInspection.botTokenStatus === "configured_unavailable"
        ? "config"
        : "none";
  const botTokenStatus: MattermostCredentialStatus = tokenInspection.botToken
    ? "available"
    : envToken
      ? "available"
      : tokenInspection.botTokenStatus === "configured_unavailable"
        ? "configured_unavailable"
        : "missing";
```

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

Reviews (1): Last reviewed commit: "fix(mattermost): keep status read-only f..." | Re-trigger Greptile

greptile-apps[bot]
greptile-apps Bot reviewed Apr 24, 2026
View reviewed changes
Comment thread extensions/mattermost/src/mattermost/accounts.ts
@afurm afurm force-pushed the af/fix-mattermost-status-secretref branch from cdf63f7 to 40bbc37 Compare April 24, 2026 08:34
@afurm afurm force-pushed the af/fix-mattermost-status-secretref branch from 40bbc37 to 88e624c Compare April 24, 2026 08:36
@afurm @greptile-apps
Update extensions/mattermost/src/mattermost/accounts.ts
3c7fc88 
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
@clawsweeper clawsweeper Bot mentioned this pull request Apr 26, 2026
Closed
@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented Apr 29, 2026 •
edited
Loading

Codex review: needs changes before merge.

Summary
The PR adds a Mattermost read-only account inspector, wires it into status snapshots, preserves bot-token availability metadata, and adds tests for unresolved SecretRef bot tokens.

Reproducibility: yes. The original crash is reproduced by a Mattermost config with baseUrl plus an unresolved exec/keychain botToken SecretRef on current main, and the PR-specific regression is reproduced by adding MATTERMOST_BOT_TOKEN to that same config.

Next step before merge
Narrow automatable PR repair: fix one credential-precedence branch, add one regression test and changelog entry, then rebase and validate.

Security
Cleared: The diff changes read-only Mattermost credential metadata and tests without adding code execution, dependency sources, broader permissions, or token-value exposure.

Review findings

  • [P2] Preserve SecretRef-unavailable precedence — extensions/mattermost/src/mattermost/accounts.ts:185-198
  • [P3] Add the required changelog entry — extensions/mattermost/src/channel-config-shared.ts:57
Review details

Best possible solution:

Land the Mattermost-owned read-only inspector/status snapshot fix after aligning credential precedence with strict runtime resolution, adding a SecretRef-plus-env regression test, and adding the active-version changelog entry.

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

Yes. The original crash is reproduced by a Mattermost config with baseUrl plus an unresolved exec/keychain botToken SecretRef on current main, and the PR-specific regression is reproduced by adding MATTERMOST_BOT_TOKEN to that same config.

Is this the best way to solve the issue?

No. The chosen read-only inspector design is the narrow maintainable fix, but this implementation should preserve configured_unavailable ahead of env fallback unless runtime resolution semantics are changed too.

Full review comments:

  • [P2] Preserve SecretRef-unavailable precedence — extensions/mattermost/src/mattermost/accounts.ts:185-198
    When a configured botToken SecretRef is unresolved and MATTERMOST_BOT_TOKEN is also present, this branch reports botTokenSource: "env" and botTokenStatus: "available". Runtime resolveMattermostAccount still strict-resolves the configured SecretRef before env fallback, so status/probe can report credentials OK for a config that startup would reject.
    Confidence: 0.88
  • [P3] Add the required changelog entry — extensions/mattermost/src/channel-config-shared.ts:57
    This is a user-visible Mattermost status fix, but the diff does not update CHANGELOG.md. Repo policy requires user-facing fix PRs to add an active-version changelog entry, with contributor credit when applicable.
    Confidence: 0.86

Overall correctness: patch is incorrect
Overall confidence: 0.88

Acceptance criteria:

  • pnpm test extensions/mattermost/src/mattermost/accounts.test.ts extensions/mattermost/src/channel-actions-setup-status.contract.test.ts
  • pnpm check:changed

What I checked:

Likely related people:

  • steipete: Recent history shows repeated work on Mattermost config helpers, status/presentation paths, and shared channel config adapter seams that this PR uses. (role: recent maintainer and shared inspection/config owner; confidence: high; commits: 51affb81b9a5, b98cccc06e27, fd0970c07737; files: extensions/mattermost/src/mattermost/accounts.ts, extensions/mattermost/src/channel-config-shared.ts, extensions/mattermost/src/channel.ts)
  • joshavant: The Mattermost SecretRef support surface traces to the broad SecretRef expansion commit that touched Mattermost account and secret-input handling. (role: adjacent SecretRef owner; confidence: medium; commits: 806803b7efe2; files: extensions/mattermost/src/mattermost/accounts.ts, extensions/mattermost/src/secret-input.ts)
  • Takhoffman: Recent merged Mattermost fixes touched default account, reply account, and action discovery account resolution near the behavior under review. (role: Mattermost account-resolution contributor; confidence: medium; commits: 9289f967dfb7, a8302e8eab21, 226e2389f6c5; files: extensions/mattermost/src/mattermost/accounts.ts, extensions/mattermost/src/channel.ts)

Remaining risk / open question:

  • Targeted tests and changed-lane validation still need to run after the repair.
  • The PR base is older than current main, so a rebase may be needed before final mergeability is known.

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

@coolmanns coolmanns mentioned this pull request Apr 30, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@greptile-apps greptile-apps[bot] greptile-apps[bot] left review comments

Assignees

No one assigned

Labels

channel: mattermost Channel integration: mattermost size: S

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

openclaw status (text mode) fails on Mattermost exec:keychain SecretRef — other channels with same pattern work

1 participant

@afurm