feat(discord): add expiresAtMs and bindSession to component spec#60763
feat(discord): add expiresAtMs and bindSession to component spec#60763geekhuashan wants to merge 3 commits intoopenclaw:mainfrom
Conversation
openclaw-barnacle
bot
added
channel: discord
Greptile SummaryAdds Confidence Score: 5/5Safe to merge; only remaining finding is a P2 test coverage gap for All logic is correct and follows existing patterns. The only gap is a missing test for No files require special attention. Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/discord/src/components.test.ts
Line: 83-89
Comment:
**Missing `expiresAtMs` test coverage**
`bindSession=false` has a read-path test, but `expiresAtMs` does not. A parallel test for `readDiscordComponentSpec` and the propagation through `buildDiscordComponentMessage` into the entries/modal would give the new field equal coverage.
```suggestion
it("reads bindSession=false from spec", () => {
const spec = readDiscordComponentSpec({
blocks: [{ type: "text", text: "hello" }],
bindSession: false,
});
expect(spec?.bindSession).toBe(false);
});
it("reads expiresAtMs from spec and propagates to entries", () => {
const ts = Date.now() + 60_000;
const spec = readDiscordComponentSpec({
blocks: [{ type: "actions", buttons: [{ label: "Go", callbackData: "x" }] }],
expiresAtMs: ts,
});
expect(spec?.expiresAtMs).toBe(ts);
const result = buildDiscordComponentMessage({ spec: spec! });
expect(result.entries[0]?.expiresAt).toBe(ts);
});
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(discord): add bindSession to compon..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR extends the Discord components v2 message spec to support explicit expiry and optional cross-session interaction routing, enabling components (buttons/selects/modals) to expire at a caller-chosen time and to route clicks through the channel’s normal session when desired.
Changes:
- Add
expiresAtMs?: numberandbindSession?: booleantoDiscordComponentMessageSpecand parse them from raw specs. - Propagate expiry (
expiresAtMs→ registryexpiresAt) and session binding behavior (bindSession === false→ omitsessionKey) when building component entries and modal entries. - Extend the message tool TypeBox schema to accept
expiresAtMsandbindSession; add a basic parsing test forbindSession.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| extensions/discord/src/message-tool-schema.ts | Adds expiresAtMs/bindSession to the TypeBox schema and updates schema description text. |
| extensions/discord/src/components.ts | Extends component spec type + parsing; propagates expiry and session binding into generated registry entries/modals. |
| extensions/discord/src/components.test.ts | Adds a test ensuring bindSession=false is parsed from the spec. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43f1e85911
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| sessionKey: boundSessionKey, | ||
| agentId: params.agentId, | ||
| accountId: params.accountId, |
There was a problem hiding this comment.
With bindSession=false, this code only clears sessionKey but still stamps each entry with agentId and accountId. Later, interaction dispatch treats those as route overrides, so the handler can run under the original agent/account while using the newly resolved channel session key, producing a mixed route context. This breaks the documented “channel’s normal session” behavior in environments where routing can change (for example multi-agent channels or changed bindings) and can persist session updates under the wrong agent store path.
Useful? React with 👍 / 👎.
Rebased onto upstream/main (2026.4.14)This PR has been rebased onto the latest upstream/main and is now mergeable. Ready for review. |
Add bindSession?: boolean to createDiscordMessageToolComponentsSchema() so callers can opt out of session-key binding for cron-owned components. Add test coverage.
🔒 Aisle Security AnalysisWe found 2 potential security issue(s) in this PR:
1. 🟠 Discord component interactions can be executed without session binding when `bindSession=false`
Description
Downstream,
Vulnerable code: const bindSession = params.spec.bindSession !== false;
const boundSessionKey = bindSession ? params.sessionKey : undefined;
...
entries.push({
...entry,
sessionKey: boundSessionKey,
agentId: boundAgentId,
accountId: boundAccountId,
});This is a broken access-control / authorization-risk pattern because the session/agent/account context is part of the security boundary for interaction routing. RecommendationTie interactive callbacks to a security principal even when routing via the “channel session”. Options:
if (params.spec.bindSession === false) {
const hasAnyAllowedUsers = (params.spec.blocks ?? []).some(/* check entries */) ||
Boolean(params.spec.modal?.allowedUsers?.length);
if (!hasAnyAllowedUsers) {
throw new Error("bindSession=false requires allowedUsers to prevent cross-user interaction");
}
}
Also document clearly that disabling session binding changes the authorization model and should not be used for privileged actions. 2. 🟡 Unbounded component expiration timestamp allows long-lived registry entries (memory/DoS risk)
Description
Vulnerable propagation: const expiresAtMs = readOptionalNumber(obj.expiresAtMs);
...
expiresAt: entry.expiresAt ?? params.spec.expiresAtMs,RecommendationEnforce safe bounds for expiration timestamps before storing them.
Example (code-side validation): const MAX_COMPONENT_TTL_MS = 30 * 60 * 1000; // or another safe upper bound
function readOptionalExpiresAtMs(value: unknown, now = Date.now()): number | undefined {
if (typeof value !== "number" || !Number.isFinite(value)) return undefined;
// clamp to [now, now + MAX]
const min = now;
const max = now + MAX_COMPONENT_TTL_MS;
if (value < min) return min;
if (value > max) return max;
return value;
}Then use Analyzed PR: #60763 at commit Last updated on: 2026-04-15T22:07:24Z |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78096dfef7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| sessionKey: boundSessionKey, | ||
| agentId: params.agentId, | ||
| accountId: params.accountId, |
There was a problem hiding this comment.
When bindSession=false, this block still persists agentId/accountId on modal registry entries even though sessionKey is intentionally unbound. On modal submit, dispatchDiscordComponentEvent applies those values as route overrides, so the event can run under the original sender's agent/account instead of the channel’s current route (e.g., multi-agent channels or changed bindings). This makes the new unbound-session mode inconsistent for forms and can write follow-up state under the wrong route context.
Useful? React with 👍 / 👎.
Summary
Add two new optional fields to
DiscordComponentMessageSpec:expiresAtMs: number— Unix epoch timestamp in ms after which the component(s) expire. When set, the components are registered with an expiry time, making them unavailable after the specified moment.bindSession: boolean— Whenfalse, interactions are not bound to the session that registered the components, allowing cross-session component usage.Rebased from original PR #60355 with conflicts resolved.