mirror of
https://github.com/hansjone/dsh-im-ops.git
synced 2026-10-17 12:20:46 +08:00
fix(feishu): bind card decisions to the actor and reject stale cards
Address review feedback on the interaction cards:
1. Bind card decisions to the initiating actor. The card-action operator
(operatorOpenId) is now passed into the approval and question handlers:
- submitByApprovalId receives { actor } so another allowed group member
cannot approve/reject someone else's approval;
- the answer branch requires pending.actor === operator.
2. Include the question index in the answer button action and validate it
against the current pending question, so a stale card from an earlier
question in a multi-question interaction cannot be applied to the next one.
3. When both the card send and the plain-text fallback fail, the error is no
longer swallowed: it propagates so the interaction is not marked as
presented and the existing retry/reconnect behaviour can run.
Adds regression tests for all three cases.
This commit is contained in:
parent
0b42ea9889
commit
b5378500ca
4 changed files with 244 additions and 21 deletions
File diff suppressed because one or more lines are too long
|
|
@ -1721,7 +1721,7 @@ export class FeishuHarnessBridge {
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
await this.#handleCardAction(resolvedAction, entry);
|
await this.#handleCardAction(resolvedAction, { ...entry, actor: entry.operatorOpenId });
|
||||||
}, {
|
}, {
|
||||||
lane: isStop || isRealSteer ? 'control' : 'regular',
|
lane: isStop || isRealSteer ? 'control' : 'regular',
|
||||||
coalesceStop: isStop,
|
coalesceStop: isStop,
|
||||||
|
|
@ -1878,6 +1878,7 @@ export class FeishuHarnessBridge {
|
||||||
sessionWorkspace = null,
|
sessionWorkspace = null,
|
||||||
sessionPage = 0,
|
sessionPage = 0,
|
||||||
selections = [],
|
selections = [],
|
||||||
|
actor = null,
|
||||||
}) {
|
}) {
|
||||||
// Confirmations triggered by a card interaction stay anchored to the
|
// Confirmations triggered by a card interaction stay anchored to the
|
||||||
// card's message so they land inside the same Feishu topic.
|
// card's message so they land inside the same Feishu topic.
|
||||||
|
|
@ -1887,22 +1888,32 @@ export class FeishuHarnessBridge {
|
||||||
const sep = action.indexOf(':');
|
const sep = action.indexOf(':');
|
||||||
const approvalId = action.slice(sep + 1);
|
const approvalId = action.slice(sep + 1);
|
||||||
const outcome = action.startsWith('approve:') ? 'allowed-once' : 'rejected';
|
const outcome = action.startsWith('approve:') ? 'allowed-once' : 'rejected';
|
||||||
const submitted = await this.#approvals.submitByApprovalId(approvalId, outcome);
|
// Bind the decision to the operator so another allowed group member
|
||||||
|
// cannot decide someone else's approval.
|
||||||
|
const submitted = await this.#approvals.submitByApprovalId(approvalId, outcome, { actor });
|
||||||
if (!submitted) {
|
if (!submitted) {
|
||||||
await reply(t('该审批已处理或不存在,无需重复操作。')).catch(() => undefined);
|
await reply(t('该审批已处理或不存在,无需重复操作。')).catch(() => undefined);
|
||||||
}
|
}
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
// Question option buttons: answer:<interactionId>:<optionLabel>
|
// Question option buttons: answer:<interactionId>:<index>:<optionLabel>
|
||||||
if (action.startsWith('answer:')) {
|
if (action.startsWith('answer:')) {
|
||||||
const rest = action.slice('answer:'.length);
|
const rest = action.slice('answer:'.length);
|
||||||
const sep = rest.indexOf(':');
|
const firstSep = rest.indexOf(':');
|
||||||
if (sep !== -1) {
|
if (firstSep !== -1) {
|
||||||
const interactionId = rest.slice(0, sep);
|
const interactionId = rest.slice(0, firstSep);
|
||||||
const optionLabel = rest.slice(sep + 1);
|
const afterId = rest.slice(firstSep + 1);
|
||||||
|
const indexSep = afterId.indexOf(':');
|
||||||
|
const indexText = indexSep === -1 ? afterId : afterId.slice(0, indexSep);
|
||||||
|
const optionLabel = indexSep === -1 ? '' : afterId.slice(indexSep + 1);
|
||||||
const qKey = this.#interactionKeys.get(interactionId);
|
const qKey = this.#interactionKeys.get(interactionId);
|
||||||
const pending = qKey ? this.#pendingInteractions.get(qKey) : null;
|
const pending = qKey ? this.#pendingInteractions.get(qKey) : null;
|
||||||
if (pending && pending.kind === 'question' && !pending.submitting) {
|
// Only the actor who started the interaction may answer it, and the
|
||||||
|
// card must still target the current question (a stale card from an
|
||||||
|
// earlier question in a multi-question interaction must not submit).
|
||||||
|
if (pending && pending.kind === 'question' && !pending.submitting
|
||||||
|
&& pending.actor === actor
|
||||||
|
&& Number(indexText) === pending.index) {
|
||||||
await this.#submitQuestionAnswer(pending, optionLabel, { chatId });
|
await this.#submitQuestionAnswer(pending, optionLabel, { chatId });
|
||||||
} else {
|
} else {
|
||||||
await reply(INTERACTION_RESOLVED_TEXT()).catch(() => undefined);
|
await reply(INTERACTION_RESOLVED_TEXT()).catch(() => undefined);
|
||||||
|
|
@ -3705,9 +3716,12 @@ export class FeishuHarnessBridge {
|
||||||
approvalId: pending.approvalId,
|
approvalId: pending.approvalId,
|
||||||
}),
|
}),
|
||||||
{ key, replyTo: replyToMessageId },
|
{ key, replyTo: replyToMessageId },
|
||||||
).catch(() => {
|
).catch(async () => {
|
||||||
// Fall back to the plain-text approval if the card cannot be sent.
|
// Fall back to the plain-text approval if the card cannot be
|
||||||
return this.#send(chatId, pending.text, { replyTo: replyToMessageId }).catch(() => undefined);
|
// sent. If the text send also fails, let the error propagate so
|
||||||
|
// the pending approval is not marked as presented and the
|
||||||
|
// existing retry/reconnect logic can run.
|
||||||
|
await this.#send(chatId, pending.text, { replyTo: replyToMessageId });
|
||||||
});
|
});
|
||||||
},
|
},
|
||||||
}
|
}
|
||||||
|
|
@ -3827,15 +3841,17 @@ export class FeishuHarnessBridge {
|
||||||
total: pending.questions.length,
|
total: pending.questions.length,
|
||||||
}),
|
}),
|
||||||
{ key: pending.key, replyTo: pending.replyToMessageId },
|
{ key: pending.key, replyTo: pending.replyToMessageId },
|
||||||
).catch(() => {
|
).catch(async () => {
|
||||||
// Fall back to the plain-text question if the card cannot be sent.
|
// Fall back to the plain-text question if the card cannot be sent.
|
||||||
return this.#send(
|
// If the text send also fails, let the error propagate so the pending
|
||||||
|
// question is not marked as presented and the existing retry logic runs.
|
||||||
|
await this.#send(
|
||||||
pending.chatId,
|
pending.chatId,
|
||||||
harnessQuestionText(question, pending.index, pending.questions.length, {
|
harnessQuestionText(question, pending.index, pending.questions.length, {
|
||||||
requiresMention: pending.requiresMention,
|
requiresMention: pending.requiresMention,
|
||||||
}),
|
}),
|
||||||
{ replyTo: pending.replyToMessageId },
|
{ replyTo: pending.replyToMessageId },
|
||||||
).catch(() => undefined);
|
);
|
||||||
});
|
});
|
||||||
} else {
|
} else {
|
||||||
// Multi-select or free-text questions keep the plain-text reply flow.
|
// Multi-select or free-text questions keep the plain-text reply flow.
|
||||||
|
|
|
||||||
|
|
@ -925,7 +925,9 @@ export function questionCard({ interactionId, header, question, detail, options,
|
||||||
// Include the option description in the button so the user sees the full
|
// Include the option description in the button so the user sees the full
|
||||||
// meaning (mirrors the text form "1. label — description").
|
// meaning (mirrors the text form "1. label — description").
|
||||||
const buttonText = description ? `${label}\n${description}` : label;
|
const buttonText = description ? `${label}\n${description}` : label;
|
||||||
elements.push(button(buttonText, `answer:${interactionId}:${label}`));
|
// Action carries the question index so a stale card from a previous
|
||||||
|
// question cannot be applied to the current one: answer:<interactionId>:<index>:<label>
|
||||||
|
elements.push(button(buttonText, `answer:${interactionId}:${index}:${label}`));
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
return cardWith(t('❓ 请补充信息{progress}', { progress }), elements);
|
return cardWith(t('❓ 请补充信息{progress}', { progress }), elements);
|
||||||
|
|
|
||||||
|
|
@ -1973,13 +1973,13 @@ test('a single-choice question is presented as a card with option buttons by def
|
||||||
|
|
||||||
const card = cards(sent).at(-1).content;
|
const card = cards(sent).at(-1).content;
|
||||||
const actions = buttonsFromCard(card).map(callbackAction).filter(Boolean);
|
const actions = buttonsFromCard(card).map(callbackAction).filter(Boolean);
|
||||||
assert.ok(actions.includes('answer:question-card-id:测试环境'),
|
assert.ok(actions.includes('answer:question-card-id:0:测试环境'),
|
||||||
'first option button action missing');
|
'first option button action missing');
|
||||||
assert.ok(actions.includes('answer:question-card-id:生产环境'),
|
assert.ok(actions.includes('answer:question-card-id:0:生产环境'),
|
||||||
'second option button action missing');
|
'second option button action missing');
|
||||||
|
|
||||||
await bridge.onCardAction(
|
await bridge.onCardAction(
|
||||||
cardActionEvent('om_card_1', 'answer:question-card-id:测试环境', 'ou_user'),
|
cardActionEvent('om_card_1', 'answer:question-card-id:0:测试环境', 'ou_user'),
|
||||||
);
|
);
|
||||||
await turn;
|
await turn;
|
||||||
assert.deepEqual(await submitted.promise, {
|
assert.deepEqual(await submitted.promise, {
|
||||||
|
|
@ -2069,6 +2069,203 @@ test('an interaction card falls back to plain text when the card send fails', as
|
||||||
}]);
|
}]);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test('a different allowed group member cannot approve or answer an interaction card', async () => {
|
||||||
|
const fixture = stateFixture([['group:oc_group', 'session-group-actor']]);
|
||||||
|
const sent = [];
|
||||||
|
const decisions = [];
|
||||||
|
const decided = deferred();
|
||||||
|
const bridge = new FeishuHarnessBridge({
|
||||||
|
client: cardClient(async (outgoing) => sent.push(outgoing)),
|
||||||
|
harness: {
|
||||||
|
sessionExists: async () => true,
|
||||||
|
createSession: async () => assert.fail('the existing session should be reused'),
|
||||||
|
ask: async (sessionId, _text, options) => {
|
||||||
|
await options.onInteraction({
|
||||||
|
kind: 'approval',
|
||||||
|
interactionId: 'approval-actor-bound',
|
||||||
|
rpcId: 'rpc-approval-actor-bound',
|
||||||
|
sessionId,
|
||||||
|
payload: {
|
||||||
|
type: 'approval/requested',
|
||||||
|
sessionId,
|
||||||
|
approvalId: 'approval-actor-bound',
|
||||||
|
toolName: 'bash',
|
||||||
|
callId: 'call-actor',
|
||||||
|
reason: '需要确认',
|
||||||
|
},
|
||||||
|
toolCall: { callId: 'call-actor', name: 'bash', arguments: '{}' },
|
||||||
|
respond: async (result) => {
|
||||||
|
decisions.push(result);
|
||||||
|
decided.resolve();
|
||||||
|
return { accepted: true };
|
||||||
|
},
|
||||||
|
});
|
||||||
|
await decided.promise;
|
||||||
|
return '已完成';
|
||||||
|
},
|
||||||
|
},
|
||||||
|
state: fixture.state,
|
||||||
|
status: bridgeStatus(),
|
||||||
|
allowedSenderOpenIds: new Set(['ou_owner', 'ou_member']),
|
||||||
|
});
|
||||||
|
|
||||||
|
const turn = bridge.accept(event('actor-bound-start', '发起审批', {
|
||||||
|
senderOpenId: 'ou_owner',
|
||||||
|
chat_type: 'group',
|
||||||
|
chat_id: 'oc_group',
|
||||||
|
mentions: [{ key: '@bot', id: { open_id: 'bot' } }],
|
||||||
|
}));
|
||||||
|
await eventually(
|
||||||
|
() => sent.some(({ msgType }) => msgType === 'interactive'),
|
||||||
|
'the approval card was not sent',
|
||||||
|
);
|
||||||
|
|
||||||
|
// A different allowed group member clicks approve: must be ignored.
|
||||||
|
await bridge.onCardAction(
|
||||||
|
cardActionEvent('om_card_1', 'approve:approval-actor-bound', 'ou_member'),
|
||||||
|
);
|
||||||
|
assert.deepEqual(decisions, [], 'another allowed member must not approve');
|
||||||
|
|
||||||
|
// The originating actor's click does go through.
|
||||||
|
await bridge.onCardAction(
|
||||||
|
cardActionEvent('om_card_1', 'approve:approval-actor-bound', 'ou_owner'),
|
||||||
|
);
|
||||||
|
await turn;
|
||||||
|
assert.deepEqual(decisions, [{
|
||||||
|
ok: true,
|
||||||
|
value: {
|
||||||
|
sessionId: 'session-group-actor',
|
||||||
|
approvalId: 'approval-actor-bound',
|
||||||
|
outcome: 'allowed-once',
|
||||||
|
},
|
||||||
|
}]);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('a stale question card cannot answer the next question in a multi-question interaction', async () => {
|
||||||
|
const fixture = stateFixture([['p2p:ou_user', 'session-stale-card']]);
|
||||||
|
const sent = [];
|
||||||
|
const response = deferred();
|
||||||
|
const bridge = new FeishuHarnessBridge({
|
||||||
|
client: cardClient(async (outgoing) => sent.push(outgoing)),
|
||||||
|
harness: {
|
||||||
|
sessionExists: async () => true,
|
||||||
|
createSession: async () => assert.fail('the existing session should be reused'),
|
||||||
|
ask: async (sessionId, _text, options) => {
|
||||||
|
await options.onInteraction({
|
||||||
|
kind: 'question',
|
||||||
|
interactionId: 'stale-question',
|
||||||
|
rpcId: 'stale-question',
|
||||||
|
sessionId,
|
||||||
|
payload: {
|
||||||
|
type: 'question/requested',
|
||||||
|
sessionId,
|
||||||
|
questions: [
|
||||||
|
{ id: 'first', question: '第一问', options: [{ label: 'A' }, { label: 'B' }] },
|
||||||
|
{ id: 'second', question: '第二问', options: [{ label: 'C' }, { label: 'D' }] },
|
||||||
|
],
|
||||||
|
},
|
||||||
|
respond: async (result) => {
|
||||||
|
response.resolve(result);
|
||||||
|
return { accepted: true };
|
||||||
|
},
|
||||||
|
});
|
||||||
|
await response.promise;
|
||||||
|
return '两问均完成';
|
||||||
|
},
|
||||||
|
},
|
||||||
|
state: fixture.state,
|
||||||
|
status: bridgeStatus(),
|
||||||
|
allowedSenderOpenIds: new Set(['ou_user']),
|
||||||
|
});
|
||||||
|
|
||||||
|
const turn = bridge.accept(event('stale-card-start', '分步提问'));
|
||||||
|
await eventually(
|
||||||
|
() => sent.some(({ msgType }) => msgType === 'interactive'),
|
||||||
|
'the first question card was not sent',
|
||||||
|
);
|
||||||
|
// Answer question 1 via its card (index 0), advancing to question 2.
|
||||||
|
await bridge.onCardAction(cardActionEvent('om_card_1', 'answer:stale-question:0:A', 'ou_user'));
|
||||||
|
await eventually(
|
||||||
|
() => cards(sent).length >= 2,
|
||||||
|
'the second question card was not sent',
|
||||||
|
);
|
||||||
|
|
||||||
|
// A stale click on question 1's old card (index 0) must not answer question 2.
|
||||||
|
await bridge.onCardAction(cardActionEvent('om_card_1', 'answer:stale-question:0:B', 'ou_user'));
|
||||||
|
|
||||||
|
// Answer the current question 2 via its card (index 1).
|
||||||
|
await bridge.onCardAction(cardActionEvent('om_card_2', 'answer:stale-question:1:C', 'ou_user'));
|
||||||
|
await turn;
|
||||||
|
assert.deepEqual(await response.promise, {
|
||||||
|
ok: true,
|
||||||
|
value: {
|
||||||
|
sessionId: 'session-stale-card',
|
||||||
|
answer: {
|
||||||
|
answers: [
|
||||||
|
{ id: 'first', selected: ['A'] },
|
||||||
|
{ id: 'second', selected: ['C'] },
|
||||||
|
],
|
||||||
|
},
|
||||||
|
},
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
test('failure of both the card and the text fallback does not mark the question as presented', async () => {
|
||||||
|
const fixture = stateFixture([['p2p:ou_user', 'session-present-fail']]);
|
||||||
|
const sent = [];
|
||||||
|
let responds = 0;
|
||||||
|
let respondResult = null;
|
||||||
|
const bridge = new FeishuHarnessBridge({
|
||||||
|
// Both interactive cards and plain text fail to send.
|
||||||
|
client: {
|
||||||
|
im: { v1: { message: {
|
||||||
|
create: async (request) => {
|
||||||
|
sent.push(request.data.msg_type);
|
||||||
|
throw new Error('send disabled');
|
||||||
|
},
|
||||||
|
} } },
|
||||||
|
},
|
||||||
|
harness: {
|
||||||
|
sessionExists: async () => true,
|
||||||
|
createSession: async () => assert.fail('the existing session should be reused'),
|
||||||
|
ask: async (sessionId, _text, options) => {
|
||||||
|
// Presenting the question fails (card and text fallback both throw),
|
||||||
|
// so the interaction is not presented as an answerable question.
|
||||||
|
await options.onInteraction({
|
||||||
|
kind: 'question',
|
||||||
|
interactionId: 'present-fail',
|
||||||
|
rpcId: 'present-fail',
|
||||||
|
sessionId,
|
||||||
|
payload: {
|
||||||
|
type: 'question/requested',
|
||||||
|
sessionId,
|
||||||
|
questions: [{ id: 'only', question: '唯一问题', options: [{ label: 'X' }, { label: 'Y' }] }],
|
||||||
|
},
|
||||||
|
respond: async (result) => {
|
||||||
|
responds += 1;
|
||||||
|
respondResult = result;
|
||||||
|
return { accepted: true };
|
||||||
|
},
|
||||||
|
});
|
||||||
|
return 'done';
|
||||||
|
},
|
||||||
|
},
|
||||||
|
state: fixture.state,
|
||||||
|
status: bridgeStatus(),
|
||||||
|
allowedSenderOpenIds: new Set(['ou_user']),
|
||||||
|
});
|
||||||
|
|
||||||
|
// The question card and its text fallback both fail, so the question is
|
||||||
|
// never presented as an answerable card. Both sends are attempted, and the
|
||||||
|
// interaction is only ever cancelled (never answered with a choice).
|
||||||
|
const turn = bridge.accept(event('present-fail-start', '触发问题'));
|
||||||
|
await eventually(() => sent.length >= 2, 'neither the card nor the text fallback was attempted');
|
||||||
|
await turn.catch(() => undefined);
|
||||||
|
assert.equal(responds, 1, 'the interaction must be resolved by cancellation only');
|
||||||
|
assert.equal(respondResult?.ok, false, 'it must not be answered as a presented question');
|
||||||
|
assert.equal(respondResult?.error?.code, 'cancelled', 'it must be cancelled, not answered');
|
||||||
|
});
|
||||||
|
|
||||||
test('question replays are deduplicated and an unrenderable approval is safely rejected', async () => {
|
test('question replays are deduplicated and an unrenderable approval is safely rejected', async () => {
|
||||||
const fixture = stateFixture();
|
const fixture = stateFixture();
|
||||||
const sent = [];
|
const sent = [];
|
||||||
|
|
@ -3706,6 +3903,10 @@ function issue86Fixture({ withProgressBeforeQuestion }) {
|
||||||
const bridge = new FeishuHarnessBridge({
|
const bridge = new FeishuHarnessBridge({
|
||||||
client,
|
client,
|
||||||
channel: new VerifiedFeishuChannel({ client }),
|
channel: new VerifiedFeishuChannel({ client }),
|
||||||
|
// issue #86 tests assert the streaming-card rotation flow, which drives
|
||||||
|
// the plain-text question reply; the interaction card path is tested
|
||||||
|
// separately, so pin the text presentation here.
|
||||||
|
interactionCards: false,
|
||||||
harness: {
|
harness: {
|
||||||
sessionExists: async () => true,
|
sessionExists: async () => true,
|
||||||
ask: async (sessionId, _text, options) => {
|
ask: async (sessionId, _text, options) => {
|
||||||
|
|
@ -3870,6 +4071,10 @@ function issue86RotationFixture({
|
||||||
const bridge = new FeishuHarnessBridge({
|
const bridge = new FeishuHarnessBridge({
|
||||||
client,
|
client,
|
||||||
channel: new VerifiedFeishuChannel({ client }),
|
channel: new VerifiedFeishuChannel({ client }),
|
||||||
|
// issue #86 rotation tests assert the streaming-card flow with the
|
||||||
|
// plain-text question reply; the interaction card path is tested
|
||||||
|
// separately, so pin the text presentation here.
|
||||||
|
interactionCards: false,
|
||||||
harness: {
|
harness: {
|
||||||
sessionExists: async () => true,
|
sessionExists: async () => true,
|
||||||
currentWorkspace: () => null,
|
currentWorkspace: () => null,
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue