Skip to content

fix(feishu): use resolved runtime config for gateway sends#89403

Draft
Pluviobyte wants to merge 1 commit into
openclaw:mainfrom
Pluviobyte:codex/feishu-media-secretref
Draft

fix(feishu): use resolved runtime config for gateway sends#89403
Pluviobyte wants to merge 1 commit into
openclaw:mainfrom
Pluviobyte:codex/feishu-media-secretref

Conversation

@Pluviobyte

@Pluviobyte Pluviobyte commented Jun 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

  • use the hot runtime config snapshot for gateway send requests when it matches the caller source config, so resolved Feishu SecretRef credentials reach delivery
  • keep Feishu mediaUrl as a plain payload input instead of resolving media URLs through SecretRef
  • add regression coverage for the gateway send handoff and Feishu media upload client credentials

Fixes #89338.

Validation

  • node scripts/run-vitest.mjs extensions/feishu/src/media.test.ts extensions/feishu/src/outbound.test.ts extensions/feishu/src/accounts.test.ts extensions/feishu/src/client.test.ts src/gateway/server-methods/send.test.ts: passed (gateway 57 tests, Feishu 126 tests)
  • node scripts/run-tsgo.mjs -p test/tsconfig/tsconfig.extensions.test.json --incremental --tsBuildInfoFile .artifacts/tsgo-cache/extensions-test.tsbuildinfo
  • git diff --check

Remaining

  • redacted live Feishu proof is still needed before this draft should be marked ready

@openclaw-barnacle openclaw-barnacle Bot added channel: feishu Channel integration: feishu size: M triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels Jun 2, 2026
@clawsweeper

clawsweeper Bot commented Jun 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 29, 2026, 9:07 PM ET / 01:07 UTC.

Summary
The branch reuses the resolved runtime config snapshot for gateway send, renames the helper shared with message.action, and adds Feishu/gateway regression tests for SecretRef-backed appSecret delivery.

PR surface: Source +1, Tests +101. Total +102 across 3 files.

Reproducibility: yes. at source level: Feishu appSecret is a documented SecretRef surface, resolveFeishuRuntimeAccount throws on unresolved non-env SecretRefs, and current media/outbound/reply paths can still pass cfg into that strict resolver. I did not run a live Feishu tenant send in this read-only review.

Review metrics: 1 noteworthy metric.

  • Runtime credential handoff: 1 gateway send path changed. Normal gateway sends would start using the active resolved runtime snapshot when source snapshots match, which determines whether Feishu SecretRef credentials reach delivery.

Stored data model
Persistent data-model change detected: serialized state: extensions/feishu/src/media.test.ts, serialized state: src/gateway/server-methods/send.test.ts. Confirm migration or upgrade compatibility proof before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #89338
Summary: This PR is one open fix candidate for the canonical Feishu SecretRef-backed MEDIA upload regression; another open candidate covers more Feishu delivery paths.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦐 gold shrimp
Result: blocked until real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Add redacted real Feishu proof for a SecretRef-backed appSecret uploading and delivering media through the changed path.
  • Either cover the Feishu outbound/reply-dispatcher media paths or narrow the PR so it does not close the canonical report.
  • [P1] Have maintainers choose this branch or the broader overlapping Feishu credential-resolution PR as the canonical fix path.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body and author comment explicitly say no redacted live Feishu proof is attached; the contributor should add redacted screenshot, recording, terminal output, live logs, or equivalent proof and update the PR body to trigger re-review, or ask a maintainer for @clawsweeper re-review if it does not rerun automatically.

Risk before merge

  • [P1] The PR still lacks redacted real Feishu proof for a SecretRef-backed appSecret media upload and delivery after the patch.
  • [P1] The branch fixes the gateway send handoff but not the Feishu outbound adapter or source-channel reply-dispatcher paths that the linked MEDIA report can exercise.
  • [P1] Because the PR uses closing syntax for the canonical issue, merging it as-is could close the user report before the broader Feishu media regression is proven fixed.
  • [P1] The changed handoff controls whether SecretRef identifiers or resolved credential strings reach delivery, so a wrong runtime-config choice affects both message delivery and credential handling.

Maintainer options:

  1. Keep draft until live Feishu proof (recommended)
    Require redacted real Feishu upload and delivery proof with a SecretRef-backed appSecret before marking this external draft ready or merging it.
  2. Pick the canonical Feishu fix
    Compare this gateway-send runtime-snapshot repair with the broader outbound/reply-dispatcher candidate before landing overlapping credential-handling changes.
  3. Narrow the closing scope
    If maintainers want this branch only as a gateway-send fix, remove or revise the closing reference and keep the remaining MEDIA paths tracked by the canonical issue or broader PR.

Next step before merge

  • [P1] This external draft needs contributor proof and maintainer selection among overlapping Feishu fix candidates rather than an automated repair lane.

Security
Cleared: The diff changes source and tests only, with no workflow, dependency, lockfile, package, or executable supply-chain changes; credential-boundary concerns are captured as merge risk.

Review findings

  • [P1] Cover Feishu reply delivery before closing the regression — src/gateway/server-methods/send.ts:634
Review details

Best possible solution:

Land one canonical Feishu SecretRef media fix after redacted proof covers the relevant gateway send, outbound adapter, and source-channel reply-dispatcher delivery paths, or narrow this PR so it no longer closes the canonical user report.

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

Yes, at source level: Feishu appSecret is a documented SecretRef surface, resolveFeishuRuntimeAccount throws on unresolved non-env SecretRefs, and current media/outbound/reply paths can still pass cfg into that strict resolver. I did not run a live Feishu tenant send in this read-only review.

Is this the best way to solve the issue?

No, not as the complete linked-issue fix. The patch is a plausible gateway-send repair, but the best fix should also cover the Feishu outbound and source-channel reply-dispatcher media paths or narrow the PR so it does not close the broader report.

Full review comments:

  • [P1] Cover Feishu reply delivery before closing the regression — src/gateway/server-methods/send.ts:634
    This routes only gateway send through the resolved runtime snapshot. The linked MEDIA report can also hit Feishu outbound/source-channel reply paths that still pass cfg directly to sendMediaFeishu on current main, so merging this with closing syntax would mark the user report fixed while a relevant delivery path remains uncovered.
    Confidence: 0.87

Overall correctness: patch is incorrect
Overall confidence: 0.84

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 54b09580f61b.

Label changes

Label justifications:

  • P1: The PR targets a reported Feishu media delivery regression involving SecretRef-backed credentials in a real channel workflow.
  • merge-risk: 🚨 auth-provider: The changed config handoff controls whether resolved Feishu appSecret credentials reach outbound sends.
  • merge-risk: 🚨 message-delivery: A wrong runtime config choice or too-narrow fix can still cause Feishu media sends to fail or degrade.
  • merge-risk: 🚨 security-boundary: The patch affects the boundary between SecretRef identifiers and resolved credential strings in delivery.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body and author comment explicitly say no redacted live Feishu proof is attached; the contributor should add redacted screenshot, recording, terminal output, live logs, or equivalent proof and update the PR body to trigger re-review, or ask a maintainer for @clawsweeper re-review if it does not rerun automatically.
Evidence reviewed

PR surface:

Source +1, Tests +101. Total +102 across 3 files.

View PR surface stats
Area Files Added Removed Net
Source 1 5 4 +1
Tests 2 106 5 +101
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 3 111 9 +102

What I checked:

  • repository policy applied: Root plus scoped extensions/gateway AGENTS files were read fully; their proof, plugin-boundary, gateway-hot-path, and compatibility guidance apply to this credential and delivery-path review. (AGENTS.md:20, 54b09580f61b)
  • current main gateway behavior: Current main resolves the runtime snapshot only for message.action; normal send destructures cfg directly from the resolved channel and passes it into target resolution and durable delivery. (src/gateway/server-methods/send.ts:660, 54b09580f61b)
  • PR gateway fix: The PR changes normal gateway send to call resolveCurrentMessageRuntimeConfig, so the delivery path receives the active resolved runtime config when it matches the source snapshot. (src/gateway/server-methods/send.ts:633, 9ffd76418625)
  • Feishu strict credential resolver: Current main's Feishu runtime account resolver throws FeishuSecretRefUnavailableError for structured SecretRefs in strict mode rather than resolving file/exec/keychain values at that boundary. (extensions/feishu/src/accounts.ts:56, 54b09580f61b)
  • remaining outbound media path: Current main's Feishu outbound local-image/media branches pass cfg directly into sendMediaFeishu, so non-gateway delivery can still reach the strict resolver with unresolved credentials. (extensions/feishu/src/outbound.ts:584, 54b09580f61b)
  • remaining reply-dispatcher media path: Current main's source-channel reply dispatcher passes its captured cfg directly into sendMediaFeishu for media replies, leaving a sibling delivery path outside this PR's gateway-only fix. (extensions/feishu/src/reply-dispatcher.ts:520, 54b09580f61b)

Likely related people:

  • vincentkoc: Remote path history shows recent gateway send and Feishu accounts/media maintenance, including outbound/runtime seams central to this PR. (role: recent area contributor; confidence: high; commits: a22a1edc8faa, b4e5aa18b3b7, ff4808f94d67; files: src/gateway/server-methods/send.ts, extensions/feishu/src/accounts.ts, extensions/feishu/src/media.ts)
  • steipete: Remote path history ties steipete to Feishu account resolution, media/client setup, outbound SDK refactors, and the runtime config/SecretRef documentation that define this boundary. (role: feature-history contributor; confidence: high; commits: 58912f8fd842, 16ef041b5d86, 1507a9701b83; files: extensions/feishu/src/accounts.ts, extensions/feishu/src/media.ts, extensions/feishu/src/outbound.ts)
  • openperf: Authored the open broader fix candidate for the same Feishu SecretRef media regression, covering outbound, message-tool, and reply-dispatcher paths. (role: likely follow-up owner; confidence: medium; commits: 6f799e50d38a; files: extensions/feishu/src/accounts.ts, extensions/feishu/src/outbound.ts, extensions/feishu/src/channel.ts)
  • LiLan0125: The local shallow checkout blames sampled gateway and Feishu lines to commit 685b95b, but the commit title is broad and remote path history gives stronger domain-routing signal. (role: current local line owner; confidence: low; commits: 685b95b607bb; files: src/gateway/server-methods/send.ts, extensions/feishu/src/accounts.ts, extensions/feishu/src/reply-dispatcher.ts)
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.

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.
Review history (1 earlier review cycle)
  • reviewed 2026-06-21T12:25:28.138Z sha 9ffd764 :: needs real behavior proof before merge. :: [P1] Cover Feishu reply delivery before closing the regression

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P1 High-priority user-facing bug, regression, or broken workflow. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. merge-risk: 🚨 auth-provider 🚨 May break OAuth, tokens, provider routing, model choice, or credentials. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. labels Jun 2, 2026
@Pluviobyte
Pluviobyte force-pushed the codex/feishu-media-secretref branch from 4959c7d to 9ffd764 Compare June 3, 2026 05:47
@openclaw-barnacle openclaw-barnacle Bot added gateway Gateway runtime size: S and removed size: M labels Jun 3, 2026
@Pluviobyte Pluviobyte changed the title fix(feishu): resolve SecretRef media before upload fix(feishu): use resolved runtime config for gateway sends Jun 3, 2026
@Pluviobyte

Pluviobyte commented Jun 3, 2026

Copy link
Copy Markdown
Contributor Author

Updated this draft to follow the review direction in the latest push (9ffd7641).

What changed:

  • Removed the previous media URL SecretRef resolution approach from the final diff.
  • Routed normal gateway send requests through the resolved runtime config snapshot when it matches the source config, matching the existing message.action behavior.
  • Added regression coverage that a Feishu appSecret SecretRef resolves before delivery and that the Feishu media upload client receives the resolved credential rather than the SecretRef id.

Validation run locally:

  • node scripts/run-vitest.mjs extensions/feishu/src/media.test.ts extensions/feishu/src/outbound.test.ts extensions/feishu/src/accounts.test.ts extensions/feishu/src/client.test.ts src/gateway/server-methods/send.test.ts passed: gateway 57 tests, Feishu 126 tests.
  • node scripts/run-tsgo.mjs -p test/tsconfig/tsconfig.extensions.test.json --incremental --tsBuildInfoFile .artifacts/tsgo-cache/extensions-test.tsbuildinfo passed.
  • git diff --check passed.

I have not attached live Feishu proof yet; this should stay draft until there is a redacted real-behavior run.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Jun 3, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 15, 2026
Open
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jun 15, 2026
This was referenced Jun 19, 2026
Open
Open
@openclaw-barnacle

openclaw-barnacle Bot commented Jul 20, 2026

Copy link
Copy Markdown

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

@openclaw-barnacle openclaw-barnacle Bot added stale Marked as stale due to inactivity triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. and removed stale Marked as stale due to inactivity labels Jul 20, 2026
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: feishu Channel integration: feishu gateway Gateway runtime merge-risk: 🚨 auth-provider 🚨 May break OAuth, tokens, provider routing, model choice, or credentials. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. P1 High-priority user-facing bug, regression, or broken workflow. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: Feishu MEDIA/image upload fails when appSecret uses SecretRef (Keychain)

2 participants

@Pluviobyte @Patrick-Erichsen